[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Stefano,

On 28/06/2021 22:40, Stefano Stabellini wrote:
On Mon, 28 Jun 2021, Julien Grall wrote:
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?

I am not aware of any firmware (Xilinx or non-Xilinx) with SMCs call
not SMCCC compliant.

Here an example that doesn't look to follow the SMCCC spec:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-omap2/omap-smc.S


That said, the most important thing is that we handle PSCI correctly,
right?
Most modern Linux version will use HVC rather than SMC for the PSCI... What we want to avoid breaking is user of a TEE (e.g. OP-TEE) and/or spectre mitigations.

After that, if we forward all calls to the firmware we should be
OK. From a calling convention perspective, it would only break if we
don't forward enough parameters on registers or save enough return
values on registers.

Then there are the problems with memory addresses you wrote below.

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

On Xilinx, we have so many SMCs that it would be difficult to figure it
out from the boot log alone.

Xen will print the function ID on the console:

gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %#x\n", funcid);

Isn't it enough to know which SMC call fails or look at the documentation?

In the sense that it is unfortunately
"normal" for one or two SMCs to fail (even without Xen!) But that could
be a Xilinx-only issue.
Right, then I don't quite see how the debugging option will help here. Or are you saying the return failure is different and therefore Linux will not be able to cope when running on 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...

The problem is that it isn't always the same person doing the work.

If it is me working on a new release or a new platform, the command line
option wouldn't help me much. In fact, it might even be faster for me to
add "goto forward_to_fw" and rebuild. I am happy with that.

The issue is when there is a bug reported by a customer or by a user on
the mailing list. In that case, it is very useful to ask them to run a
little experiment to narrow down the possibilities, and it i easier to
ask them to add a command line option than to apply a patch. If
passthrough is involved, then we need to ask the user to forward all
SMCs, not just Dom0. If passthrough is not involved, then forwarding
only Dom0 calls is fine.

The problem is the OS may be able to cope with all the SMCs returning "not handled" up to the one you are interested in. Now if you pass the option to Xen, the firmware will handle all the SMCs and this may change the behavior of the OS. IOW, you are potentially adding more potential reasons for bugs.

I mention one OS here, but the problem is exactly the same with multiple one. You may have a working dom0 OS but non-working guest OS. Enabling the option may break your dom0.



That said, I am not so sure we want this patch upstream: I think it
would benefit Xilinx users and a recent request from Alex made me think
that it would benefit other platforms too, but maybe the benefits on
other platforms are not enough to introduce an option like this, which
could easily break things. >
So I am happy to follow your preference:

1) I can drop the patch
2) forward platform_smc only for dom0
3) forward platform_smc for all domains
2) would be acceptable. Although, this doesn't entirely cover the concern I wrote above. One possibility would be to allow the user to list the platform SMCs they want to passthrough.

This could be easily implemented using the rangeset framework in Xen.

Anyway, for now I would be OK with just forwarding all unhandled platform SMCs with some documentation explaining the limitations of the option.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.