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

[Julien Grall <julien@xxxxxxx>]

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

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

> However, I think we shouldn't print an error if we return
> with an error code from scmi_check_smccc_ver(). And the print
> inside scmi_check_smccc_ver() should be a XENLOG_WARN.
>
> So, I think we should print a XENLOG_ERR only if we figured
> we need to initialize, and we started to do it but it failed.
... with the two other points.

Cheers,

--
Julien Grall