Re: [PATCH v18 1/6] hw/arm/virt: Introduce RAS platform version and RAS machine option

[Xiang Zheng]

On 2019/9/27 22:02, Peter Maydell wrote:
> On Fri, 6 Sep 2019 at 09:33, Xiang Zheng <address@hidden> wrote:
>>
>> From: Dongjiu Geng <address@hidden>
>>
>> Support RAS Virtualization feature since version 4.2, disable it by
>> default in the old versions. Also add a machine option which allows user
>> to enable it explicitly.
>>
>> Signed-off-by: Dongjiu Geng <address@hidden>
>> Signed-off-by: Xiang Zheng <address@hidden>
>> ---
>>  hw/arm/virt.c         | 33 +++++++++++++++++++++++++++++++++
>>  include/hw/arm/virt.h |  2 ++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d74538b021..e0451433c8 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1783,6 +1783,20 @@ static void virt_set_its(Object *obj, bool value, 
>> Error **errp)
>>      vms->its = value;
>>  }
>>
>> +static bool virt_get_ras(Object *obj, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    return vms->ras;
>> +}
>> +
>> +static void virt_set_ras(Object *obj, bool value, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    vms->ras = value;
>> +}
>> +
>>  static char *virt_get_gic_version(Object *obj, Error **errp)
>>  {
>>      VirtMachineState *vms = VIRT_MACHINE(obj);
>> @@ -2026,6 +2040,19 @@ static void virt_instance_init(Object *obj)
>>                                      "Valid values are none and smmuv3",
>>                                      NULL);
>>
>> +    if (vmc->no_ras) {
>> +        vms->ras = false;
>> +    } else {
>> +        /* Default disallows RAS instantiation */
>> +        vms->ras = false;
>> +        object_property_add_bool(obj, "ras", virt_get_ras,
>> +                                 virt_set_ras, NULL);
>> +        object_property_set_description(obj, "ras",
>> +                                        "Set on/off to enable/disable "
>> +                                        "RAS instantiation",
>> +                                        NULL);
>> +    }
> 
> For a property which is disabled by default, you don't need
> to have a separate flag in the VirtMachineClass struct.
> Those are only needed for properties where we need the old machine
> types to have the property be 'off' but new machine types
> need to default to it be 'on'. Since vms->ras is false
> by default anyway, you can just have this part:
> 
>> +        /* Default disallows RAS instantiation */
>> +        vms->ras = false;
>> +        object_property_add_bool(obj, "ras", virt_get_ras,
>> +                                 virt_set_ras, NULL);
>> +        object_property_set_description(obj, "ras",
>> +                                        "Set on/off to enable/disable "
>> +                                        "RAS instantiation",
>> +                                        NULL);
> 
> Compare the 'vms->secure' flag and associated property
> for an example of this.

Thanks for pointing it out, I will remove the no_ras in the VirtMachineClass 
struct.

> 
>>      vms->irqmap = a15irqmap;
>>
>>      virt_flash_create(vms);
>> @@ -2058,8 +2085,14 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
>>
>>  static void virt_machine_4_1_options(MachineClass *mc)
>>  {
>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>> +
>>      virt_machine_4_2_options(mc);
>>      compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
>> +    /* Disable memory recovery feature for 4.1 as RAS support was
>> +     * introduced with 4.2.
>> +     */
>> +    vmc->no_ras = true;
>>  }
>>  DEFINE_VIRT_MACHINE(4, 1)
> 
> thanks
> -- PMM
> 
> .
> 

-- 

Thanks,
Xiang