Re: [PATCH v4 23/27] hw/arm/smmuv3-accel: Add property to specify OAS bits

[Eric Auger <eric.auger@redhat.com>]

On 9/29/25 3:36 PM, Shameer Kolothum wrote:
> QEMU SMMUv3 currently sets the output address size (OAS) to 44 bits. With
> accelerator mode enabled, a guest device may use SVA where CPU page tables
> are shared with SMMUv3, requiring OAS at least equal to the CPU OAS. Add
physical CPU OAS.
> a user option to set this.
>
> Note: Linux kernel docs currently state the OAS field in the IDR register
> is not meaningful for users. But looks like we need this information.
My concern is how will the above layers (libvirt) know how to set those
low level props?
At least in 14/27, during the compatibility check, you shall print the
host values to help the user determine the relevant option values
>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>  hw/arm/smmuv3-accel.c    | 15 +++++++++++++++
>  hw/arm/smmuv3-internal.h |  3 ++-
>  hw/arm/smmuv3.c          | 15 ++++++++++++++-
>  include/hw/arm/smmuv3.h  |  1 +
>  4 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index eee54316bf..ba37d690ad 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -86,6 +86,17 @@ smmuv3_accel_check_hw_compatible(SMMUv3State *s,
>          return false;
>      }
>  
> +    /*
> +     * ToDo: OAS is not something Linux kernel doc says meaningful for user.
> +     * But looks like OAS needs to be compatibe for accelerator support. Please
compatible
> +     * check.
> +     */
> +    val = FIELD_EX32(info->idr[5], IDR5, OAS);
> +    if (val < FIELD_EX32(s->idr[5], IDR5, OAS)) {
> +        error_setg(errp, "Host SUMMUv3 OAS not compatible");
print the host value too
> +        return false;
> +    }
> +
>      val = FIELD_EX32(info->idr[5], IDR5, GRAN4K);
>      if (val != FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
>          error_setg(errp, "Host SMMUv3 doesn't support 64K translation granule");
> @@ -648,6 +659,10 @@ void smmuv3_accel_idr_override(SMMUv3State *s)
>      if (s->ats) {
>          s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ATS, 1); /* ATS */
>      }
> +    /* QEMU SMMUv3 has oas set 44. Update IDR5 if user has it set to 48 bits*/
> +    if (s->oas == 48) {
> +        s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS_48);
> +    }
>  }
>  
>  /*
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index b0dfa9465c..910a34e05b 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -111,7 +111,8 @@ REG32(IDR5,                0x14)
>       FIELD(IDR5, VAX,        10, 2);
>       FIELD(IDR5, STALL_MAX,  16, 16);
>  
> -#define SMMU_IDR5_OAS 4
> +#define SMMU_IDR5_OAS_44 4
> +#define SMMU_IDR5_OAS_48 5
>  
>  REG32(IIDR,                0x18)
>  REG32(AIDR,                0x1c)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 77d46a9cd6..7c391ab711 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -294,7 +294,8 @@ static void smmuv3_init_regs(SMMUv3State *s)
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
>  
> -    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
> +    /* OAS: 44 bits */
> +    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS_44);
>      /* 4K, 16K and 64K granule support */
>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> @@ -1943,6 +1944,10 @@ static bool smmu_validate_property(SMMUv3State *s, Error **errp)
>      }
>  #endif
>      if (s->accel) {
> +        if (s->oas != 44 && s->oas != 48) {
> +            error_setg(errp, "oas can only be set to 44 or 48 bits");
s/oas/OAS
> +            return false;
> +        }
>          return true;
>      }
>      if (!s->ril) {
> @@ -1953,6 +1958,10 @@ static bool smmu_validate_property(SMMUv3State *s, Error **errp)
>          error_setg(errp, "ats can only be enabled if accel=on");
>          return false;
>      }
> +    if (s->oas != 44) {
> +        error_setg(errp, "oas can only be set to 44 bits if accel=off");
ditto
> +        return false;
> +    }
>      return true;
>  }
>  
> @@ -2078,6 +2087,7 @@ static const Property smmuv3_properties[] = {
>      /* RIL can be turned off for accel cases */
>      DEFINE_PROP_BOOL("ril", SMMUv3State, ril, true),
>      DEFINE_PROP_BOOL("ats", SMMUv3State, ats, false),
> +    DEFINE_PROP_UINT8("oas", SMMUv3State, oas, 44),
>  };
>  
>  static void smmuv3_instance_init(Object *obj)
> @@ -2110,6 +2120,9 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
>      object_class_property_set_description(klass, "ats",
>          "Enable/disable ATS support. Please ensure host platform has ATS "
>          "support before enabling this");
> +    object_class_property_set_description(klass, "oas",
> +        "Specify Output Address Size. Supported values are 44 or 48 bits "
> +        "Defaults to 44 bits");
>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index 6f07baa144..d3788b2d85 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -70,6 +70,7 @@ struct SMMUv3State {
>      Error  *migration_blocker;
>      bool ril;
>      bool ats;
> +    uint8_t oas;
>  };
>  
>  typedef enum {
Thanks

Eric