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
