Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Julien Grall <julien@xxxxxxx>]

Hi,

On 03/10/2024 17:40, Andrei Cherechesu wrote:
> Hi Julien,
>
> On 03/10/2024 19:07, Julien Grall wrote:
> > On 03/10/2024 16:27, Andrei Cherechesu wrote:
> > > Hi Julien,
> > Hi Andrei,
> >
> > > On 01/10/2024 12:35, Julien Grall wrote:
> > > > > , the initialization fails silently, as it's not mandatory.
> > > > > Otherwise, we get the 'arm,smc-id' DT property from the node,
> > > > > to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> > > > > are not validated, as the SMC calls are only passed through
> > > > > to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> > > > > running.
> > > > >
> > > > > Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> > > > > ---
> > > > >     xen/arch/arm/Kconfig                |  10 ++
> > > > >     xen/arch/arm/Makefile               |   1 +
> > > > >     xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
> > > > >     xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
> > > > >     4 files changed, 226 insertions(+)
> > > > >     create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
> > > > >     create mode 100644 xen/arch/arm/scmi-smc.c
> > > > >
> > > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > > index 323c967361..adf53e2de1 100644
> > > > > --- a/xen/arch/arm/Kconfig
> > > > > +++ b/xen/arch/arm/Kconfig
> > > > > @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
> > > > >           not been emulated to their complete functionality. Enabling this might
> > > > >           result in unwanted/non-spec compliant behavior.
> > > > >     +config SCMI_SMC
> > > > > +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
> > > > Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it 
> > > > is dom0 but it doesn't need to.
> > > Well, SCMI is Arm-specific and so are SMCs, but I agree to change
> > > to "hardware domain" / "hwdom" in order to keep the language generic.
> > To clarify, I meant that it would be possible to have an hardware domain on 
> > Arm. This is not used/implemented today but most of the code is adhering to the 
> > language. The only reason where we would use dom0 is when we explicitely check 
> > for domain_id 0 in the code.
> >
> > [...]
> Thank you for the clarification.
>
> > > > > +        return -EINVAL;
> > > > > +    }
> > > > > +
> > > > > +    ret = scmi_check_smccc_ver();
> > > > > +    if ( ret )
> > > > > +        goto err;
> > > > > +
> > > > > +    ret = scmi_dt_init_smccc();
> > > > > +    if ( ret == -EOPNOTSUPP )
> > > > > +        return ret;
> > > > > +    if ( ret )
> > > > > +        goto err;
> > > > > +
> > > > > +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
> > > > > +
> > > > > +    return 0;
> > > > > +
> > > > > +err:
> > > > > +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
> > > > In the commit message, you said the SCMI subsystem was optional. But here you 
> > > > use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
> > > Well, SCMI is optional, in the sense that if we don't find the
> > > SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
> > > and we return right away (no error printed).
> > >
> > > But if we do find the SCMI node, it means that we should initialize
> > > the SCMI subsystem, right? And if we're trying to do that and we
> > > find that the DT node is not correctly formatted (i.e. the
> > > 'arm,smc-id' property), I think we should print an error.
> > Correct me if I am wrong, but from the doc, I understood that the property 
> > arm,smc-id is only necessary if the transport is SMC/HVC.
> >
> > So I don't think we should print an error if it is not found as this is 
> > effectively optional.
> I believe your understanding is correct, the 'arm,smc-id' property
> is needed if the SCMI firmware node has either "arm,scmi-smc" or
> "arm,scmi-smc-param" compatibles [0]. We only support the
> "arm,scmi-smc" compatible with our implementation, though, as you
> mentioned there should not be any addresses passed in the regs
> alongside the SMC call. That would need to happen with the
> "arm,scmi-smc-param" compatible.
>
> But, by "if we do find the SCMI firmware node" I effectively meant
> "if we find the node in Host DT with 'arm,scmi-smc' compatible",
> since we're only implementing this specific variant. And that's
> why I implied that a valid 'arm,smc-id' property is equivalent to
> having a correctly defined SCMI firmware node. Having found the
> "arm,scmi-smc" compatible node (which IMO means we should commit to
> initializing the SCMI layer), shouldn't we print if we tried to init
> the SCMI layer and failed (due to incorrect formatting of the node)?
>
> I hope I explained it better now.
Yes. Thanks for the clarification!

 If there's still anything unclear,
> let me know.
It is now clear. For I agree this should be an error if we can't find 
the property arm,smc-id.
Cheers,

--
Julien Grall