[Qemu-devel] [PATCH for-2.6 0/2] spapr: Use XHCI as default USB type for the pseries machine

[Qemu-devel] [PATCH for-2.6 0/2] spapr: Use XHCI as default USB type for the pseries machine

[Thomas Huth]

The emulated OHCI controller in QEMU has some bugs and performance
issues, so for pseries machine, it's desirable to switch to the XHCI
device instead. To keep the old pseries machine versions compatible,
this patch series also introduces a flag in the SPAPR machine class
that can be used to signal that the OHCI controller should be still
instantiated instead when the default settings are used.

Note: The patches should be applied after my 'Remove duplicated
"pseries" alias' patch that I've sent a couple of days ago (it's
currently not in master yet, just in David's ppc-for-2.5 branch).

Thomas Huth (2):
  hw/ppc/spapr: Create pseries-2.6 machine
  hw/ppc/spapr: Use XHCI as host controller for new spapr machines

 hw/ppc/spapr.c         | 36 +++++++++++++++++++++++++++++++++---
 include/hw/ppc/spapr.h |  3 ++-
 2 files changed, 35 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Thomas Huth]

Add a new pseries-2.6 machine class version to make sure we can
keep the old types compatible to previous versions of QEMU in
later patches.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6bfb908..10b7c35 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2439,8 +2439,6 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
 
     mc->name = "pseries-2.5";
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
-    mc->alias = "pseries";
-    mc->is_default = 1;
     smc->dr_lmb_enabled = true;
 }
 
@@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = {
     .class_init    = spapr_machine_2_5_class_init,
 };
 
+static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
+
+    mc->name = "pseries-2.6";
+    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6";
+    mc->alias = "pseries";
+    mc->is_default = 1;
+    smc->dr_lmb_enabled = true;
+}
+
+static const TypeInfo spapr_machine_2_6_info = {
+    .name          = MACHINE_TYPE_NAME("pseries-2.6"),
+    .parent        = TYPE_SPAPR_MACHINE,
+    .class_init    = spapr_machine_2_6_class_init,
+};
+
 static void spapr_machine_register_types(void)
 {
     type_register_static(&spapr_machine_info);
@@ -2458,6 +2474,7 @@ static void spapr_machine_register_types(void)
     type_register_static(&spapr_machine_2_3_info);
     type_register_static(&spapr_machine_2_4_info);
     type_register_static(&spapr_machine_2_5_info);
+    type_register_static(&spapr_machine_2_6_info);
 }
 
 type_init(spapr_machine_register_types)
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] hw/ppc/spapr: Use XHCI as host controller for new spapr machines

[Thomas Huth]

The OHCI has some bugs and performance issues, so for newer
machines it's preferable to use XHCI instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c         | 15 ++++++++++++++-
 include/hw/ppc/spapr.h |  3 ++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 10b7c35..f6e6888 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1914,7 +1914,11 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     if (machine->usb) {
-        pci_create_simple(phb->bus, -1, "pci-ohci");
+        if (smc->use_ohci_by_default) {
+            pci_create_simple(phb->bus, -1, "pci-ohci");
+        } else {
+            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
+        }
 
         if (spapr->has_graphics) {
             USBBus *usb_bus = usb_bus_find(-1);
@@ -2364,9 +2368,11 @@ static void spapr_machine_2_1_class_init(ObjectClass *oc, void *data)
         SPAPR_COMPAT_2_1
         { /* end of list */ }
     };
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
 
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.1";
     mc->compat_props = compat_props;
+    smc->use_ohci_by_default = true;
 }
 
 static const TypeInfo spapr_machine_2_1_info = {
@@ -2383,9 +2389,11 @@ static void spapr_machine_2_2_class_init(ObjectClass *oc, void *data)
         { /* end of list */ }
     };
     MachineClass *mc = MACHINE_CLASS(oc);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
 
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.2";
     mc->compat_props = compat_props;
+    smc->use_ohci_by_default = true;
 }
 
 static const TypeInfo spapr_machine_2_2_info = {
@@ -2402,9 +2410,11 @@ static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
         { /* end of list */ }
     };
     MachineClass *mc = MACHINE_CLASS(oc);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
 
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
     mc->compat_props = compat_props;
