Re: [RFC v2 6/8] tools/arm: Introduce force_assign_without_iommu option to xl.cfg

[Julien Grall <julien@xxxxxxx>]

On 18/02/2022 09:16, Oleksii Moisieiev wrote:
> Hi Julien,
Hi Oleksii,

> On Thu, Feb 17, 2022 at 03:20:36PM +0000, Julien Grall wrote:
> > >        xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat",
> > > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > index 093bb4403f..f1f19bf711 100644
> > > --- a/xen/common/domain.c
> > > +++ b/xen/common/domain.c
> > > @@ -512,7 +512,7 @@ static int sanitise_domain_config(struct 
> > > xen_domctl_createdomain *config)
> > >        if ( iommu )
> > >        {
> > > -        if ( config->iommu_opts & ~XEN_DOMCTL_IOMMU_no_sharept )
> > > +        if ( config->iommu_opts >> XEN_DOMCTL_IOMMU_MAX )
> > XEN_DOMCTL_IOMMU_MAX will be defined as:
> >
> > (1U << _XEN_DOMCTL_IOMMU_force_iommu)
> >
> > This means the shift will do the wrong thing. However, AFAICT, this new
> > option will only be supported by Arm and likely only for platform device for
> > the time being.
> Thanks, I will fix that.
>
> > That said, I am not convinced this flag should be per-domain in Xen.
> > Instead, I think it would be better to pass the flag via the device assign
> > domctl.
> Do you mean that it's better to set this flag per device, not per
> domain? > This will require setting this flag for each device which should
> require either changing the dtdev format in dom.cfg or setting
> xen,force-assign-without-iommu in partial device-tree.
>
> Both of those ways will complicate the configuration. As was mentioned
> before, we don't want to make domain configuration more complicated.
> What do you think about that?
We have two interfaces here:
  1) User -> tools
  2) tools -> Xen

We can chose different policy for each interface.

For the tools -> Xen interface, I think this should be per device 
(similar to XEN_DOMCTL_DEV_RDM_RELAXED).
For the User -> tools, I am open to discussion. One advantage with per 
device is the user explicitely vet each device. So it is harder to 
passthrough a device wrongly.
But I agree this also complicates the interface. What do other thinks?

> > >            return -EOPNOTSUPP;
> > > @@ -542,7 +545,7 @@ int iommu_do_domctl(
> > >    #endif
> > >    #ifdef CONFIG_HAS_DEVICE_TREE
> > > -    if ( ret == -ENODEV )
> > > +    if ( ret == -ENOSYS )
> > AFAICT, none of the code (including callee) before ret have been modified.
> > So why are you modifying the check here?
> Because this check will fail if we have CONFIG_HAS_DEVICE_TREE define,
> but do not have CONFIG_HAS_PCI and iommu_do_dt_domctl will not be
> called.
Below the implementation of iommu_do_domctl() on staging:

int iommu_do_domctl(
    struct xen_domctl *domctl, struct domain *d,
    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
    int ret = -ENODEV;

    if ( !is_iommu_enabled(d) )
        return -EOPNOTSUPP;

#ifdef CONFIG_HAS_PCI
    ret = iommu_do_pci_domctl(domctl, d, u_domctl);
#endif

#ifdef CONFIG_HAS_DEVICE_TREE
    if ( ret == -ENODEV )
        ret = iommu_do_dt_domctl(domctl, d, u_domctl);
#endif

    return ret;
}

'ret' is initialized to -ENODEV. So for !CONFIG_HAS_PCI, then ret will 
not be changed. Therefore the current check is correct.
AFAICT, your patch is setting 'ret' so I don't expect any change here.

> Same thing if switch/case inside iommu_do_pci_domctl go to default and
> return -ENOSYS. This part looked strange for me. But I will definitely
> go through this part once again.
We use the same sub-op to assign/deassign a PCI and "DT" device. So we 
are not interested in -ENOSYS but -ENODEV that would be returned by the 
checks:
if ( domct->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )

At the moment, there are no sub-op specific to "DT" device. So it is not 
necessary for us to check -ENOSYS yet.
I haven't looked at the rest of the series to see if we need it. But if 
we do, then I think the check should be extended in the patch that 
requires it.
Cheers,

--
Julien Grall