Re: [PATCH v2 1/2] domain_validate: Account for NVDIMM label size properly when checking for memory conflicts

On Tue, Feb 20, 2024 at 04:53:29PM +0100, Michal Privoznik wrote: > As of v9.8.0-rc1~7 we check whether two <memory/> devices don't > overlap (since we allow setting where a <memory/> device should > be mapped to). We do this pretty straightforward, by comparing > start and end address of each <memory/> device combination. > But since only the start address is given (an exposed in the > XML), the end address is computed trivially as: > >  start + mem->size * 1024 > > And for majority of memory device types this works. Except for > NVDIMMs. For them the <memory/> device consists of two separate > regions: 1) actual memory device, and 2) label. > > Label is where NVDIMM stores some additional information like > namespaces partition and so on. But it's not mapped into the > guest the same way as actual memory device. In fact, mem->size is > a sum of both actual memory device and label sizes. And to make > things a bit worse, both sizes are subject to alignment (either > the alignsize value specified in XML, or system page size if not > specified in XML). > > Therefore, to get the size of actual memory device we need to > take mem->size and substract label size rounded up to alignment. > > If we don't do this we report there's an overlap between two > NVDIMMs even when in reality there's none. > > Fixes: 3fd64fb0e236fc80ffa2cc977c0d471f11fc39bf > Fixes: 91f9a9fb4fc0d34ed8d7a869de3d9f87687c3618 > Resolves: > https://issues.redhat.com/browse/RHEL-4452?focusedId=23805174#comment-238... > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/conf/domain_validate.c | 50 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 48 insertions(+), 2 deletions(-) > > diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c > index 46479f10f2..5a9398b545 100644 > --- a/src/conf/domain_validate.c > +++ b/src/conf/domain_validate.c > @@ -2225,6 +2225,52 @@ virDomainHostdevDefValidate(const > virDomainHostdevDef *hostdev) > } > > > +/** > + * virDomainMemoryGetMappedSize: > + * @mem: memory device definition > + * > + * For given memory device definition (@mem) calculate size mapped into > + * the guest. This is usually mem->size, except for NVDIMM where its > + * label is mapped elsewhere. > + * > + * Returns: Number of bytes a memory device takes when mapped into a > + * guest. > + */ > +static unsigned long long > +virDomainMemoryGetMappedSize(const virDomainMemoryDef *mem) > +{ > +    unsigned long long ret = mem->size; > + > +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { > +        unsigned long long alignsize = mem->source.nvdimm.alignsize; > +        unsigned long long labelsize = 0; > + > +        /* For NVDIMM the situation is a bit more complicated. Firstly, > +         * its <label/> is not mapped as a part of memory device, so we > +         * must subtract label size from NVDIMM size. Secondly, label is > +         * also subject to alignment so we need to round it up before > +         * subtraction. */ > + > +        if (alignsize == 0) { > +            long pagesize = virGetSystemPageSizeKB(); > + > +            /* If no alignment is specified in the XML, fallback to > +             * system page size alignment. */ > +            if (pagesize > 0) > +                alignsize = pagesize; I'm not very well versed in memory modules and architectures, but isn't this restricted a bit more, or rather isn't the QEMU default slightly different?  The following two functions suggest it might be: qemuDomainGetMemorySizeAlignment qemuDomainGetMemoryModuleSizeAlignment