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

[Maxime Coquelin]

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?

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

Yes, that makes sense.

> (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,
Maxime