>-----Original Message-----
>From: Jason Wang <jasow...@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.d...@intel.com> wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang <jasow...@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.d...@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.d...@intel.com>
>> >> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--
>d...@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
