Re: [PATCH] mem/x86: add processor address space check for VM memory

[Ani Sinha]

> On 14-Sep-2023, at 5:19 PM, David Hildenbrand <david@redhat.com> wrote:
> 
>>>>> We requested a to hotplug a maximum of "8 GiB", and sized the area 
>>>>> slightly larger to allow for some flexibility
>>>>> when it comes to placing DIMMs in that "device-memory" area.
>>>> Right but here in this example you do not hot plug memory while the VM is 
>>>> running. We can hot plug 8G yes, but the memory may not physically exist 
>>>> yet (and may never exist). How can we use this math to provision 
>>>> device-memory when the memory may not exist physically?
>>> 
>>> We simply reserve a region in GPA space where we can coldplug and hotplug a
>>> predefined maximum amount of memory we can hotplug.
>>> 
>>> What do you think is wrong with that?
>> The only issue I have is that even though we are accounting for it, the 
>> memory actually might not be physically present.
> 
> Not sure if "accounting" is the right word; the memory is not present and 
> nowhere indicated as present. It's just a reservation of GPA space, like the 
> PCI hole is as well.
> 
> [...]
> 
>>> 
>>> Yes. In this case ms->ram_size == ms->maxram_size and you cannot 
>>> cold/hotplug any memory devices.
>>> 
>>> See how pc_memory_init() doesn't call machine_memory_devices_init() in that 
>>> case.
>>> 
>>> That's what the QEMU user asked for when *not* specifying maxmem (e.g., -m 
>>> 4g).
>>> 
>>> In order to cold/hotplug any memory devices, you have to tell QEMU ahead of 
>>> time how much memory
>>> you are intending to provide using memory devices (DIMM, NVDIMM, 
>>> virtio-pmem, virtio-mem).
>> So that means that when we are actually hot plugging the memory, there is no 
>> need to actually perform additional checks. It can be done statically when 
>> -mem and -maxmem etc are provided in the command line.
> 
> What memory device code does, is find a free location inside the reserved GPA 
> space for memory devices. Then, it maps that device at
> that location.

Yes I see the function memory_device_get_free_addr() that does all the range 
checks and validations and this gets called from the pre_plug() handler. 

> 
> [...]
> 
>>> /*
>>> * The 64bit pci hole starts after "above 4G RAM" and
>>> * potentially the space reserved for memory hotplug.
>>> */
>>> 
>>> There is the
>>>     ROUND_UP(hole64_start, 1 * GiB);
>>> in there that is not really required for the !hole64 case. It
>>> shouldn't matter much in practice I think (besides an aligned value
>>> showing up in the error message).
>>> 
>>> We could factor out most of that calculation into a
>>> separate function, skipping that alignment to make that
>>> clearer.
>> Yeah this whole memory segmentation is quite complicated and might benefit 
>> from a qemu doc or a refactoring.
> 
> Absolutely. Do you have time to work on that (including the updated fix?).

Other than the fix you proposed I am not sure if we need to fix anything else 
atm. Seems physical address space bound checks are already in place.
Re: doc, maybe. I will add it to my TODO list.

> 
> -- 
> Cheers,
> 
> David / dhildenb