[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC v1 5/5] xen/arm: add SCI mediator support for DomUs
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |