| Message ID | 20241010075354.3582221-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] intel_iommu: Introduce property "x-stale-tm" to control Transient Mapping (TM) field | expand |
On Thu, Oct 10, 2024 at 3:57 PM Zhenzhong Duan <zhenzhong.duan@intel.com> 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 "x-stale-tm" to allow user to control the setting. Nit: I think we probably don't need the x prefix? As we try to maintain the compatibility via: > + { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, ? Thanks
>-----Original Message----- >From: Jason Wang <jasowang@redhat.com> >Subject: Re: [PATCH v2] intel_iommu: Introduce property "x-stale-tm" to control >Transient Mapping (TM) field > >On Thu, Oct 10, 2024 at 3:57 PM Zhenzhong Duan <zhenzhong.duan@intel.com> >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 "x-stale-tm" to allow user to control the setting. > >Nit: I think we probably don't need the x prefix? As we try to >maintain the compatibility via: > >> + { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, > >? I'm fine to remove it. But, The prefix "x-" is used to indicate that a feature is experimental. Couldn't we have a property both experimental and compatible? I see a lot of such properties: # grep "x-" /sdb/qemu/hw/i386/pc.c { "ICH9-LPC", "x-smi-swsmi-timer", "off" }, { "ICH9-LPC", "x-smi-periodic-timer", "off" }, { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, { TYPE_X86_CPU, "x-amd-topoext-features-only", "false" }, { TYPE_X86_CPU, "x-l1-cache-per-thread", "false" }, { "ICH9-LPC", "x-keep-pci-slot-hpc", "false" }, { TYPE_X86_CPU, "x-vendor-cpuid-only", "off" }, { "ICH9-LPC", "x-keep-pci-slot-hpc", "true" }, { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" }, { TYPE_X86_CPU, "x-hv-synic-kvm-only", "on" }, { TYPE_X86_CPU, "x-migrate-smi-count", "off" }, { TYPE_X86_CPU, "x-hv-max-vps", "0x40" }, { "i440FX-pcihost", "x-pci-hole64-fix", "off" }, { "q35-pcihost", "x-pci-hole64-fix", "off" }, { "kvmclock", "x-mach-use-reliable-get-clock", "off" }, { "ICH9-LPC", "x-smi-broadcast", "off" }, Thanks Zhenzhong
On Thu, Oct 17, 2024 at 3:58 PM Duan, Zhenzhong <zhenzhong.duan@intel.com> wrote: > > > > >-----Original Message----- > >From: Jason Wang <jasowang@redhat.com> > >Subject: Re: [PATCH v2] intel_iommu: Introduce property "x-stale-tm" to control > >Transient Mapping (TM) field > > > >On Thu, Oct 10, 2024 at 3:57 PM Zhenzhong Duan <zhenzhong.duan@intel.com> > >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 "x-stale-tm" to allow user to control the setting. > > > >Nit: I think we probably don't need the x prefix? As we try to > >maintain the compatibility via: > > > >> + { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, > > > >? > > I'm fine to remove it. But, > The prefix "x-" is used to indicate that a feature is experimental. > Couldn't we have a property both experimental and compatible? > I see a lot of such properties: Not sure, adding Peter for clarification. > > # grep "x-" /sdb/qemu/hw/i386/pc.c > { "ICH9-LPC", "x-smi-swsmi-timer", "off" }, > { "ICH9-LPC", "x-smi-periodic-timer", "off" }, > { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, > { TYPE_X86_CPU, "x-amd-topoext-features-only", "false" }, > { TYPE_X86_CPU, "x-l1-cache-per-thread", "false" }, > { "ICH9-LPC", "x-keep-pci-slot-hpc", "false" }, > { TYPE_X86_CPU, "x-vendor-cpuid-only", "off" }, > { "ICH9-LPC", "x-keep-pci-slot-hpc", "true" }, > { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, > { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, > { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" }, > { TYPE_X86_CPU, "x-hv-synic-kvm-only", "on" }, > { TYPE_X86_CPU, "x-migrate-smi-count", "off" }, > { TYPE_X86_CPU, "x-hv-max-vps", "0x40" }, > { "i440FX-pcihost", "x-pci-hole64-fix", "off" }, > { "q35-pcihost", "x-pci-hole64-fix", "off" }, > { "kvmclock", "x-mach-use-reliable-get-clock", "off" }, > { "ICH9-LPC", "x-smi-broadcast", "off" }, > > Thanks > Zhenzhong Thanks
On Thu, Oct 17, 2024 at 07:57:53AM +0000, Duan, Zhenzhong wrote: > > > >-----Original Message----- > >From: Jason Wang <jasowang@redhat.com> > >Subject: Re: [PATCH v2] intel_iommu: Introduce property "x-stale-tm" to control > >Transient Mapping (TM) field > > > >On Thu, Oct 10, 2024 at 3:57 PM Zhenzhong Duan <zhenzhong.duan@intel.com> > >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 "x-stale-tm" to allow user to control the setting. > > > >Nit: I think we probably don't need the x prefix? As we try to > >maintain the compatibility via: > > > >> + { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, > > > >? > > I'm fine to remove it. But, > The prefix "x-" is used to indicate that a feature is experimental. No, it is used to indicate that the feature is not part of a stable API. > Couldn't we have a property both experimental and compatible? > I see a lot of such properties: > > # grep "x-" /sdb/qemu/hw/i386/pc.c > { "ICH9-LPC", "x-smi-swsmi-timer", "off" }, > { "ICH9-LPC", "x-smi-periodic-timer", "off" }, > { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, > { TYPE_X86_CPU, "x-amd-topoext-features-only", "false" }, > { TYPE_X86_CPU, "x-l1-cache-per-thread", "false" }, > { "ICH9-LPC", "x-keep-pci-slot-hpc", "false" }, > { TYPE_X86_CPU, "x-vendor-cpuid-only", "off" }, > { "ICH9-LPC", "x-keep-pci-slot-hpc", "true" }, > { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, > { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, > { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" }, > { TYPE_X86_CPU, "x-hv-synic-kvm-only", "on" }, > { TYPE_X86_CPU, "x-migrate-smi-count", "off" }, > { TYPE_X86_CPU, "x-hv-max-vps", "0x40" }, > { "i440FX-pcihost", "x-pci-hole64-fix", "off" }, > { "q35-pcihost", "x-pci-hole64-fix", "off" }, > { "kvmclock", "x-mach-use-reliable-get-clock", "off" }, > { "ICH9-LPC", "x-smi-broadcast", "off" }, > > Thanks > Zhenzhong
>-----Original Message----- >From: Michael S. Tsirkin <mst@redhat.com> >Subject: Re: [PATCH v2] intel_iommu: Introduce property "x-stale-tm" to control >Transient Mapping (TM) field > >On Thu, Oct 17, 2024 at 07:57:53AM +0000, Duan, Zhenzhong wrote: >> >> >> >-----Original Message----- >> >From: Jason Wang <jasowang@redhat.com> >> >Subject: Re: [PATCH v2] intel_iommu: Introduce property "x-stale-tm" to >control >> >Transient Mapping (TM) field >> > >> >On Thu, Oct 10, 2024 at 3:57 PM Zhenzhong Duan ><zhenzhong.duan@intel.com> >> >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 "x-stale-tm" to allow user to control the setting. >> > >> >Nit: I think we probably don't need the x prefix? As we try to >> >maintain the compatibility via: >> > >> >> + { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, >> > >> >? >> >> I'm fine to remove it. But, >> The prefix "x-" is used to indicate that a feature is experimental. > >No, it is used to indicate that the feature is not part of >a stable API. OK, Transient Mapping (TM) was removed in spec since 2020, so I will remove "x-" then. Thanks for confirming. BRs. Zhenzhong > >> Couldn't we have a property both experimental and compatible? >> I see a lot of such properties: >> >> # grep "x-" /sdb/qemu/hw/i386/pc.c >> { "ICH9-LPC", "x-smi-swsmi-timer", "off" }, >> { "ICH9-LPC", "x-smi-periodic-timer", "off" }, >> { TYPE_INTEL_IOMMU_DEVICE, "x-stale-tm", "on" }, >> { TYPE_X86_CPU, "x-amd-topoext-features-only", "false" }, >> { TYPE_X86_CPU, "x-l1-cache-per-thread", "false" }, >> { "ICH9-LPC", "x-keep-pci-slot-hpc", "false" }, >> { TYPE_X86_CPU, "x-vendor-cpuid-only", "off" }, >> { "ICH9-LPC", "x-keep-pci-slot-hpc", "true" }, >> { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" }, >> { "ICH9-LPC", "x-smi-cpu-hotplug", "off" }, >> { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" }, >> { TYPE_X86_CPU, "x-hv-synic-kvm-only", "on" }, >> { TYPE_X86_CPU, "x-migrate-smi-count", "off" }, >> { TYPE_X86_CPU, "x-hv-max-vps", "0x40" }, >> { "i440FX-pcihost", "x-pci-hole64-fix", "off" }, >> { "q35-pcihost", "x-pci-hole64-fix", "off" }, >> { "kvmclock", "x-mach-use-reliable-get-clock", "off" }, >> { "ICH9-LPC", "x-smi-broadcast", "off" }, >> >> Thanks >> Zhenzhong
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..ed668a8eff 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("x-stale-tm", IntelIOMMUState, stale_tm, false), DEFINE_PROP_END_OF_LIST(), }; @@ -4111,8 +4112,6 @@ static void vtd_cap_init(IntelIOMMUState *s) */ static void vtd_init(IntelIOMMUState *s) { - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); - memset(s->csr, 0, DMAR_REG_SIZE); memset(s->wmask, 0, DMAR_REG_SIZE); memset(s->w1cmask, 0, DMAR_REG_SIZE); @@ -4137,16 +4136,15 @@ static void vtd_init(IntelIOMMUState *s) * Rsvd field masks for spte */ vtd_spte_rsvd[0] = ~0ULL; - vtd_spte_rsvd[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits, - x86_iommu->dt_supported); + vtd_spte_rsvd[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits, 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); + s->stale_tm); vtd_spte_rsvd_large[3] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, - 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 8d84c22458..2569771700 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, "x-stale-tm", "on" }, }; const size_t pc_compat_9_1_len = G_N_ELEMENTS(pc_compat_9_1);
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 "x-stale-tm" to allow user to control the setting. Use hw_compat_9_1 to handle the compatibility for machines before 9.2 which 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> --- 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 | 10 ++++------ hw/i386/pc.c | 1 + 4 files changed, 14 insertions(+), 12 deletions(-)