Hi Jason,
On 1/11/23 08:14, Jason Wang wrote:
> On Tue, Jan 10, 2023 at 5:11 AM Eric Auger <eau...@redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/9/23 14:24, Eric Auger wrote:
>>> Hi,
>>>
>>> we have a trouble with virtio-iommu and protected assigned devices
>>> downstream to a pcie-to-pci bridge. In that use case we observe the
>>> assigned devices are not put to any group. This is true on both x86 and
>>> aarch64. This use case works with intel-iommu.
>>>
>>> *** Guest PCI topology is:
>>> lspci -tv
>>> -[0000:00]-+-00.0 Intel Corporation 82G33/G31/P35/P31 Express DRAM
>>> Controller
>>> +-01.0 Device 1234:1111
>>> +-02.0-[01-02]----00.0-[02]----01.0 Broadcom Inc. and
>>> subsidiaries BCM57416 NetXtreme-E Dual-Media 10G RDMA Ethernet Controller
>>> +-02.1-[03]--
>>> +-02.2-[04]----00.0 Red Hat, Inc. Virtio block device
>>> +-0a.0 Red Hat, Inc. Device 1057
>>> +-1f.0 Intel Corporation 82801IB (ICH9) LPC Interface Controller
>>> +-1f.2 Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port
>>> SATA Controller [AHCI mode]
>>> \-1f.3 Intel Corporation 82801I (ICH9 Family) SMBus Controller
>>>
>>>
>>> All the assigned devices are aliased and they get devfn=0x0.
>>> see qemu pci_device_iommu_address_space in hw/pci.c
>>>
>>> Initially I see the following traces
>>> pci_device_iommu_address_space name=vfio-pci BDF=0x8 bus=0 devfn=0x8
>>> pci_device_iommu_address_space name=vfio-pci BDF=0x8 bus=0 devfn=0x8
>>> call iommu_fn with bus=0x55f556dde180 and devfn=0
>>> virtio_iommu_init_iommu_mr init virtio-iommu-memory-region-0-0
>>>
>>> Note the bus is 0 at this time and devfn that is used in the
>>> virtio-iommu is 0. So an associated IOMMU MR is created with this bus at
>>> devfn=0 slot. This is before bus actual numbering.
>>>
>>> However later on, I see virtio_iommu_probe() and virtio_iommu_attach()
>>> getting called with ep_id=520
>>> because in the qemu virtio-iommu device, virtio_iommu_mr(pe_id) fails to
>>> find the iommu_mr and returns -ENOENT
>>>
>>> On guest side I see that
>>> acpi_iommu_configure_id/iommu_probe_device() fails
>>> (__iommu_probe_device) and also __iommu_attach_device would also fail
>>> anyway.
>>>
>>> I guess those get called before actual bus number recomputation?
>>>
>>> on aarch64 I eventually see the "good" MR beeing created, ie. featuring
>>> the right bus number:
>>> qemu-system-aarch64: pci_device_iommu_address_space name=vfio-pci
>>> BDF=0x208 bus=2 devfn=0x8
>>> qemu-system-aarch64: pci_device_iommu_address_space name=vfio-pci
>>> BDF=0x208 bus=2 devfn=0x8 call iommu_fn with bus=0xaaaaef12c450 and devfn=0
>>>
>>> But this does not happen on x86.
>>>
>>> Jean, do you have any idea about how to fix that? Do you think we have a
>>> trouble in the acpi/viot setup or virtio-iommu probe sequence. It looks
>>> like virtio probe and attach commands are called too early, before the
>>> bus is actually correctly numbered.
>>
>> So after further investigations looks this is not a problem of bus
>> number, which is good at the time of the virtio cmd calls but rather a
>> problem related to the devfn (0 was used when creating the IOMMU MR)
>> whereas the virtio-iommu cmds looks for the non aliased devfn. With that
>> fixed, the probe and attach at least succeeds. The device still does not
>> work for me but I will continue my investigations and send a tentative fix.
>
> Haven't thought this deeply, just one thing in my mind and in case
> that may help:
Sorry for the delay, I did not see the follow-ups on this thread :-(,
>
> intel-iommu doesn't use bus no as the key for hashing address spaces
> since it could be configured by the guest:
>
> /*
> * Note that we use pointer to PCIBus as the key, so hashing/shifting
> * based on the pointer value is intended. Note that we deal with
> * collisions through vtd_as_equal().
> */
> static guint vtd_as_hash(gconstpointer v)
> {
> const struct vtd_as_key *key = v;
> guint value = (guint)(uintptr_t)key->bus;
>
> return (guint)(value << 8 | key->devfn);
> }
I think we have something similar on virtio-iommu. We use the old
flavour "as_by_busptr" whose key is the PCIBus pointer. This was
basically copied from the intel-iommu and then you replaced it with
da8d439c8048 ("intel-iommu: drop VTDBus")
Thanks
Eric
>
> Thanks
>
>>
>> Thanks
>>
>> Eric
>>>
>>> Thanks
>>>
>>> Eric
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
