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.
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 (!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",
--
2.34.1
On Mon, Sep 30, 2024 at 5:30 PM Zhenzhong Duan <zhenzhong.duan@intel.com> 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.
>
> 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;
> + }
I don't see how we maintain migration compatibility here.
Thanks
> + }
> +
> 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",
> --
> 2.34.1
>
>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Sent: Friday, November 8, 2024 12:42 PM
>Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable
>modern mode
>
>On Mon, Sep 30, 2024 at 5:30 PM Zhenzhong Duan <zhenzhong.duan@intel.com>
>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.
>>
>> 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;
>> + }
>
>I don't see how we maintain migration compatibility here.
Imagine this cmdline: "-device intel-iommu,x-scalable-mode=on" which hints
scalable legacy mode(a.k.a, stage-2 page table mode),
without this patch, initial s->aw_bits value is VTD_HOST_ADDRESS_WIDTH(39).
after this patch, initial s->aw_bit value is VTD_HOST_AW_AUTO(0xff),
vtd_decide_config() is called by vtd_realize() to set s->aw_bit to VTD_HOST_AW_39BIT(39).
So as long as the QEMU cmdline is same, s->aw_bit is same with or without this patch.
Thanks
Zhenzhong
On Fri, Nov 8, 2024 at 1:30 PM Duan, Zhenzhong <zhenzhong.duan@intel.com> wrote:
>
>
>
> >-----Original Message-----
> >From: Jason Wang <jasowang@redhat.com>
> >Sent: Friday, November 8, 2024 12:42 PM
> >Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable
> >modern mode
> >
> >On Mon, Sep 30, 2024 at 5:30 PM Zhenzhong Duan <zhenzhong.duan@intel.com>
> >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.
> >>
> >> 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;
> >> + }
> >
> >I don't see how we maintain migration compatibility here.
>
> Imagine this cmdline: "-device intel-iommu,x-scalable-mode=on" which hints
> scalable legacy mode(a.k.a, stage-2 page table mode),
>
> without this patch, initial s->aw_bits value is VTD_HOST_ADDRESS_WIDTH(39).
>
> after this patch, initial s->aw_bit value is VTD_HOST_AW_AUTO(0xff),
> vtd_decide_config() is called by vtd_realize() to set s->aw_bit to VTD_HOST_AW_39BIT(39).
>
> So as long as the QEMU cmdline is same, s->aw_bit is same with or without this patch.
Ok, I guess the point is that the scalabe-modern mode is introduced in
this series so we won't bother.
But I see this:
+ if (s->scalable_modern && s->aw_bits != VTD_HOST_AW_48BIT) {
In previous patches. So I wonder instead of mandating management to
set AUTO which seems like a burden. How about just increase the
default AW to 48bit and do the compatibility work here?
Thanks
>
> Thanks
> Zhenzhong
>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Sent: Monday, November 11, 2024 9:24 AM
>Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable
>modern mode
>
>On Fri, Nov 8, 2024 at 1:30 PM Duan, Zhenzhong <zhenzhong.duan@intel.com>
>wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang <jasowang@redhat.com>
>> >Sent: Friday, November 8, 2024 12:42 PM
>> >Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in
>scalable
>> >modern mode
>> >
>> >On Mon, Sep 30, 2024 at 5:30 PM Zhenzhong Duan
><zhenzhong.duan@intel.com>
>> >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.
>> >>
>> >> 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;
>> >> + }
>> >
>> >I don't see how we maintain migration compatibility here.
>>
>> Imagine this cmdline: "-device intel-iommu,x-scalable-mode=on" which hints
>> scalable legacy mode(a.k.a, stage-2 page table mode),
>>
>> without this patch, initial s->aw_bits value is VTD_HOST_ADDRESS_WIDTH(39).
>>
>> after this patch, initial s->aw_bit value is VTD_HOST_AW_AUTO(0xff),
>> vtd_decide_config() is called by vtd_realize() to set s->aw_bit to
>VTD_HOST_AW_39BIT(39).
>>
>> So as long as the QEMU cmdline is same, s->aw_bit is same with or without this
>patch.
>
>Ok, I guess the point is that the scalabe-modern mode is introduced in
>this series so we won't bother.
>
>But I see this:
>
>+ if (s->scalable_modern && s->aw_bits != VTD_HOST_AW_48BIT) {
>
>In previous patches. So I wonder instead of mandating management to
>set AUTO which seems like a burden. How about just increase the
>default AW to 48bit and do the compatibility work here?
Good idea! Then we don't need VTD_HOST_AW_AUTO(0xff).
Default is 48 starting from qemu 9.2 both for modern and legacy mode,
Default is still 39 for qemu before 9.2. Will be like below, let me know if
any misunderstandings.
--- 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_ADDRESS_WIDTH VTD_HOST_AW_48BIT
#define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1)
#define DMAR_REPORT_F_INTR (1)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 830614d930..bdb67f1fd4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -83,6 +83,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" },
+ { TYPE_INTEL_IOMMU_DEVICE, "aw-bits", "39" },
};
const size_t pc_compat_9_1_len = G_N_ELEMENTS(pc_compat_9_1);
Thanks
Zhenzhong
On Mon, Nov 11, 2024 at 10:58 AM Duan, Zhenzhong
<zhenzhong.duan@intel.com> wrote:
>
>
>
> >-----Original Message-----
> >From: Jason Wang <jasowang@redhat.com>
> >Sent: Monday, November 11, 2024 9:24 AM
> >Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable
> >modern mode
> >
> >On Fri, Nov 8, 2024 at 1:30 PM Duan, Zhenzhong <zhenzhong.duan@intel.com>
> >wrote:
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Jason Wang <jasowang@redhat.com>
> >> >Sent: Friday, November 8, 2024 12:42 PM
> >> >Subject: Re: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in
> >scalable
> >> >modern mode
> >> >
> >> >On Mon, Sep 30, 2024 at 5:30 PM Zhenzhong Duan
> ><zhenzhong.duan@intel.com>
> >> >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.
> >> >>
> >> >> 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;
> >> >> + }
> >> >
> >> >I don't see how we maintain migration compatibility here.
> >>
> >> Imagine this cmdline: "-device intel-iommu,x-scalable-mode=on" which hints
> >> scalable legacy mode(a.k.a, stage-2 page table mode),
> >>
> >> without this patch, initial s->aw_bits value is VTD_HOST_ADDRESS_WIDTH(39).
> >>
> >> after this patch, initial s->aw_bit value is VTD_HOST_AW_AUTO(0xff),
> >> vtd_decide_config() is called by vtd_realize() to set s->aw_bit to
> >VTD_HOST_AW_39BIT(39).
> >>
> >> So as long as the QEMU cmdline is same, s->aw_bit is same with or without this
> >patch.
> >
> >Ok, I guess the point is that the scalabe-modern mode is introduced in
> >this series so we won't bother.
> >
> >But I see this:
> >
> >+ if (s->scalable_modern && s->aw_bits != VTD_HOST_AW_48BIT) {
> >
> >In previous patches. So I wonder instead of mandating management to
> >set AUTO which seems like a burden. How about just increase the
> >default AW to 48bit and do the compatibility work here?
>
> Good idea! Then we don't need VTD_HOST_AW_AUTO(0xff).
> Default is 48 starting from qemu 9.2 both for modern and legacy mode,
> Default is still 39 for qemu before 9.2. Will be like below, let me know if
> any misunderstandings.
>
> --- 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_ADDRESS_WIDTH VTD_HOST_AW_48BIT
> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1)
>
> #define DMAR_REPORT_F_INTR (1)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 830614d930..bdb67f1fd4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -83,6 +83,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" },
> + { TYPE_INTEL_IOMMU_DEVICE, "aw-bits", "39" },
> };
> const size_t pc_compat_9_1_len = G_N_ELEMENTS(pc_compat_9_1);
Ack.
Thanks
>
>
>
> Thanks
> Zhenzhong
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",
--
Regards,
Yi Liu
>-----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> -- Regards, Yi Liu
© 2016 - 2024 Red Hat, Inc.