[PATCH v2 0/2] Two minor fixes on virtio-iommu and smmu

[PATCH v2 0/2] Two minor fixes on virtio-iommu and smmu

[Zhenzhong Duan]

Hi,

PATCH1 fixes a potential issue with vfio devices when reboot to a
different OS which set bus number differently from previous OS.
I didn't reproduce the issue in reality, but it's still possible
in theory. VTD doesn't have same issue as it use some verify logic
to ensure right iommu MR is picked.

PATCH2 does same thing for smmu.

v2:
- Remove redundant memset in realize (Cédric)
- Add a patch for smmu (Eric)
- Drop the patch to support PCI device alias for now, as it's tricky in
  using two different IOMMU MRs and Eric already sent a smarter fix.


Thanks
Zhenzhong

Zhenzhong Duan (2):
  virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
  smmu: Clear SMMUPciBus pointer cache when system reset

 hw/arm/smmu-common.c     | 2 ++
 hw/virtio/virtio-iommu.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] virtio_iommu: Clear IOMMUPciBus pointer cache when system reset

[Zhenzhong Duan]

s->iommu_pcibus_by_bus_num is a IOMMUPciBus pointer cache indexed
by bus number, bus number may not always be a fixed value,
i.e., guest reboot to different kernel which set bus number with
different algorithm.

This could lead to endpoint binding to wrong iommu MR in
virtio_iommu_get_endpoint(), then vfio device setup wrong
mapping from other device.

Remove the memset in virtio_iommu_device_realize() to avoid
redundancy with memset in system reset.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/virtio/virtio-iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 8a4bd933c6..86623d55a5 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1264,6 +1264,8 @@ static void virtio_iommu_system_reset(void *opaque)
 
     trace_virtio_iommu_system_reset();
 
+    memset(s->iommu_pcibus_by_bus_num, 0, sizeof(s->iommu_pcibus_by_bus_num));
+
     /*
      * config.bypass is sticky across device reset, but should be restored on
      * system reset
@@ -1302,8 +1304,6 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
 
     virtio_init(vdev, VIRTIO_ID_IOMMU, sizeof(struct virtio_iommu_config));
 
-    memset(s->iommu_pcibus_by_bus_num, 0, sizeof(s->iommu_pcibus_by_bus_num));
-
     s->req_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE,
                              virtio_iommu_handle_command);
     s->event_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE, NULL);
-- 
2.34.1

[PATCH v2 2/2] smmu: Clear SMMUPciBus pointer cache when system reset

[Zhenzhong Duan]

s->smmu_pcibus_by_bus_num is a SMMUPciBus pointer cache indexed
by bus number, bus number may not always be a fixed value,
i.e., guest reboot to different kernel which set bus number with
different algorithm.

This could lead to smmu_iommu_mr() providing the wrong iommu MR.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/arm/smmu-common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 9a8ac45431..f58261bb81 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -675,6 +675,8 @@ static void smmu_base_reset_hold(Object *obj)
 {
     SMMUState *s = ARM_SMMU(obj);
 
+    memset(s->smmu_pcibus_by_bus_num, 0, sizeof(s->smmu_pcibus_by_bus_num));
+
     g_hash_table_remove_all(s->configs);
     g_hash_table_remove_all(s->iotlb);
 }
-- 
2.34.1

Re: [PATCH v2 0/2] Two minor fixes on virtio-iommu and smmu

[Eric Auger]

Hi Zhenzhong,

On 1/25/24 08:37, Zhenzhong Duan wrote:
> Hi,
>
> PATCH1 fixes a potential issue with vfio devices when reboot to a
> different OS which set bus number differently from previous OS.
> I didn't reproduce the issue in reality, but it's still possible
> in theory. VTD doesn't have same issue as it use some verify logic
> to ensure right iommu MR is picked.
>
> PATCH2 does same thing for smmu.
>
> v2:
> - Remove redundant memset in realize (Cédric)
> - Add a patch for smmu (Eric)
> - Drop the patch to support PCI device alias for now, as it's tricky in
>   using two different IOMMU MRs and Eric already sent a smarter fix.

For the series:

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

>
>
> Thanks
> Zhenzhong
>
> Zhenzhong Duan (2):
>   virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
>   smmu: Clear SMMUPciBus pointer cache when system reset
>
>  hw/arm/smmu-common.c     | 2 ++
>  hw/virtio/virtio-iommu.c | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>

RE: [PATCH v2 0/2] Two minor fixes on virtio-iommu and smmu

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v2 0/2] Two minor fixes on virtio-iommu and smmu
>
>Hi Zhenzhong,
>
>On 1/25/24 08:37, Zhenzhong Duan wrote:
>> Hi,
>>
>> PATCH1 fixes a potential issue with vfio devices when reboot to a
>> different OS which set bus number differently from previous OS.
>> I didn't reproduce the issue in reality, but it's still possible
>> in theory. VTD doesn't have same issue as it use some verify logic
>> to ensure right iommu MR is picked.
>>
>> PATCH2 does same thing for smmu.
>>
>> v2:
>> - Remove redundant memset in realize (Cédric)
>> - Add a patch for smmu (Eric)
>> - Drop the patch to support PCI device alias for now, as it's tricky in
>>   using two different IOMMU MRs and Eric already sent a smarter fix.
>
>For the series:
>
>Reviewed-by: Eric Auger <eric.auger@redhat.com>
>Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks Eric.

BRs.
Zhenzhong