Re: [Qemu-devel] [PATCH 2/3] exec: simplify address_space_get_iotlb_entry

[Peter Xu]

On Fri, Jun 02, 2017 at 07:49:58PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 02, 2017 at 07:50:53PM +0800, Peter Xu wrote:
> > This patch let address_space_get_iotlb_entry() to use the newly
> > introduced page_mask parameter in address_space_do_translate(). Then we
> > will be sure the IOTLB can be aligned to page mask, also we should
> > nicely support huge pages now when introducing a764040.
> > 
> > Fixes: a764040 ("exec: abstract address_space_do_translate()")
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  exec.c | 29 ++++++++++-------------------
> >  1 file changed, 10 insertions(+), 19 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 63a3ff0..1f86253 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -544,14 +544,14 @@ IOMMUTLBEntry 
> > address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> >                                              bool is_write)
> >  {
> >      MemoryRegionSection section;
> > -    hwaddr xlat, plen;
> > +    hwaddr xlat, page_mask;
> >  
> > -    /* Try to get maximum page mask during translation. */
> > -    plen = (hwaddr)-1;
> > -
> > -    /* This can never be MMIO. */
> > -    section = address_space_do_translate(as, addr, &xlat, &plen,
> > -                                         NULL, is_write, false);
> > +    /*
> > +     * This can never be MMIO, and we don't really care about plen,
> > +     * but page mask.
> > +     */
> > +    section = address_space_do_translate(as, addr, &xlat, NULL,
> > +                                         &page_mask, is_write, false);
> >  
> >      /* Illegal translation */
> >      if (section.mr == &io_mem_unassigned) {
> 
> 
> Can we just use section.size - xlat here?

I replied in the other thread about what I thought... So will skip
here.

> 
> > @@ -562,20 +562,11 @@ IOMMUTLBEntry 
> > address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> >      xlat += section.offset_within_address_space -
> >          section.offset_within_region;
> >  
> > -    if (plen == (hwaddr)-1) {
> > -        /* If not specified during translation, use default mask */
> > -        plen = TARGET_PAGE_MASK;
> > -    } else {
> > -        /* Make it a valid page mask */
> > -        assert(plen);
> > -        plen = pow2floor(plen) - 1;
> > -    }
> > -
> >      return (IOMMUTLBEntry) {
> >          .target_as = section.address_space,
> > -        .iova = addr & ~plen,
> > -        .translated_addr = xlat & ~plen,
> > -        .addr_mask = plen,
> > +        .iova = addr & ~page_mask,
> > +        .translated_addr = xlat & ~page_mask,
> > +        .addr_mask = page_mask,
> >          /* IOTLBs are for DMAs, and DMA only allows on RAMs. */
> 
> BTW this comment is pretty confusing. What does it mean?

This function, address_space_get_iotlb_entry(), is for device to get
IOTLB entry when they want to request for page translations for DMA,
and DMA should only be allowed to RAM, right? Then we need IOMMU_RW
permission here.

Maybe I should add one more check above on the returned MR - it should
be RAM typed as well. But I don't really know whether that's too
strict, since if guest setup the IOMMU page table to allow one IOVA
points to a non-RAM region, I thought it should still be legal from
hypervisor POV (I see it a guest OS bug though)?

> 
> >          .perm = IOMMU_RW,
> >      };
> 
> Looks like we should change IOMMUTLBEntry to pass size and not mask -
> then we could simply pass info from section as is. iova would be
> addr - xlat.

I don't sure whether it'll be a good interface for IOTLB. AFAIU at
least for VT-d, the IOMMU translation is page aligned which is defined
by spec, so it makes sense that (again at least for VT-d) here we'd
better just use page_mask/addr_mask.

That's also how I know about IOMMU in general - I assume it do the
translations always with page masks (never arbitary length), though
page size can differ from platfrom to platform, that's why here the
IOTLB interface used addr_mask, then it works for all platforms. I
don't know whether I'm 100% correct here though.

Maybe David/Paolo/... would comment as well?

(CC David)

Thanks,

-- 
Peter Xu