On Mon, Nov 11, 2024 at 10:58 AM Duan, Zhenzhong
<zhenzhong.d...@intel.com> wrote:
>
>
>
> >-----Original Message-----
> >From: Jason Wang <jasow...@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.d...@intel.com>
> >wrote:
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Jason Wang <jasow...@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.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 backward 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 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
