[9/9] hw/arm/virt: Add virt-3.0 machine type
diff mbox series

Message ID 1528879723-24675-10-git-send-email-eric.auger@redhat.com
State New
Headers show
Series
  • KVM/ARM: virt-3.0: Multiple redistributor regions and 256MB ECAM region
Related show

Commit Message

Auger Eric June 13, 2018, 8:48 a.m. UTC
This machine type supports two new features:
- highmem 256MB ECAM (default). This feature is disabled for
  earlier machine types and if highmem is off.
- max_cpus set to 512 vcpus (255 before)

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>

---

PATCH: merge of ECAM and VCPU extension
- Laszlo reviewed the ECAM changes but I dropped his R-b
  due to the squash

RFC -> v1
- check firmware_loaded and aarch64 value
- do all the computation in machvirt_init
---
 hw/arm/virt.c         | 36 ++++++++++++++++++++++++++++++------
 include/hw/arm/virt.h |  1 +
 2 files changed, 31 insertions(+), 6 deletions(-)

Comments

Laszlo Ersek June 13, 2018, 9:05 p.m. UTC | #1
On 06/13/18 10:48, Eric Auger wrote:

> PATCH: merge of ECAM and VCPU extension
> - Laszlo reviewed the ECAM changes but I dropped his R-b
>   due to the squash

Was there any particular reason why the previous patch set (with only
the ECAM enlargement) couldn't be merged first? To be honest I'm not
super happy when my R-b is dropped for non-technical reasons; it seems
like wasted work for both of us.

Obviously if there's a technical dependency or some other reason why
committing the ECAM enlargement in separation would be *wrong*, that's
different. Even in that case, wouldn't it be possible to keep the
initial virt-3.0 machtype addition as I reviewed it, and then add the
rest in an incremental patch?

Thanks,
Laszlo
Auger Eric June 14, 2018, 6:27 a.m. UTC | #2
Hi Laszlo,

On 06/13/2018 11:05 PM, Laszlo Ersek wrote:
> On 06/13/18 10:48, Eric Auger wrote:
> 
>> PATCH: merge of ECAM and VCPU extension
>> - Laszlo reviewed the ECAM changes but I dropped his R-b
>>   due to the squash
> 
> Was there any particular reason why the previous patch set (with only
> the ECAM enlargement) couldn't be merged first? To be honest I'm not
> super happy when my R-b is dropped for non-technical reasons; it seems
> like wasted work for both of us.
> 
> Obviously if there's a technical dependency or some other reason why
> committing the ECAM enlargement in separation would be *wrong*, that's
> different. Even in that case, wouldn't it be possible to keep the
> initial virt-3.0 machtype addition as I reviewed it, and then add the
> rest in an incremental patch?

Sorry about that. My fear was about migration. We would have had 2 virt
3.0 machine models not supporting the same features. While bisecting
migration we could have had the source using the high mem ECAM and the
destination not supporting it. So I preferred to avoid this trouble by
merging the 2 features in one patch. However I may have kept your R-b
restricting its scope to the ECAM stuff.

Thanks

Eric
> 
> Thanks,
> Laszlo
>
Laszlo Ersek June 14, 2018, 8:56 a.m. UTC | #3
Hi Eric,

On 06/14/18 08:27, Auger Eric wrote:
> Hi Laszlo,
> 
> On 06/13/2018 11:05 PM, Laszlo Ersek wrote:
>> On 06/13/18 10:48, Eric Auger wrote:
>>
>>> PATCH: merge of ECAM and VCPU extension
>>> - Laszlo reviewed the ECAM changes but I dropped his R-b
>>>   due to the squash
>>
>> Was there any particular reason why the previous patch set (with only
>> the ECAM enlargement) couldn't be merged first? To be honest I'm not
>> super happy when my R-b is dropped for non-technical reasons; it seems
>> like wasted work for both of us.
>>
>> Obviously if there's a technical dependency or some other reason why
>> committing the ECAM enlargement in separation would be *wrong*, that's
>> different. Even in that case, wouldn't it be possible to keep the
>> initial virt-3.0 machtype addition as I reviewed it, and then add the
>> rest in an incremental patch?
> 
> Sorry about that. My fear was about migration. We would have had 2 virt
> 3.0 machine models not supporting the same features. While bisecting
> migration we could have had the source using the high mem ECAM and the
> destination not supporting it. So I preferred to avoid this trouble by
> merging the 2 features in one patch. However I may have kept your R-b
> restricting its scope to the ECAM stuff.

