Re: [RFC PATCH 6/8] hw/arm/smmuv3-accel: Introduce _AUTO support for SSID size

[Markus Armbruster <armbru@redhat.com>]

Nathan Chen <nathanc@nvidia.com> writes:

> From: Nathan Chen <nathanc@nvidia.com>
>
> Allow accelerated SMMUv3 SSID size property to be derived from host
> IOMMU capabilities. Derive host values using IOMMU_GET_HW_INFO,
> retrieving SSID size from IDR1.
>
> Set the default ssidsize value to auto. The default SSID size used
> to be 0, but we change it to match what the host IOMMU properties
> report so that users do not have to introspect host IDR1 for the
> Substream ID support. When the auto SSID size is resolved to a
> non-zero value, PASID capability is advertised to the vIOMMU and
> accelerated use cases such as Shared Virtual Addressing (SVA) are
> supported.
>
> Signed-off-by: Nathan Chen <nathanc@nvidia.com>
> ---
>  hw/arm/smmuv3-accel.c   | 30 +++++++++++++++++++++++++++---
>  hw/arm/smmuv3.c         | 16 ++++++----------
>  include/hw/arm/smmuv3.h |  3 ++-
>  3 files changed, 35 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 02e3f7a9f3..bd27b0da7c 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -64,6 +64,13 @@ static void smmuv3_accel_auto_finalise(SMMUv3State *s, PCIDevice *pdev,
>                                 FIELD_EX32(info->idr[3], IDR3, RIL));
>      }
>  
> +    /* Update SSIDSIZE if auto from info */
> +    if (s->ssidsize == SSID_SIZE_MODE_AUTO) {
> +        /* Store for get_viommu_flags() to determine PASID support */
> +        s->idr[1] = FIELD_DP32(s->idr[1], IDR1, SSIDSIZE,
> +                               FIELD_EX32(info->idr[1], IDR1, SSIDSIZE));
> +    }
> +
>      accel->auto_finalised = true;
>  }
>  
> @@ -839,7 +846,10 @@ static uint64_t smmuv3_accel_get_viommu_flags(void *opaque)
>      SMMUState *bs = opaque;
>      SMMUv3State *s = ARM_SMMUV3(bs);
>  
> -    if (s->ssidsize) {
> +    if ((s->ssidsize != SSID_SIZE_MODE_0 &&
> +         s->ssidsize != SSID_SIZE_MODE_AUTO) ||
> +        (s->ssidsize == SSID_SIZE_MODE_AUTO &&
> +         FIELD_EX32(s->idr[1], IDR1, SSIDSIZE))) {
>          flags |= VIOMMU_FLAG_PASID_SUPPORTED;
>      }
>      return flags;
> @@ -854,6 +864,16 @@ static const PCIIOMMUOps smmuv3_accel_ops = {
>      .get_msi_direct_gpa = smmuv3_accel_get_msi_gpa,
>  };
>  
> +static uint8_t ssidsize_mode_to_value(SsidSizeMode mode)
> +{
> +    /* SSID_SIZE_MODE_0 = 1, SSID_SIZE_MODE_1 = 2, etc. */
> +    /* SSID_SIZE_MODE_AUTO = 0 */
> +    if (mode == SSID_SIZE_MODE_AUTO) {
> +        return 0;
> +    }
> +    return mode - 1;  /* Enum values are offset by 1 from actual values */

This relies on how the enum values are ordered in the schema, and on how
the QAPI generator encodes enums.  The latter is fine: we document it in
docs/devel/qapi-code-gen.rst section "Enumeration types".  The former is
worth a non-doc comment in the schema, like this:

   { 'enum': 'SsidSizeMode',
     'data': [ 'auto', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
               '10', '11', '12', '13', '14', '15', '16', '17', '18',
               '19', '20' ] } # order matters, see ssid_size_mode_auto()


> +}
> +
>  void smmuv3_accel_idr_override(SMMUv3State *s)
>  {
>      if (!s->accel) {
> @@ -886,7 +906,10 @@ void smmuv3_accel_idr_override(SMMUv3State *s)
>       * By default QEMU SMMUv3 has no SubstreamID support. Update IDR1 if user
>       * has enabled it.
>       */
> -    s->idr[1] = FIELD_DP32(s->idr[1], IDR1, SSIDSIZE, s->ssidsize);
> +    if (s->ssidsize != SSID_SIZE_MODE_AUTO) {
> +        s->idr[1] = FIELD_DP32(s->idr[1], IDR1, SSIDSIZE,
> +                               ssidsize_mode_to_value(s->ssidsize));
> +    }
>  }
>  
>  /* Based on SMUUv3 GPBA.ABORT configuration, attach a corresponding HWPT */
> @@ -955,7 +978,8 @@ void smmuv3_accel_init(SMMUv3State *s)
>      smmuv3_accel_as_init(s);
>  
>      if (s->ats == ON_OFF_AUTO_AUTO ||
> -        s->ril == ON_OFF_AUTO_AUTO) {
> +        s->ril == ON_OFF_AUTO_AUTO ||
> +        s->ssidsize == SSID_SIZE_MODE_AUTO) {
>          s->s_accel->auto_mode = true;
>      }
>  }
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 7791e5294d..bc03353759 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -20,6 +20,7 @@
>  #include "qemu/bitops.h"
>  #include "hw/core/irq.h"
>  #include "hw/core/sysbus.h"
> +#include "hw/core/qdev-properties-system.h"
>  #include "migration/blocker.h"
>  #include "migration/vmstate.h"
>  #include "hw/core/qdev-properties.h"
> @@ -626,7 +627,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>      }
>  
>      /* Multiple context descriptors require SubstreamID support */
> -    if (!s->ssidsize && STE_S1CDMAX(ste) != 0) {
> +    if (s->ssidsize == SSID_SIZE_MODE_0 && STE_S1CDMAX(ste) != 0) {
>          qemu_log_mask(LOG_UNIMP,
>                  "SMMUv3: multiple S1 context descriptors require SubstreamID support. "
>                  "Configure ssidsize > 0 (requires accel=on)\n");
> @@ -1985,7 +1986,7 @@ static bool smmu_validate_property(SMMUv3State *s, Error **errp)
>              error_setg(errp, "OAS must be 44 bits when accel=off");
>              return false;
>          }
> -        if (s->ssidsize) {
> +        if (s->ssidsize > SSID_SIZE_MODE_0) {
>              error_setg(errp, "ssidsize can only be set if accel=on");
>              return false;
>          }
> @@ -2003,11 +2004,6 @@ static bool smmu_validate_property(SMMUv3State *s, Error **errp)
>          error_setg(errp, "OAS can only be set to 44 or 48 bits");
>          return false;
>      }
> -    if (s->ssidsize > SMMU_SSID_MAX_BITS) {
> -        error_setg(errp, "ssidsize must be in the range 0 to %d",
> -                   SMMU_SSID_MAX_BITS);
> -        return false;
> -    }
>  
>      return true;
>  }
> @@ -2136,7 +2132,8 @@ static const Property smmuv3_properties[] = {
>      DEFINE_PROP_ON_OFF_AUTO("ril", SMMUv3State, ril, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_ON_OFF_AUTO("ats", SMMUv3State, ats, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_UINT8("oas", SMMUv3State, oas, 44),
> -    DEFINE_PROP_UINT8("ssidsize", SMMUv3State, ssidsize, 0),
> +    DEFINE_PROP_SSIDSIZE_MODE("ssidsize", SMMUv3State, ssidsize,
> +                              SSID_SIZE_MODE_AUTO),

Is property "ssidsize" accessible via QMP or JSON command line?  If yes,
this is an incompatible change: JSON integer values no longer work.

>  };
>  
>  static void smmuv3_instance_init(Object *obj)
> @@ -2176,8 +2173,7 @@ static void smmuv3_class_init(ObjectClass *klass, const void *data)
>          "Number of bits used to represent SubstreamIDs (SSIDs). "
>          "A value of N allows SSIDs in the range [0 .. 2^N - 1]. "
>          "Valid range is 0-20, where 0 disables SubstreamID support. "
> -        "Defaults to 0. A value greater than 0 is required to enable "
> -        "PASID support.");
> +        "A value greater than 0 is required to enable PASID support.");
>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index 9124bfe751..ae8158a5c3 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -21,6 +21,7 @@
>  
>  #include "hw/arm/smmu-common.h"
>  #include "qom/object.h"
> +#include "qapi/qapi-types-misc-arm.h"
>  
>  #define TYPE_SMMUV3_IOMMU_MEMORY_REGION "smmuv3-iommu-memory-region"
>  
> @@ -72,7 +73,7 @@ struct SMMUv3State {
>      OnOffAuto ril;
>      OnOffAuto ats;
>      uint8_t oas;
> -    uint8_t ssidsize;
> +    SsidSizeMode ssidsize;
>  };
>  
>  typedef enum {