| Message ID | 20240718081636.879544-4-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | intel_iommu: Enable stage-1 translation for emulated device | expand |
On 18/07/2024 10:16, Zhenzhong Duan 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. > > > Add an new element scalable_mode in IntelIOMMUState to mark scalable > modern mode, this element will be exposed as an intel_iommu property > finally. > > For now, it's only a placehholder and used for cap/ecap initialization, > compatibility check and block host device passthrough until nesting > is supported. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/i386/intel_iommu_internal.h | 2 ++ > include/hw/i386/intel_iommu.h | 1 + > hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- > 3 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index c0ca7b372f..4e0331caba 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -195,6 +195,7 @@ > #define VTD_ECAP_PASID (1ULL << 40) > #define VTD_ECAP_SMTS (1ULL << 43) > #define VTD_ECAP_SLTS (1ULL << 46) > +#define VTD_ECAP_FLTS (1ULL << 47) > > /* CAP_REG */ > /* (offset >> 4) << 24 */ > @@ -211,6 +212,7 @@ > #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) > #define VTD_CAP_DRAIN_WRITE (1ULL << 54) > #define VTD_CAP_DRAIN_READ (1ULL << 55) > +#define VTD_CAP_FS1GP (1ULL << 56) > #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE) > #define VTD_CAP_CM (1ULL << 7) > #define VTD_PASID_ID_SHIFT 20 > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 1eb05c29fc..788ed42477 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -262,6 +262,7 @@ struct IntelIOMMUState { > > bool caching_mode; /* RO - is cap CM enabled? */ > bool scalable_mode; /* RO - is Scalable Mode supported? */ > + bool scalable_modern; /* RO - is modern SM supported? */ > bool snoop_control; /* RO - is SNP filed supported? */ > > dma_addr_t root; /* Current root table pointer */ > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1cff8b00ae..40cbd4a0f4 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -755,16 +755,20 @@ static inline bool vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) > } > > /* Return true if check passed, otherwise false */ > -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, > - VTDPASIDEntry *pe) > +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) > { What about using the cap/ecap registers to know if the translation types are supported or not. Otherwise, we could add a comment to explain why we expect s->scalable_modern to give us enough information. > + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); > + > switch (VTD_PE_GET_TYPE(pe)) { > + case VTD_SM_PASID_ENTRY_FLT: > + return s->scalable_modern; > case VTD_SM_PASID_ENTRY_SLT: > - return true; > + return !s->scalable_modern; > + case VTD_SM_PASID_ENTRY_NESTED: > + /* Not support NESTED page table type yet */ > + return false; > case VTD_SM_PASID_ENTRY_PT: > return x86_iommu->pt_supported; > - case VTD_SM_PASID_ENTRY_FLT: > - case VTD_SM_PASID_ENTRY_NESTED: > default: > /* Unknown type */ > return false; > @@ -813,7 +817,6 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, > uint8_t pgtt; > uint32_t index; > dma_addr_t entry_size; > - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); > > index = VTD_PASID_TABLE_INDEX(pasid); > entry_size = VTD_PASID_ENTRY_SIZE; > @@ -827,7 +830,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, > } > > /* Do translation type check */ > - if (!vtd_pe_type_check(x86_iommu, pe)) { > + if (!vtd_pe_type_check(s, pe)) { > return -VTD_FR_PASID_TABLE_ENTRY_INV; > } > > @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod, > return false; > } > > - return true; > + if (!s->scalable_modern) { > + /* All checks requested by VTD non-modern mode pass */ > + return true; > + } > + > + error_setg(errp, "host device is unsupported in scalable modern mode yet"); > + return false; > } > > static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > @@ -4084,7 +4093,10 @@ static void vtd_cap_init(IntelIOMMUState *s) > } > > /* TODO: read cap/ecap from host to decide which cap to be exposed. */ > - if (s->scalable_mode) { > + if (s->scalable_modern) { > + s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS; > + s->cap |= VTD_CAP_FS1GP; > + } else if (s->scalable_mode) { > s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS; > } > > @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > } > } > > - /* Currently only address widths supported are 39 and 48 bits */ > if ((s->aw_bits != VTD_HOST_AW_39BIT) && > - (s->aw_bits != VTD_HOST_AW_48BIT)) { > + (s->aw_bits != VTD_HOST_AW_48BIT) && > + !s->scalable_modern) { > error_setg(errp, "Supported values for aw-bits are: %d, %d", > VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); > return false; > -- > 2.34.1 >
>-----Original Message----- >From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >scalable modern mode > > > >On 18/07/2024 10:16, Zhenzhong Duan 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. >> >> >> Add an new element scalable_mode in IntelIOMMUState to mark scalable >> modern mode, this element will be exposed as an intel_iommu property >> finally. >> >> For now, it's only a placehholder and used for cap/ecap initialization, >> compatibility check and block host device passthrough until nesting >> is supported. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/i386/intel_iommu_internal.h | 2 ++ >> include/hw/i386/intel_iommu.h | 1 + >> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- >> 3 files changed, 26 insertions(+), 11 deletions(-) >> >> diff --git a/hw/i386/intel_iommu_internal.h >b/hw/i386/intel_iommu_internal.h >> index c0ca7b372f..4e0331caba 100644 >> --- a/hw/i386/intel_iommu_internal.h >> +++ b/hw/i386/intel_iommu_internal.h >> @@ -195,6 +195,7 @@ >> #define VTD_ECAP_PASID (1ULL << 40) >> #define VTD_ECAP_SMTS (1ULL << 43) >> #define VTD_ECAP_SLTS (1ULL << 46) >> +#define VTD_ECAP_FLTS (1ULL << 47) >> >> /* CAP_REG */ >> /* (offset >> 4) << 24 */ >> @@ -211,6 +212,7 @@ >> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >> #define VTD_CAP_DRAIN_READ (1ULL << 55) >> +#define VTD_CAP_FS1GP (1ULL << 56) >> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >VTD_CAP_DRAIN_WRITE) >> #define VTD_CAP_CM (1ULL << 7) >> #define VTD_PASID_ID_SHIFT 20 >> diff --git a/include/hw/i386/intel_iommu.h >b/include/hw/i386/intel_iommu.h >> index 1eb05c29fc..788ed42477 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >> >> bool caching_mode; /* RO - is cap CM enabled? */ >> bool scalable_mode; /* RO - is Scalable Mode supported? */ >> + bool scalable_modern; /* RO - is modern SM supported? */ >> bool snoop_control; /* RO - is SNP filed supported? */ >> >> dma_addr_t root; /* Current root table pointer */ >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 1cff8b00ae..40cbd4a0f4 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -755,16 +755,20 @@ static inline bool >vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >> } >> >> /* Return true if check passed, otherwise false */ >> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >> - VTDPASIDEntry *pe) >> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >VTDPASIDEntry *pe) >> { >What about using the cap/ecap registers to know if the translation types >are supported or not. >Otherwise, we could add a comment to explain why we expect >s->scalable_modern to give us enough information. What about below: /* *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. *So it's simpler to check s->scalable_modern directly for a PASID entry type instead ecap bits. */ Thanks Zhenzhong >> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >> + >> switch (VTD_PE_GET_TYPE(pe)) { >> + case VTD_SM_PASID_ENTRY_FLT: >> + return s->scalable_modern; >> case VTD_SM_PASID_ENTRY_SLT: >> - return true; >> + return !s->scalable_modern; >> + case VTD_SM_PASID_ENTRY_NESTED: >> + /* Not support NESTED page table type yet */ >> + return false; >> case VTD_SM_PASID_ENTRY_PT: >> return x86_iommu->pt_supported; >> - case VTD_SM_PASID_ENTRY_FLT: >> - case VTD_SM_PASID_ENTRY_NESTED: >> default: >> /* Unknown type */ >> return false; >> @@ -813,7 +817,6 @@ static int >vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, >> uint8_t pgtt; >> uint32_t index; >> dma_addr_t entry_size; >> - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >> >> index = VTD_PASID_TABLE_INDEX(pasid); >> entry_size = VTD_PASID_ENTRY_SIZE; >> @@ -827,7 +830,7 @@ static int >vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, >> } >> >> /* Do translation type check */ >> - if (!vtd_pe_type_check(x86_iommu, pe)) { >> + if (!vtd_pe_type_check(s, pe)) { >> return -VTD_FR_PASID_TABLE_ENTRY_INV; >> } >> >> @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState >*s, HostIOMMUDevice *hiod, >> return false; >> } >> >> - return true; >> + if (!s->scalable_modern) { >> + /* All checks requested by VTD non-modern mode pass */ >> + return true; >> + } >> + >> + error_setg(errp, "host device is unsupported in scalable modern mode >yet"); >> + return false; >> } >> >> static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int >devfn, >> @@ -4084,7 +4093,10 @@ static void vtd_cap_init(IntelIOMMUState *s) >> } >> >> /* TODO: read cap/ecap from host to decide which cap to be exposed. >*/ >> - if (s->scalable_mode) { >> + if (s->scalable_modern) { >> + s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS; >> + s->cap |= VTD_CAP_FS1GP; >> + } else if (s->scalable_mode) { >> s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS; >> } >> >> @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState >*s, Error **errp) >> } >> } >> >> - /* Currently only address widths supported are 39 and 48 bits */ >> if ((s->aw_bits != VTD_HOST_AW_39BIT) && >> - (s->aw_bits != VTD_HOST_AW_48BIT)) { >> + (s->aw_bits != VTD_HOST_AW_48BIT) && >> + !s->scalable_modern) { >> error_setg(errp, "Supported values for aw-bits are: %d, %d", >> VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); >> return false; >> -- >> 2.34.1 >>
On 2024/7/19 10:47, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >> On 18/07/2024 10:16, Zhenzhong Duan 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. >>> >>> >>> Add an new element scalable_mode in IntelIOMMUState to mark scalable >>> modern mode, this element will be exposed as an intel_iommu property >>> finally. >>> >>> For now, it's only a placehholder and used for cap/ecap initialization, >>> compatibility check and block host device passthrough until nesting >>> is supported. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/i386/intel_iommu_internal.h | 2 ++ >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- >>> 3 files changed, 26 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/i386/intel_iommu_internal.h >> b/hw/i386/intel_iommu_internal.h >>> index c0ca7b372f..4e0331caba 100644 >>> --- a/hw/i386/intel_iommu_internal.h >>> +++ b/hw/i386/intel_iommu_internal.h >>> @@ -195,6 +195,7 @@ >>> #define VTD_ECAP_PASID (1ULL << 40) >>> #define VTD_ECAP_SMTS (1ULL << 43) >>> #define VTD_ECAP_SLTS (1ULL << 46) >>> +#define VTD_ECAP_FLTS (1ULL << 47) >>> >>> /* CAP_REG */ >>> /* (offset >> 4) << 24 */ >>> @@ -211,6 +212,7 @@ >>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>> +#define VTD_CAP_FS1GP (1ULL << 56) >>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >> VTD_CAP_DRAIN_WRITE) >>> #define VTD_CAP_CM (1ULL << 7) >>> #define VTD_PASID_ID_SHIFT 20 >>> diff --git a/include/hw/i386/intel_iommu.h >> b/include/hw/i386/intel_iommu.h >>> index 1eb05c29fc..788ed42477 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>> >>> bool caching_mode; /* RO - is cap CM enabled? */ >>> bool scalable_mode; /* RO - is Scalable Mode supported? */ >>> + bool scalable_modern; /* RO - is modern SM supported? */ >>> bool snoop_control; /* RO - is SNP filed supported? */ >>> >>> dma_addr_t root; /* Current root table pointer */ >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 1cff8b00ae..40cbd4a0f4 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -755,16 +755,20 @@ static inline bool >> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>> } >>> >>> /* Return true if check passed, otherwise false */ >>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >>> - VTDPASIDEntry *pe) >>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >> VTDPASIDEntry *pe) >>> { >> What about using the cap/ecap registers to know if the translation types >> are supported or not. >> Otherwise, we could add a comment to explain why we expect >> s->scalable_modern to give us enough information. > > What about below: > > /* > *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. > *So it's simpler to check s->scalable_modern directly for a PASID entry type instead ecap bits. > */ Since this helper is for pasid entry check, so you can just return false if the pe's PGTT is SS-only. It might make more sense to check the ecap/cap here as anyhow the capability is listed in ecap/cap. This may also bring us some convenience. Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) that supports both FS and SS for guest, we may need to update this helper as well if we check the scalable_modern. But if we check the ecap/cap, then the future change just needs to control the ecap/cap setting at the beginning of the vIOMMU init. To keep the code aligned, you may need to check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) > >>> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >>> + >>> switch (VTD_PE_GET_TYPE(pe)) { >>> + case VTD_SM_PASID_ENTRY_FLT: >>> + return s->scalable_modern; >>> case VTD_SM_PASID_ENTRY_SLT: >>> - return true; >>> + return !s->scalable_modern; >>> + case VTD_SM_PASID_ENTRY_NESTED: >>> + /* Not support NESTED page table type yet */ >>> + return false; >>> case VTD_SM_PASID_ENTRY_PT: >>> return x86_iommu->pt_supported; >>> - case VTD_SM_PASID_ENTRY_FLT: >>> - case VTD_SM_PASID_ENTRY_NESTED: >>> default: >>> /* Unknown type */ >>> return false; >>> @@ -813,7 +817,6 @@ static int >> vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, >>> uint8_t pgtt; >>> uint32_t index; >>> dma_addr_t entry_size; >>> - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >>> >>> index = VTD_PASID_TABLE_INDEX(pasid); >>> entry_size = VTD_PASID_ENTRY_SIZE; >>> @@ -827,7 +830,7 @@ static int >> vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, >>> } >>> >>> /* Do translation type check */ >>> - if (!vtd_pe_type_check(x86_iommu, pe)) { >>> + if (!vtd_pe_type_check(s, pe)) { >>> return -VTD_FR_PASID_TABLE_ENTRY_INV; >>> } >>> >>> @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState >> *s, HostIOMMUDevice *hiod, >>> return false; >>> } >>> >>> - return true; >>> + if (!s->scalable_modern) { >>> + /* All checks requested by VTD non-modern mode pass */ >>> + return true; >>> + } >>> + >>> + error_setg(errp, "host device is unsupported in scalable modern mode >> yet"); >>> + return false; >>> } >>> >>> static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int >> devfn, >>> @@ -4084,7 +4093,10 @@ static void vtd_cap_init(IntelIOMMUState *s) >>> } >>> >>> /* TODO: read cap/ecap from host to decide which cap to be exposed. >> */ >>> - if (s->scalable_mode) { >>> + if (s->scalable_modern) { >>> + s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS; >>> + s->cap |= VTD_CAP_FS1GP; >>> + } else if (s->scalable_mode) { >>> s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS; >>> } >>> >>> @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState >> *s, Error **errp) >>> } >>> } >>> >>> - /* Currently only address widths supported are 39 and 48 bits */ >>> if ((s->aw_bits != VTD_HOST_AW_39BIT) && >>> - (s->aw_bits != VTD_HOST_AW_48BIT)) { >>> + (s->aw_bits != VTD_HOST_AW_48BIT) && >>> + !s->scalable_modern) { >>> error_setg(errp, "Supported values for aw-bits are: %d, %d", >>> VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); >>> return false; >>> -- >>> 2.34.1 >>>
>-----Original Message----- >From: Liu, Yi L <yi.l.liu@intel.com> >Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >scalable modern mode > >On 2024/7/19 10:47, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >for >>> scalable modern mode >>> >>> >>> >>> On 18/07/2024 10:16, Zhenzhong Duan 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. >>>> >>>> >>>> Add an new element scalable_mode in IntelIOMMUState to mark >scalable >>>> modern mode, this element will be exposed as an intel_iommu property >>>> finally. >>>> >>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>> compatibility check and block host device passthrough until nesting >>>> is supported. >>>> >>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>> include/hw/i386/intel_iommu.h | 1 + >>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++--------- >-- >>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/hw/i386/intel_iommu_internal.h >>> b/hw/i386/intel_iommu_internal.h >>>> index c0ca7b372f..4e0331caba 100644 >>>> --- a/hw/i386/intel_iommu_internal.h >>>> +++ b/hw/i386/intel_iommu_internal.h >>>> @@ -195,6 +195,7 @@ >>>> #define VTD_ECAP_PASID (1ULL << 40) >>>> #define VTD_ECAP_SMTS (1ULL << 43) >>>> #define VTD_ECAP_SLTS (1ULL << 46) >>>> +#define VTD_ECAP_FLTS (1ULL << 47) >>>> >>>> /* CAP_REG */ >>>> /* (offset >> 4) << 24 */ >>>> @@ -211,6 +212,7 @@ >>>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>>> +#define VTD_CAP_FS1GP (1ULL << 56) >>>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >>> VTD_CAP_DRAIN_WRITE) >>>> #define VTD_CAP_CM (1ULL << 7) >>>> #define VTD_PASID_ID_SHIFT 20 >>>> diff --git a/include/hw/i386/intel_iommu.h >>> b/include/hw/i386/intel_iommu.h >>>> index 1eb05c29fc..788ed42477 100644 >>>> --- a/include/hw/i386/intel_iommu.h >>>> +++ b/include/hw/i386/intel_iommu.h >>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>> >>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>> bool scalable_mode; /* RO - is Scalable Mode supported? */ >>>> + bool scalable_modern; /* RO - is modern SM supported? */ >>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>> >>>> dma_addr_t root; /* Current root table pointer */ >>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>> --- a/hw/i386/intel_iommu.c >>>> +++ b/hw/i386/intel_iommu.c >>>> @@ -755,16 +755,20 @@ static inline bool >>> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>>> } >>>> >>>> /* Return true if check passed, otherwise false */ >>>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >>>> - VTDPASIDEntry *pe) >>>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >>> VTDPASIDEntry *pe) >>>> { >>> What about using the cap/ecap registers to know if the translation types >>> are supported or not. >>> Otherwise, we could add a comment to explain why we expect >>> s->scalable_modern to give us enough information. >> >> What about below: >> >> /* >> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >> *So it's simpler to check s->scalable_modern directly for a PASID entry >type instead ecap bits. >> */ > >Since this helper is for pasid entry check, so you can just return false >if the pe's PGTT is SS-only. It depends on which scalable mode is chosed. In scalable legacy mode, PGTT is SS-only and we should return true. > >It might make more sense to check the ecap/cap here as anyhow the >capability is listed in ecap/cap. This may also bring us some convenience. > >Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >that supports both FS and SS for guest, we may need to update this helper >as well if we check the scalable_modern. But if we check the ecap/cap, then >the future change just needs to control the ecap/cap setting at the >beginning of the vIOMMU init. To keep the code aligned, you may need to >check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) OK, will be like below: --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -826,14 +826,14 @@ static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) switch (VTD_PE_GET_TYPE(pe)) { case VTD_SM_PASID_ENTRY_FLT: - return s->scalable_modern; + return !!(s->ecap & VTD_ECAP_FLTS); case VTD_SM_PASID_ENTRY_SLT: - return !s->scalable_modern; + return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS); case VTD_SM_PASID_ENTRY_NESTED: /* Not support NESTED page table type yet */ return false; case VTD_SM_PASID_ENTRY_PT: - return x86_iommu->pt_supported; + return !!(s->ecap & VTD_ECAP_PT); default: /* Unknown type */ return false; Thanks Zhenzhong
>-----Original Message----- >From: Duan, Zhenzhong] >Subject: RE: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >scalable modern mode > > > >>-----Original Message----- >>From: Liu, Yi L <yi.l.liu@intel.com> >>Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >>scalable modern mode >> >>On 2024/7/19 10:47, Duan, Zhenzhong wrote: >>> >>> >>>> -----Original Message----- >>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >>for >>>> scalable modern mode >>>> >>>> >>>> >>>> On 18/07/2024 10:16, Zhenzhong Duan 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. >>>>> >>>>> >>>>> Add an new element scalable_mode in IntelIOMMUState to mark >>scalable >>>>> modern mode, this element will be exposed as an intel_iommu >property >>>>> finally. >>>>> >>>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>>> compatibility check and block host device passthrough until nesting >>>>> is supported. >>>>> >>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>>> include/hw/i386/intel_iommu.h | 1 + >>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++------- >-- >>-- >>>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/hw/i386/intel_iommu_internal.h >>>> b/hw/i386/intel_iommu_internal.h >>>>> index c0ca7b372f..4e0331caba 100644 >>>>> --- a/hw/i386/intel_iommu_internal.h >>>>> +++ b/hw/i386/intel_iommu_internal.h >>>>> @@ -195,6 +195,7 @@ >>>>> #define VTD_ECAP_PASID (1ULL << 40) >>>>> #define VTD_ECAP_SMTS (1ULL << 43) >>>>> #define VTD_ECAP_SLTS (1ULL << 46) >>>>> +#define VTD_ECAP_FLTS (1ULL << 47) >>>>> >>>>> /* CAP_REG */ >>>>> /* (offset >> 4) << 24 */ >>>>> @@ -211,6 +212,7 @@ >>>>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>>>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>>>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>>>> +#define VTD_CAP_FS1GP (1ULL << 56) >>>>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >>>> VTD_CAP_DRAIN_WRITE) >>>>> #define VTD_CAP_CM (1ULL << 7) >>>>> #define VTD_PASID_ID_SHIFT 20 >>>>> diff --git a/include/hw/i386/intel_iommu.h >>>> b/include/hw/i386/intel_iommu.h >>>>> index 1eb05c29fc..788ed42477 100644 >>>>> --- a/include/hw/i386/intel_iommu.h >>>>> +++ b/include/hw/i386/intel_iommu.h >>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>>> >>>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>>> bool scalable_mode; /* RO - is Scalable Mode supported? >*/ >>>>> + bool scalable_modern; /* RO - is modern SM supported? */ >>>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>>> >>>>> dma_addr_t root; /* Current root table pointer */ >>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>>> --- a/hw/i386/intel_iommu.c >>>>> +++ b/hw/i386/intel_iommu.c >>>>> @@ -755,16 +755,20 @@ static inline bool >>>> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>>>> } >>>>> >>>>> /* Return true if check passed, otherwise false */ >>>>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >>>>> - VTDPASIDEntry *pe) >>>>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >>>> VTDPASIDEntry *pe) >>>>> { >>>> What about using the cap/ecap registers to know if the translation types >>>> are supported or not. >>>> Otherwise, we could add a comment to explain why we expect >>>> s->scalable_modern to give us enough information. >>> >>> What about below: >>> >>> /* >>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >>VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >>> *So it's simpler to check s->scalable_modern directly for a PASID entry >>type instead ecap bits. >>> */ >> >>Since this helper is for pasid entry check, so you can just return false >>if the pe's PGTT is SS-only. > >It depends on which scalable mode is chosed. >In scalable legacy mode, PGTT is SS-only and we should return true. > >> >>It might make more sense to check the ecap/cap here as anyhow the >>capability is listed in ecap/cap. This may also bring us some convenience. >> >>Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >>that supports both FS and SS for guest, we may need to update this helper >>as well if we check the scalable_modern. But if we check the ecap/cap, then >>the future change just needs to control the ecap/cap setting at the >>beginning of the vIOMMU init. To keep the code aligned, you may need to >>check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) > >OK, will be like below: > >--- a/hw/i386/intel_iommu.c >+++ b/hw/i386/intel_iommu.c >@@ -826,14 +826,14 @@ static inline bool >vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) > > switch (VTD_PE_GET_TYPE(pe)) { > case VTD_SM_PASID_ENTRY_FLT: >- return s->scalable_modern; >+ return !!(s->ecap & VTD_ECAP_FLTS); > case VTD_SM_PASID_ENTRY_SLT: >- return !s->scalable_modern; >+ return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS); Sorry typo err, should be: + return !!(s->ecap & VTD_ECAP_SLTS) || !(s->ecap & VTD_ECAP_SMTS); > case VTD_SM_PASID_ENTRY_NESTED: > /* Not support NESTED page table type yet */ > return false; > case VTD_SM_PASID_ENTRY_PT: >- return x86_iommu->pt_supported; >+ return !!(s->ecap & VTD_ECAP_PT); > default: > /* Unknown type */ > return false; > >Thanks >Zhenzhong
On 19/07/2024 04:47, Duan, Zhenzhong 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. > > >> -----Original Message----- >> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >> On 18/07/2024 10:16, Zhenzhong Duan 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. >>> >>> Add an new element scalable_mode in IntelIOMMUState to mark scalable >>> modern mode, this element will be exposed as an intel_iommu property >>> finally. >>> >>> For now, it's only a placehholder and used for cap/ecap initialization, >>> compatibility check and block host device passthrough until nesting >>> is supported. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/i386/intel_iommu_internal.h | 2 ++ >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- >>> 3 files changed, 26 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/i386/intel_iommu_internal.h >> b/hw/i386/intel_iommu_internal.h >>> index c0ca7b372f..4e0331caba 100644 >>> --- a/hw/i386/intel_iommu_internal.h >>> +++ b/hw/i386/intel_iommu_internal.h >>> @@ -195,6 +195,7 @@ >>> #define VTD_ECAP_PASID (1ULL << 40) >>> #define VTD_ECAP_SMTS (1ULL << 43) >>> #define VTD_ECAP_SLTS (1ULL << 46) >>> +#define VTD_ECAP_FLTS (1ULL << 47) >>> >>> /* CAP_REG */ >>> /* (offset >> 4) << 24 */ >>> @@ -211,6 +212,7 @@ >>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>> +#define VTD_CAP_FS1GP (1ULL << 56) >>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >> VTD_CAP_DRAIN_WRITE) >>> #define VTD_CAP_CM (1ULL << 7) >>> #define VTD_PASID_ID_SHIFT 20 >>> diff --git a/include/hw/i386/intel_iommu.h >> b/include/hw/i386/intel_iommu.h >>> index 1eb05c29fc..788ed42477 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>> >>> bool caching_mode; /* RO - is cap CM enabled? */ >>> bool scalable_mode; /* RO - is Scalable Mode supported? */ >>> + bool scalable_modern; /* RO - is modern SM supported? */ >>> bool snoop_control; /* RO - is SNP filed supported? */ >>> >>> dma_addr_t root; /* Current root table pointer */ >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 1cff8b00ae..40cbd4a0f4 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -755,16 +755,20 @@ static inline bool >> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>> } >>> >>> /* Return true if check passed, otherwise false */ >>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >>> - VTDPASIDEntry *pe) >>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >> VTDPASIDEntry *pe) >>> { >> What about using the cap/ecap registers to know if the translation types >> are supported or not. >> Otherwise, we could add a comment to explain why we expect >> s->scalable_modern to give us enough information. > What about below: > > /* > *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. > *So it's simpler to check s->scalable_modern directly for a PASID entry type instead ecap bits. > */ Fine ;) > > Thanks > Zhenzhong > >>> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >>> + >>> switch (VTD_PE_GET_TYPE(pe)) { >>> + case VTD_SM_PASID_ENTRY_FLT: >>> + return s->scalable_modern; >>> case VTD_SM_PASID_ENTRY_SLT: >>> - return true; >>> + return !s->scalable_modern; >>> + case VTD_SM_PASID_ENTRY_NESTED: >>> + /* Not support NESTED page table type yet */ >>> + return false; >>> case VTD_SM_PASID_ENTRY_PT: >>> return x86_iommu->pt_supported; >>> - case VTD_SM_PASID_ENTRY_FLT: >>> - case VTD_SM_PASID_ENTRY_NESTED: >>> default: >>> /* Unknown type */ >>> return false; >>> @@ -813,7 +817,6 @@ static int >> vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, >>> uint8_t pgtt; >>> uint32_t index; >>> dma_addr_t entry_size; >>> - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >>> >>> index = VTD_PASID_TABLE_INDEX(pasid); >>> entry_size = VTD_PASID_ENTRY_SIZE; >>> @@ -827,7 +830,7 @@ static int >> vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, >>> } >>> >>> /* Do translation type check */ >>> - if (!vtd_pe_type_check(x86_iommu, pe)) { >>> + if (!vtd_pe_type_check(s, pe)) { >>> return -VTD_FR_PASID_TABLE_ENTRY_INV; >>> } >>> >>> @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState >> *s, HostIOMMUDevice *hiod, >>> return false; >>> } >>> >>> - return true; >>> + if (!s->scalable_modern) { >>> + /* All checks requested by VTD non-modern mode pass */ >>> + return true; >>> + } >>> + >>> + error_setg(errp, "host device is unsupported in scalable modern mode >> yet"); >>> + return false; >>> } >>> >>> static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int >> devfn, >>> @@ -4084,7 +4093,10 @@ static void vtd_cap_init(IntelIOMMUState *s) >>> } >>> >>> /* TODO: read cap/ecap from host to decide which cap to be exposed. >> */ >>> - if (s->scalable_mode) { >>> + if (s->scalable_modern) { >>> + s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS; >>> + s->cap |= VTD_CAP_FS1GP; >>> + } else if (s->scalable_mode) { >>> s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS; >>> } >>> >>> @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState >> *s, Error **errp) >>> } >>> } >>> >>> - /* Currently only address widths supported are 39 and 48 bits */ >>> if ((s->aw_bits != VTD_HOST_AW_39BIT) && >>> - (s->aw_bits != VTD_HOST_AW_48BIT)) { >>> + (s->aw_bits != VTD_HOST_AW_48BIT) && >>> + !s->scalable_modern) { >>> error_setg(errp, "Supported values for aw-bits are: %d, %d", >>> VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); >>> return false; >>> -- >>> 2.34.1 >>>
On 19/07/2024 05:39, Duan, Zhenzhong 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. > > >> -----Original Message----- >> From: Duan, Zhenzhong] >> Subject: RE: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >>> -----Original Message----- >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >>> scalable modern mode >>> >>> On 2024/7/19 10:47, Duan, Zhenzhong wrote: >>>> >>>>> -----Original Message----- >>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >>> for >>>>> scalable modern mode >>>>> >>>>> >>>>> >>>>> On 18/07/2024 10:16, Zhenzhong Duan 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. >>>>>> >>>>>> Add an new element scalable_mode in IntelIOMMUState to mark >>> scalable >>>>>> modern mode, this element will be exposed as an intel_iommu >> property >>>>>> finally. >>>>>> >>>>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>>>> compatibility check and block host device passthrough until nesting >>>>>> is supported. >>>>>> >>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>> --- >>>>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>>>> include/hw/i386/intel_iommu.h | 1 + >>>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++------- >> -- >>> -- >>>>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/hw/i386/intel_iommu_internal.h >>>>> b/hw/i386/intel_iommu_internal.h >>>>>> index c0ca7b372f..4e0331caba 100644 >>>>>> --- a/hw/i386/intel_iommu_internal.h >>>>>> +++ b/hw/i386/intel_iommu_internal.h >>>>>> @@ -195,6 +195,7 @@ >>>>>> #define VTD_ECAP_PASID (1ULL << 40) >>>>>> #define VTD_ECAP_SMTS (1ULL << 43) >>>>>> #define VTD_ECAP_SLTS (1ULL << 46) >>>>>> +#define VTD_ECAP_FLTS (1ULL << 47) >>>>>> >>>>>> /* CAP_REG */ >>>>>> /* (offset >> 4) << 24 */ >>>>>> @@ -211,6 +212,7 @@ >>>>>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>>>>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>>>>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>>>>> +#define VTD_CAP_FS1GP (1ULL << 56) >>>>>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >>>>> VTD_CAP_DRAIN_WRITE) >>>>>> #define VTD_CAP_CM (1ULL << 7) >>>>>> #define VTD_PASID_ID_SHIFT 20 >>>>>> diff --git a/include/hw/i386/intel_iommu.h >>>>> b/include/hw/i386/intel_iommu.h >>>>>> index 1eb05c29fc..788ed42477 100644 >>>>>> --- a/include/hw/i386/intel_iommu.h >>>>>> +++ b/include/hw/i386/intel_iommu.h >>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>>>> >>>>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>>>> bool scalable_mode; /* RO - is Scalable Mode supported? >> */ >>>>>> + bool scalable_modern; /* RO - is modern SM supported? */ >>>>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>>>> >>>>>> dma_addr_t root; /* Current root table pointer */ >>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>>>> --- a/hw/i386/intel_iommu.c >>>>>> +++ b/hw/i386/intel_iommu.c >>>>>> @@ -755,16 +755,20 @@ static inline bool >>>>> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>>>>> } >>>>>> >>>>>> /* Return true if check passed, otherwise false */ >>>>>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >>>>>> - VTDPASIDEntry *pe) >>>>>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >>>>> VTDPASIDEntry *pe) >>>>>> { >>>>> What about using the cap/ecap registers to know if the translation types >>>>> are supported or not. >>>>> Otherwise, we could add a comment to explain why we expect >>>>> s->scalable_modern to give us enough information. >>>> What about below: >>>> >>>> /* >>>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >>> VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >>>> *So it's simpler to check s->scalable_modern directly for a PASID entry >>> type instead ecap bits. >>>> */ >>> Since this helper is for pasid entry check, so you can just return false >>> if the pe's PGTT is SS-only. >> It depends on which scalable mode is chosed. >> In scalable legacy mode, PGTT is SS-only and we should return true. >> >>> It might make more sense to check the ecap/cap here as anyhow the >>> capability is listed in ecap/cap. This may also bring us some convenience. >>> >>> Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >>> that supports both FS and SS for guest, we may need to update this helper >>> as well if we check the scalable_modern. But if we check the ecap/cap, then >>> the future change just needs to control the ecap/cap setting at the >>> beginning of the vIOMMU init. To keep the code aligned, you may need to >>> check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) >> OK, will be like below: >> >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -826,14 +826,14 @@ static inline bool >> vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) >> >> switch (VTD_PE_GET_TYPE(pe)) { >> case VTD_SM_PASID_ENTRY_FLT: >> - return s->scalable_modern; >> + return !!(s->ecap & VTD_ECAP_FLTS); >> case VTD_SM_PASID_ENTRY_SLT: >> - return !s->scalable_modern; >> + return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS); > Sorry typo err, should be: > > + return !!(s->ecap & VTD_ECAP_SLTS) || !(s->ecap & VTD_ECAP_SMTS); > I agree with Yi's point of view, this version looks good >> case VTD_SM_PASID_ENTRY_NESTED: >> /* Not support NESTED page table type yet */ >> return false; >> case VTD_SM_PASID_ENTRY_PT: >> - return x86_iommu->pt_supported; >> + return !!(s->ecap & VTD_ECAP_PT); >> default: >> /* Unknown type */ >> return false; >> >> Thanks >> Zhenzhong
On 19/07/2024 05:39, Duan, Zhenzhong 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. > > >> -----Original Message----- >> From: Duan, Zhenzhong] >> Subject: RE: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >>> -----Original Message----- >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >>> scalable modern mode >>> >>> On 2024/7/19 10:47, Duan, Zhenzhong wrote: >>>> >>>>> -----Original Message----- >>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >>> for >>>>> scalable modern mode >>>>> >>>>> >>>>> >>>>> On 18/07/2024 10:16, Zhenzhong Duan 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. >>>>>> >>>>>> Add an new element scalable_mode in IntelIOMMUState to mark >>> scalable >>>>>> modern mode, this element will be exposed as an intel_iommu >> property >>>>>> finally. >>>>>> >>>>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>>>> compatibility check and block host device passthrough until nesting >>>>>> is supported. >>>>>> >>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>> --- >>>>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>>>> include/hw/i386/intel_iommu.h | 1 + >>>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++------- >> -- >>> -- >>>>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/hw/i386/intel_iommu_internal.h >>>>> b/hw/i386/intel_iommu_internal.h >>>>>> index c0ca7b372f..4e0331caba 100644 >>>>>> --- a/hw/i386/intel_iommu_internal.h >>>>>> +++ b/hw/i386/intel_iommu_internal.h >>>>>> @@ -195,6 +195,7 @@ >>>>>> #define VTD_ECAP_PASID (1ULL << 40) >>>>>> #define VTD_ECAP_SMTS (1ULL << 43) >>>>>> #define VTD_ECAP_SLTS (1ULL << 46) >>>>>> +#define VTD_ECAP_FLTS (1ULL << 47) >>>>>> >>>>>> /* CAP_REG */ >>>>>> /* (offset >> 4) << 24 */ >>>>>> @@ -211,6 +212,7 @@ >>>>>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>>>>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>>>>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>>>>> +#define VTD_CAP_FS1GP (1ULL << 56) >>>>>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >>>>> VTD_CAP_DRAIN_WRITE) >>>>>> #define VTD_CAP_CM (1ULL << 7) >>>>>> #define VTD_PASID_ID_SHIFT 20 >>>>>> diff --git a/include/hw/i386/intel_iommu.h >>>>> b/include/hw/i386/intel_iommu.h >>>>>> index 1eb05c29fc..788ed42477 100644 >>>>>> --- a/include/hw/i386/intel_iommu.h >>>>>> +++ b/include/hw/i386/intel_iommu.h >>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>>>> >>>>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>>>> bool scalable_mode; /* RO - is Scalable Mode supported? >> */ >>>>>> + bool scalable_modern; /* RO - is modern SM supported? */ >>>>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>>>> >>>>>> dma_addr_t root; /* Current root table pointer */ >>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>>>> --- a/hw/i386/intel_iommu.c >>>>>> +++ b/hw/i386/intel_iommu.c >>>>>> @@ -755,16 +755,20 @@ static inline bool >>>>> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>>>>> } >>>>>> >>>>>> /* Return true if check passed, otherwise false */ >>>>>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, >>>>>> - VTDPASIDEntry *pe) >>>>>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >>>>> VTDPASIDEntry *pe) >>>>>> { >>>>> What about using the cap/ecap registers to know if the translation types >>>>> are supported or not. >>>>> Otherwise, we could add a comment to explain why we expect >>>>> s->scalable_modern to give us enough information. >>>> What about below: >>>> >>>> /* >>>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >>> VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >>>> *So it's simpler to check s->scalable_modern directly for a PASID entry >>> type instead ecap bits. >>>> */ >>> Since this helper is for pasid entry check, so you can just return false >>> if the pe's PGTT is SS-only. >> It depends on which scalable mode is chosed. >> In scalable legacy mode, PGTT is SS-only and we should return true. >> >>> It might make more sense to check the ecap/cap here as anyhow the >>> capability is listed in ecap/cap. This may also bring us some convenience. >>> >>> Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >>> that supports both FS and SS for guest, we may need to update this helper >>> as well if we check the scalable_modern. But if we check the ecap/cap, then >>> the future change just needs to control the ecap/cap setting at the >>> beginning of the vIOMMU init. To keep the code aligned, you may need to >>> check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) >> OK, will be like below: >> >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -826,14 +826,14 @@ static inline bool >> vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) >> >> switch (VTD_PE_GET_TYPE(pe)) { >> case VTD_SM_PASID_ENTRY_FLT: >> - return s->scalable_modern; >> + return !!(s->ecap & VTD_ECAP_FLTS); >> case VTD_SM_PASID_ENTRY_SLT: >> - return !s->scalable_modern; >> + return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS); > Sorry typo err, should be: > > + return !!(s->ecap & VTD_ECAP_SLTS) || !(s->ecap & VTD_ECAP_SMTS); > Moreover, shouldn't we declare the capabilities after the feature is implemented? I think FLTS and FS1GP should not be declared that early. >> case VTD_SM_PASID_ENTRY_NESTED: >> /* Not support NESTED page table type yet */ >> return false; >> case VTD_SM_PASID_ENTRY_PT: >> - return x86_iommu->pt_supported; >> + return !!(s->ecap & VTD_ECAP_PT); >> default: >> /* Unknown type */ >> return false; >> >> Thanks >> Zhenzhong
>-----Original Message----- >From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >scalable modern mode > > > >On 19/07/2024 05:39, Duan, Zhenzhong 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. >> >> >>> -----Original Message----- >>> From: Duan, Zhenzhong] >>> Subject: RE: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >for >>> scalable modern mode >>> >>> >>> >>>> -----Original Message----- >>>> From: Liu, Yi L <yi.l.liu@intel.com> >>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >for >>>> scalable modern mode >>>> >>>> On 2024/7/19 10:47, Duan, Zhenzhong wrote: >>>>> >>>>>> -----Original Message----- >>>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>>>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder >variable >>>> for >>>>>> scalable modern mode >>>>>> >>>>>> >>>>>> >>>>>> On 18/07/2024 10:16, Zhenzhong Duan 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. >>>>>>> >>>>>>> Add an new element scalable_mode in IntelIOMMUState to mark >>>> scalable >>>>>>> modern mode, this element will be exposed as an intel_iommu >>> property >>>>>>> finally. >>>>>>> >>>>>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>>>>> compatibility check and block host device passthrough until nesting >>>>>>> is supported. >>>>>>> >>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>>> --- >>>>>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>>>>> include/hw/i386/intel_iommu.h | 1 + >>>>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++-- >----- >>> -- >>>> -- >>>>>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/i386/intel_iommu_internal.h >>>>>> b/hw/i386/intel_iommu_internal.h >>>>>>> index c0ca7b372f..4e0331caba 100644 >>>>>>> --- a/hw/i386/intel_iommu_internal.h >>>>>>> +++ b/hw/i386/intel_iommu_internal.h >>>>>>> @@ -195,6 +195,7 @@ >>>>>>> #define VTD_ECAP_PASID (1ULL << 40) >>>>>>> #define VTD_ECAP_SMTS (1ULL << 43) >>>>>>> #define VTD_ECAP_SLTS (1ULL << 46) >>>>>>> +#define VTD_ECAP_FLTS (1ULL << 47) >>>>>>> >>>>>>> /* CAP_REG */ >>>>>>> /* (offset >> 4) << 24 */ >>>>>>> @@ -211,6 +212,7 @@ >>>>>>> #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) >>>>>>> #define VTD_CAP_DRAIN_WRITE (1ULL << 54) >>>>>>> #define VTD_CAP_DRAIN_READ (1ULL << 55) >>>>>>> +#define VTD_CAP_FS1GP (1ULL << 56) >>>>>>> #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | >>>>>> VTD_CAP_DRAIN_WRITE) >>>>>>> #define VTD_CAP_CM (1ULL << 7) >>>>>>> #define VTD_PASID_ID_SHIFT 20 >>>>>>> diff --git a/include/hw/i386/intel_iommu.h >>>>>> b/include/hw/i386/intel_iommu.h >>>>>>> index 1eb05c29fc..788ed42477 100644 >>>>>>> --- a/include/hw/i386/intel_iommu.h >>>>>>> +++ b/include/hw/i386/intel_iommu.h >>>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>>>>> >>>>>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>>>>> bool scalable_mode; /* RO - is Scalable Mode supported? >>> */ >>>>>>> + bool scalable_modern; /* RO - is modern SM supported? >*/ >>>>>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>>>>> >>>>>>> dma_addr_t root; /* Current root table pointer */ >>>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>>>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>>>>> --- a/hw/i386/intel_iommu.c >>>>>>> +++ b/hw/i386/intel_iommu.c >>>>>>> @@ -755,16 +755,20 @@ static inline bool >>>>>> vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) >>>>>>> } >>>>>>> >>>>>>> /* Return true if check passed, otherwise false */ >>>>>>> -static inline bool vtd_pe_type_check(X86IOMMUState >*x86_iommu, >>>>>>> - VTDPASIDEntry *pe) >>>>>>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, >>>>>> VTDPASIDEntry *pe) >>>>>>> { >>>>>> What about using the cap/ecap registers to know if the translation >types >>>>>> are supported or not. >>>>>> Otherwise, we could add a comment to explain why we expect >>>>>> s->scalable_modern to give us enough information. >>>>> What about below: >>>>> >>>>> /* >>>>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >>>> VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >>>>> *So it's simpler to check s->scalable_modern directly for a PASID >entry >>>> type instead ecap bits. >>>>> */ >>>> Since this helper is for pasid entry check, so you can just return false >>>> if the pe's PGTT is SS-only. >>> It depends on which scalable mode is chosed. >>> In scalable legacy mode, PGTT is SS-only and we should return true. >>> >>>> It might make more sense to check the ecap/cap here as anyhow the >>>> capability is listed in ecap/cap. This may also bring us some convenience. >>>> >>>> Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >>>> that supports both FS and SS for guest, we may need to update this >helper >>>> as well if we check the scalable_modern. But if we check the ecap/cap, >then >>>> the future change just needs to control the ecap/cap setting at the >>>> beginning of the vIOMMU init. To keep the code aligned, you may need >to >>>> check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) >>> OK, will be like below: >>> >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -826,14 +826,14 @@ static inline bool >>> vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) >>> >>> switch (VTD_PE_GET_TYPE(pe)) { >>> case VTD_SM_PASID_ENTRY_FLT: >>> - return s->scalable_modern; >>> + return !!(s->ecap & VTD_ECAP_FLTS); >>> case VTD_SM_PASID_ENTRY_SLT: >>> - return !s->scalable_modern; >>> + return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & >VTD_ECAP_SMTS); >> Sorry typo err, should be: >> >> + return !!(s->ecap & VTD_ECAP_SLTS) || !(s->ecap & >VTD_ECAP_SMTS); >> >Moreover, shouldn't we declare the capabilities after the feature is >implemented? >I think FLTS and FS1GP should not be declared that early. OK, I can move it to " [PATCH v1 16/17] intel_iommu: Modify x-scalable-mode to be string option", In fact, before patch16, there is no way to enable s->scalable_modern, so those caps can never be enabled. Thanks Zhenzhong >>> case VTD_SM_PASID_ENTRY_NESTED: >>> /* Not support NESTED page table type yet */ >>> return false; >>> case VTD_SM_PASID_ENTRY_PT: >>> - return x86_iommu->pt_supported; >>> + return !!(s->ecap & VTD_ECAP_PT); >>> default: >>> /* Unknown type */ >>> return false; >>> >>> Thanks >>> Zhenzhong
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h index c0ca7b372f..4e0331caba 100644 --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -195,6 +195,7 @@ #define VTD_ECAP_PASID (1ULL << 40) #define VTD_ECAP_SMTS (1ULL << 43) #define VTD_ECAP_SLTS (1ULL << 46) +#define VTD_ECAP_FLTS (1ULL << 47) /* CAP_REG */ /* (offset >> 4) << 24 */ @@ -211,6 +212,7 @@ #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) #define VTD_CAP_DRAIN_WRITE (1ULL << 54) #define VTD_CAP_DRAIN_READ (1ULL << 55) +#define VTD_CAP_FS1GP (1ULL << 56) #define VTD_CAP_DRAIN (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE) #define VTD_CAP_CM (1ULL << 7) #define VTD_PASID_ID_SHIFT 20 diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 1eb05c29fc..788ed42477 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -262,6 +262,7 @@ struct IntelIOMMUState { bool caching_mode; /* RO - is cap CM enabled? */ bool scalable_mode; /* RO - is Scalable Mode supported? */ + bool scalable_modern; /* RO - is modern SM supported? */ bool snoop_control; /* RO - is SNP filed supported? */ dma_addr_t root; /* Current root table pointer */ diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 1cff8b00ae..40cbd4a0f4 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -755,16 +755,20 @@ static inline bool vtd_is_level_supported(IntelIOMMUState *s, uint32_t level) } /* Return true if check passed, otherwise false */ -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu, - VTDPASIDEntry *pe) +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) { + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); + switch (VTD_PE_GET_TYPE(pe)) { + case VTD_SM_PASID_ENTRY_FLT: + return s->scalable_modern; case VTD_SM_PASID_ENTRY_SLT: - return true; + return !s->scalable_modern; + case VTD_SM_PASID_ENTRY_NESTED: + /* Not support NESTED page table type yet */ + return false; case VTD_SM_PASID_ENTRY_PT: return x86_iommu->pt_supported; - case VTD_SM_PASID_ENTRY_FLT: - case VTD_SM_PASID_ENTRY_NESTED: default: /* Unknown type */ return false; @@ -813,7 +817,6 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, uint8_t pgtt; uint32_t index; dma_addr_t entry_size; - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); index = VTD_PASID_TABLE_INDEX(pasid); entry_size = VTD_PASID_ENTRY_SIZE; @@ -827,7 +830,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, } /* Do translation type check */ - if (!vtd_pe_type_check(x86_iommu, pe)) { + if (!vtd_pe_type_check(s, pe)) { return -VTD_FR_PASID_TABLE_ENTRY_INV; } @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod, return false; } - return true; + if (!s->scalable_modern) { + /* All checks requested by VTD non-modern mode pass */ + return true; + } + + error_setg(errp, "host device is unsupported in scalable modern mode yet"); + return false; } static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, @@ -4084,7 +4093,10 @@ static void vtd_cap_init(IntelIOMMUState *s) } /* TODO: read cap/ecap from host to decide which cap to be exposed. */ - if (s->scalable_mode) { + if (s->scalable_modern) { + s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS; + s->cap |= VTD_CAP_FS1GP; + } else if (s->scalable_mode) { s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS; } @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) } } - /* Currently only address widths supported are 39 and 48 bits */ if ((s->aw_bits != VTD_HOST_AW_39BIT) && - (s->aw_bits != VTD_HOST_AW_48BIT)) { + (s->aw_bits != VTD_HOST_AW_48BIT) && + !s->scalable_modern) { error_setg(errp, "Supported values for aw-bits are: %d, %d", VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); return false;