Re: [Qemu-devel] [PATCH v2 1/2] exec: add page_mask for flatview_do_translate

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 06, 2017 at 03:03:50PM +0200, Maxime Coquelin wrote:
> 
> 
> On 10/06/2017 02:48 PM, Paolo Bonzini wrote:
> >On 06/10/2017 14:46, Maxime Coquelin wrote:
> >>>>           addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
> >>>>                   | (addr & iotlb.addr_mask));
> >>>>-        *plen = MIN(*plen, (addr | iotlb.addr_mask) - addr + 1);
> >>>>+        page_mask = iotlb.addr_mask;
> >>>
> >>>Should this be "page_mask &= iotlb.addr_mask"?
> >>>
> >>>If you have multiple IOMMUs on top of each other (yeah, I know...) I
> >>>think the smallest size should win.  This is also consistent with the
> >>>MIN in the line below.
> >>
> >>I agree, but changin to "page_mask &= iotlb.addr_mask" will not be
> >>enough, we also have to change the init value. Else we will always end
> >>up with 0xfff.
> >>
> >>Maybe we could do as plen was handled before, i.e. setting page_mask
> >>init value to (hwaddr)(-1), and after the loop set it to
> >>~TARGET_PAGE_MASK if it hasn't been changed.
> >>
> >>Does that sound reasonable?
> >
> >True that, in fact it makes sense for the "IOTLB entry" to represent all
> >of memory if there's no IOMMU at all.
> 
> Indeed, that makes sense as no iommu means identity mapping. It would
> moreover improve performance, as the vhost backend will only have a
> single IOTLB entry in its cache.
> 
> Maybe it is better to wait for Peter to understand the reason he limited
> it to the target page size?

Sorry, just came back from a long holiday.

I was trying to use 4K as default to be safe (but yes the mask was not
correct, thanks for fixing that!), to make sure the translated range
covered by the IOMMUTLBEntry will always be safe to access (I thought
that was how IOTLB was defined, but I may be wrong).  Using (-1) is
good especially from performance POV as long as the caller knows the
real memory boundary, but I'm not sure whether it'll break the IOTLB
scemantic somehow.

If we want to make it -1 for transparent mappings, maybe worth
commenting it in definition of IOMMUTLBEntry.page_mask?

(Btw, thanks again for moving these patches forward; I tried to, but I
 failed :)

-- 
Peter Xu