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

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

On 22/12/2021 13:34, Volodymyr Babchuk wrote:
> 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.
Not really. Not all the platforms have IOMMUs and not all the platforms 
with IOMMU have DMA-capable device protected by an IOMMU.
> > > > 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].
Well, the Xen memory layout is not something that is stable nor AFAIK 
based on any memory layout. In fact, there is no such generic layout on Arm.
It is quite possible that it will work well with 1:1 MMIO on some 
platform (e.g. Renesas, IMX) but you can't expect to work for every Xen 
release or all the platforms.
> 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.
I understand and this is not ideal. I would be happy to consider your 
approach. However, this will have to enabled only when the guest is 
re-using the host layout.
Cheers,

--
Julien Grall