On Wed, Jul 05, 2023 at 03:16:31PM +0200, Eric Auger wrote:
> 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.au...@redhat.com>
> >>> Sent: Tuesday, July 4, 2023 7:15 PM
> >>> To: eric.auger....@gmail.com; eric.au...@redhat.com; qemu-
> >>> de...@nongnu.org; qemu-...@nongnu.org; m...@redhat.com; jean-
> >>> phili...@linaro.org; Duan, Zhenzhong <zhenzhong.d...@intel.com>
> >>> Cc: alex.william...@redhat.com; c...@redhap.com;
> >> bharat.bhus...@nxp.com;
> >>> peter.mayd...@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.au...@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.
Maybe add a comment down the road. Not urgent.
> 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.d...@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