to my understanding, it is normal to *gradually* add new properties
during the development cycle, to the new machine type of the upcoming
QEMU release. To my understanding, it's not expected that migration work
between development snapshots built from git. What matters is that two
official releases, specifying the same machine type, enable the user to
migrate a guest between them (in forward direction).

In every release, so many new features are introduced that it's
impossible to introduce the new machine type with all the compat knobs
added at once. Instead, the new machine type is introduced when the
first feature that requires a compat knob is added to git. All other
such features extend the compat knobs gradually, during the development
cycle. Until the new official release is made (which contains all the
compat knobs for all the new features), the new machine type simply
doesn't exist, as far as the public is concerned, so it cannot partake
in migration either.

This is my understanding anyway.

Thanks!
Laszlo
Daniel P. Berrangé June 14, 2018, 8:59 a.m. UTC | #4
On Thu, Jun 14, 2018 at 10:56:20AM +0200, Laszlo Ersek wrote:
> Hi Eric,
> 
> On 06/14/18 08:27, Auger Eric wrote:
> > Hi Laszlo,
> > 
> > On 06/13/2018 11:05 PM, Laszlo Ersek wrote:
> >> On 06/13/18 10:48, Eric Auger wrote:
> >>
> >>> PATCH: merge of ECAM and VCPU extension
> >>> - Laszlo reviewed the ECAM changes but I dropped his R-b
> >>>   due to the squash
> >>
> >> Was there any particular reason why the previous patch set (with only
> >> the ECAM enlargement) couldn't be merged first? To be honest I'm not
> >> super happy when my R-b is dropped for non-technical reasons; it seems
> >> like wasted work for both of us.
> >>
> >> Obviously if there's a technical dependency or some other reason why
> >> committing the ECAM enlargement in separation would be *wrong*, that's
> >> different. Even in that case, wouldn't it be possible to keep the
> >> initial virt-3.0 machtype addition as I reviewed it, and then add the
> >> rest in an incremental patch?
> > 
> > Sorry about that. My fear was about migration. We would have had 2 virt
> > 3.0 machine models not supporting the same features. While bisecting
> > migration we could have had the source using the high mem ECAM and the
> > destination not supporting it. So I preferred to avoid this trouble by
> > merging the 2 features in one patch. However I may have kept your R-b
> > restricting its scope to the ECAM stuff.
> 
> to my understanding, it is normal to *gradually* add new properties
> during the development cycle, to the new machine type of the upcoming
> QEMU release. To my understanding, it's not expected that migration work
> between development snapshots built from git. What matters is that two
> official releases, specifying the same machine type, enable the user to
> migrate a guest between them (in forward direction).
> 
> In every release, so many new features are introduced that it's
> impossible to introduce the new machine type with all the compat knobs
> added at once. Instead, the new machine type is introduced when the
> first feature that requires a compat knob is added to git. All other
> such features extend the compat knobs gradually, during the development
> cycle. Until the new official release is made (which contains all the
> compat knobs for all the new features), the new machine type simply
> doesn't exist, as far as the public is concerned, so it cannot partake
> in migration either.
> 
> This is my understanding anyway.

That is correct - there is ZERO expectation of migration / ABI stability
between arbitrary GIT snapshots, only official releases.  Prior to the
first release including it, a versioned machine type can be changed
arbitrarily.

