| Message ID | 1527690380-9782-3-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM virt: Support up to 256 PCIe buses | expand |
On 05/30/18 16:26, Eric Auger wrote: > Add virt-3.0 machine type. > > This machine type supports highmem 256MB ECAM by default. > This feature is disabled for earlier machine types and > if highmem is off. > > The high 256MB ECAM region is chosen instead of the legacy > 16MB one if the machine type allows it, if highmem is set > (LPAE supported by the guest) and (!firmware_loaded || aarch64). > Indeed aarch32 mode FW may not support this high ECAM region. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > RFC -> v1 > - check firmware_loaded and aarch64 value > - do all the computation in machvirt_init > --- > hw/arm/virt.c | 31 ++++++++++++++++++++++++++++--- > include/hw/arm/virt.h | 1 + > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index d4247d0..83c2b5a 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine) > int n, virt_max_cpus; > MemoryRegion *ram = g_new(MemoryRegion, 1); > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); > + bool aarch64 = true; > > /* We can probe only here because during property set > * KVM is not available yet > @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine) > numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), > &error_fatal); > > + aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); > + > if (!vms->secure) { > object_property_set_bool(cpuobj, false, "has_el3", NULL); > } > @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine) > create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); > } > > + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); > + > create_rtc(vms, pic); > > create_pcie(vms, pic); > @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void) > } > type_init(machvirt_machine_init); > > -static void virt_2_12_instance_init(Object *obj) > +static void virt_3_0_instance_init(Object *obj) > { > VirtMachineState *vms = VIRT_MACHINE(obj); > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); > @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj) > "Set GIC version. " > "Valid values are 2, 3 and host", NULL); > > + vms->highmem_ecam = vmc->no_highmem_ecam ? false : true; > + I think this should be written as: vms->highmem_ecam = !vmc->no_highmem_ecam; With that change: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Obviously the series has to be reviewed by others as well. Thanks! Laszlo > if (vmc->no_its) { > vms->its = false; > } else { > @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj) > vms->irqmap = a15irqmap; > } > > -static void virt_machine_2_12_options(MachineClass *mc) > +static void virt_machine_3_0_options(MachineClass *mc) > { > } > -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12) > +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0) > + > +#define VIRT_COMPAT_2_12 \ > + HW_COMPAT_2_12 > + > +static void virt_2_12_instance_init(Object *obj) > +{ > + virt_3_0_instance_init(obj); > +} > + > +static void virt_machine_2_12_options(MachineClass *mc) > +{ > + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); > + > + virt_machine_3_0_options(mc); > + SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12); > + vmc->no_highmem_ecam = true; > +} > +DEFINE_VIRT_MACHINE(2, 12) > > #define VIRT_COMPAT_2_11 \ > HW_COMPAT_2_11 > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index e9423a7..10a5c71 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -95,6 +95,7 @@ typedef struct { > bool no_pmu; > bool claim_edge_triggered_timers; > bool smbios_old_sys_ver; > + bool no_highmem_ecam; > } VirtMachineClass; > > typedef struct { >
On 2018/5/31 0:18, Laszlo Ersek wrote: > + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >> + Does it need a info log here to tell user that even though you enable the highmem_ecam but due to some other reasons it's disabled. Also, if user enables highmem_ecam but finally it can't enable, does it need to error out? Thanks,
Hi Shannon, On 05/31/2018 03:42 AM, Shannon Zhao wrote: > > > On 2018/5/31 0:18, Laszlo Ersek wrote: >> + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >>> + > Does it need a info log here to tell user that even though you enable > the highmem_ecam but due to some other reasons it's disabled. > > Also, if user enables highmem_ecam but finally it can't enable, does it > need to error out? highmem_ecam is not a user settable option. highmem is. highmem_ecam is computed based on the machine type capability to support this feature, the fact highmem is set and we don't have an aarch32 FW being used. Thanks Eric > > Thanks, >
On 05/30/2018 06:18 PM, Laszlo Ersek wrote: > On 05/30/18 16:26, Eric Auger wrote: >> Add virt-3.0 machine type. >> >> This machine type supports highmem 256MB ECAM by default. >> This feature is disabled for earlier machine types and >> if highmem is off. >> >> The high 256MB ECAM region is chosen instead of the legacy >> 16MB one if the machine type allows it, if highmem is set >> (LPAE supported by the guest) and (!firmware_loaded || aarch64). >> Indeed aarch32 mode FW may not support this high ECAM region. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> RFC -> v1 >> - check firmware_loaded and aarch64 value >> - do all the computation in machvirt_init >> --- >> hw/arm/virt.c | 31 ++++++++++++++++++++++++++++--- >> include/hw/arm/virt.h | 1 + >> 2 files changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index d4247d0..83c2b5a 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine) >> int n, virt_max_cpus; >> MemoryRegion *ram = g_new(MemoryRegion, 1); >> bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); >> + bool aarch64 = true; >> >> /* We can probe only here because during property set >> * KVM is not available yet >> @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine) >> numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), >> &error_fatal); >> >> + aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); >> + >> if (!vms->secure) { >> object_property_set_bool(cpuobj, false, "has_el3", NULL); >> } >> @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine) >> create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); >> } >> >> + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >> + >> create_rtc(vms, pic); >> >> create_pcie(vms, pic); >> @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void) >> } >> type_init(machvirt_machine_init); >> >> -static void virt_2_12_instance_init(Object *obj) >> +static void virt_3_0_instance_init(Object *obj) >> { >> VirtMachineState *vms = VIRT_MACHINE(obj); >> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); >> @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj) >> "Set GIC version. " >> "Valid values are 2, 3 and host", NULL); >> >> + vms->highmem_ecam = vmc->no_highmem_ecam ? false : true; >> + > > I think this should be written as: > > vms->highmem_ecam = !vmc->no_highmem_ecam; sure ;-) > > With that change: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thank you for the early feedbacks. Eric > > Obviously the series has to be reviewed by others as well. > > Thanks! > Laszlo > > >> if (vmc->no_its) { >> vms->its = false; >> } else { >> @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj) >> vms->irqmap = a15irqmap; >> } >> >> -static void virt_machine_2_12_options(MachineClass *mc) >> +static void virt_machine_3_0_options(MachineClass *mc) >> { >> } >> -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12) >> +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0) >> + >> +#define VIRT_COMPAT_2_12 \ >> + HW_COMPAT_2_12 >> + >> +static void virt_2_12_instance_init(Object *obj) >> +{ >> + virt_3_0_instance_init(obj); >> +} >> + >> +static void virt_machine_2_12_options(MachineClass *mc) >> +{ >> + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); >> + >> + virt_machine_3_0_options(mc); >> + SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12); >> + vmc->no_highmem_ecam = true; >> +} >> +DEFINE_VIRT_MACHINE(2, 12) >> >> #define VIRT_COMPAT_2_11 \ >> HW_COMPAT_2_11 >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >> index e9423a7..10a5c71 100644 >> --- a/include/hw/arm/virt.h >> +++ b/include/hw/arm/virt.h >> @@ -95,6 +95,7 @@ typedef struct { >> bool no_pmu; >> bool claim_edge_triggered_timers; >> bool smbios_old_sys_ver; >> + bool no_highmem_ecam; >> } VirtMachineClass; >> >> typedef struct { >> > >
On 30 May 2018 at 15:26, Eric Auger <eric.auger@redhat.com> wrote: > Add virt-3.0 machine type. > > This machine type supports highmem 256MB ECAM by default. > This feature is disabled for earlier machine types and > if highmem is off. > > The high 256MB ECAM region is chosen instead of the legacy > 16MB one if the machine type allows it, if highmem is set > (LPAE supported by the guest) and (!firmware_loaded || aarch64). > Indeed aarch32 mode FW may not support this high ECAM region. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> I think these patches should be the other way round -- create the new 3.0 machine type first, and then in the patch adding the high ECAM region you can make it be only if newer machine type. thanks -- PMM
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d4247d0..83c2b5a 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine) int n, virt_max_cpus; MemoryRegion *ram = g_new(MemoryRegion, 1); bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); + bool aarch64 = true; /* We can probe only here because during property set * KVM is not available yet @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine) numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), &error_fatal); + aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); + if (!vms->secure) { object_property_set_bool(cpuobj, false, "has_el3", NULL); } @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine) create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); } + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); + create_rtc(vms, pic); create_pcie(vms, pic); @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void) } type_init(machvirt_machine_init); -static void virt_2_12_instance_init(Object *obj) +static void virt_3_0_instance_init(Object *obj) { VirtMachineState *vms = VIRT_MACHINE(obj); VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj) "Set GIC version. " "Valid values are 2, 3 and host", NULL); + vms->highmem_ecam = vmc->no_highmem_ecam ? false : true; + if (vmc->no_its) { vms->its = false; } else { @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj) vms->irqmap = a15irqmap; } -static void virt_machine_2_12_options(MachineClass *mc) +static void virt_machine_3_0_options(MachineClass *mc) { } -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12) +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0) + +#define VIRT_COMPAT_2_12 \ + HW_COMPAT_2_12 + +static void virt_2_12_instance_init(Object *obj) +{ + virt_3_0_instance_init(obj); +} + +static void virt_machine_2_12_options(MachineClass *mc) +{ + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); + + virt_machine_3_0_options(mc); + SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12); + vmc->no_highmem_ecam = true; +} +DEFINE_VIRT_MACHINE(2, 12) #define VIRT_COMPAT_2_11 \ HW_COMPAT_2_11 diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index e9423a7..10a5c71 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -95,6 +95,7 @@ typedef struct { bool no_pmu; bool claim_edge_triggered_timers; bool smbios_old_sys_ver; + bool no_highmem_ecam; } VirtMachineClass; typedef struct {
Add virt-3.0 machine type. This machine type supports highmem 256MB ECAM by default. This feature is disabled for earlier machine types and if highmem is off. The high 256MB ECAM region is chosen instead of the legacy 16MB one if the machine type allows it, if highmem is set (LPAE supported by the guest) and (!firmware_loaded || aarch64). Indeed aarch32 mode FW may not support this high ECAM region. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- RFC -> v1 - check firmware_loaded and aarch64 value - do all the computation in machvirt_init --- hw/arm/virt.c | 31 ++++++++++++++++++++++++++++--- include/hw/arm/virt.h | 1 + 2 files changed, 29 insertions(+), 3 deletions(-)