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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.03.2025 15:25, Grygorii Strashko wrote:
> On 11.03.25 13:43, Jan Beulich wrote:> On 11.03.2025 12:16, Grygorii Strashko 
> wrote:
>>> @@ -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

Thanks for all the details, but I feel overwhelmed. I'd like to see this 
clarified
in more basic terms. For example the comment says "This change will allow ...".
What's "this change" here? Together with "TODO: RFC" it feels a little as if the
code comment was really meant to live elsewhere (patch description? post-commit-
message area of the submission?), and thus offering little value to an observer
like me. Yet as this is common code, I'd like to have at least a rough, high
level understanding of what's going on here. This doesn't need to go into any 
Arm
details that I may not fully understand / appreciate anyway.

Jan