RE: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue

[Peng Fan <peng.fan@nxp.com>]

> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue
> 
> Hi Julien,
> 
> On 1/31/24 13:22, Julien Grall wrote:
> > Hi,
> >
> > On 31/01/2024 11:50, John Ernberg wrote:
> >> When using Linux for dom0 there are a bunch of drivers that need to
> >> do SMC SIP calls into the PSCI provider to enable certain hardware
> >> bits like the watchdog.
> >
> > Do you know which protocol this is under the hood. Is this SCMI?
> 
> I think I confused myself here when I wrote the commit log.
> 
> The EL3 code in our case is ATF, and it does not appear to be SCMI, nor PSCI.
> The register usage of these SMC SIP calls are as follows:
> a0 - service
> a1 - function
> a2-a7 - args
> 
> In ATF the handler is declared as a runtime service.
> 
> Would the appropriate commmit message here be something along the lines
> of below?
> """
> When using Linux for dom0 there are a bunch of drivers that need to do
> SMC
> SIP calls into the firmware to enable certain hardware bits like the watchdog.
> """
> >
> >>
> >> Provide a basic platform glue that implements the needed SMC forwarding.
> >>
> >> Signed-off-by: John Ernberg <john.ernberg@actia.se>
> >> ---
> >> NOTE: This is based on code found in NXP Xen tree located here:
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> >> hub.com%2Fnxp-imx%2Fimx-xen%2Fblob%2Flf-
> 5.10.y_4.13%2Fxen%2Farch%2Far
> >>
> m%2Fplatforms%2Fimx8qm.c&data=05%7C02%7Cpeng.fan%40nxp.com%7C
> 573b599a
> >>
> 4b4143ceca1d08dc2271e5be%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7
> >>
> C638423119777601548%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQI
> >>
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=ZO
> 0TXjL6
> >> g0W7TIZo8x8lTNBXEZW%2BDNcLPndWlEf5D2A%3D&reserved=0
> >
> > Anything after --- will be removed while applied to the three. I think
> > this NOTE should be written down in the commit message.
> 
> Ack.
> >
> > You also possibly want a signed-off-by from Peng as this is his code.
> 
> @Peng: May I add a sign-off from you?

Yeah. You could add my sign off.

> >
> >>
> >>   xen/arch/arm/platforms/Makefile |  1 +
> >>   xen/arch/arm/platforms/imx8qm.c | 65
> >> +++++++++++++++++++++++++++++++++
> >>   2 files changed, 66 insertions(+)
> >>   create mode 100644 xen/arch/arm/platforms/imx8qm.c
> >>
> >> diff --git a/xen/arch/arm/platforms/Makefile
> >> b/xen/arch/arm/platforms/Makefile index 8632f4115f..bec6e55d1f
> 100644
> >> --- a/xen/arch/arm/platforms/Makefile
> >> +++ b/xen/arch/arm/platforms/Makefile
> >> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
> >>   obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> >>   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> >>   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> >> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> >>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
> >>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o diff --git
> >> a/xen/arch/arm/platforms/imx8qm.c
> b/xen/arch/arm/platforms/imx8qm.c
> >> new file mode 100644 index 0000000000..a9cd9c3615
> >> --- /dev/null
> >> +++ b/xen/arch/arm/platforms/imx8qm.c
> >> @@ -0,0 +1,65 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * xen/arch/arm/platforms/imx8qm.c
> >> + *
> >> + * i.MX 8QM setup
> >> + *
> >> + * Copyright (c) 2016 Freescale Inc.
> >> + * Copyright 2018-2019 NXP
> >> + *
> >> + *
> >> + * Peng Fan <peng.fan@nxp.com>
> >> + */
> >> +
> >> +#include <asm/platform.h>
> >> +#include <asm/smccc.h>
> >> +
> >> +static const char * const imx8qm_dt_compat[] __initconst = {
> >> +    "fsl,imx8qm",
> >> +    "fsl,imx8qxp",
> >> +    NULL
> >> +};
> >> +
> >> +static bool imx8qm_smc(struct cpu_user_regs *regs) {
> >
> > Your implementation below will not only forward SMC for dom0 but also
> > for any non-trusted domains. Have you investigated that all the SIP
> > calls are safe to be called by anyone?
> 
> We use pure virtualized domUs, so we do not expect any calls to this SMC
> interface from the guest. I'll limit it to dom0.

Would you mind to share what features are supported in your DomU?

Pure virtualized, you using xen pv or virtio?

Thanks,
Peng.

> >
> > But even if we restrict to dom0, have you checked that none of the
> > SMCs use buffers?
> I haven't found any such instances in the Linux kernel where a buffer is used.
> Adding a call filtering like suggested below additions of such functions can be
> discovered and adapted for if they would show up later.
> >
> > Rather than providing a blanket forward, to me it sounds more like you
> > want to provide an allowlist of the SMCs. This is more futureproof and
> > avoid the risk to expose unsafe SMCs to any domain.
> >
> > For an example, you can have a look at the EEMI mediator for Xilinx.
> 
> Ack. Do you prefer to see only on SMCCC service level or also on function
> level? (a1 register, per description earlier)
> >
> > Cheers,
> >
> 
> Thanks! // John Ernberg