diff mbox series

[v3,14/17] intel_iommu: Set default aw_bits to 48 in scalable modern mode

Message ID 20240911052255.1294071-15-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong Sept. 11, 2024, 5:22 a.m. UTC 
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 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(-)

Comments

Jason Wang Sept. 27, 2024, 4:08 a.m. UTC | #1 
On Wed, Sep 11, 2024 at 1:27 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 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 c25211ddaf..949f120456 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3771,7 +3771,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),

Such command line API seems to be wired.

I think we can stick the current default and when scalable modern is
enabled by aw is not specified, we can change aw to 48?

>      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),
> @@ -4686,6 +4686,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->aw_bits != VTD_HOST_AW_39BIT) &&
>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
>          !s->scalable_modern) {
> --
> 2.34.1
>

Thanks
Duan, Zhenzhong Sept. 27, 2024, 6:38 a.m. UTC | #2 
>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Subject: Re: [PATCH v3 14/17] intel_iommu: Set default aw_bits to 48 in
>scalable modern mode
>
>On Wed, Sep 11, 2024 at 1:27 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 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 c25211ddaf..949f120456 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3771,7 +3771,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),
>
>Such command line API seems to be wired.
>
>I think we can stick the current default and when scalable modern is
>enabled by aw is not specified, we can change aw to 48?

Current default is 39. I use VTD_HOST_AW_AUTO to initialize aw as not specified.
Do we have other way to catch the update if we stick to 39?

Thanks
Zhenzhong

>
>>      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),
>> @@ -4686,6 +4686,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->aw_bits != VTD_HOST_AW_39BIT) &&
>>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>          !s->scalable_modern) {
>> --
>> 2.34.1
>>
>
>Thanks
Jason Wang Sept. 29, 2024, 2:02 a.m. UTC | #3 
On Fri, Sep 27, 2024 at 2:39 PM Duan, Zhenzhong
<zhenzhong.duan@intel.com> wrote:
>
>
>
> >-----Original Message-----
> >From: Jason Wang <jasowang@redhat.com>
> >Subject: Re: [PATCH v3 14/17] intel_iommu: Set default aw_bits to 48 in
> >scalable modern mode
> >
> >On Wed, Sep 11, 2024 at 1:27 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 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 c25211ddaf..949f120456 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -3771,7 +3771,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),
> >
> >Such command line API seems to be wired.
> >
> >I think we can stick the current default and when scalable modern is
> >enabled by aw is not specified, we can change aw to 48?
>
> Current default is 39. I use VTD_HOST_AW_AUTO to initialize aw as not specified.

If I read the code correctly, aw=0xff means "auto". This seems a
little bit wried.

And even if we change it to auto, we need deal with the migration
compatibility that stick 39 for old machine types.

> Do we have other way to catch the update if we stick to 39?

I meant I don't understand if there will be any issue if we keep use
39 as default. Or I may not get the point of this question.

Thanks

>
> Thanks
> Zhenzhong
>
> >
> >>      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),
> >> @@ -4686,6 +4686,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->aw_bits != VTD_HOST_AW_39BIT) &&
> >>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
> >>          !s->scalable_modern) {
> >> --
> >> 2.34.1
> >>
> >
> >Thanks
>
Duan, Zhenzhong Sept. 29, 2024, 2:57 a.m. UTC | #4 
>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Subject: Re: [PATCH v3 14/17] intel_iommu: Set default aw_bits to 48 in
>scalable modern mode
>
>On Fri, Sep 27, 2024 at 2:39 PM Duan, Zhenzhong
><zhenzhong.duan@intel.com> wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang <jasowang@redhat.com>
>> >Subject: Re: [PATCH v3 14/17] intel_iommu: Set default aw_bits to 48 in
>> >scalable modern mode
>> >
>> >On Wed, Sep 11, 2024 at 1:27 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 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 c25211ddaf..949f120456 100644
>> >> --- a/hw/i386/intel_iommu.c
>> >> +++ b/hw/i386/intel_iommu.c
>> >> @@ -3771,7 +3771,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),
>> >
>> >Such command line API seems to be wired.
>> >
>> >I think we can stick the current default and when scalable modern is
>> >enabled by aw is not specified, we can change aw to 48?
>>
>> Current default is 39. I use VTD_HOST_AW_AUTO to initialize aw as not
>specified.
>
>If I read the code correctly, aw=0xff means "auto". This seems a
>little bit wried.
>
>And even if we change it to auto, we need deal with the migration
>compatibility that stick 39 for old machine types.

0xff isn't the final initial value, in vtd_decide_config(), there is code to check 0xff
to do final initialization:

    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 old machine types force aw to 39, then above code is bypassed and 39 is sticked.

>
>> Do we have other way to catch the update if we stick to 39?
>
>I meant I don't understand if there will be any issue if we keep use
>39 as default. Or I may not get the point of this question.

If we default aw to 39, there is no way to decide if it's user forced value which we need to stick
or initial default value which we can change.

Thanks
Zhenzhong
diff mbox series

Patch

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 c25211ddaf..949f120456 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3771,7 +3771,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),
@@ -4686,6 +4686,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->aw_bits != VTD_HOST_AW_39BIT) &&
         (s->aw_bits != VTD_HOST_AW_48BIT) &&
         !s->scalable_modern) {