| Message ID | 20180516152026.2920-3-shameerali.kolothum.thodi@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: Add support for non-contiguous iova regions | expand |
Hi Shameer, On 05/16/2018 05:20 PM, Shameer Kolothum wrote: > Register ram_memory_region_init notifier to allocate memory region > from system memory. At this stage the commit message does not explain why you need a machine init done notifier. Also the commit title does not summarize the actual change. Thanks Eric > > Signed-off-by: Zhu Yijun <zhuyijun@huawei.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > hw/arm/virt.c | 28 ++++++++++++++++++++++------ > include/hw/arm/virt.h | 1 + > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 94dcb12..05fcb62 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1171,6 +1171,19 @@ void virt_machine_done(Notifier *notifier, void *data) > virt_build_smbios(vms); > } > > +static void virt_ram_memory_region_init(Notifier *notifier, void *data) > +{ > + MemoryRegion *sysmem = get_system_memory(); > + MemoryRegion *ram = g_new(MemoryRegion, 1); > + VirtMachineState *vms = container_of(notifier, VirtMachineState, > + ram_memory_region_init); > + MachineState *machine = MACHINE(vms); > + > + memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > + machine->ram_size); > + memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); > +} > + > static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) > { > uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER; > @@ -1204,7 +1217,6 @@ static void machvirt_init(MachineState *machine) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *secure_sysmem = NULL; > int n, virt_max_cpus; > - MemoryRegion *ram = g_new(MemoryRegion, 1); > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); > > /* We can probe only here because during property set > @@ -1361,10 +1373,6 @@ static void machvirt_init(MachineState *machine) > fdt_add_timer_nodes(vms); > fdt_add_cpu_nodes(vms); > > - memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > - machine->ram_size); > - memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); > - > create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); > > create_gic(vms, pic); > @@ -1405,15 +1413,23 @@ static void machvirt_init(MachineState *machine) > vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base; > vms->bootinfo.get_dtb = machvirt_dtb; > vms->bootinfo.firmware_loaded = firmware_loaded; > + > + /* Register notifiers. They are executed in registration reverse order */ > arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo); > > /* > * arm_load_kernel machine init done notifier registration must > * happen before the platform_bus_create call. In this latter, > * another notifier is registered which adds platform bus nodes. > - * Notifiers are executed in registration reverse order. > */ > create_platform_bus(vms, pic); > + > + /* > + * Register memory region notifier last as this has to be executed > + * first. > + */ > + vms->ram_memory_region_init.notify = virt_ram_memory_region_init; > + qemu_add_machine_init_done_notifier(&vms->ram_memory_region_init); > } > > static bool virt_get_secure(Object *obj, Error **errp) > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index ba0c1a4..fc24f3a 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -91,6 +91,7 @@ typedef struct { > typedef struct { > MachineState parent; > Notifier machine_done; > + Notifier ram_memory_region_init; > FWCfgState *fw_cfg; > bool secure; > bool highmem; >
On Wed, May 16, 2018 at 04:20:22PM +0100, Shameer Kolothum wrote: > Register ram_memory_region_init notifier to allocate memory region > from system memory. > > Signed-off-by: Zhu Yijun <zhuyijun@huawei.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > hw/arm/virt.c | 28 ++++++++++++++++++++++------ > include/hw/arm/virt.h | 1 + > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 94dcb12..05fcb62 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1171,6 +1171,19 @@ void virt_machine_done(Notifier *notifier, void *data) > virt_build_smbios(vms); > } > > +static void virt_ram_memory_region_init(Notifier *notifier, void *data) > +{ > + MemoryRegion *sysmem = get_system_memory(); > + MemoryRegion *ram = g_new(MemoryRegion, 1); > + VirtMachineState *vms = container_of(notifier, VirtMachineState, > + ram_memory_region_init); > + MachineState *machine = MACHINE(vms); > + > + memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > + machine->ram_size); > + memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); > +} > + > static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) > { > uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER; > @@ -1204,7 +1217,6 @@ static void machvirt_init(MachineState *machine) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *secure_sysmem = NULL; > int n, virt_max_cpus; > - MemoryRegion *ram = g_new(MemoryRegion, 1); > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); > > /* We can probe only here because during property set > @@ -1361,10 +1373,6 @@ static void machvirt_init(MachineState *machine) > fdt_add_timer_nodes(vms); > fdt_add_cpu_nodes(vms); > > - memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > - machine->ram_size); > - memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); > - > create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); > > create_gic(vms, pic); > @@ -1405,15 +1413,23 @@ static void machvirt_init(MachineState *machine) > vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base; > vms->bootinfo.get_dtb = machvirt_dtb; > vms->bootinfo.firmware_loaded = firmware_loaded; > + > + /* Register notifiers. They are executed in registration reverse order */ > arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo); > > /* > * arm_load_kernel machine init done notifier registration must > * happen before the platform_bus_create call. In this latter, > * another notifier is registered which adds platform bus nodes. > - * Notifiers are executed in registration reverse order. > */ > create_platform_bus(vms, pic); > + > + /* > + * Register memory region notifier last as this has to be executed > + * first. > + */ > + vms->ram_memory_region_init.notify = virt_ram_memory_region_init; > + qemu_add_machine_init_done_notifier(&vms->ram_memory_region_init); I realize we've been slow to get to this for reviewing, but a lot has changed here since this patch was written. virt now only has a single machine done notifier. On rebase we should attempt to integrate with that one rather than introduce a new one. Thanks, drew > } > > static bool virt_get_secure(Object *obj, Error **errp) > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index ba0c1a4..fc24f3a 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -91,6 +91,7 @@ typedef struct { > typedef struct { > MachineState parent; > Notifier machine_done; > + Notifier ram_memory_region_init; > FWCfgState *fw_cfg; > bool secure; > bool highmem; > -- > 2.7.4 > > >
Hi Eric. > -----Original Message----- > From: Auger Eric [mailto:eric.auger@redhat.com] > Sent: Monday, May 28, 2018 3:22 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > qemu-devel@nongnu.org; qemu-arm@nongnu.org > Cc: drjones@redhat.com; imammedo@redhat.com; peter.maydell@linaro.org; > alex.williamson@redhat.com; Zhaoshenglong <zhaoshenglong@huawei.com>; > Jonathan Cameron <jonathan.cameron@huawei.com>; Linuxarm > <linuxarm@huawei.com> > Subject: Re: [RFC v2 2/6] hw/arm/virt: Enable dynamic generation of guest > RAM memory regions > > Hi Shameer, > On 05/16/2018 05:20 PM, Shameer Kolothum wrote: > > Register ram_memory_region_init notifier to allocate memory region > > from system memory. > > At this stage the commit message does not explain why you need a machine > init done notifier. Also the commit title does not summarize the actual > change. > Thanks Right. I will reword/rephrase the text. Thanks, Shameer > Eric > > > > Signed-off-by: Zhu Yijun <zhuyijun@huawei.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > hw/arm/virt.c | 28 ++++++++++++++++++++++------ > > include/hw/arm/virt.h | 1 + > > 2 files changed, 23 insertions(+), 6 deletions(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 94dcb12..05fcb62 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1171,6 +1171,19 @@ void virt_machine_done(Notifier *notifier, void > *data) > > virt_build_smbios(vms); > > } > > > > +static void virt_ram_memory_region_init(Notifier *notifier, void *data) > > +{ > > + MemoryRegion *sysmem = get_system_memory(); > > + MemoryRegion *ram = g_new(MemoryRegion, 1); > > + VirtMachineState *vms = container_of(notifier, VirtMachineState, > > + ram_memory_region_init); > > + MachineState *machine = MACHINE(vms); > > + > > + memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > > + machine->ram_size); > > + memory_region_add_subregion(sysmem, vms- > >memmap[VIRT_MEM].base, ram); > > +} > > + > > static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) > > { > > uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER; > > @@ -1204,7 +1217,6 @@ static void machvirt_init(MachineState *machine) > > MemoryRegion *sysmem = get_system_memory(); > > MemoryRegion *secure_sysmem = NULL; > > int n, virt_max_cpus; > > - MemoryRegion *ram = g_new(MemoryRegion, 1); > > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); > > > > /* We can probe only here because during property set > > @@ -1361,10 +1373,6 @@ static void machvirt_init(MachineState *machine) > > fdt_add_timer_nodes(vms); > > fdt_add_cpu_nodes(vms); > > > > - memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > > - machine->ram_size); > > - memory_region_add_subregion(sysmem, vms- > >memmap[VIRT_MEM].base, ram); > > - > > create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); > > > > create_gic(vms, pic); > > @@ -1405,15 +1413,23 @@ static void machvirt_init(MachineState > *machine) > > vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base; > > vms->bootinfo.get_dtb = machvirt_dtb; > > vms->bootinfo.firmware_loaded = firmware_loaded; > > + > > + /* Register notifiers. They are executed in registration reverse order */ > > arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo); > > > > /* > > * arm_load_kernel machine init done notifier registration must > > * happen before the platform_bus_create call. In this latter, > > * another notifier is registered which adds platform bus nodes. > > - * Notifiers are executed in registration reverse order. > > */ > > create_platform_bus(vms, pic); > > + > > + /* > > + * Register memory region notifier last as this has to be executed > > + * first. > > + */ > > + vms->ram_memory_region_init.notify = virt_ram_memory_region_init; > > + qemu_add_machine_init_done_notifier(&vms- > >ram_memory_region_init); > > } > > > > static bool virt_get_secure(Object *obj, Error **errp) > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > > index ba0c1a4..fc24f3a 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -91,6 +91,7 @@ typedef struct { > > typedef struct { > > MachineState parent; > > Notifier machine_done; > > + Notifier ram_memory_region_init; > > FWCfgState *fw_cfg; > > bool secure; > > bool highmem; > >
Hi Drew, Thanks for going through this. > -----Original Message----- > From: Andrew Jones [mailto:drjones@redhat.com] > Sent: Monday, May 28, 2018 5:47 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > peter.maydell@linaro.org; Jonathan Cameron > <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>; > eric.auger@redhat.com; alex.williamson@redhat.com; Zhaoshenglong > <zhaoshenglong@huawei.com>; imammedo@redhat.com > Subject: Re: [Qemu-devel] [RFC v2 2/6] hw/arm/virt: Enable dynamic > generation of guest RAM memory regions > > On Wed, May 16, 2018 at 04:20:22PM +0100, Shameer Kolothum wrote: > > Register ram_memory_region_init notifier to allocate memory region > > from system memory. > > > > Signed-off-by: Zhu Yijun <zhuyijun@huawei.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > hw/arm/virt.c | 28 ++++++++++++++++++++++------ > > include/hw/arm/virt.h | 1 + > > 2 files changed, 23 insertions(+), 6 deletions(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 94dcb12..05fcb62 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1171,6 +1171,19 @@ void virt_machine_done(Notifier *notifier, void > *data) > > virt_build_smbios(vms); > > } > > > > +static void virt_ram_memory_region_init(Notifier *notifier, void *data) > > +{ > > + MemoryRegion *sysmem = get_system_memory(); > > + MemoryRegion *ram = g_new(MemoryRegion, 1); > > + VirtMachineState *vms = container_of(notifier, VirtMachineState, > > + ram_memory_region_init); > > + MachineState *machine = MACHINE(vms); > > + > > + memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > > + machine->ram_size); > > + memory_region_add_subregion(sysmem, vms- > >memmap[VIRT_MEM].base, ram); > > +} > > + > > static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) > > { > > uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER; > > @@ -1204,7 +1217,6 @@ static void machvirt_init(MachineState *machine) > > MemoryRegion *sysmem = get_system_memory(); > > MemoryRegion *secure_sysmem = NULL; > > int n, virt_max_cpus; > > - MemoryRegion *ram = g_new(MemoryRegion, 1); > > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); > > > > /* We can probe only here because during property set > > @@ -1361,10 +1373,6 @@ static void machvirt_init(MachineState *machine) > > fdt_add_timer_nodes(vms); > > fdt_add_cpu_nodes(vms); > > > > - memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", > > - machine->ram_size); > > - memory_region_add_subregion(sysmem, vms- > >memmap[VIRT_MEM].base, ram); > > - > > create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); > > > > create_gic(vms, pic); > > @@ -1405,15 +1413,23 @@ static void machvirt_init(MachineState > *machine) > > vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base; > > vms->bootinfo.get_dtb = machvirt_dtb; > > vms->bootinfo.firmware_loaded = firmware_loaded; > > + > > + /* Register notifiers. They are executed in registration reverse order */ > > arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo); > > > > /* > > * arm_load_kernel machine init done notifier registration must > > * happen before the platform_bus_create call. In this latter, > > * another notifier is registered which adds platform bus nodes. > > - * Notifiers are executed in registration reverse order. > > */ > > create_platform_bus(vms, pic); > > + > > + /* > > + * Register memory region notifier last as this has to be executed > > + * first. > > + */ > > + vms->ram_memory_region_init.notify = virt_ram_memory_region_init; > > + qemu_add_machine_init_done_notifier(&vms- > >ram_memory_region_init); > > I realize we've been slow to get to this for reviewing, but a lot has > changed here since this patch was written. virt now only has a single > machine done notifier. On rebase we should attempt to integrate with > that one rather than introduce a new one. Ok. I will take a look on that one. Thanks, Shameer > Thanks, > drew > > > > } > > > > static bool virt_get_secure(Object *obj, Error **errp) > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > > index ba0c1a4..fc24f3a 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -91,6 +91,7 @@ typedef struct { > > typedef struct { > > MachineState parent; > > Notifier machine_done; > > + Notifier ram_memory_region_init; > > FWCfgState *fw_cfg; > > bool secure; > > bool highmem; > > -- > > 2.7.4 > > > > > >
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 94dcb12..05fcb62 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1171,6 +1171,19 @@ void virt_machine_done(Notifier *notifier, void *data) virt_build_smbios(vms); } +static void virt_ram_memory_region_init(Notifier *notifier, void *data) +{ + MemoryRegion *sysmem = get_system_memory(); + MemoryRegion *ram = g_new(MemoryRegion, 1); + VirtMachineState *vms = container_of(notifier, VirtMachineState, + ram_memory_region_init); + MachineState *machine = MACHINE(vms); + + memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", + machine->ram_size); + memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); +} + static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) { uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER; @@ -1204,7 +1217,6 @@ static void machvirt_init(MachineState *machine) MemoryRegion *sysmem = get_system_memory(); MemoryRegion *secure_sysmem = NULL; int n, virt_max_cpus; - MemoryRegion *ram = g_new(MemoryRegion, 1); bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); /* We can probe only here because during property set @@ -1361,10 +1373,6 @@ static void machvirt_init(MachineState *machine) fdt_add_timer_nodes(vms); fdt_add_cpu_nodes(vms); - memory_region_allocate_system_memory(ram, NULL, "mach-virt.ram", - machine->ram_size); - memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); - create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); create_gic(vms, pic); @@ -1405,15 +1413,23 @@ static void machvirt_init(MachineState *machine) vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base; vms->bootinfo.get_dtb = machvirt_dtb; vms->bootinfo.firmware_loaded = firmware_loaded; + + /* Register notifiers. They are executed in registration reverse order */ arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo); /* * arm_load_kernel machine init done notifier registration must * happen before the platform_bus_create call. In this latter, * another notifier is registered which adds platform bus nodes. - * Notifiers are executed in registration reverse order. */ create_platform_bus(vms, pic); + + /* + * Register memory region notifier last as this has to be executed + * first. + */ + vms->ram_memory_region_init.notify = virt_ram_memory_region_init; + qemu_add_machine_init_done_notifier(&vms->ram_memory_region_init); } static bool virt_get_secure(Object *obj, Error **errp) diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index ba0c1a4..fc24f3a 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -91,6 +91,7 @@ typedef struct { typedef struct { MachineState parent; Notifier machine_done; + Notifier ram_memory_region_init; FWCfgState *fw_cfg; bool secure; bool highmem;