Re: [PATCH v8 1/5] xen/domctl: chain SCI handling before IOMMU in assign_device domctl

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On 26/01/2026 12:37, Jan Beulich wrote:
> On 26.01.2026 11:29, Oleksii Moisieiev wrote:
>> Hi Jan,
>> Thank you for your comments.
>>
>> On 22/01/2026 09:51, Jan Beulich wrote:
>>> On 21.01.2026 19:43, Oleksii Moisieiev wrote:
>>>> --- a/xen/arch/arm/firmware/sci.c
>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>> @@ -126,6 +126,41 @@ int sci_assign_dt_device(struct domain *d, struct 
>>>> dt_device_node *dev)
>>>>        return 0;
>>>>    }
>>>>    
>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>> +{
>>>> +    struct dt_device_node *dev;
>>>> +    int ret = 0;
>>>> +
>>>> +    switch ( domctl->cmd )
>>>> +    {
>>>> +    case XEN_DOMCTL_assign_device:
>>>> +        ret = -ENXIO;
>>>> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>>>> +            break;
>>>> +
>>>> +        if ( !cur_mediator )
>>>> +            break;
>>>> +
>>>> +        if ( !cur_mediator->assign_dt_device )
>>>> +            break;
>>>> +
>>>> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>>>> +                                    domctl->u.assign_device.u.dt.size, 
>>>> &dev);
>>>> +        if ( ret )
>>>> +            return ret;
>>>> +
>>>> +        ret = sci_assign_dt_device(d, dev);
>>>> +
>>>> +        break;
>>>> +    default:
>>> Nit: Blank line above here please.
>>>
>>>> @@ -274,7 +277,7 @@ static bool is_stable_domctl(uint32_t cmd)
>>>>    
>>>>    long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>    {
>>>> -    long ret = 0;
>>>> +    long ret = 0, ret1 = 0;
>>> This initializer isn't really needed, with ...
>>>
>>>> @@ -833,7 +836,28 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>>> u_domctl)
>>>>        case XEN_DOMCTL_test_assign_device:
>>>>        case XEN_DOMCTL_deassign_device:
>>>>        case XEN_DOMCTL_get_device_group:
>>>> +        /*
>>>> +         * Chain SCI DT handling ahead of the IOMMU path so an SCI 
>>>> mediator
>>>> +         * can authorise access-controlled DT devices. Unhandled cases 
>>>> report
>>>> +         * -ENXIO, which is ignored.
>>>> +         */
>>>> +        ret1 = -ENXIO;
>>> ... this, is it? In fact, why not use -ENXIO as initializer?
>>>
>>>> +    #if IS_ENABLED(CONFIG_ARM_SCI)
>>>> +        ret1 = sci_do_domctl(op, d, u_domctl);
>>>> +    #endif
>>> Why the indentation of the pre-processor directives? At the very least the 
>>> #-es
>>> want to be in the first column, but I see no reason for the indentation at 
>>> all.
>>>
>>>>            ret = iommu_do_domctl(op, d, u_domctl);
>>>> +        if ( ret < 0 )
>>>> +            return ret;
>>> You can't simply return here, as we may be holding an RCU lock on the 
>>> domain.
>>>
>>>> +        /*
>>>> +         * If IOMMU processing was successful, check for SCI processing 
>>>> return
>>>> +         * code and if it failed then overwrite the return code to 
>>>> propagate
>>>> +         * SCI failure back to caller.
>>>> +         */
>>>> +        if ( ret1 != -ENXIO && ret1 < 0 )
>>>> +            ret = ret1;
>>> So if IOMMU processing was successful and because of SCI you return an error
>>> here, why would the IOMMU part not need undoing? Or to ask differently, if
>>> SCI said "no", why even try the IOMMU operation?
>>>
>>> Jan
>> My intention was to preserve the original behavior as much as possible.
>> This means that if the SCI assign operation returns an error, we still
>> attempt the IOMMU assignment, but filter the return code and ultimately
>> return the SCI error if the IOMMU assignment was successful.
>> However, in this scenario, we would still return an error from
>> the domctl call, which could lead to unexpected results.
>>
>> I agree with your point.
>>
>> With your suggestion, the code becomes much cleaner:
>>
>> #if IS_ENABLED(CONFIG_ARM_SCI)
>>           ret = sci_do_domctl(op, d, u_domctl);
>>           if ( ret < 0 && ret != -ENXIO )
>>               break;
>> #endif
>>
>>           ret = iommu_do_domctl(op, d, u_domctl);
>>           break;
>>
>> What do you think about this approach?
> Yes. Just to double-check though: There's nothing that needs undoing after
> a successful sci_do_domctl() when the subsequent iommu_do_domctl() failed?
Just rechecked. for multiagent (apart from parameters check) is sends SCMI
request to the firmware. So I don't see anything that should be undone 
in case
of error.
> One other remark: No IS_ENABLED() please in pre-processor directives. It
> wants to be #ifdef there. IS_ENABLED() is for use in if().
Will fix.
> Jan