Re: [Qemu-devel] [PATCH V2 05/11] exec: introduce address_space_get_iotlb_entry()

[Jason Wang]

On 2016年11月04日 01:03, Paolo Bonzini wrote:
> On 03/11/2016 10:27, Jason Wang wrote:
> > This patch introduces a helper to query the iotlb entry for a
> > possible iova. This will be used by later device IOTLB API to enable
> > the capability for a dataplane (e.g vhost) to query the IOTLB.
> >
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Peter Crosthwaite <address@hidden>
> > Cc: Richard Henderson <address@hidden>
> > Signed-off-by: Jason Wang <address@hidden>
> > ---
> >   exec.c                | 33 +++++++++++++++++++++++++++++++++
> >   include/exec/memory.h |  6 ++++++
> >   2 files changed, 39 insertions(+)
> >
> > diff --git a/exec.c b/exec.c
> > index b1094c0..00c7a2b 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -449,6 +449,39 @@ address_space_translate_internal(AddressSpaceDispatch *d, 
> > hwaddr addr, hwaddr *x
> >   }
> >   
> >   /* Called from RCU critical section */
> > +IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> > +                                            bool is_write)
> > +{
> > +    IOMMUTLBEntry iotlb = {0};
> > +    MemoryRegionSection *section;
> > +    MemoryRegion *mr;
> > +    hwaddr plen;
> plen must be initialized on entry to address_space_translate_internal,
> since it's set to
>
>     MIN(plen, MIN(section->size,
>                   addr - section->offset_within_address_space))
>
> after address_space_translate_internal calls
> address_space_lookup_region.  But you don't really need plen, so you
> should do this:
>
>         section = address_space_lookup_region(d, addr, false);
>         addr = addr
>                 - section->offset_within_address_space
>                 + section->offset_within_region;
>
> instead of
>
>         section = address_space_translate_internal(d, addr, &addr,
>                                                    &plen, true);
>
> and then you don't even need a plen anymore.

Cool, will do this in next version.

> Also:
>
>
> > +    for (;;) {
> > +        AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
> > +        section = address_space_translate_internal(d, addr, &addr, &plen, 
> > true);
> > +        mr = section->mr;
> > +
> > +        if (!mr->iommu_ops) {
> > +            break;
> > +        }
> > +
> > +        iotlb = mr->iommu_ops->translate(mr, addr, is_write);
> > +        addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
> > +                | (addr & iotlb.addr_mask));
> This addr assignment is only needed after the iotlb.perm check, so move
> it there.
>
> Thanks,
>
> Paolo

Right.

Thanks

> > +        plen = MIN(plen, (addr | iotlb.addr_mask) - addr + 1);
> > +        if (!(iotlb.perm & (1 << is_write))) {
> > +            iotlb.target_as = NULL;
> > +            break;
> > +        }
> > +
> > +        as = iotlb.target_as;
> > +    }
> > +
> > +    return iotlb;
> > +}
> > +
> > +/* Called from RCU critical section */
> >   MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
> >                                         hwaddr *xlat, hwaddr *plen,
> >                                         bool is_write)
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 9728a2f..e605de3 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1404,6 +1404,12 @@ void address_space_stq_le(AddressSpace *as, hwaddr addr, 
> > uint64_t val,
> >   void address_space_stq_be(AddressSpace *as, hwaddr addr, uint64_t val,
> >                               MemTxAttrs attrs, MemTxResult *result);
> >   
> > +/* address_space_get_iotlb_entry: translate an address into an IOTLB
> > + * entry. Should be called from an RCU critical section.
> > + */
> > +IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> > +                                            bool is_write);
> > +
> >   /* address_space_translate: translate an address range into an address space
> >    * into a MemoryRegion and an address range into that section.  Should be
> >    * called from an RCU critical section, to avoid that the last reference