[v2] hw/i386/x86-iommu: Correctly set pt bit in extended capability register

[PATCH v2] hw/i386/x86-iommu: Correctly set pt bit in extended capability register

Posted by no92 via qemu development 1 month ago

With the changes in c7b2e22bd957, the `pt` bit was set in the (wrong)
capability register, instead of the (correct) extended capability
register.

Fixes: c7b2e22bd957 ("hw/i386/x86-iommu: Remove X86IOMMUState::pt_supported field")
Signed-off-by: no92 <leo@managarm.org>
Reviewed-by: Clement Mathieu--Drif <clement.mathieu--drif@bull.com>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
---
v1: https://lore.kernel.org/qemu-devel/20260622162138.1045170-1-leo@managarm.org/

 hw/i386/intel_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 744cdfd2e6..d1af7a3135 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4988,7 +4988,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
 {
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);

-    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
+    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
              VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SSLPS | VTD_CAP_DRAIN |
              VTD_CAP_ESRTPS | VTD_CAP_MGAW(s->aw_bits);
     if (x86_iommu->dma_translation) {
@@ -4999,7 +4999,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
                     s->cap |= VTD_CAP_SAGAW_48bit;
             }
     }
-    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
+    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO | VTD_ECAP_PT;

     if (x86_iommu_ir_supported(x86_iommu)) {
         s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
--
2.54.0

Re: [PATCH v2] hw/i386/x86-iommu: Correctly set pt bit in extended capability register

Posted by Michael S. Tsirkin 1 month ago

On Tue, Jun 23, 2026 at 10:22:23AM +0200, no92 wrote:
> With the changes in c7b2e22bd957, the `pt` bit was set in the (wrong)
> capability register, instead of the (correct) extended capability
> register.
> 
> Fixes: c7b2e22bd957 ("hw/i386/x86-iommu: Remove X86IOMMUState::pt_supported field")
> Signed-off-by: no92 <leo@managarm.org>
> Reviewed-by: Clement Mathieu--Drif <clement.mathieu--drif@bull.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> ---
> v1: https://lore.kernel.org/qemu-devel/20260622162138.1045170-1-leo@managarm.org/

changes since v1?

>  hw/i386/intel_iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 744cdfd2e6..d1af7a3135 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4988,7 +4988,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
>  {
>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> 
> -    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
> +    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
>               VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SSLPS | VTD_CAP_DRAIN |
>               VTD_CAP_ESRTPS | VTD_CAP_MGAW(s->aw_bits);
>      if (x86_iommu->dma_translation) {
> @@ -4999,7 +4999,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
>                      s->cap |= VTD_CAP_SAGAW_48bit;
>              }
>      }
> -    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
> +    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO | VTD_ECAP_PT;
> 
>      if (x86_iommu_ir_supported(x86_iommu)) {
>          s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
> --
> 2.54.0

Re: [PATCH v2] hw/i386/x86-iommu: Correctly set pt bit in extended capability register

Posted by Clément MATHIEU--DRIF 1 month ago

Just noticed that the commit title prefix is not correct: s@hw/i386/x86-iommu:@intel_iommu:

cmd

On Tue, 2026-06-23 at 05:12 -0400, Michael S. Tsirkin wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> On Tue, Jun 23, 2026 at 10:22:23AM +0200, no92 wrote:
>
> > With the changes in c7b2e22bd957, the `pt` bit was set in the (wrong)
> > capability register, instead of the (correct) extended capability
> > register.
> >
> > Fixes: c7b2e22bd957 ("hw/i386/x86-iommu: Remove X86IOMMUState::pt_supported field")
> > Signed-off-by: no92 <[leo@managarm.org](mailto:leo@managarm.org)>
> > Reviewed-by: Clement Mathieu--Drif <[clement.mathieu--drif@bull.com](mailto:clement.mathieu--drif@bull.com)>
> > Reviewed-by: Yi Liu <[yi.l.liu@intel.com](mailto:yi.l.liu@intel.com)>
> > ---
> > v1: [https://lore.kernel.org/qemu-devel/20260622162138.1045170-1-leo@managarm.org/](https://lore.kernel.org/qemu-devel/20260622162138.1045170-1-leo@managarm.org/)
>
>
> changes since v1?
>
>
> >  hw/i386/intel_iommu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 744cdfd2e6..d1af7a3135 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -4988,7 +4988,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
> >  {
> >      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> >
> > -    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
> > +    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
> >               VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SSLPS | VTD_CAP_DRAIN |
> >               VTD_CAP_ESRTPS | VTD_CAP_MGAW(s->aw_bits);
> >      if (x86_iommu->dma_translation) {
> > @@ -4999,7 +4999,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
> >                      s->cap |= VTD_CAP_SAGAW_48bit;
> >              }
> >      }
> > -    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
> > +    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO | VTD_ECAP_PT;
> >
> >      if (x86_iommu_ir_supported(x86_iommu)) {
> >          s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
> > --
> > 2.54.0
>
>

[PATCH v3] intel_iommu: Correctly set pt bit in extended capability register

Posted by no92 via qemu development 1 month ago

With the changes in c7b2e22bd957, the `pt` bit was set in the (wrong)
capability register, instead of the (correct) extended capability
register.

Fixes: c7b2e22bd957 ("hw/i386/x86-iommu: Remove X86IOMMUState::pt_supported field")
Signed-off-by: no92 <leo@managarm.org>
Reviewed-by: Clement Mathieu--Drif <clement.mathieu--drif@bull.com>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
---
The only changes to v1 are in the commit message phrasing, as well as
adding the Reviewed-by lines. Apologies for the confusion, I'm not that
well-versed in the mailing list workflow.

 hw/i386/intel_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 744cdfd2e6..d1af7a3135 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4988,7 +4988,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
 {
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);

-    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
+    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
              VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SSLPS | VTD_CAP_DRAIN |
              VTD_CAP_ESRTPS | VTD_CAP_MGAW(s->aw_bits);
     if (x86_iommu->dma_translation) {
@@ -4999,7 +4999,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
                     s->cap |= VTD_CAP_SAGAW_48bit;
             }
     }
-    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
+    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO | VTD_ECAP_PT;

     if (x86_iommu_ir_supported(x86_iommu)) {
         s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
--
2.54.0

Re: [PATCH v3] intel_iommu: Correctly set pt bit in extended capability register

Posted by Yi Liu 1 month ago

Just a few nits for your future work on the mailing list.

On 6/24/26 18:39, no92 wrote:
> With the changes in c7b2e22bd957, the `pt` bit was set in the (wrong)
> capability register, instead of the (correct) extended capability
> register.
> 
> Fixes: c7b2e22bd957 ("hw/i386/x86-iommu: Remove X86IOMMUState::pt_supported field")
> Signed-off-by: no92 <leo@managarm.org>

It's generally preferred to use your full name (First Last) here unless 
you have a specific reason not to — but it's entirely up to you. 🙂

> Reviewed-by: Clement Mathieu--Drif <clement.mathieu--drif@bull.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> ---
> The only changes to v1 are in the commit message phrasing, as well as
> adding the Reviewed-by lines. Apologies for the confusion, I'm not that
> well-versed in the mailing list workflow.

Since this is a single patch, placing the change log between the --- and 
the diffstat is the right spot — good instinct! That said, it helps to 
be a bit more structured. Here's a suggested format for reference:

"
---
Change log:

v3:
- Tweak the prefix of the patch subject (Clement)

v2: 
https://lore.kernel.org/qemu-devel/20260623082223.1431499-1-leo@managarm.org/
- Use 12bits commit id in the fix tag (Yi)
- Adding r-bs

v1: 
https://lore.kernel.org/qemu-devel/20260622162138.1045170-1-leo@managarm.org/
---
"

The per-version links are especially useful when the patch subject
changes or the patch gets split during review, as they help reviewers,
maintainers, and anyone following along to trace the full history.

Last but not least, thanks for tracking down and fixing this bug!

> 
>   hw/i386/intel_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 744cdfd2e6..d1af7a3135 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4988,7 +4988,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
>   {
>       X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> 
> -    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
> +    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
>                VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SSLPS | VTD_CAP_DRAIN |
>                VTD_CAP_ESRTPS | VTD_CAP_MGAW(s->aw_bits);
>       if (x86_iommu->dma_translation) {
> @@ -4999,7 +4999,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
>                       s->cap |= VTD_CAP_SAGAW_48bit;
>               }
>       }
> -    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
> +    s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO | VTD_ECAP_PT;
> 
>       if (x86_iommu_ir_supported(x86_iommu)) {
>           s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
> --
> 2.54.0
>