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

[Maxime Coquelin]

On 10/06/2017 02:31 PM, Paolo Bonzini wrote:
> On 06/10/2017 13:46, Maxime Coquelin wrote:
> > +    hwaddr page_mask = ~TARGET_PAGE_MASK;
> > +    hwaddr plen = (hwaddr)(-1);
> > +
> > +    if (plen_out) {
> > +        plen = *plen_out;
> > +    }
> >   
> >       for (;;) {
> >           section = address_space_translate_internal(
> >                   flatview_to_dispatch(fv), addr, &addr,
> > -                plen, is_mmio);
> > +                &plen, is_mmio);
> >   
> >           iommu_mr = memory_region_get_iommu(section->mr);
> >           if (!iommu_mr) {
> > @@ -496,7 +520,8 @@ static MemoryRegionSection flatview_do_translate(FlatView 
> > *fv,
> >                                   IOMMU_WO : IOMMU_RO);
> >           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?

Thanks,
Maxime
> Otherwise looks good.
>
> Paolo
>
> > +        plen = MIN(plen, (addr | iotlb.addr_mask) - addr + 1);
> >           if (!(iotlb.perm & (1 << is_write))) {
> >               goto translate_fail;