| Message ID | 20240930092631.2997543-15-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | intel_iommu: Enable stage-1 translation for emulated device | expand |
On 2024/9/30 17:26, Zhenzhong Duan wrote: > According to VTD spec, stage-1 page table could support 4-level and > 5-level paging. > > However, 5-level paging translation emulation is unsupported yet. > That means the only supported value for aw_bits is 48. > > So default aw_bits to 48 in scalable modern mode. In other cases, > it is still default to 39 for backward compatibility. > > Add a check to ensure user specified value is 48 in modern mode > for now. this is not a simple check. I think your patch makes an auto selection of aw_bits. > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com> > --- > include/hw/i386/intel_iommu.h | 2 +- > hw/i386/intel_iommu.c | 10 +++++++++- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index b843d069cc..48134bda11 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -45,7 +45,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, INTEL_IOMMU_DEVICE) > #define DMAR_REG_SIZE 0x230 > #define VTD_HOST_AW_39BIT 39 > #define VTD_HOST_AW_48BIT 48 > -#define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT > +#define VTD_HOST_AW_AUTO 0xff > #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) > > #define DMAR_REPORT_F_INTR (1) > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 91d7b1abfa..068a08f522 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3776,7 +3776,7 @@ static Property vtd_properties[] = { > ON_OFF_AUTO_AUTO), > DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), > DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits, > - VTD_HOST_ADDRESS_WIDTH), > + VTD_HOST_AW_AUTO), > DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE), > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE), > DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false), > @@ -4683,6 +4683,14 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > } > } > > + if (s->aw_bits == VTD_HOST_AW_AUTO) { > + if (s->scalable_modern) { > + s->aw_bits = VTD_HOST_AW_48BIT; > + } else { > + s->aw_bits = VTD_HOST_AW_39BIT; > + } > + } If the default value of s->aw_bits is still 39, you don't know if it's set by the admin or the orchestration stack. This is why you need to change it. right? > + > if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT && > s->aw_bits != VTD_HOST_AW_48BIT) { > error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",
>-----Original Message----- >From: Liu, Yi L <yi.l.liu@intel.com> >Sent: Monday, November 4, 2024 11:16 AM >Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable >modern mode > >On 2024/9/30 17:26, Zhenzhong Duan wrote: >> According to VTD spec, stage-1 page table could support 4-level and >> 5-level paging. >> >> However, 5-level paging translation emulation is unsupported yet. >> That means the only supported value for aw_bits is 48. >> >> So default aw_bits to 48 in scalable modern mode. In other cases, >> it is still default to 39 for backward compatibility. >> >> Add a check to ensure user specified value is 48 in modern mode >> for now. > >this is not a simple check. I think your patch makes an auto selection >of aw_bits. Yes, if user doesn't specify it, will auto select a default. > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com> >> --- >> include/hw/i386/intel_iommu.h | 2 +- >> hw/i386/intel_iommu.c | 10 +++++++++- >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >> index b843d069cc..48134bda11 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -45,7 +45,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >INTEL_IOMMU_DEVICE) >> #define DMAR_REG_SIZE 0x230 >> #define VTD_HOST_AW_39BIT 39 >> #define VTD_HOST_AW_48BIT 48 >> -#define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >> +#define VTD_HOST_AW_AUTO 0xff >> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >> >> #define DMAR_REPORT_F_INTR (1) >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 91d7b1abfa..068a08f522 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -3776,7 +3776,7 @@ static Property vtd_properties[] = { >> ON_OFF_AUTO_AUTO), >> DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), >> DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits, >> - VTD_HOST_ADDRESS_WIDTH), >> + VTD_HOST_AW_AUTO), >> DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, >FALSE), >> DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, >FALSE), >> DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, >false), >> @@ -4683,6 +4683,14 @@ static bool vtd_decide_config(IntelIOMMUState *s, >Error **errp) >> } >> } >> >> + if (s->aw_bits == VTD_HOST_AW_AUTO) { >> + if (s->scalable_modern) { >> + s->aw_bits = VTD_HOST_AW_48BIT; >> + } else { >> + s->aw_bits = VTD_HOST_AW_39BIT; >> + } >> + } > >If the default value of s->aw_bits is still 39, you don't know if it's >set by the admin or the orchestration stack. This is why you need >to change it. right? Exactly, that's reason of introducing VTD_HOST_AW_AUTO(0xff). Thanks Zhenzhong
On 2024/11/4 11:19, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Liu, Yi L <yi.l.liu@intel.com> >> Sent: Monday, November 4, 2024 11:16 AM >> Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable >> modern mode >> >> On 2024/9/30 17:26, Zhenzhong Duan wrote: >>> According to VTD spec, stage-1 page table could support 4-level and >>> 5-level paging. >>> >>> However, 5-level paging translation emulation is unsupported yet. >>> That means the only supported value for aw_bits is 48. >>> >>> So default aw_bits to 48 in scalable modern mode. In other cases, >>> it is still default to 39 for backward compatibility. >>> >>> Add a check to ensure user specified value is 48 in modern mode >>> for now. >> >> this is not a simple check. I think your patch makes an auto selection >> of aw_bits. > > Yes, if user doesn't specify it, will auto select a default. > >> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com> >>> --- >>> include/hw/i386/intel_iommu.h | 2 +- >>> hw/i386/intel_iommu.c | 10 +++++++++- >>> 2 files changed, 10 insertions(+), 2 deletions(-) Reviewed-by: Yi Liu <yi.l.liu@intel.com>
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index b843d069cc..48134bda11 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -45,7 +45,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, INTEL_IOMMU_DEVICE) #define DMAR_REG_SIZE 0x230 #define VTD_HOST_AW_39BIT 39 #define VTD_HOST_AW_48BIT 48 -#define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT +#define VTD_HOST_AW_AUTO 0xff #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) #define DMAR_REPORT_F_INTR (1) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 91d7b1abfa..068a08f522 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3776,7 +3776,7 @@ static Property vtd_properties[] = { ON_OFF_AUTO_AUTO), DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits, - VTD_HOST_ADDRESS_WIDTH), + VTD_HOST_AW_AUTO), DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE), DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE), DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false), @@ -4683,6 +4683,14 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) } } + if (s->aw_bits == VTD_HOST_AW_AUTO) { + if (s->scalable_modern) { + s->aw_bits = VTD_HOST_AW_48BIT; + } else { + s->aw_bits = VTD_HOST_AW_39BIT; + } + } + if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT && s->aw_bits != VTD_HOST_AW_48BIT) { error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",