Re: [RFC v1 5/5] xen/arm: add SCI mediator support for DomUs

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall <julien@xxxxxxx> writes:

> Hi Stefano,
>
> On 21/12/2021 22:39, Stefano Stabellini wrote:
>> On Tue, 21 Dec 2021, Anthony PERARD wrote:
>>> On Fri, Dec 17, 2021 at 12:15:25PM +0000, Oleksii Moisieiev wrote:
>>>>> On 14.12.21 11:34, Oleksii Moisieiev wrote:
>>>>>> @@ -1817,17 +1858,24 @@ static void libxl__add_dtdevs(libxl__egc *egc, 
>>>>>> libxl__ao *ao, uint32_t domid,
>>>>>>    {
>>>>>>        AO_GC;
>>>>>>        libxl__ao_device *aodev = libxl__multidev_prepare(multidev);
>>>>>> -    int i, rc = 0;
>>>>>> +    int i, rc = 0, rc_sci = 0;
>>>>>>        for (i = 0; i < d_config->num_dtdevs; i++) {
>>>>>>            const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
>>>>>>            LOGD(DEBUG, domid, "Assign device \"%s\" to domain", 
>>>>>> dtdev->path);
>>>>>>            rc = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
>>>>>> -        if (rc < 0) {
>>>>>> -            LOGD(ERROR, domid, "xc_assign_dtdevice failed: %d", rc);
>>>>>> +        rc_sci = xc_domain_add_sci_device(CTX->xch, domid, dtdev->path);
>>>>>> +
>>>>>> +        if ((rc < 0) && (rc_sci < 0)) {
>>>>>> +            LOGD(ERROR, domid, "xc_assign_dt_device failed: %d; "
>>>>>> +                 "xc_domain_add_sci_device failed: %d",
>>>>>> +                 rc, rc_sci);
>>>>>>                goto out;
>>>>>>            }
>>>>>> +
>>>>>> +        if (rc)
>>>>>> +            rc = rc_sci;
>>>>>
>>>>>
>>>>> If I get this code right, it sounds like the dom.cfg's dtdev property is
>>>>> reused to describe sci devices as well, but the libxl__add_dtdevs() does 
>>>>> not
>>>>> (and can not) differentiate them. So it has no option but to send two
>>>>> domctls for each device in dtdevs[] hoping to at least one domctl to
>>>>> succeeded. Or I really missed something?
>>>>>
>>>>> It feels to me that:
>>>>>   - either new dom.cfg's property could be introduced (scidev?) to 
>>>>> describe
>>>>> sci devices together with new parsing logic/management code, so you will 
>>>>> end
>>>>> up having new libxl__add_scidevs() to invoke XEN_DOMCTL_add_sci_device,
>>>>> so no mixing things.
>>>>>   - or indeed dtdev logic could be *completely* reused including extending
>>>>> XEN_DOMCTL_assign_device to cover your use-case, although sounds generic, 
>>>>> it
>>>>> is used to describe devices for the passthrough (to configure an IOMMU for
>>>>> the device), in that case you wouldn't need an extra
>>>>> XEN_DOMCTL_add_sci_device introduced by current patch.
>> I realize I did my review before reading Oleksandr's comments. I
>> fully
>> agree with his feedback. Having seen how difficult is for users to setup
>> a domU configuration correctly today, I would advise to try to reuse the
>> existing dtdev property instead of adding yet one new property to make
>> the life of our users easier.
>> 
>> 
>>>>> Personally I would use the first option as I am not sure that second 
>>>>> option
>>>>> is conceptually correct && possible. I would leave this for the 
>>>>> maintainers
>>>>> to clarify.
>>>>>
>>>>
>>>> Thank you for the point. I agree that reusing XEN_DOMCTL_assign_device
>>>> seems not to be conceptually correct. Introducing new dom.cfg property
>>>> seems to be the only way to avoid extra Domctl calls.
>>>> I will handle this in v2 if there will be no complains from Maintainers.
>>>
>>> I don't know if there will be a complains in v2 or not :-), but using
>>> something different from dtdev sound good.
>>>
>>> If I understand correctly, dtdev seems to be about passing through an
>>> existing device to a guest, and this new sci device seems to be about 
>>> having Xen
>>> "emulating" an sci device which the guest will use. So introducing
>>> something new (scidev or other name) sounds good.
>> Users already have to provide 4 properties (dtdev, iomem, irqs,
>> device_tree) to setup device assignment. I think that making it 5
>> properties would not be a step forward :-)
>
> IIRC, in the past, we discussed to fetch the information directly from
> the partial device-tree. Maybe this discussion needs to be revived?
>
> Although, this is a separate topic from this series.
>
>> To me dtdev and XEN_DOMCTL_assign_device are appropriate because
>> they
>> signify device assignment of one or more devices. We are not adding any
>> additional "meaning" to them. It is just that we'll automatically detect
>> and generate any SCMI configurations based on the list of assigned
>> devices. Because if SCMI is enabled and a device is assigned to the
>> guest, then I think we want to add the device to the SCMI list of
>> devices automatically.
>
> I am OK with re-using dtdev/XEN_DOMCTL_assign_device however there is
> a pitfall with that approach.
>
> At the moment, they are only used for device protected by the
> IOMMU. If the device is not protected by the IOMMU then it will return
> an error.

IIRC there was a change, that allowed to assign device without a IOMMU. At
least we discussed this internally.

>
> Now, with your approach we may have a device that is not protected by
> the IOMMU but require to a SCMI configuration.

You need to protect only DMA-capable devices.

> I don't think it would be sensible to just return "succeed" here
> because technically you are asking to assign a non-protected
> device. But at the same time, it would prevent a user to assign a
> non-DMA capable device.
>
> So how do you suggest to approach this?

Well, in my head assign_device ideally should do the following:

1. Assign IOMMU if it is configured for the device
2. Assign SCMI access rights
(Not related to this patch series, but...)
3. Assign IRQs
4. Assign IO memory ranges.

Points 3. and 4. would allow us to not provide additional irq=[] and
iomem=[] entries in a guest config.


-- 
Volodymyr Babchuk at EPAM