Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 1 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/10/2019 00:24, Stefano Stabellini wrote:
> > On Mon, 30 Sep 2019, Julien Grall wrote:
> > > Hi Oleksandr,
> > > 
> > > On 30/09/2019 10:34, Oleksandr wrote:
> > > > On 28.09.19 02:28, Stefano Stabellini wrote:
> > > > > > > I have to admit that I don't know about dom0less feature enough
> > > > > > > ...
> > > > > > > 
> > > > > > > 
> > > > > > > But, shouldn't we check if the device is behind the IOMMU and try
> > > > > > > to
> > > > > > > add
> > > > > > > it (iommu_add_dt_device) before assigning it (this is needed for
> > > > > > > drivers
> > > > > > > which support generic IOMMU DT bindings in the first place).
> > > > > > > 
> > > > > > > [please take a look at
> > > > > > > https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
> > > > > > > if so]
> > > > > > > 
> > > > > > > Julien, what do you think?
> > > > > > Yes you are right.
> > > > > > 
> > > > > > @Stefano, this is a recently merged feature. Without it, you will
> > > > > > not be
> > > > > > able to use passthrough with dom0less guest when the IOMMU (such as
> > > > > > IPMMU) is using the generic DT bindings.
> > > > > Just double-checking but it should be only a matter of the following,
> > > > > right?
> > > > > 
> > > > > +        res = iommu_add_dt_device(node);
> > > > > +        if ( res < 0 )
> > > > > +            return res;
> > > > 
> > > > I think, the change above is correct.
> > > > 
> > > > 
> > > > > +
> > > > > +        if ( dt_device_is_protected(node) )
> > > > > +        {
> > > > > +            res = iommu_assign_dt_device(kinfo->d, node);
> > > > > +            if ( res < 0 )
> > > > > +                return res;
> > > > > +        }
> > > > > +
> > > > > 
> > > > > (I am asking because I couldn't quite test it due to the error with
> > > > > mmu-masters I mentioned in the other email.)
> > > > Regarding the check "if (dt_device_is_protected(node))" here. I think,
> > > > it
> > > > depends on the "xen,path" purpose.
> > > > 
> > > > 1. If "xen,path" property is, let say, close to "dtdev" property in
> > > > domain
> > > > config file, where we describe master devices which are behind the
> > > > IOMMU, so
> > > > *must* be protected, then that check should be removed. Please see
> > > > iommu_do_dt_domctl().
> > > > 
> > > > 2. If "xen,path" property can also be used to describe devices which are
> > > > not
> > > > behind the IOMMU (so don't need to be protected), but just for the
> > > > "interrupt mappings" purposes, then that check is correct and should
> > > > remain.
> > > 
> > > Some device may not be behind an IOMMU but still do DMA. We are not doing
> > > a
> > > favor to the user to continue the assignment as this could lead to at best
> > > to
> > > a non-working device (at worst a security issue).
> > > 
> > > Therefore I am against the solution 2).
> > 
> > I agree. (And honestly, "xen,path" was introduced as an equivalent of
> > "dtdev" initially.)
> > 
> > 
> > > However, this raises some questions why MMIOs are treated differently (i.e
> > > they don't need an IOMMU).
> > > 
> > > In the current setup, you would not be able to passthrough a non
> > > DMA-capable
> > > to a guest if they needs interrupts (e.g. an UART) but you would be if
> > > they
> > > don't use interrupts.
> > > 
> > > So I think we need a couple of more changes:
> > >     1) Introduce an option to allow the user to ignore IOMMU issues
> > > (something
> > > like "xen,force-assign-without-iommu").
> > >     2) "xen,reg" cannot be specified without "xen,path". This allows us to
> > > police the user DT.
> > 
> > Interesting questions.
> > 
> > Something like "xen,force-assign-without-iommu" would be useful. The
> > upside of being able to assign a non-IOMMU-protected non-DMA-capable
> > device outweighs the downsides.
> > 
> > I am less sure about having to specify "xen,reg" together with
> > "xen,path". It is fairly common to have a control register MMIO region
> > page in FPGA that doesn't do any DMA and has no related interrupts. In
> > those cases, it is nice to be able to handle it by just having one
> > "xen,reg" property. But maybe if the user also passes
> > "xen,force-assign-without-iommu" then we could also ignore a missing
> > "xen,path".
> > 
> > In any case, my preference would be to keep the series as is for now,
> > and make these changes later. 
> 
> Bindings are meant to be stable, so we would end up to have to create a new
> bindings to cater the solution discussed here. So I would rather avoid to take
> that approach.

Adding a note here from our discussion on IRC. One idea would be to keep
the code as is (v7a) but to make sure the docs reflect that xen,reg and
xen,path are both required. That would be good. However, the docs
already imply it so I didn't actually need to make any changes in that
respect in v7a. In any case, I could certainly add a statement or two to
the docs if it helps.


> > However, for the sake of moving things
> > forward quickly, I also implemented Julien's suggestions. So I'll send
> > two v7 updates to this series:
> > 
> > - v7a: the minimal changes version, without things discussed here except
> >         for removing the "if (dt_device_is_protected(node))" check
> > - v7b: a version with all the changes discussed here
> > 
> > Julien, I'll let you pick your favorite, hopefully one of them will be
> > to your liking.
> 
> Thank you for suggesting the two versions. I will have a look at them.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel