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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 25/06/2021 18:47, Stefano Stabellini wrote:
> On Fri, 25 Jun 2021, Julien Grall wrote:
> > 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.
> Yes, you are totally right. I thought about it this morning as well.
> This patch would break even PSCI :-(
>
> It would be best implemented in platform_smc as forward_to_fw (see
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c:forward_to_fw).
There is one problem though. How do you know which calling convention to 
use? IOW, will all the firmware call (in particular on older platform) 
follow the SMCCC?
> > 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?
> This is meant to help with bug triage only. There are a number of bugs
> that can happen if certain platform SMCs are intercerpted by Xen instead
> of being forwarded to the hardware.
We already print a message informating the user that the SMC call was 
trapped and terminated in Xen. So I am not entirely sure why you also 
need to passthrough all the SMC calls to triage it. You already know 
that the SMC will have to be implemented in Xen...

> I found myself having to provide a
> patch to forward_to_fw all platform SMCs as a first test to
> triage bugs a few times recently. It is never a fix, only a way to
> understand the next step of debugging. Also Alex stumbled across
> something similar on a non-Xilinx board (MacchiatoBin) so I thought it
> was time for a better debugging option.
>
> I think for debugging purposes it would be sufficient if all platform
> SMCs were forward_to_fw from all domains. Of course it is totally
> unsafe, but it is just for debugging.
In order to add a debugging option in Xen, we need to be reasonably 
confident that the option will not make more damage (I am not speaking 
about security here...) than it is actually worth it.
I can see how this helps in both your situation to boot dom0. However, I 
am not sure this can be generalized to every platform. A developper (or 
user) enabling this debugging option may end up to see corruption/hang 
because:
  1) SMC call may pass memory address. A domain would pass a guest 
physical address but the firmware will interpret as host physical 
address. This working(ish) for dom0 because both are equivalent, but for 
other domain this will break.
 2) SMC call may change the behavior of the system (i.e. turning off 
the UART)...
It would be difficult to pinpoint whether the problem is because an SMC 
(or else) without implementing each SMC call in Xen.
I don't think it is a lot of work to implement SMCs in Xen as you find 
them (sooner or later, you will have to do it anyway...). At which 
point, forwarding all the unknowns SMCs to attempt to boot further is 
probably more risky than it is worth it.
If the problem is re-building, then we could consider to provide a 
command line option to easily specify which SMC call is passthrough...
Cheers,

--
Julien Grall