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

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.au...@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 <c...@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.d...@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