Re: [RFC PATCH v3 1/7] xen/arm: add generic SCI subsystem

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Jan,

Thank you for your comments.

On 11.03.25 13:43, Jan Beulich wrote:> On 11.03.2025 12:16, Grygorii Strashko 
wrote:
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -526,6 +526,12 @@ S: Supported
> >   F:    xen/arch/arm/include/asm/tee/
> >   F:    xen/arch/arm/tee/
> >   
> > +SCI MEDIATORS
> > +M:     Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > +S:     Supported
> > +F:     xen/arch/arm/sci
> > +F:     xen/include/asm-arm/sci
> > +
> >   TOOLSTACK
> >   M:    Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >   S:    Supported
> Please can you respect alphabetical sorting in this file?
>
> > @@ -851,6 +852,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > u_domctl)
> >       case XEN_DOMCTL_deassign_device:
> >       case XEN_DOMCTL_get_device_group:
> >           ret = iommu_do_domctl(op, d, u_domctl);
> > +
> > +        if ( ret >= 0 || (ret == -EOPNOTSUPP) || (ret == -ENODEV) )
> > +        {
> > +            /*
> > +             * TODO: RFC
> > +             * This change will allow to pass DT nodes/devices to
> > +             * XEN_DOMCTL_assign_device OP using xl.cfg:"dtdev" property even
> > +             * if those DT nodes/devices even are not behind IOMMU (or IOMMU
> > +             * is disabled) without failure.
> > +             */
> > +            ret = sci_do_domctl(op, d, u_domctl);
> > +        }
> >           break;
> Despite the comment I fear I don't understand what you're trying to do here.
It enables in toolstack "Device specific access control" function, which is 
implemented in SCI FW.
SCI FW has privileged management interface assigned to Xen,
and non-privileged interfaces assigned to guest (VM) and identified by agent_id.

SCI FW manages access to HW resources clocks, resets, etc, which considered 
shared and
which can't be accessed from multiple domains due to HW limitations.
SCI FW can also manage safety specific resources like HW firewalls for example.

Each device identified by device_id and can have HW resources assigned to it
device_id_res = {clk_1, clk_2, reset_1, pd_1 } - FW implementation specific.

Device can be assigned:
1) to any VM, but only to one - dynamic configuration;
2) only one, specific VM identified by agent_id - static configuration.
The policy is FW implementation specific.

Here and below the case (1) is considered, while in the case (2) - nothing need 
to be done.
To enable VMx access to device_id (and its resources) the special request need
to be sent to the FW management interface to get device_id accessible from VMx.

In case of SCMI, ARM System Control and Management Interface (SCMI)
specification (DEN0056E) - functionality defined in sections
4.2.1.1 Device specific access control
4.2.2.11 BASE_SET_DEVICE_PERMISSIONS

The HW configuration described in device tree, like in the below example
(abstract, not related to any specific FW, but principle is generic)

Host DT:
/sci_fw {
        #access-controller-cells = <1>;
        #reset-cells = <1>;
        #clock-cells = <1>;
        #power-domain-cells = <1>;
}

/soc/deviceA {
        clocks = <&sci_fw 1>, <&sci_fw 2>;
        power-domains = <&sci_fw 1>;
        resets = <&sci_fw 1>;
        access-controllers = <&sci_fw 0>;
}

To trigger SCI FW it required to pass Host DT device path "/soc/deviceA" down
to the corresponding SCI FW driver during domain creation by toolstack.
And it has to be done as for devices behind IOMMU, as for devices
not protected by IOMMU.

To achieve above xl.cfg:"dtdev" property was selected to be used due to:
- xen doc says
"
Specifies the host device tree nodes to pass through to this guest.
Each DTDEV_PATH is an absolute path in the device tree.
"
- toolstack triggers XEN_DOMCTL_assign_device(XEN_DOMCTL_DEV_DT) hypercall
nothing from above says it's strictly limited to IOMMU-protected devices only.

But now ARM XEN_DOMCTL_assign_device actually limited to IOMMU-protected devices
and will return to toolstack:
-EOPNOTSUPP if iommu is not enabled
-EINVAL if DT device is not IOMMU-protected

in both cases toolstack will fail.

Idea behind this change (and change in iommu_do_dt_domctl()) is to enable
XEN_DOMCTL_assign_device(XEN_DOMCTL_DEV_DT) and so xl.cfg:"dtdev"
for DT devices which
- IOMMU-protected only (has "iommus" property)
- require device access control (has "access-controllers" property)
- both


> In any event you may not clobber the original "ret", if all you do is some
> "add-on".
I'll check and rework return code handling.

> > --- a/xen/include/asm-generic/device.h
> > +++ b/xen/include/asm-generic/device.h
> > @@ -18,6 +18,7 @@ enum device_class
> >       DEVICE_IOMMU,
> >       DEVICE_INTERRUPT_CONTROLLER,
> >       DEVICE_PCI_HOSTBRIDGE,
> > +    DEVICE_ARM_SCI,
> >       /* Use for error */
> >       DEVICE_UNKNOWN,
> This is a generic header - I don't see how anything arch-specific could
> validly end up here.
Huh. Not a long time ago it was ARM specific only.

I could rename it to DEVICE_SCI, or may be to even more generic one 
DEVICE_FIRMWARE.


[...]

--
Best regards,
-grygorii