[PATCH 0/2] Cleanups and fixes (PART 2)

Sairaj Kodilkar posted 2 patches 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251008164324.21553-1-sarunkod@amd.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
hw/i386/amd_iommu.c | 166 +++++++++++++++++++++++++++-----------------
hw/i386/amd_iommu.h |   7 +-
2 files changed, 106 insertions(+), 67 deletions(-)
[PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Sairaj Kodilkar 1 month, 1 week ago
This series provide fixes for following two issues:

1. AMD IOMMU fails to detect the devices when they are attached to PCI bus with
   bus id != 0.
   e.g. With following command line, dhclient command fails inside the guest

    -device pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x5 \
    -netdev user,id=USER0,hostfwd=tcp::3333-:22 \
    -device virtio-net-pci,id=vnet0,iommu_platform=on,disable-legacy=on,romfile=,netdev=USER0,bus=pci.1,addr=0 \

2. Current AMD IOMMU supports IOVAs upto 60 bit which cause failure while
   setting up the devices when guest is booted with command line 
   "iommu.forcedac=1".

   One example of the failure is when there are two virtio ethernet devices
   attached to the guest with command line
   
       -netdev user,id=USER0 \
       -netdev user,id=USER1 \
       -device virtio-net-pci,id=vnet0,iommu_platform=on,disable-legacy=on,romfile=,netdev=USER0 \
       -device virtio-net-pci,id=vnet1,iommu_platform=on,disable-legacy=on,romfile=,netdev=USER1 \
   
   In this case dhclient fails for second device with following dmesg
   
   [   24.802644] virtio_net virtio0 enp0s1: TX timeout on queue: 0, sq: output.0, vq: 0x1, name: output.0, 5664000 usecs ago
   [   29.856716] virtio_net virtio0 enp0s1: NETDEV WATCHDOG: CPU: 59: transmit queue 0 timed out 10720 ms
   [   29.858585] virtio_net virtio0 enp0s1: TX timeout on queue: 0, sq: output.0, vq: 0x1, name: output.0, 10720000 usecs ago

Base commit: (qemu uptream) eb7abb4a719f

Sairaj Kodilkar (2):
  hw/i386/amd_iommu: Fix handling device on buses != 0
  hw/i386/amd_iommu: Support 64 bit address for IOTLB lookup

 hw/i386/amd_iommu.c | 166 +++++++++++++++++++++++++++-----------------
 hw/i386/amd_iommu.h |   7 +-
 2 files changed, 106 insertions(+), 67 deletions(-)

-- 
2.34.1
Re: [PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Alejandro Jimenez 1 month ago

On 10/8/25 12:43 PM, Sairaj Kodilkar wrote:
> This series provide fixes for following two issues:
> 

I was able to reproduce and confirm the fixes for the issues you list. 
Please see my replies to the individual patches for my suggestions.

> 1. AMD IOMMU fails to detect the devices when they are attached to PCI bus with
>     bus id != 0.
>     e.g. With following command line, dhclient command fails inside the guest
> 
>      -device pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x5 \
>      -netdev user,id=USER0,hostfwd=tcp::3333-:22 \
>      -device virtio-net-pci,id=vnet0,iommu_platform=on,disable-legacy=on,romfile=,netdev=USER0,bus=pci.1,addr=0 \
> 
> 2. Current AMD IOMMU supports IOVAs upto 60 bit which cause failure while
>     setting up the devices when guest is booted with command line
>     "iommu.forcedac=1".
> 
>     One example of the failure is when there are two virtio ethernet devices
>     attached to the guest with command line
>     
>         -netdev user,id=USER0 \
>         -netdev user,id=USER1 \
>         -device virtio-net-pci,id=vnet0,iommu_platform=on,disable-legacy=on,romfile=,netdev=USER0 \
>         -device virtio-net-pci,id=vnet1,iommu_platform=on,disable-legacy=on,romfile=,netdev=USER1 \
>     
>     In this case dhclient fails for second device with following dmesg
>     
>     [   24.802644] virtio_net virtio0 enp0s1: TX timeout on queue: 0, sq: output.0, vq: 0x1, name: output.0, 5664000 usecs ago
>     [   29.856716] virtio_net virtio0 enp0s1: NETDEV WATCHDOG: CPU: 59: transmit queue 0 timed out 10720 ms
>     [   29.858585] virtio_net virtio0 enp0s1: TX timeout on queue: 0, sq: output.0, vq: 0x1, name: output.0, 10720000 usecs ago
> 
> Base commit: (qemu uptream) eb7abb4a719f
> 

General feedback for both patches:

I know the commit log is not consistent so far, but going forward I 
propose we adopt the shorter prefix "amd_iommu: " for commit summaries. 
There is no ambiguity (only one arch has amd_iommu), so the full path is 
not required (i.e. avoid 'hw/i386/amd_iommu: '). Shorter boilerplate 
leaves more space for relevant details, and helps people like me who 
struggle to comply with character limits :).

Thank you,
Alejandro

> Sairaj Kodilkar (2):
>    hw/i386/amd_iommu: Fix handling device on buses != 0
>    hw/i386/amd_iommu: Support 64 bit address for IOTLB lookup
> 
>   hw/i386/amd_iommu.c | 166 +++++++++++++++++++++++++++-----------------
>   hw/i386/amd_iommu.h |   7 +-
>   2 files changed, 106 insertions(+), 67 deletions(-)
>
Re: [PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Philippe Mathieu-Daudé 1 month ago
Hi Alejandro,

On 10/10/25 03:33, Alejandro Jimenez wrote:

> I know the commit log is not consistent so far, but going forward I 
> propose we adopt the shorter prefix "amd_iommu: " for commit summaries. 
> There is no ambiguity (only one arch has amd_iommu), so the full path is 
> not required (i.e. avoid 'hw/i386/amd_iommu: '). Shorter boilerplate 
> leaves more space for relevant details, and helps people like me who 
> struggle to comply with character limits :).

What about "hw/amd_iommu:" to keep 'hw' in subject?
Re: [PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Alejandro Jimenez 1 month ago
Hi Philippe,

On 10/9/25 10:19 PM, Philippe Mathieu-Daudé wrote:
> Hi Alejandro,
> 
> On 10/10/25 03:33, Alejandro Jimenez wrote:
> 
>> I know the commit log is not consistent so far, but going forward I 
>> propose we adopt the shorter prefix "amd_iommu: " for commit 
>> summaries. There is no ambiguity (only one arch has amd_iommu), so the 
>> full path is not required (i.e. avoid 'hw/i386/amd_iommu: '). Shorter 
>> boilerplate leaves more space for relevant details, and helps people 
>> like me who struggle to comply with character limits :).
> 
> What about "hw/amd_iommu:" to keep 'hw' in subject?

Is there any tooling that relies on the hw prefix? Skipping the arch in 
the prefix is confusing I think, since hw/amd_iommu is not a valid path 
in the repository.

I was looking for precedent of any preferred format in the commit logs 
under hw/i386/ and there is a lot of variance. But specifically for 
IOMMU emulation code, my interpretation is that the short prefix style 
is most commonly used e.g.

Common x86 IOMMU uses "x86-iommu: "

The VT-d changes are typically in the form:
"intel_iommu: XYZ", which Clément also pointed out recently in:
https://lore.kernel.org/qemu-devel/f97bc435e8ed1c295919350d300068e45ab0bb67.camel@eviden.com/

virtio IOMMU uses "virtio-iommu: "

RISC-V IOMMU uses the full path: "hw/riscv/riscv-iommu: "

SPARC64 has a few commits with "sun4u_iommu: "

I don't believe the 'hw' component is required to avoid ambiguity, but 
perhaps there is something else I am missing...

Thank you,
Alejandro

Re: [PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Michael S. Tsirkin 1 month ago
On Fri, Oct 10, 2025 at 10:37:55AM -0400, Alejandro Jimenez wrote:
> Hi Philippe,
> 
> On 10/9/25 10:19 PM, Philippe Mathieu-Daudé wrote:
> > Hi Alejandro,
> > 
> > On 10/10/25 03:33, Alejandro Jimenez wrote:
> > 
> > > I know the commit log is not consistent so far, but going forward I
> > > propose we adopt the shorter prefix "amd_iommu: " for commit
> > > summaries. There is no ambiguity (only one arch has amd_iommu), so
> > > the full path is not required (i.e. avoid 'hw/i386/amd_iommu: ').
> > > Shorter boilerplate leaves more space for relevant details, and
> > > helps people like me who struggle to comply with character limits
> > > :).
> > 
> > What about "hw/amd_iommu:" to keep 'hw' in subject?
> 
> Is there any tooling that relies on the hw prefix? Skipping the arch in the
> prefix is confusing I think, since hw/amd_iommu is not a valid path in the
> repository.
> 
> I was looking for precedent of any preferred format in the commit logs under
> hw/i386/ and there is a lot of variance. But specifically for IOMMU
> emulation code, my interpretation is that the short prefix style is most
> commonly used e.g.
> 
> Common x86 IOMMU uses "x86-iommu: "
> 
> The VT-d changes are typically in the form:
> "intel_iommu: XYZ", which Clément also pointed out recently in:
> https://lore.kernel.org/qemu-devel/f97bc435e8ed1c295919350d300068e45ab0bb67.camel@eviden.com/
> 
> virtio IOMMU uses "virtio-iommu: "
> 
> RISC-V IOMMU uses the full path: "hw/riscv/riscv-iommu: "
> 
> SPARC64 has a few commits with "sun4u_iommu: "
> 
> I don't believe the 'hw' component is required to avoid ambiguity, but
> perhaps there is something else I am missing...
> 
> Thank you,
> Alejandro

FWIW I like amd_iommu

-- 
MST
Re: [PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Sairaj Kodilkar 1 month ago

On 10/10/2025 7:49 AM, Philippe Mathieu-Daudé wrote:
> Hi Alejandro,
>
> On 10/10/25 03:33, Alejandro Jimenez wrote:
>
>> I know the commit log is not consistent so far, but going forward I 
>> propose we adopt the shorter prefix "amd_iommu: " for commit 
>> summaries. There is no ambiguity (only one arch has amd_iommu), so 
>> the full path is not required (i.e. avoid 'hw/i386/amd_iommu: '). 
>> Shorter boilerplate leaves more space for relevant details, and helps 
>> people like me who struggle to comply with character limits :).
>
> What about "hw/amd_iommu:" to keep 'hw' in subject?
I also prefer "amd_iommu". But I'll wait for alejandro's reply before 
posting V2.

Re: [PATCH 0/2] Cleanups and fixes (PART 2)
Posted by Michael S. Tsirkin 1 month ago
On Fri, Oct 10, 2025 at 10:55:45AM +0530, Sairaj Kodilkar wrote:
> 
> 
> On 10/10/2025 7:49 AM, Philippe Mathieu-Daudé wrote:
> > Hi Alejandro,
> > 
> > On 10/10/25 03:33, Alejandro Jimenez wrote:
> > 
> > > I know the commit log is not consistent so far, but going forward I
> > > propose we adopt the shorter prefix "amd_iommu: " for commit
> > > summaries. There is no ambiguity (only one arch has amd_iommu), so
> > > the full path is not required (i.e. avoid 'hw/i386/amd_iommu: ').
> > > Shorter boilerplate leaves more space for relevant details, and
> > > helps people like me who struggle to comply with character limits
> > > :).
> > 
> > What about "hw/amd_iommu:" to keep 'hw' in subject?
> I also prefer "amd_iommu". But I'll wait for alejandro's reply before
> posting V2.

If possible, pls use that prefix on the cover letter too.

Thanks!

-- 
MST