+    smc->use_ohci_by_default = true;
 }
 
 static const TypeInfo spapr_machine_2_3_info = {
@@ -2421,9 +2431,11 @@ static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
         { /* end of list */ }
     };
     MachineClass *mc = MACHINE_CLASS(oc);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
 
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
     mc->compat_props = compat_props;
+    smc->use_ohci_by_default = true;
 }
 
 static const TypeInfo spapr_machine_2_4_info = {
@@ -2440,6 +2452,7 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
     mc->name = "pseries-2.5";
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
     smc->dr_lmb_enabled = true;
+    smc->use_ohci_by_default = true;
 }
 
 static const TypeInfo spapr_machine_2_5_info = {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5baa906..53af76a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -35,7 +35,8 @@ struct sPAPRMachineClass {
     MachineClass parent_class;
 
     /*< public >*/
-    bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
+    bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
+    bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
 };
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Alexander Graf]

On 27.11.15 10:32, Thomas Huth wrote:
> Add a new pseries-2.6 machine class version to make sure we can
> keep the old types compatible to previous versions of QEMU in
> later patches.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6bfb908..10b7c35 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2439,8 +2439,6 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
>  
>      mc->name = "pseries-2.5";
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
> -    mc->alias = "pseries";
> -    mc->is_default = 1;
>      smc->dr_lmb_enabled = true;
>  }
>  
> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = {
>      .class_init    = spapr_machine_2_5_class_init,
>  };
>  
> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> +
> +    mc->name = "pseries-2.6";
> +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6";
> +    mc->alias = "pseries";
> +    mc->is_default = 1;
> +    smc->dr_lmb_enabled = true;

We should probably start to follow a scheme similar to x86 where the new
machine initialization calls its predecessor (2.5 in this case) to
ensure we don't forget feature flags and quirks.

So you can either directly call spapr_machine_2_5_class_init() from
spapr_machine_2_6_class_init() or extract the quirk part
(dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in
its function name and call it from 2_5_class_init and from a "from 2.6
on" function that gets called from 2_6_class_init.


Alex

Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr: Use XHCI as host controller for new spapr machines

[Alexander Graf]

On 27.11.15 10:32, Thomas Huth wrote:
> The OHCI has some bugs and performance issues, so for newer
> machines it's preferable to use XHCI instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c         | 15 ++++++++++++++-
>  include/hw/ppc/spapr.h |  3 ++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 10b7c35..f6e6888 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1914,7 +1914,11 @@ static void ppc_spapr_init(MachineState *machine)
>      }
>  
>      if (machine->usb) {
> -        pci_create_simple(phb->bus, -1, "pci-ohci");
> +        if (smc->use_ohci_by_default) {
> +            pci_create_simple(phb->bus, -1, "pci-ohci");
> +        } else {
> +            pci_create_simple(phb->bus, -1, "nec-usb-xhci");
> +        }
>  
>          if (spapr->has_graphics) {
>              USBBus *usb_bus = usb_bus_find(-1);
> @@ -2364,9 +2368,11 @@ static void spapr_machine_2_1_class_init(ObjectClass *oc, void *data)
>          SPAPR_COMPAT_2_1
>          { /* end of list */ }
>      };
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>  
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.1";
>      mc->compat_props = compat_props;
> +    smc->use_ohci_by_default = true;
>  }
>  
>  static const TypeInfo spapr_machine_2_1_info = {
> @@ -2383,9 +2389,11 @@ static void spapr_machine_2_2_class_init(ObjectClass *oc, void *data)
>          { /* end of list */ }
>      };
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>  
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.2";
>      mc->compat_props = compat_props;
> +    smc->use_ohci_by_default = true;
>  }
>  
>  static const TypeInfo spapr_machine_2_2_info = {
> @@ -2402,9 +2410,11 @@ static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
>          { /* end of list */ }
>      };
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>  
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
>      mc->compat_props = compat_props;
> +    smc->use_ohci_by_default = true;
>  }
>  
>  static const TypeInfo spapr_machine_2_3_info = {
> @@ -2421,9 +2431,11 @@ static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
>          { /* end of list */ }
>      };
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>  
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
>      mc->compat_props = compat_props;
> +    smc->use_ohci_by_default = true;
>  }
>  
>  static const TypeInfo spapr_machine_2_4_info = {
> @@ -2440,6 +2452,7 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data)
>      mc->name = "pseries-2.5";
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5";
>      smc->dr_lmb_enabled = true;
> +    smc->use_ohci_by_default = true;
>  }
>  
>  static const TypeInfo spapr_machine_2_5_info = {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5baa906..53af76a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -35,7 +35,8 @@ struct sPAPRMachineClass {
>      MachineClass parent_class;
>  
>      /*< public >*/
> -    bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> +    bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> +    bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */

I think with the x86 scheme of always calling the predecessor quirk
functions, we're better off with a flag saying "use_xhci_by_default"
that gets set from 2.6 onwards.

I also do agree that switching to xhci is the right path forward. I
think the main reason we didn't do it from the beginning was lack of
support in SLOF ;).


Alex

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Thomas Huth]

