Re: [Qemu-devel] [PATCH 14/27] iommu: Add IOMMU index concept to IOMMU API

[Peter Maydell <peter.maydell@linaro.org>]

On 23 May 2018 at 02:06, Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 22, 2018 at 12:11:38PM +0100, Peter Maydell wrote:
>> On 22 May 2018 at 12:02, Peter Xu <peterx@redhat.com> wrote:
>> > On Tue, May 22, 2018 at 09:40:44AM +0100, Peter Maydell wrote:

>> > And if we see current implementation for this (still, I copied code
>> > from other patch in the series to here to ease discussion):
>> >
>> > @@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm
>> >      do {
>> >          hwaddr addr = *xlat;
>> >          IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
>> > -        IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ?
>> > -                                              IOMMU_WO : IOMMU_RO);
>> > +        int iommu_idx = 0;
>> > +        IOMMUTLBEntry iotlb;
>> > +
>> > +        if (imrc->attrs_to_index) {
>> > +            iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
>> > +        }
>> > +
>> > +        iotlb = imrc->translate(iommu_mr, addr, is_write ?
>> > +                                IOMMU_WO : IOMMU_RO, iommu_idx);
>> >
>> > Here what if we pass attrs directly into imrc->translate() and just
>> > call imrc->attrs_to_index() inside the arch-dependent translate()
>> > function?  Will that work too?
>>
>> You don't always have the attributes at the point where you want
>> to call translate. (For instance, memory_region_notify_iommu()
>> doesn't have attributes.)
>>
>> I started off with "pass the tx attrs into the translate method",
>> which is fine for the code flows which are actually doing
>> memory transactions, but breaks down when you try to incorporate
>> notifiers.
>
> Could you elaborate a bit more on why IOMMU notifier failed to
> corporate when passing in MemTxAttrs?  I am not sure I caught the idea
> here, but can we copy the MemTxAttrs into IOMMUTLBEntry when
> translating, then in IOMMU notifiers we can know the attrs (if that is
> what MPC wants)?

(1) The notifier API lets you register a notifier before you've
called the translate API
(2) An IOMMUTLBEntry can be valid for more than just the txattrs
that it was passed in (for instance, if an IOMMU doesn't care
about txattrs at all, then the resulting TLB entry is valid for
any txattrs; or if the IOMMU only cares about attrs.secure the
resulting TLB entries are valid for both attrs.user=0 and
attrs.user=1).
(3) when the IOMMU calls the notifier because the guest config
changed it doesn't have tx attributes, so it would have to
fabricate some; and the guest config will invalidate transactions
with some combinations of tx attributes and not others.

As Paolo pointed out you could also implement this by rather
of having an iommu_index concept, instead having some kind
of "here is a mask of which txattrs fields matter, and here's
another parameter with which txattrs fields are affected".
That makes it awkward though to implement "txattrs.unspecified
acts like txattrs.secure == 1" type behaviour, though, which is
easy with an index abstraction layer. It also would be harder
to implement the default 'replay' method, I think.

Plus I think that handling this the same way TCG does is a
reasonable approach -- we know that it's a usefully flexible
concept.

>> > I had a quick glance at the series, I have no thorough idea on the
>> > whole stuff, but I'd say some of the patches are exactly what I wanted
>> > if to support MemTxAttrs in VT-d emulation one day (now DMAR of VT-d
>> > is bypassing MemTxAttrs and IMHO that's incorrect).  If we can somehow
>> > pass in the MemTxAttrs then that'll be perfect and I can continue to
>> > work on that.  If we pass in iommu_idx now instead, it would take some
>> > time for me to figure out how to further achieve the same goal for
>> > VT-d in the future, e.g., I would still want to pass in MemTxAttrs,
>> > but that's obviously duplicated with iommu_idx.
>>
>> The idea is that you should never need to pass in the MemTxAttrs,
>> because everything that the IOMMU might care about in the tx attrs
>> must be encoded into the iommu index. (The point where the IOMMU
>> gets to do that encoding is in its attrs_to_index() method.)
>
> For the DMAR issue I would care about MemTxAttrs.requester_id.  Just
> to confirm - do you mean I encode the 16bits field into iommu_idx too,
> or is there any smarter way to do so?  Asked since otherwise iommu_idx
> will gradually grow into another MemTxAttrs to me.

It will only need to do so for IOMMUs that care about that field.

(See also the other thread with Eric Auger talking about
maybe caring about requester_id like that. Needing to look
at requester_id is an area I haven't thought too much about,
and it is a bit of an odd one because it's a much larger
space than any of the other parts of the txattrs. In some
cases it ought to be possible to say "requester_id lets
us determine an iommu index, and there are a lot fewer
than 2^16 actual iommu indexes because a lot of the requestor_id
values indicate the same actual iommu translation", I suspect.)

thanks
-- PMM