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

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

On Fri, 2 Feb 2024, Julien Grall wrote:
> On 01/02/2024 16:17, John Ernberg wrote:
> > On 2/1/24 13:20, Julien Grall wrote:
> > > 
> > > 
> > > On 31/01/2024 15:32, John Ernberg wrote:
> > > > Hi Julien,
> > > 
> > > Hi John,
> > > 
> > > > 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.
> > > > """
> > > 
> > > It reads better thanks.
> > > 
> > > [...]
> > > 
> > > > > 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)
> > > 
> > > I am not sure. It will depend on whether it is correct to expose *all*
> > > the functions within a service level and they have the same format.
> > > 
> > > If you can't guarantee that, then you will most likely need to allowlist
> > > at the function level.
> > > 
> > > Also, do you have a spec in hand that would help to understand which
> > > service/function is implemented via those SMCs?
> > 
> > I don't have the spec unfortunately, but I will add a filter on both
> > service and function for V2 and we'll take it from there.
> 
> @Peng, do you have any specification you could share? How stable is the
> interface?

Just to add some context to make the reason for the question clearer, if
we have a specification we could check the patch for correctness.
Without it, it is difficult to know if it is doing the right thing.

The other aspect is about expectation of forward and backward
compatibility. Can we guarantee that the next version of Xen and the one
after it will still work against this interface? If not, can we check
for the version of the interface before continuing? If not, can we at
least document that the interface is only known-to-work with specific
firmware versions?

This is basically just to provide the right expectations to users and
ideally to prevent a future version of Xen to break on boot silently
without information.

If we don't have a spec and we don't know if the interface is stable, I
think we should try to detect the version of the interface and print a
warning in Xen if it not a known version.