On 27/11/15 10:55, Alexander Graf wrote:
> 
> On 27.11.15 10:32, Thomas Huth wrote:
>> Add a new pseries-2.6 machine class version to make sure we can
>> keep the old types compatible to previous versions of QEMU in
>> later patches.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 6bfb908..10b7c35 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = {
>>      .class_init    = spapr_machine_2_5_class_init,
>>  };
>>  
>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>> +
>> +    mc->name = "pseries-2.6";
>> +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6";
>> +    mc->alias = "pseries";
>> +    mc->is_default = 1;
>> +    smc->dr_lmb_enabled = true;
> 
> We should probably start to follow a scheme similar to x86 where the new
> machine initialization calls its predecessor (2.5 in this case) to
> ensure we don't forget feature flags and quirks.
> 
> So you can either directly call spapr_machine_2_5_class_init() from
> spapr_machine_2_6_class_init() or extract the quirk part
> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in
> its function name and call it from 2_5_class_init and from a "from 2.6
> on" function that gets called from 2_6_class_init.

I like the idea of calling the functions in a chain. However, the i386
people seem to do it the other way round, for example
pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options().
That of course works, too, but it sounds a little bit cumbersome to me,
since when introducing a new machine class version, you do not only have
to introduce a function for the new class anymore, but also you have to
update the previous version to change the behavior that has been
introduced by the new function (see commit 87e896abe6d926 for example).

Thus here's a question for the x86 people: Was this order done on
purpose and if so, why? Or has it just grown historically that way and
would it maybe make sense to change the order to the IMHO more intuitive
way, so that the newer machine setup function calls the older one
instead of the other way round?

Next question is of course: What do we do in sPAPR land? Go the x86 way
or do it the "big endian" way ;-) and do it the other way round?

 Thomas

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Eduardo Habkost]

