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

[Yi Liu <yi.l.liu@intel.com>]

On 2024/9/30 17:26, Zhenzhong Duan 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.

this is not a simple check. I think your patch makes an auto selection
of aw_bits.

> 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;
> +        }
> +    }

If the default value of s->aw_bits is still 39, you don't know if it's
set by the admin or the orchestration stack. This is why you need
to change it. right?

> +
>       if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT &&
>           s->aw_bits != VTD_HOST_AW_48BIT) {
>           error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",

-- 
Regards,
Yi Liu