Re: [Qemu-devel] [PATCH v6 14/18] hw/arm/virt: Allocate device_memory

[David Hildenbrand <david@redhat.com>]

On 21.02.19 13:37, Auger Eric wrote:
> Hi Igor,
> 
> On 2/21/19 10:36 AM, Igor Mammedov wrote:
>> On Tue, 19 Feb 2019 16:53:22 +0100
>> Auger Eric <eric.auger@redhat.com> wrote:
>>
>>> Hi Igor,
>>>
>>> On 2/18/19 10:31 AM, Igor Mammedov wrote:
>>>> On Tue,  5 Feb 2019 18:33:02 +0100
>>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>>   
>>>>> The device memory region is located after the initial RAM.
>>>>> its start/size are 1GB aligned.
>>>>>
>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
>>>>>
>>>>> ---
>>>>> v4 -> v5:
>>>>> - device memory set after the initial RAM
>>>>>
>>>>> v3 -> v4:
>>>>> - remove bootinfo.device_memory_start/device_memory_size
>>>>> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM
>>>>> ---
>>>>>  hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>> index 783468ba77..b683902991 100644
>>>>> --- a/hw/arm/virt.c
>>>>> +++ b/hw/arm/virt.c
>>>>> @@ -61,6 +61,7 @@
>>>>>  #include "hw/arm/smmuv3.h"
>>>>>  #include "hw/mem/pc-dimm.h"
>>>>>  #include "hw/mem/nvdimm.h"
>>>>> +#include "hw/acpi/acpi.h"
>>>>>  
>>>>>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>>>>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>>>>> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms,
>>>>>      g_free(nodename);
>>>>>  }
>>>>>  
>>>>> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem)
>>>>> +{
>>>>> +    MachineState *ms = MACHINE(vms);
>>>>> +    uint64_t device_memory_size = ms->maxram_size - ms->ram_size;  
>>>> should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes),
>>>> see enforce_aligned_dimm usage and associated commit for more details  
>>> I don't understand the computation done in pc machine. eventually we are
>>> likely to have more device memory than requested by the user. Why don't
>>> we check (machine->maxram_size - machine->ram_size) >=
>>> machine->ram_slots * GiB
>>> instead of adding 1GiB/slot to the initial user requirements?
>>>
>>> Also machine->maxram_size - machine->ram_size is checked to be aligned
>>> with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest
>>> PAGE in accelerated mode? Is it valid ro require an alignment on 1GB
>>> boundary as I do in this patch?
>> See commit 085f8e88b for explanation,
>> What we are basically are doing there is sizing hotpluggbale address space
>> to allow max possible huge page aligned DIMM to be successfully plugged in
>> even if address space if fragmented.
> In v7, I also added ram_slots * GiB to (maxram_size - ram_size).
> 

Depending on the way the system handles it, this might be confusing for
the end user and has to be documented somewhere.

E.g. if there are certain memory limits (say 2TB) and the user specifies
something like "maxmem=2TB,slots=20" it might be confusing if he gets an
error like "more than 2TB are not supported".

> Thanks
> 
> Eric
-- 

Thanks,

David / dhildenb