1. Patchew
  2. QEMU
  3. [PATCH 0/3] Two minor fixes on virtio-iommu
  4. View patch

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

Zhenzhong Duan posted 3 patches 10 months, 1 week ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Eric Auger <eric.auger@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Peter Xu <peterx@redhat.com>, Jason Wang <jasowang@redhat.com>, Helge Deller <deller@gmx.de>, Andrey Smirnov <andrew.smirnov@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, "Frédéric Barrat" <fbarrat@linux.ibm.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, BALATON Zoltan <balaton@eik.bme.hu>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>
[PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
Posted by Zhenzhong Duan 10 months, 1 week ago 
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
-- 
2.34.1
Re: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
Posted by Cédric Le Goater 10 months, 1 week ago 
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.
RE: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
Posted by Duan, Zhenzhong 10 months, 1 week ago 

>-----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
Re: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
Posted by Eric Auger 10 months ago 
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
RE: [PATCH 1/3] virtio_iommu: Clear IOMMUPciBus pointer cache when system reset
Posted by Duan, Zhenzhong 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.