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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


> 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. 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. But I can also see the value in
having a command line option to forward_to_fw all platform SMCs from
dom0 only and maybe a separate patch later to add a per-domain option to
forward_to_fw platform SMCs for specific domains if needed. That would
be safer and more flexible but a little more work.