Re: [PATCH] xen/arm: add forward_smc command line option for debugging

[Julien Grall <julien@xxxxxxx>]

Hi,

On 25/06/2021 02:51, Stefano Stabellini wrote:
> It has become clear that an option to disable trapping SMC calls to Xen
> is very useful for debugging user issues.
>
> Instead of having to provide a
> patch to users every time, it would be great if we could just tell them
> to add forward_smc=true to the Xen command line.
I can understand this woud be useful to go a bit further in dom0 boot. 
But I am quite sceptical on the idea of providing an option directly in 
Xen because:
1) This breaks other SMC uses in Xen (optee, VM monitor...)
2) There are no guarantee that the SMC call will not wreck Xen. To be 
clear, I don't refer to a malicious OS here, but a normal OS that boot
3) Very likely the next steps for the user (or better call it the 
developper because that option should really not be used by a normal 
user) will be to decide whether they should modify the kernel or 
implement a mediator in Xen.
> This option is obviously unsafe and unsecure and only meant for
> debugging. Make clear in the description that if you pass
> forward_smc=true then the system is not security supported.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 3ece83a427..0833fe80fc 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2501,6 +2501,16 @@ vwfi to `native` reduces irq latency significantly. It 
> can also lead to
>   suboptimal scheduling decisions, but only when the system is
>   oversubscribed (i.e., in total there are more vCPUs than pCPUs).
>   
> +### forward_smc (arm)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +If enabled, instead of trapping firmware SMC calls to Xen, allow SMC
> +calls from VMs directly to the firmware. This option is UNSAFE and it is
> +only meant for debugging. Systems with forward_smc=true are not security
> +supported.
> +
>   ### watchdog (x86)
>   > `= force | <boolean>`
>   
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index e7384381cc..0580ac5762 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -95,11 +95,15 @@ static int __init parse_vwfi(const char *s)
>   }
>   custom_param("vwfi", parse_vwfi);
>   
> +static bool forward_smc = false;
> +boolean_param("forward_smc", forward_smc);
> +
>   register_t get_default_hcr_flags(void)
>   {
>       return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>                (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
> -             HCR_TID3|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB|HCR_TSW);
> +             (forward_smc ? 0 : HCR_TSC) |
> +             HCR_TID3|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB|HCR_TSW);
A system wide option to turn off SMC trapping is a no-go because this 
would only be usable for debugging dom0 and not a guest.
So at the minimum this should be a per-domain option. Also, I think we 
still want to integrate with the rest of the SMC users. So Xen should 
still trap the SMC and the forward should happen in vsmccc_handle_call().
This would cover my first point. For the second and third point, I still 
like to understand how this is going to help the developer to fully port 
the board/OS to Xen with this option disabled?
Cheers,

--
Julien Grall