On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote:
> On 27/11/15 10:55, Alexander Graf wrote:
> > 
> > On 27.11.15 10:32, Thomas Huth wrote:
> >> Add a new pseries-2.6 machine class version to make sure we can
> >> keep the old types compatible to previous versions of QEMU in
> >> later patches.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 21 +++++++++++++++++++--
> >>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 6bfb908..10b7c35 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = {
> >>      .class_init    = spapr_machine_2_5_class_init,
> >>  };
> >>  
> >> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    MachineClass *mc = MACHINE_CLASS(oc);
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> >> +
> >> +    mc->name = "pseries-2.6";
> >> +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6";
> >> +    mc->alias = "pseries";
> >> +    mc->is_default = 1;
> >> +    smc->dr_lmb_enabled = true;
> > 
> > We should probably start to follow a scheme similar to x86 where the new
> > machine initialization calls its predecessor (2.5 in this case) to
> > ensure we don't forget feature flags and quirks.
> > 
> > So you can either directly call spapr_machine_2_5_class_init() from
> > spapr_machine_2_6_class_init() or extract the quirk part
> > (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in
> > its function name and call it from 2_5_class_init and from a "from 2.6
> > on" function that gets called from 2_6_class_init.
> 
> I like the idea of calling the functions in a chain. However, the i386
> people seem to do it the other way round, for example
> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options().
> That of course works, too, but it sounds a little bit cumbersome to me,
> since when introducing a new machine class version, you do not only have
> to introduce a function for the new class anymore, but also you have to
> update the previous version to change the behavior that has been
> introduced by the new function (see commit 87e896abe6d926 for example).

The alias/is_default changes are only needed because we don't
have a generic class alias system (yet), that would allow us to
declare the "pc" alias and a default machine outside the
machine_options() function. I agree it's cumbersome.

commit 87e896abe6d926 has the extra broken_reserved_end change
because for some reason we decided to add the broken_reserved_end
quirk to pc-2.4 before we even introduced pc-2.5. That was an
exception. The common case is to add the pc-2.4 quirks only after
we added a pc-2.5 machine.

The patch that adds pc-1.6, for example, looks like this:

  -static void pc_i440fx_2_5_machine_options(MachineClass *m)
  +static void pc_i440fx_2_6_machine_options(MachineClass *m)
   {
       pc_i440fx_machine_options(m);
       m->alias = "pc";
       m->is_default = 1;
   }
   
  +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
  +                      pc_i440fx_2_6_machine_options);
  +
  +static void pc_i440fx_2_5_machine_options(MachineClass *m)
  +{
  +    pc_i440fx_2_6_machine_options(m);
  +    m->alias = NULL;
  +    m->is_default = 0;
  +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
  +}

Except for the alias/is_default stuff, it looks very simple to
me.

That said, I don't understand what you would suggest as
alternative. Let's use pc-1.7 and pc-1.6 as examples:

static void pc_compat_1_7(MachineState *machine)
{
    pc_compat_2_0(machine);
    smbios_defaults = false;
    gigabyte_align = false;
    option_rom_has_mr = true;
    legacy_acpi_table_size = 6414;
    x86_cpu_change_kvm_default("x2apic", NULL);
}

static void pc_compat_1_6(MachineState *machine)
{
    pc_compat_1_7(machine);
    rom_file_has_mr = false;
    has_acpi_build = false;
}

pc-1.7 and older need the smbios_defaults/gigabyte_align/
option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0
and later don't need those quirks. How exactly would you make
pc-1.6 and older inherit the quirks from pc-1.7, if not by
reusing pc_compat_1_7() inside pc_compat_1_6()?

(I am showing pc_compat_*() instead of *_machine_options(),
because we're still moving compat stuff from pc_compat_*() to
*_machine_options() functions. But the same questions apply once
we move the compat code above to *_machine_options() functions).

What's the alternative you propose?


> 
> Thus here's a question for the x86 people: Was this order done on
> purpose and if so, why? Or has it just grown historically that way and
> would it maybe make sense to change the order to the IMHO more intuitive
> way, so that the newer machine setup function calls the older one
> instead of the other way round?

Why would we ever want a new machine-type call the older one? Why
would a newer machine-type inherit the quirks from the older
machine-types? The newer machine-types need less quirks than the
older ones, not more quirks.

> 
> Next question is of course: What do we do in sPAPR land? Go the x86 way
> or do it the "big endian" way ;-) and do it the other way round?

If you decide to do it in a different way, I am curious to see
how you plan to make it work.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Thomas Huth]

