[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_
From: |
Eric Auger |
Subject: |
Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask() |
Date: |
Wed, 5 Jul 2023 15:16:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 |
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.
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
- RE: [PATCH 1/2] virtio-iommu: Fix 64kB host page size VFIO device assignment, (continued)
- RE: [PATCH 1/2] virtio-iommu: Fix 64kB host page size VFIO device assignment, Duan, Zhenzhong, 2023/07/05
- Re: [PATCH 1/2] virtio-iommu: Fix 64kB host page size VFIO device assignment, Jean-Philippe Brucker, 2023/07/05
- RE: [PATCH 1/2] virtio-iommu: Fix 64kB host page size VFIO device assignment, Duan, Zhenzhong, 2023/07/05
- Re: [PATCH 1/2] virtio-iommu: Fix 64kB host page size VFIO device assignment, Jean-Philippe Brucker, 2023/07/05
- RE: [PATCH 1/2] virtio-iommu: Fix 64kB host page size VFIO device assignment, Duan, Zhenzhong, 2023/07/06
[PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Eric Auger, 2023/07/04
- RE: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Duan, Zhenzhong, 2023/07/05
- RE: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Duan, Zhenzhong, 2023/07/05
- Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(),
Eric Auger <=
- RE: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Duan, Zhenzhong, 2023/07/06
- Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Jean-Philippe Brucker, 2023/07/06
- Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Eric Auger, 2023/07/06
- RE: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Duan, Zhenzhong, 2023/07/06
- Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask(), Michael S. Tsirkin, 2023/07/06