Re: [PATCH v4] intel_iommu: Introduce property "stale-tm" to control Transient Mapping (TM) field

[Yi Liu <yi.l.liu@intel.com>]

On 2024/10/23 15:57, Zhenzhong Duan wrote:
> VT-d spec removed Transient Mapping (TM) field from second-level page-tables
> and treat the field as Reserved(0) since revision 3.2.
> 
> Changing the field as reserved(0) will break backward compatibility, so
> introduce a property "stale-tm" to allow user to control the setting.
> 
> Use hw_compat_9_1 to handle the compatibility for machines before 9.2 which

is hw_compat_9_1 a typo? Looks to be pc_compat_9_1. :)

Otherwise I think it is good.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> allow guest to set the field. Starting from 9.2, this field is reserved(0)
> by default to match spec. Of course, user can force it on command line.
> 
> This doesn't impact function of vIOMMU as there was no logic to emulate
> Transient Mapping.
> 
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> v4: s/x-stale-tm/stale-tm (Jason)
> v3: still need to check x86_iommu->dt_supported
> v2: introcude "x-stale-tm" to handle migration compatibility (Jason)
> 
>   hw/i386/intel_iommu_internal.h | 12 ++++++------
>   include/hw/i386/intel_iommu.h  |  3 +++
>   hw/i386/intel_iommu.c          |  7 ++++---
>   hw/i386/pc.c                   |  1 +
>   4 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 13d5d129ae..2f9bc0147d 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -412,8 +412,8 @@ typedef union VTDInvDesc VTDInvDesc;
>   /* Rsvd field masks for spte */
>   #define VTD_SPTE_SNP 0x800ULL
>   
> -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> -        dt_supported ? \
> +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, stale_tm) \
> +        stale_tm ? \
>           (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
>           (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
>   #define VTD_SPTE_PAGE_L2_RSVD_MASK(aw) \
> @@ -423,12 +423,12 @@ typedef union VTDInvDesc VTDInvDesc;
>   #define VTD_SPTE_PAGE_L4_RSVD_MASK(aw) \
>           (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
>   
> -#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw, dt_supported) \
> -        dt_supported ? \
> +#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw, stale_tm) \
> +        stale_tm ? \
>           (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
>           (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw, dt_supported) \
> -        dt_supported ? \
> +#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw, stale_tm) \
> +        stale_tm ? \
>           (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
>           (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
>   
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 1eb05c29fc..d372cd396b 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -306,6 +306,9 @@ struct IntelIOMMUState {
>       bool dma_translation;           /* Whether DMA translation supported */
>       bool pasid;                     /* Whether to support PASID */
>   
> +    /* Transient Mapping, Reserved(0) since VTD spec revision 3.2 */
> +    bool stale_tm;
> +
>       /*
>        * Protects IOMMU states in general.  Currently it protects the
>        * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 08fe218935..8612d0917b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3372,6 +3372,7 @@ static Property vtd_properties[] = {
>       DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>       DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
>       DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
> +    DEFINE_PROP_BOOL("stale-tm", IntelIOMMUState, stale_tm, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -4138,15 +4139,15 @@ static void vtd_init(IntelIOMMUState *s)
>        */
>       vtd_spte_rsvd[0] = ~0ULL;
>       vtd_spte_rsvd[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits,
> -                                                  x86_iommu->dt_supported);
> +                                        x86_iommu->dt_supported && s->stale_tm);
>       vtd_spte_rsvd[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits);
>       vtd_spte_rsvd[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits);
>       vtd_spte_rsvd[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s->aw_bits);
>   
>       vtd_spte_rsvd_large[2] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits,
> -                                                    x86_iommu->dt_supported);
> +                                        x86_iommu->dt_supported && s->stale_tm);
>       vtd_spte_rsvd_large[3] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits,
> -                                                    x86_iommu->dt_supported);
> +                                        x86_iommu->dt_supported && s->stale_tm);
>   
>       if (s->scalable_mode || s->snoop_control) {
>           vtd_spte_rsvd[1] &= ~VTD_SPTE_SNP;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2047633e4c..830614d930 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -82,6 +82,7 @@
>   GlobalProperty pc_compat_9_1[] = {
>       { "ICH9-LPC", "x-smi-swsmi-timer", "off" },
>       { "ICH9-LPC", "x-smi-periodic-timer", "off" },
> +    { TYPE_INTEL_IOMMU_DEVICE, "stale-tm", "on" },
>   };
>   const size_t pc_compat_9_1_len = G_N_ELEMENTS(pc_compat_9_1);
>   

-- 
Regards,
Yi Liu