RE: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask()

[Duan, Zhenzhong]

Hi Eric,
>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Wednesday, July 5, 2023 9:17 PM
>Subject: Re: [PATCH 2/2] virtio-iommu: Rework the trace in
>virtio_iommu_set_page_size_mask()
>
>Hi Zhenghong,
>
>On 7/5/23 10:17, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Duan, Zhenzhong
>>> Sent: Wednesday, July 5, 2023 12:56 PM
>>> Subject: RE: [PATCH 2/2] virtio-iommu: Rework the trace in
>>> virtio_iommu_set_page_size_mask()
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>> Sent: Tuesday, July 4, 2023 7:15 PM
>>>> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
>>>> devel@nongnu.org; qemu-arm@nongnu.org; mst@redhat.com; jean-
>>>> philippe@linaro.org; Duan, Zhenzhong <zhenzhong.duan@intel.com>
>>>> Cc: alex.williamson@redhat.com; clg@redhap.com;
>>> bharat.bhushan@nxp.com;
>>>> peter.maydell@linaro.org
>>>> Subject: [PATCH 2/2] virtio-iommu: Rework the trace in
>>>> virtio_iommu_set_page_size_mask()
>>>>
>>>> The current error messages in virtio_iommu_set_page_size_mask()
>>>> sound quite similar for different situations and miss the IOMMU
>>>> memory region that causes the issue.
>>>>
>>>> Clarify them and rework the comment.
>>>>
>>>> Also remove the trace when the new page_size_mask is not applied as
>>>> the current frozen granule is kept. This message is rather confusing
>>>> for the end user and anyway the current granule would have been used
>>>> by the driver
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>> hw/virtio/virtio-iommu.c | 19 +++++++------------
>>>> 1 file changed, 7 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>>> index 1eaf81bab5..0d9f7196fe 100644
>>>> --- a/hw/virtio/virtio-iommu.c
>>>> +++ b/hw/virtio/virtio-iommu.c
>>>> @@ -1101,29 +1101,24 @@ static int
>>>> virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>>>>                                           new_mask);
>>>>
>>>>     if ((cur_mask & new_mask) == 0) {
>>>> -        error_setg(errp, "virtio-iommu page mask 0x%"PRIx64
>>>> -                   " is incompatible with mask 0x%"PRIx64, cur_mask,
>new_mask);
>>>> +        error_setg(errp, "virtio-iommu %s reports a page size mask
>0x%"PRIx64
>>>> +                   " incompatible with currently supported mask 
>>>> 0x%"PRIx64,
>>>> +                   mr->parent_obj.name, new_mask, cur_mask);
>>>>         return -1;
>>>>     }
>>>>
>>>>     /*
>>>>      * Once the granule is frozen we can't change the mask anymore. If by
>>>>      * chance the hotplugged device supports the same granule, we can still
>>>> -     * accept it. Having a different masks is possible but the guest will 
>>>> use
>>>> -     * sub-optimal block sizes, so warn about it.
>>>> +     * accept it.
>>>>      */
>>>>     if (s->granule_frozen) {
>>>> -        int new_granule = ctz64(new_mask);
>>>>         int cur_granule = ctz64(cur_mask);
>>>>
>>>> -        if (new_granule != cur_granule) {
>>>> -            error_setg(errp, "virtio-iommu page mask 0x%"PRIx64
>>>> -                       " is incompatible with mask 0x%"PRIx64, cur_mask,
>>>> -                       new_mask);
>>>> +        if (!(BIT(cur_granule) & new_mask)) {
>> Sorry, I read this piece code again and got a question, if new_mask
>> has finer granularity than cur_granule, should we allow it to pass
>> even though
>> BIT(cur_granule) is not set?
>I think this should work but this is not straightforward to test.
>virtio-iommu would use the current granule for map/unmap. In map/unmap
>notifiers, this is split into pow2 ranges and cascaded to VFIO through
>vfio_dma_map/unmap. The iova and size are aligned with the smaller
>supported granule.

I see, guess you mean malicious guest which may send any mapping request
to virtio-iommu backend. A well designed guest may use at least cur_granule
as the granularity of its mapping, even with pow2 split, cur_granule is
guaranteed at backend.

Thanks
Zhenzhong

>
>Jean, do you share this understanding or do I miss something.
>
>Nevertheless the current code would have rejected that case and nobody
>complained at that point ;-)
>
>thanks
>
>Eric
>
>>
>> Thanks
>> Zhenzhong
>>
>>> Good catch.
>>>
>>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>
>>> Thanks
>>> Zhenzhong
>>>
>>>> +            error_setg(errp, "virtio-iommu %s does not support
>>>> + frozen granule
>>>> 0x%"PRIx64,
>>>> +                       mr->parent_obj.name, BIT(cur_granule));
>>>>             return -1;
>>>> -        } else if (new_mask != cur_mask) {
>>>> -            warn_report("virtio-iommu page mask 0x%"PRIx64
>>>> -                        " does not match 0x%"PRIx64, cur_mask, new_mask);
>>>>         }
>>>>         return 0;
>>>>     }
>>>> --
>>>> 2.38.1