On 27/11/15 18:56, Eduardo Habkost wrote:
> On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote:
>> On 27/11/15 10:55, Alexander Graf wrote:
>>>
>>> On 27.11.15 10:32, Thomas Huth wrote:
>>>> Add a new pseries-2.6 machine class version to make sure we can
>>>> keep the old types compatible to previous versions of QEMU in
>>>> later patches.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  hw/ppc/spapr.c | 21 +++++++++++++++++++--
>>>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 6bfb908..10b7c35 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = {
>>>>      .class_init    = spapr_machine_2_5_class_init,
>>>>  };
>>>>  
>>>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data)
>>>> +{
>>>> +    MachineClass *mc = MACHINE_CLASS(oc);
>>>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
>>>> +
>>>> +    mc->name = "pseries-2.6";
>>>> +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6";
>>>> +    mc->alias = "pseries";
>>>> +    mc->is_default = 1;
>>>> +    smc->dr_lmb_enabled = true;
>>>
>>> We should probably start to follow a scheme similar to x86 where the new
>>> machine initialization calls its predecessor (2.5 in this case) to
>>> ensure we don't forget feature flags and quirks.
>>>
>>> So you can either directly call spapr_machine_2_5_class_init() from
>>> spapr_machine_2_6_class_init() or extract the quirk part
>>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in
>>> its function name and call it from 2_5_class_init and from a "from 2.6
>>> on" function that gets called from 2_6_class_init.
>>
>> I like the idea of calling the functions in a chain. However, the i386
>> people seem to do it the other way round, for example
>> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options().
>> That of course works, too, but it sounds a little bit cumbersome to me,
>> since when introducing a new machine class version, you do not only have
>> to introduce a function for the new class anymore, but also you have to
>> update the previous version to change the behavior that has been
>> introduced by the new function (see commit 87e896abe6d926 for example).
> 
> The alias/is_default changes are only needed because we don't
> have a generic class alias system (yet), that would allow us to
> declare the "pc" alias and a default machine outside the
> machine_options() function. I agree it's cumbersome.
> 
> commit 87e896abe6d926 has the extra broken_reserved_end change
> because for some reason we decided to add the broken_reserved_end
> quirk to pc-2.4 before we even introduced pc-2.5. That was an
> exception. The common case is to add the pc-2.4 quirks only after
> we added a pc-2.5 machine.
> 
> The patch that adds pc-1.6, for example, looks like this:
> 
>   -static void pc_i440fx_2_5_machine_options(MachineClass *m)
>   +static void pc_i440fx_2_6_machine_options(MachineClass *m)
>    {
>        pc_i440fx_machine_options(m);
>        m->alias = "pc";
>        m->is_default = 1;
>    }
>    
>   +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
>   +                      pc_i440fx_2_6_machine_options);
>   +
>   +static void pc_i440fx_2_5_machine_options(MachineClass *m)
>   +{
>   +    pc_i440fx_2_6_machine_options(m);
>   +    m->alias = NULL;
>   +    m->is_default = 0;
>   +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
>   +}
> 
> Except for the alias/is_default stuff, it looks very simple to
> me.
> 
> That said, I don't understand what you would suggest as
> alternative. Let's use pc-1.7 and pc-1.6 as examples:
> 
> static void pc_compat_1_7(MachineState *machine)
> {
>     pc_compat_2_0(machine);
>     smbios_defaults = false;
>     gigabyte_align = false;
>     option_rom_has_mr = true;
>     legacy_acpi_table_size = 6414;
>     x86_cpu_change_kvm_default("x2apic", NULL);
> }
> 
> static void pc_compat_1_6(MachineState *machine)
> {
>     pc_compat_1_7(machine);
>     rom_file_has_mr = false;
>     has_acpi_build = false;
> }
> 
> pc-1.7 and older need the smbios_defaults/gigabyte_align/
> option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0
> and later don't need those quirks. How exactly would you make
> pc-1.6 and older inherit the quirks from pc-1.7, if not by
> reusing pc_compat_1_7() inside pc_compat_1_6()?
> 
> (I am showing pc_compat_*() instead of *_machine_options(),
> because we're still moving compat stuff from pc_compat_*() to
> *_machine_options() functions. But the same questions apply once
> we move the compat code above to *_machine_options() functions).
> 
> What's the alternative you propose?

The quirk would have be set to false in the oldest machine instead,
something like:

static void pc_compat_1_7(MachineState *machine)
{
    pc_compat_1_6(machine);
    rom_file_has_mr = true;
    has_acpi_build = true;
    ...
}

static void pc_compat_1_6(MachineState *machine)
{
    pc_compat_1_5(machine);
}

...

static void pc_compat_0_13(MachineState *machine)
{
    rom_file_has_mr = false;
    has_acpi_build = false;
}

And since "false" should also be the default for these variables, they
also could be omitted there and it would be sufficient to set
"rom_file_has_mr = true" and "has_acpi_build = true" once in the
pc_compat_1_7() function.
IMHO that should work fine, too, but maybe I just miss a point since I'm
quite new to these compatibility management stuff...

 Thomas

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Eduardo Habkost]

