| Message ID | 20230704111527.3424992-3-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VIRTIO-IOMMU/VFIO page size related fixes | expand |
>-----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)) { 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
>-----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? 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
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
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
On Wed, Jul 05, 2023 at 03:16:31PM +0200, Eric Auger wrote: > >>> 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. Yes, I also think that would work. The guest would only issue mappings with the larger granularity, which can be applied by VFIO with a finer granule. However I doubt we're going to encounter this case, because seeing a cur_granule larger than 4k here means that a VFIO device has already been assigned with a large granule like 64k, and we're trying to add a new device with 4k. This indicates two HW IOMMUs supporting different granules in the same system, which seems unlikely. Hopefully by the time we actually need this (if ever) we will support per-endpoint probe properties, which allow informing the guest about different hardware properties instead of relying on one global property in the virtio config. Thanks, Jean
Hi Jean, On 7/6/23 16:35, Jean-Philippe Brucker wrote: > On Wed, Jul 05, 2023 at 03:16:31PM +0200, Eric Auger wrote: >>>>> 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. > Yes, I also think that would work. The guest would only issue mappings > with the larger granularity, which can be applied by VFIO with a finer > granule. However I doubt we're going to encounter this case, because > seeing a cur_granule larger than 4k here means that a VFIO device has > already been assigned with a large granule like 64k, and we're trying to > add a new device with 4k. This indicates two HW IOMMUs supporting > different granules in the same system, which seems unlikely. agreed > > Hopefully by the time we actually need this (if ever) we will support > per-endpoint probe properties, which allow informing the guest about > different hardware properties instead of relying on one global property in > the virtio config. yes looking forward to reviewing that. Thanks Eric > > Thanks, > Jean >
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.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. 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.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
>-----Original Message----- >From: Jean-Philippe Brucker <jean-philippe@linaro.org> >Sent: Thursday, July 6, 2023 10:36 PM >Subject: Re: [PATCH 2/2] virtio-iommu: Rework the trace in >virtio_iommu_set_page_size_mask() > >On Wed, Jul 05, 2023 at 03:16:31PM +0200, Eric Auger wrote: >> >>> 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. > >Yes, I also think that would work. The guest would only issue mappings with >the larger granularity, which can be applied by VFIO with a finer granule. >However I doubt we're going to encounter this case, because seeing a >cur_granule larger than 4k here means that a VFIO device has already been >assigned with a large granule like 64k, and we're trying to add a new device >with 4k. This indicates two HW IOMMUs supporting different granules in the >same system, which seems unlikely. Indeed. Another scenario I can think of is migration from src with 64k granule to dest with 4k, then hotplug a new device with 4k. Not clear if it's allowed to migrate between host with different granule. Thanks Zhenzhong
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)) { + 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; }
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(-)