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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Julien Grall <julien@xxxxxxx> writes:

> Hi,
>
> On 22/12/2021 12:17, Volodymyr Babchuk wrote:
>> Julien Grall <julien@xxxxxxx> writes:
>>> 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.
>
> I am not aware of any upstream. Do you have a pointer if there is any
> public discussion?

No, this is the first public discussion on this topic.

>
>>>
>>> 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.
>
> Xen doesn't know if the device is DMA-capable or not. So...
>

But it knows if there is a iommus=<> node present for the device.

>> 
>>> 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
>
> ... with this approach you are at the risk to let the user passthrough
> a device that cannot be protected.
>
>> 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.
>
> That could only work if your guest is using the same layout as the
> host.

Agreed. But in my experience, in most cases this is the true. We worked
with Renesas and IMX hardware and in both cases iomems were
mapped 1:1.

> Otherwise, there is a risk it will clash with other parts of the
> memory layout.
>

> Today, guests started via the toolstack is only using a virtual
> layout, so you would first need to add support to use the host memory
> layout.

I can't say for all the possible configurations in the wild, but I'm
assuming that in most cases it will be fine to use 1:1 mapping. For
those more exotic cases it would be possible to implement some
configuration option like iomem_map=[mfn:gfn].

As Stefano pointed, right now user needs to provide 3 configuration
options to pass a device to a guest: dt_dev, irq, iomem. But in fact,
Xen is already knowing about irq and iomem from device tree.


-- 
Volodymyr Babchuk at EPAM