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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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