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

[Jason Wang]

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
>