On Fri, Nov 27, 2015 at 11:15:10PM +0100, Thomas Huth wrote:
> On 27/11/15 18:56, Eduardo Habkost wrote:
> > On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote:
> >> On 27/11/15 10:55, Alexander Graf wrote:
> >>>
> >>> On 27.11.15 10:32, Thomas Huth wrote:
> >>>> Add a new pseries-2.6 machine class version to make sure we can
> >>>> keep the old types compatible to previous versions of QEMU in
> >>>> later patches.
> >>>>
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>> ---
> >>>>  hw/ppc/spapr.c | 21 +++++++++++++++++++--
> >>>>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 6bfb908..10b7c35 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = {
> >>>>      .class_init    = spapr_machine_2_5_class_init,
> >>>>  };
> >>>>  
> >>>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data)
> >>>> +{
> >>>> +    MachineClass *mc = MACHINE_CLASS(oc);
> >>>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> >>>> +
> >>>> +    mc->name = "pseries-2.6";
> >>>> +    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6";
> >>>> +    mc->alias = "pseries";
> >>>> +    mc->is_default = 1;
> >>>> +    smc->dr_lmb_enabled = true;
> >>>
> >>> We should probably start to follow a scheme similar to x86 where the new
> >>> machine initialization calls its predecessor (2.5 in this case) to
> >>> ensure we don't forget feature flags and quirks.
> >>>
> >>> So you can either directly call spapr_machine_2_5_class_init() from
> >>> spapr_machine_2_6_class_init() or extract the quirk part
> >>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in
> >>> its function name and call it from 2_5_class_init and from a "from 2.6
> >>> on" function that gets called from 2_6_class_init.
> >>
> >> I like the idea of calling the functions in a chain. However, the i386
> >> people seem to do it the other way round, for example
> >> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options().
> >> That of course works, too, but it sounds a little bit cumbersome to me,
> >> since when introducing a new machine class version, you do not only have
> >> to introduce a function for the new class anymore, but also you have to
> >> update the previous version to change the behavior that has been
> >> introduced by the new function (see commit 87e896abe6d926 for example).
> > 
> > The alias/is_default changes are only needed because we don't
> > have a generic class alias system (yet), that would allow us to
> > declare the "pc" alias and a default machine outside the
> > machine_options() function. I agree it's cumbersome.
> > 
> > commit 87e896abe6d926 has the extra broken_reserved_end change
> > because for some reason we decided to add the broken_reserved_end
> > quirk to pc-2.4 before we even introduced pc-2.5. That was an
> > exception. The common case is to add the pc-2.4 quirks only after
> > we added a pc-2.5 machine.
> > 
> > The patch that adds pc-1.6, for example, looks like this:
> > 
> >   -static void pc_i440fx_2_5_machine_options(MachineClass *m)
> >   +static void pc_i440fx_2_6_machine_options(MachineClass *m)
> >    {
> >        pc_i440fx_machine_options(m);
> >        m->alias = "pc";
> >        m->is_default = 1;
> >    }
> >    
> >   +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
> >   +                      pc_i440fx_2_6_machine_options);
> >   +
> >   +static void pc_i440fx_2_5_machine_options(MachineClass *m)
> >   +{
> >   +    pc_i440fx_2_6_machine_options(m);
> >   +    m->alias = NULL;
> >   +    m->is_default = 0;
> >   +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
> >   +}
> > 
> > Except for the alias/is_default stuff, it looks very simple to
> > me.
> > 
> > That said, I don't understand what you would suggest as
> > alternative. Let's use pc-1.7 and pc-1.6 as examples:
> > 
> > static void pc_compat_1_7(MachineState *machine)
> > {
> >     pc_compat_2_0(machine);
> >     smbios_defaults = false;
> >     gigabyte_align = false;
> >     option_rom_has_mr = true;
> >     legacy_acpi_table_size = 6414;
> >     x86_cpu_change_kvm_default("x2apic", NULL);
> > }
> > 
> > static void pc_compat_1_6(MachineState *machine)
> > {
> >     pc_compat_1_7(machine);
> >     rom_file_has_mr = false;
> >     has_acpi_build = false;
> > }
> > 
> > pc-1.7 and older need the smbios_defaults/gigabyte_align/
> > option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0
> > and later don't need those quirks. How exactly would you make
> > pc-1.6 and older inherit the quirks from pc-1.7, if not by
> > reusing pc_compat_1_7() inside pc_compat_1_6()?
> > 
> > (I am showing pc_compat_*() instead of *_machine_options(),
> > because we're still moving compat stuff from pc_compat_*() to
> > *_machine_options() functions. But the same questions apply once
> > we move the compat code above to *_machine_options() functions).
> > 
> > What's the alternative you propose?
> 
> The quirk would have be set to false in the oldest machine instead,
> something like:
> 
> static void pc_compat_1_7(MachineState *machine)
> {
>     pc_compat_1_6(machine);
>     rom_file_has_mr = true;
>     has_acpi_build = true;
>     ...
> }
> 
> static void pc_compat_1_6(MachineState *machine)
> {
>     pc_compat_1_5(machine);
> }
> 
> ...
> 
> static void pc_compat_0_13(MachineState *machine)
> {
>     rom_file_has_mr = false;
>     has_acpi_build = false;
> }
> 
> And since "false" should also be the default for these variables, they
> also could be omitted there and it would be sufficient to set
> "rom_file_has_mr = true" and "has_acpi_build = true" once in the
> pc_compat_1_7() function.
> IMHO that should work fine, too, but maybe I just miss a point since I'm
> quite new to these compatibility management stuff...

