Re: [PATCH 07/18] xen/arm: Introduce a command line parameter for SErrors/Aborts

[Wei Chen <Wei.Chen@arm.com>]

Hi Stefano,

On 2017/3/15 8:45, Stefano Stabellini wrote:
> On Mon, 13 Mar 2017, Wei Chen wrote:
>> In order to distinguish guest-generated SErrors from hypervisor-generated
>> SErrors. We have to place SError checking code in every EL1 -> EL2 paths.
>          ^ remove .
>

Ok.

>> That will be an overhead on entries caused by dsb/isb.
>>
>> But not all platforms want to categorize the SErrors. For example, a host
>> that is running with trusted guests. The administrator can confirm that
>> all guests that are running on the host will not trigger such SErrors. In
>> this user scene, we should provide some options to administrator to avoid
>        ^use-case
>

Ok.

>
>> categorizing the SErrors. And then reduce the overhead of dsb/isb.
>                ^ remove   ^ remove
>

Ok.

>
>> We provided following 3 options to administrator to determine how to handle
>> the SErrors:
>>
>> * `diverse`:
>>   The hypervisor will distinguish guest SErrors from hypervisor SErrors.
>>   The guest generated SErrors will be forwarded to guests, the hypervisor
>>   generated SErrors will cause the whole system crash.
>>   It requires:
>>   1. Place dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors
>>      correctly.
>>   2. Place dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
>>      SErrors to guests.
>>   3. Place dsb/isb in context switch to isolate the SErrors between 2 vCPUs.
>>
>> * `forward`:
>>   The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
>>   All SErrors will be forwarded to guests, except the SErrors generated when
>>   idle vCPU is running. The idle domain doesn't have the ability to hanle the
>>   SErrors, so we have to crash the whole system when we get SErros with idle
>>   vCPU. This option will avoid most overhead of the dsb/isb, except the dsb/isb
>>   in context switch which is used to isolate the SErrors between 2 vCPUs.
>>
>> * `panic`:
>>   The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
>>   All SErrors will crash the whole system. This option will avoid all overhead
>>   of the dsb/isb.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>
>> ---
>> About adding dsb/isb to prevent slipping Hypervisor SErrors to Guests if the
>> selected option is "diverse". Some Hypervisor SErrors could not be avoid by
>> software, for example ECC Error. But I don't know whether it's worth adding
>> the overhead by default.
>> ---
>>  docs/misc/xen-command-line.markdown | 44 +++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/traps.c                | 19 ++++++++++++++++
>>  2 files changed, 63 insertions(+)
>>
>> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
>> index 4daf5b5..79554ce 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1481,6 +1481,50 @@ enabling more sockets and cores to go into deeper sleep states.
>>
>>  Set the serial transmit buffer size.
>>
>> +### serrors (ARM)
>> +> `= diverse | forward | panic`
>> +
>> +> Default: `diverse`
>> +
>> +This parameter is provided to administrator to determine how to handle the
>> +SErrors.
>
>   This parameter is provided to administrators to determine how the
>   hypervisors handle SErrors.
>

Thanks for reorganization.

>
>> +In order to distinguish guest-generated SErrors from hypervisor-generated
>> +SErrors. We have to place SError checking code in every EL1 -> EL2 paths.
>           ^remove .
>

Ok.

>> +That will be an overhead on entries caused by dsb/isb. But not all platforms
>
>    That will cause overhead on entries due to dsb/isb. However, not all platforms
>

Thanks.

>> +need to categorize the SErrors. For example, a host that is running with
>                        ^ remove the
>

Ok.

>> +trusted guests. The administrator can confirm that all guests that are
>> +running on the host will not trigger such SErrors. In this case, the
>> +administrator can use this parameter to skip categorizing the SErrors. And
>                                                              ^ remove   ^ remove
>

Ok.

>> +reduce the overhead of dsb/isb.
>> +
>> +We provided following 3 options to administrator to determine how to handle
>               ^ the following         ^ administrators
>

Ok.

>> +the SErrors:
>    ^ remove the

Ok.

>
>
>> +
>> +* `diverse`:
>> +  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
>> +  The guest generated SErrors will be forwarded to guests, the hypervisor
>> +  generated SErrors will cause the whole system crash.
>                                                   ^ to crash
>

Ok.

>> +  It requires:
>> +  1. Place dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors
>         ^ remove Place

Ok.

>> +     correctly.
>> +  2. Place dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
>         ^ remove Place

Ok.

>> +     SErrors to guests.
>> +  3. Place dsb/isb in context switch to isolate the SErrors between 2 vCPUs.
>         ^ remove Place                             ^ remove the

Ok.

>> +
>> +* `forward`:
>> +  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
>> +  All SErrors will be forwarded to guests, except the SErrors generated when
>> +  idle vCPU is running. The idle domain doesn't have the ability to hanle the
>      ^ the idle                                                        ^ handle ^ remove the
>

Ok.

>> +  SErrors, so we have to crash the whole system when we get SErros with idle
>                                                                           ^ the idle
>

Ok.

>> +  vCPU. This option will avoid most overhead of the dsb/isb, except the dsb/isb
>> +  in context switch which is used to isolate the SErrors between 2 vCPUs.
>> +
>> +* `panic`:
>> +  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
>> +  All SErrors will crash the whole system. This option will avoid all overhead
>> +  of the dsb/isb.
>
>     of the dsb/isb pairs.
>

Ok.

> Please make these changes to the commit message too, when applicable.
> With these changes:
>

I will do it in next version.

> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
>
>>  ### smap
>>  > `= <boolean> | hvm`
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index e425832..5e31699 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -115,6 +115,25 @@ static void __init parse_vwfi(const char *s)
>>  }
>>  custom_param("vwfi", parse_vwfi);
>>
>> +static enum {
>> +    SERRORS_DIVERSE,
>> +    SERRORS_FORWARD,
>> +    SERRORS_PANIC,
>> +} serrors_op;
>> +
>> +static void __init parse_serrors_behavior(const char *str)
>> +{
>> +    if ( !strcmp(str, "forward") )
>> +        serrors_op = SERRORS_FORWARD;
>> +    else if ( !strcmp(str, "panic") )
>> +        serrors_op = SERRORS_PANIC;
>> +    else
>> +        serrors_op = SERRORS_DIVERSE;
>> +
>> +    return;
>> +}
>> +custom_param("serrors", parse_serrors_behavior);
>> +
>>  register_t get_default_hcr_flags(void)
>>  {
>>      return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
>>
>


-- 
Regards,
Wei Chen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel