qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable


From: Duan, Zhenzhong
Subject: RE: [PATCH v4 14/17] intel_iommu: Set default aw_bits to 48 in scalable modern mode
Date: Mon, 11 Nov 2024 02:58:00 +0000

>-----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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]