Re: [PATCH] hw: add compat machines for 5.1

[Cornelia Huck]

On Mon, 27 Apr 2020 11:58:28 +0200
Auger Eric <address@hidden> wrote:

> Hi Connie,
> 
> On 4/27/20 11:06 AM, Cornelia Huck wrote:
> > On Mon, 27 Apr 2020 09:43:33 +0200
> > Auger Eric <address@hidden> wrote:
> >   
> >> Hi Connie,
> >>
> >> On 4/24/20 11:03 AM, Cornelia Huck wrote:  
> >>> Add 5.1 machine types for arm/i440fx/q35/s390x/spapr.
> >>>
> >>> Signed-off-by: Cornelia Huck <address@hidden>
> >>> ---
> >>>
> >>> Still keeping the default cpu model version on x86 at v1.
> >>>
> >>> I'll queue this to my s390-next branch, as I'm planning to send a pull
> >>> req as soon as 5.0 is out; if someone else wants to queue this, we'll
> >>> figure it out :)
> >>>
> >>> ---
> >>>  hw/arm/virt.c              |  9 ++++++++-
> >>>  hw/core/machine.c          |  3 +++
> >>>  hw/i386/pc.c               |  3 +++
> >>>  hw/i386/pc_piix.c          | 14 +++++++++++++-
> >>>  hw/i386/pc_q35.c           | 13 ++++++++++++-
> >>>  hw/ppc/spapr.c             | 15 +++++++++++++--
> >>>  hw/s390x/s390-virtio-ccw.c | 14 +++++++++++++-
> >>>  include/hw/boards.h        |  3 +++
> >>>  include/hw/i386/pc.h       |  3 +++
> >>>  9 files changed, 71 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >>> index 7dc96abf72cf..5e84c09402dd 100644
> >>> --- a/hw/arm/virt.c
> >>> +++ b/hw/arm/virt.c
> >>> @@ -2309,15 +2309,22 @@ static void machvirt_machine_init(void)
> >>>  }
> >>>  type_init(machvirt_machine_init);
> >>>  
> >>> +static void virt_machine_5_1_options(MachineClass *mc)
> >>> +{
> >>> +}
> >>> +DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> >>> +
> >>>  static void virt_machine_5_0_options(MachineClass *mc)
> >>>  {
> >>>      static GlobalProperty compat[] = {
> >>>          { TYPE_TPM_TIS_SYSBUS, "ppi", "false" },
> >>>      };
> >>>  
> >>> +    virt_machine_5_1_options(mc);
> >>> +    compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> >>>      compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));    
> >> As spotted by patchew we need { TYPE_TPM_TIS_SYSBUS, "ppi", "false" } to
> >> be applied for all machine types 5_0 onwards as our tpm-tis-device does
> >> not support PPI. So I guess we need to move compat[] declaration outside
> >> of virt_machine_5_0_options and call
> >> "compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));"
> >> for each  virt_machine_5_*_options? Or is a misuse of compats?  
> > 
> > Ah, that was the error (I could not really make sense of it).
> > 
> > So, if I understand it correctly, ppi needs to be disabled for all virt
> > machines.  
> ppi property of tpm-tis-sysbus device needs to be disabled for all virt
> machines whose version >= 5.0. tpm-tis-sysbus device is supported from
> 5.0 onwards.

It would probably be ok to set for all versions; if the device is not
available, a compat entry should not hurt.

> 
> Looking again at the code, I should rather set the default value of the
> ppi property to false in tpm_tis_sysbus_properties. Before we shared the
> same device with x86, hence the compat but now we have a separate one. I
> guess that should work directly without the pain of that compat :-( Let
> me check ...

Yes, that seems like a better solution.

> 
> Thanks
> 
> Eric
>  I think the correct way to do this would be to add the
> > "compat" prop in virt_machine_class_init() (even if it is not strictly
> > compat, as it always needs to be disabled). It can be removed there and
> > added to a compat machine should support be added in the future.
> > 
> > Or does anyone have a better idea?
> > 
> >