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?
Oleksii
