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

[Peter Xu]

On Mon, Oct 09, 2017 at 10:30:07AM +0200, Maxime Coquelin wrote:
> Hi Peter,
> 
> On 10/09/2017 07:17 AM, Peter Xu wrote:
> >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.
> 
> No problem.
> 
> >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.
> 
> Good point.
> Maybe it would be safer to wrap the IOTLB entry to the memory region?

The problem is that MR size may not be aligned with address masks.  I
see it less meaningful if we need to further man-made a smaller mask.

And wait, since you mentioned about MR... I think using -1 here may be
wrong.  Although current MR is transparently mapped (the MR that
covers the address to be translated), it does not mean the whole
address space is transparently mapped.  SPAPR should be a good example
that some ranges of the address space are mapped by IOMMU but some are
not.

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

According to above, I would vote for your previous solution: first use
-1 to get the minimum mask, then switch to PAGE_MASK before returning
when needed.

> 
> >(Btw, thanks again for moving these patches forward; I tried to, but I
> >  failed :)
> 
> I'm a bit faulty not to have reviewed/tested it in the first place ;)

:-)

Thanks!

> 
> Thanks,
> Maxime

-- 
Peter Xu