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?
One other remark: No IS_ENABLED() please in pre-processor directives. It
wants to be #ifdef there. IS_ENABLED() is for use in if().
Jan
