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

[Julien Grall <julien@xxxxxxx>]

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

> > 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. 
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.
Cheers,

--
Julien Grall