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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 :-)

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.

If we really want to introduce a new list of devices, please make it
optional so that most times the user can skip it unless really required.