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

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 <[email protected]>

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 <[email protected]>
> Reviewed-by: Yi Liu <[email protected]>
> ---
> 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/[email protected]/
- Use 12bits commit id in the fix tag (Yi)
- Adding r-bs

v1: 
https://lore.kernel.org/qemu-devel/[email protected]/
---
"

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
>