| Message ID | 20240122064015.94630-2-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Two minor fixes on virtio-iommu | expand |
On 1/22/24 07:40, Zhenzhong Duan wrote: > IOMMUPciBus pointer cache is 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. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/virtio/virtio-iommu.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index 8a4bd933c6..bfce3237f3 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 you could remove the memset in virtio_iommu_device_realize() then ? Thanks, C.
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache >when system reset > >On 1/22/24 07:40, Zhenzhong Duan wrote: >> IOMMUPciBus pointer cache is 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. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/virtio/virtio-iommu.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index 8a4bd933c6..bfce3237f3 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 > >you could remove the memset in virtio_iommu_device_realize() then ? Good suggestion, will do. Thanks Zhenzhong
Hi Zhenzhong, On 1/23/24 11:03, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Subject: Re: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache >> when system reset >> >> On 1/22/24 07:40, Zhenzhong Duan wrote: >>> IOMMUPciBus pointer cache is 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 cannot harm to reset it but I don't know if this can be encountered. >>> >>> This could lead to endpoint binding to wrong iommu MR in >>> virtio_iommu_get_endpoint(), then vfio device setup wrong >>> mapping from other device. >>> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/virtio/virtio-iommu.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >>> index 8a4bd933c6..bfce3237f3 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 >> you could remove the memset in virtio_iommu_device_realize() then ? > Good suggestion, will do. By the way what about the vtd implementation. s->vtd_address_spaces is hash table but I don't see it reset either? Also if is requested here we would need it on smmuv3 as well. Thanks Eric > > Thanks > Zhenzhong
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache >when system reset > >Hi Zhenzhong, > >On 1/23/24 11:03, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Cédric Le Goater <clg@redhat.com> >>> Subject: Re: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer >cache >>> when system reset >>> >>> On 1/22/24 07:40, Zhenzhong Duan wrote: >>>> IOMMUPciBus pointer cache is 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 cannot harm to reset it but I don't know if this can be encountered. >>>> >>>> This could lead to endpoint binding to wrong iommu MR in >>>> virtio_iommu_get_endpoint(), then vfio device setup wrong >>>> mapping from other device. >>>> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> hw/virtio/virtio-iommu.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >>>> index 8a4bd933c6..bfce3237f3 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 >>> you could remove the memset in virtio_iommu_device_realize() then ? >> Good suggestion, will do. >By the way what about the vtd implementation. s->vtd_address_spaces is >hash table but I don't see it reset either? Good question! s->vtd_address_spaces is indexed by a key containing (PCIBus *) which is reliable. /* * PCI bus number (or SID) is not reliable since the device is usaully * initialized before guest can configure the PCI bridge * (SECONDARY_BUS_NUMBER). */ struct vtd_as_key { PCIBus *bus; uint8_t devfn; uint32_t pasid; }; So I don’t think it should reset, same for s->as_by_busptr in virtio-iommu. s->vtd_as_cache[bus_num] is unstable after guest reboot, but vtd_get_as_by_sid() has logic to verify and update cache, so it doesn't have issue. But if we hotplug/unplug bridge in a loop, there may be issue with s->vtd_address_spaces Because old AS is never released. Anyway that's beyond scope of this patch. >Also if is requested here we would need it on smmuv3 as well. Good suggestion, I think so, I'll add a patch to smmuv3 for review. Thanks Zhenzhong
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 8a4bd933c6..bfce3237f3 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
IOMMUPciBus pointer cache is 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. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/virtio/virtio-iommu.c | 2 ++ 1 file changed, 2 insertions(+)