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

[Auger Eric]

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.

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 ...

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?
> 
>