Regards,
Daniel
Auger Eric June 14, 2018, 9:03 a.m. UTC | #5
Hi Laszlo,
On 06/14/2018 10:56 AM, Laszlo Ersek wrote:
> Hi Eric,
> 
> On 06/14/18 08:27, Auger Eric wrote:
>> Hi Laszlo,
>>
>> On 06/13/2018 11:05 PM, Laszlo Ersek wrote:
>>> On 06/13/18 10:48, Eric Auger wrote:
>>>
>>>> PATCH: merge of ECAM and VCPU extension
>>>> - Laszlo reviewed the ECAM changes but I dropped his R-b
>>>>   due to the squash
>>>
>>> Was there any particular reason why the previous patch set (with only
>>> the ECAM enlargement) couldn't be merged first? To be honest I'm not
>>> super happy when my R-b is dropped for non-technical reasons; it seems
>>> like wasted work for both of us.
>>>
>>> Obviously if there's a technical dependency or some other reason why
>>> committing the ECAM enlargement in separation would be *wrong*, that's
>>> different. Even in that case, wouldn't it be possible to keep the
>>> initial virt-3.0 machtype addition as I reviewed it, and then add the
>>> rest in an incremental patch?
>>
>> Sorry about that. My fear was about migration. We would have had 2 virt
>> 3.0 machine models not supporting the same features. While bisecting
>> migration we could have had the source using the high mem ECAM and the
>> destination not supporting it. So I preferred to avoid this trouble by
>> merging the 2 features in one patch. However I may have kept your R-b
>> restricting its scope to the ECAM stuff.
> 
> to my understanding, it is normal to *gradually* add new properties
> during the development cycle, to the new machine type of the upcoming
> QEMU release. To my understanding, it's not expected that migration work
> between development snapshots built from git. What matters is that two
> official releases, specifying the same machine type, enable the user to
> migrate a guest between them (in forward direction).
> 
> In every release, so many new features are introduced that it's
> impossible to introduce the new machine type with all the compat knobs
> added at once. Instead, the new machine type is introduced when the
> first feature that requires a compat knob is added to git. All other
> such features extend the compat knobs gradually, during the development
> cycle. Until the new official release is made (which contains all the
> compat knobs for all the new features), the new machine type simply
> doesn't exist, as far as the public is concerned, so it cannot partake
> in migration either.
> 
> This is my understanding anyway.

Thank you for sharing your understanding. Maybe my concerns were
superficial indeed. If Peter confirms there is no concern with
bisection, I can easily repost the series splitting the virt machine
model modifications in several patches, keeping your R-b ;-)

Thanks

Eric
> 
> Thanks!
> Laszlo
>
Auger Eric June 14, 2018, 9:04 a.m. UTC | #6
Hi Dan, Laszlo,

On 06/14/2018 10:59 AM, Daniel P. Berrangé wrote:
> On Thu, Jun 14, 2018 at 10:56:20AM +0200, Laszlo Ersek wrote:
>> Hi Eric,
>>
>> On 06/14/18 08:27, Auger Eric wrote:
>>> Hi Laszlo,
>>>
>>> On 06/13/2018 11:05 PM, Laszlo Ersek wrote:
>>>> On 06/13/18 10:48, Eric Auger wrote:
>>>>
>>>>> PATCH: merge of ECAM and VCPU extension
>>>>> - Laszlo reviewed the ECAM changes but I dropped his R-b
>>>>>   due to the squash
>>>>
>>>> Was there any particular reason why the previous patch set (with only
>>>> the ECAM enlargement) couldn't be merged first? To be honest I'm not
>>>> super happy when my R-b is dropped for non-technical reasons; it seems
>>>> like wasted work for both of us.
>>>>
>>>> Obviously if there's a technical dependency or some other reason why
>>>> committing the ECAM enlargement in separation would be *wrong*, that's
>>>> different. Even in that case, wouldn't it be possible to keep the
>>>> initial virt-3.0 machtype addition as I reviewed it, and then add the
>>>> rest in an incremental patch?
>>>
>>> Sorry about that. My fear was about migration. We would have had 2 virt
>>> 3.0 machine models not supporting the same features. While bisecting
>>> migration we could have had the source using the high mem ECAM and the
>>> destination not supporting it. So I preferred to avoid this trouble by
>>> merging the 2 features in one patch. However I may have kept your R-b
>>> restricting its scope to the ECAM stuff.
>>
>> to my understanding, it is normal to *gradually* add new properties
>> during the development cycle, to the new machine type of the upcoming
>> QEMU release. To my understanding, it's not expected that migration work
>> between development snapshots built from git. What matters is that two
>> official releases, specifying the same machine type, enable the user to
>> migrate a guest between them (in forward direction).
>>
>> In every release, so many new features are introduced that it's
>> impossible to introduce the new machine type with all the compat knobs
>> added at once. Instead, the new machine type is introduced when the
>> first feature that requires a compat knob is added to git. All other
>> such features extend the compat knobs gradually, during the development
>> cycle. Until the new official release is made (which contains all the
>> compat knobs for all the new features), the new machine type simply
>> doesn't exist, as far as the public is concerned, so it cannot partake
>> in migration either.
>>
>> This is my understanding anyway.
> 
> That is correct - there is ZERO expectation of migration / ABI stability
> between arbitrary GIT snapshots, only official releases.  Prior to the
> first release including it, a versioned machine type can be changed
> arbitrarily.
OK so sufficient consensus on this then.

Thanks