I think I see what you mean. It would work, but I see two issues:

1) The defaults in the QEMU hardware emulation code is the more
recently introduced (and recommended) behavior, not the oldest
legacy behavior. So the oldest machine-types would really need to
set the variables to false.

2) I prefer to make the newer machine-types' code simpler and
with less dependencies. The existing approch moves the complexity
to the older machine-types, your suggestion moves the complexity
to the newer ones. Any mistake done in the old (and probably
unmaintained and unused) machine-types would affect all the newer
ones. Also, this prevents us from easily deleting very old
machine-types we don't want to support anymore.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Thomas Huth]

On 28/11/15 16:09, Eduardo Habkost wrote:
> On Fri, Nov 27, 2015 at 11:15:10PM +0100, Thomas Huth wrote:
>> On 27/11/15 18:56, Eduardo Habkost wrote:
>>> On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote:
>>>> On 27/11/15 10:55, Alexander Graf wrote:
>>>>>
>>>>> On 27.11.15 10:32, Thomas Huth wrote:
>>>>>> Add a new pseries-2.6 machine class version to make sure we can
>>>>>> keep the old types compatible to previous versions of QEMU in
>>>>>> later patches.
[...]
>>>>> We should probably start to follow a scheme similar to x86 where the new
>>>>> machine initialization calls its predecessor (2.5 in this case) to
>>>>> ensure we don't forget feature flags and quirks.
>>>>>
>>>>> So you can either directly call spapr_machine_2_5_class_init() from
>>>>> spapr_machine_2_6_class_init() or extract the quirk part
>>>>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in
>>>>> its function name and call it from 2_5_class_init and from a "from 2.6
>>>>> on" function that gets called from 2_6_class_init.
>>>>
>>>> I like the idea of calling the functions in a chain. However, the i386
>>>> people seem to do it the other way round, for example
>>>> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options().
>>>> That of course works, too, but it sounds a little bit cumbersome to me,
>>>> since when introducing a new machine class version, you do not only have
>>>> to introduce a function for the new class anymore, but also you have to
>>>> update the previous version to change the behavior that has been
>>>> introduced by the new function (see commit 87e896abe6d926 for example).
>>>
>>> The alias/is_default changes are only needed because we don't
>>> have a generic class alias system (yet), that would allow us to
>>> declare the "pc" alias and a default machine outside the
>>> machine_options() function. I agree it's cumbersome.
>>>
>>> commit 87e896abe6d926 has the extra broken_reserved_end change
>>> because for some reason we decided to add the broken_reserved_end
>>> quirk to pc-2.4 before we even introduced pc-2.5. That was an
>>> exception. The common case is to add the pc-2.4 quirks only after
>>> we added a pc-2.5 machine.
>>>
>>> The patch that adds pc-1.6, for example, looks like this:
>>>
>>>   -static void pc_i440fx_2_5_machine_options(MachineClass *m)
>>>   +static void pc_i440fx_2_6_machine_options(MachineClass *m)
>>>    {
>>>        pc_i440fx_machine_options(m);
>>>        m->alias = "pc";
>>>        m->is_default = 1;
>>>    }
>>>    
>>>   +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL,
>>>   +                      pc_i440fx_2_6_machine_options);
>>>   +
>>>   +static void pc_i440fx_2_5_machine_options(MachineClass *m)
>>>   +{
>>>   +    pc_i440fx_2_6_machine_options(m);
>>>   +    m->alias = NULL;
>>>   +    m->is_default = 0;
>>>   +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
>>>   +}
>>>
>>> Except for the alias/is_default stuff, it looks very simple to
>>> me.
>>>
>>> That said, I don't understand what you would suggest as
>>> alternative. Let's use pc-1.7 and pc-1.6 as examples:
>>>
>>> static void pc_compat_1_7(MachineState *machine)
>>> {
>>>     pc_compat_2_0(machine);
>>>     smbios_defaults = false;
>>>     gigabyte_align = false;
>>>     option_rom_has_mr = true;
>>>     legacy_acpi_table_size = 6414;
>>>     x86_cpu_change_kvm_default("x2apic", NULL);
>>> }
>>>
>>> static void pc_compat_1_6(MachineState *machine)
>>> {
>>>     pc_compat_1_7(machine);
>>>     rom_file_has_mr = false;
>>>     has_acpi_build = false;
>>> }
>>>
>>> pc-1.7 and older need the smbios_defaults/gigabyte_align/
>>> option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0
>>> and later don't need those quirks. How exactly would you make
>>> pc-1.6 and older inherit the quirks from pc-1.7, if not by
>>> reusing pc_compat_1_7() inside pc_compat_1_6()?
>>>
>>> (I am showing pc_compat_*() instead of *_machine_options(),
>>> because we're still moving compat stuff from pc_compat_*() to
>>> *_machine_options() functions. But the same questions apply once
>>> we move the compat code above to *_machine_options() functions).
>>>
>>> What's the alternative you propose?
>>
>> The quirk would have be set to false in the oldest machine instead,
>> something like:
>>
>> static void pc_compat_1_7(MachineState *machine)
>> {
>>     pc_compat_1_6(machine);
>>     rom_file_has_mr = true;
>>     has_acpi_build = true;
>>     ...
>> }
>>
>> static void pc_compat_1_6(MachineState *machine)
>> {
>>     pc_compat_1_5(machine);
>> }
>>
>> ...
>>
>> static void pc_compat_0_13(MachineState *machine)
>> {
>>     rom_file_has_mr = false;
>>     has_acpi_build = false;
>> }
>>
>> And since "false" should also be the default for these variables, they
>> also could be omitted there and it would be sufficient to set
>> "rom_file_has_mr = true" and "has_acpi_build = true" once in the
>> pc_compat_1_7() function.
>> IMHO that should work fine, too, but maybe I just miss a point since I'm
>> quite new to these compatibility management stuff...
> 
> I think I see what you mean. It would work, but I see two issues:
> 
> 1) The defaults in the QEMU hardware emulation code is the more
> recently introduced (and recommended) behavior, not the oldest
> legacy behavior. So the oldest machine-types would really need to
> set the variables to false.
> 
> 2) I prefer to make the newer machine-types' code simpler and
> with less dependencies. The existing approch moves the complexity
> to the older machine-types, your suggestion moves the complexity
> to the newer ones. Any mistake done in the old (and probably
> unmaintained and unused) machine-types would affect all the newer
> ones. Also, this prevents us from easily deleting very old
> machine-types we don't want to support anymore.

Ok, thanks a lot for the explanation, that makes sense! So I think we'll
do it the same way in the pseries machine, too ... and apparently David
has even already posted a patch to do so :-)

 Thomas