Eric
> 
> Regards,
> Daniel
>
Andrew Jones June 14, 2018, 2:17 p.m. UTC | #7
On Wed, Jun 13, 2018 at 10:48:43AM +0200, Eric Auger wrote:
> This machine type supports two new features:
> - highmem 256MB ECAM (default). This feature is disabled for
>   earlier machine types and if highmem is off.
> - max_cpus set to 512 vcpus (255 before)
> 
> 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>
> 
> ---
> 
> PATCH: merge of ECAM and VCPU extension
> - Laszlo reviewed the ECAM changes but I dropped his R-b
>   due to the squash
> 
> RFC -> v1
> - check firmware_loaded and aarch64 value
> - do all the computation in machvirt_init
> ---
>  hw/arm/virt.c         | 36 ++++++++++++++++++++++++++++++------
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 22b9bd1..5ed25b4 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1317,6 +1317,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
> @@ -1432,6 +1433,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);
>          }
> @@ -1490,6 +1493,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);
> @@ -1700,11 +1705,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>  
>      mc->init = machvirt_init;
> -    /* Start max_cpus at the maximum QEMU supports. We'll further restrict
> -     * it later in machvirt_init, where we have more information about the
> -     * configuration of the particular instance.
> +    /* Start with max_cpus set to 512. This value is chosen since achievable
> +     * in accelerated mode with GICv3 and recent host supporting up to 512 vcpus
> +     * and multiple redistributor region registration.

The word 'recent' probably shouldn't be used in comments. Time flies...
Actually, I'd just write

 Start with max_cpus set to 512, which is the maximum supported by KVM.
 The value may be reduced later when we have more information about the
 configuration of the particular instance.


> +     * This value will be refined later on once we collect more information
> +     * about the configuration of the particular instance.
>       */
> -    mc->max_cpus = 255;
> +    mc->max_cpus = 512;
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
>      mc->block_default_type = IF_VIRTIO;
> @@ -1743,7 +1750,7 @@ type_init(machvirt_machine_init);
>  #define VIRT_COMPAT_2_12 \
>      HW_COMPAT_2_12
>  
> -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);
> @@ -1786,6 +1793,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;
> +
>      if (vmc->no_its) {
>          vms->its = false;
>      } else {
> @@ -1811,11 +1820,26 @@ static void virt_2_12_instance_init(Object *obj)
>      vms->irqmap = a15irqmap;
>  }
>  
> +static void virt_machine_3_0_options(MachineClass *mc)
> +{
> +}
> +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
> +
> +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;
> +    mc->max_cpus = 255;
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
> +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 2c18a59..8c74d4c 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -98,6 +98,7 @@ typedef struct {
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
>      bool smbios_old_sys_ver;
> +    bool no_highmem_ecam;
>  } VirtMachineClass;
>  
>  typedef struct {
> -- 
> 2.5.5
> 
>

Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>

Patch
diff mbox series

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 22b9bd1..5ed25b4 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1317,6 +1317,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
@@ -1432,6 +1433,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);
         }
@@ -1490,6 +1493,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);
@@ -1700,11 +1705,13 @@  static void virt_machine_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = machvirt_init;
-    /* Start max_cpus at the maximum QEMU supports. We'll further restrict
-     * it later in machvirt_init, where we have more information about the
-     * configuration of the particular instance.
+    /* Start with max_cpus set to 512. This value is chosen since achievable
+     * in accelerated mode with GICv3 and recent host supporting up to 512 vcpus
+     * and multiple redistributor region registration.
+     * This value will be refined later on once we collect more information
+     * about the configuration of the particular instance.
      */
-    mc->max_cpus = 255;
+    mc->max_cpus = 512;
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
     mc->block_default_type = IF_VIRTIO;
@@ -1743,7 +1750,7 @@  type_init(machvirt_machine_init);
 #define VIRT_COMPAT_2_12 \
     HW_COMPAT_2_12
 
-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);
@@ -1786,6 +1793,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;
+
     if (vmc->no_its) {
         vms->its = false;
     } else {
@@ -1811,11 +1820,26 @@  static void virt_2_12_instance_init(Object *obj)
     vms->irqmap = a15irqmap;
 }
 
+static void virt_machine_3_0_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
+
+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;
+    mc->max_cpus = 255;
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
+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 2c18a59..8c74d4c 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -98,6 +98,7 @@  typedef struct {
     bool no_pmu;
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
+    bool no_highmem_ecam;
 } VirtMachineClass;
 
 typedef struct {