[PATCH 0/2] ARM SMMUv3 StreamID Implementation

[PATCH 0/2] ARM SMMUv3 StreamID Implementation

[Nabih Estefan]

This patch series modifies the ARM SMMUv3 to be able to work with an
implementation specific StreamID that does not match exactly the PCIe BDF.
The way to achieve this is by converting the smmu_get_sid and smmu_iommu_mr
functions to virtual functions that can be overridden by inheritance, making
sure the StreamID is consistently 32 bits and removing the hardcoding of the
SMMU_IDR1.SIDSIZE to 16 bits.

Roque Arcudia Hernandez (2):
  hw/arm/smmuv3: Check StreamIDs against SMMU_IDR1.SIDSIZE value
  hw/arm/smmu-common: Create virtual function for implementation defined
    StreamID

 hw/arm/smmu-common.c         | 12 ++++++++++++
 hw/arm/smmuv3.c              |  4 +++-
 include/hw/arm/smmu-common.h | 16 +++++++++++-----
 3 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 1/2] hw/arm/smmuv3: Check StreamIDs against SMMU_IDR1.SIDSIZE value

[Nabih Estefan]

From: Roque Arcudia Hernandez <roqueh@google.com>

Current implementation checks the StreamIDs against STRTAB_BASE_CFG.LOG2SIZE
register field value and a constant SMMU_IDR1_SIDSIZE which is also used as
initial value for field SMMU_IDR1.SIDSIZE.

This limits the possibility of extending the SMMUv3 by inheritance and
redefining the value of SMMU_IDR1.SIDSIZE because the check is hardcoded to the
constant SMMU_IDR1_SIDSIZE rather than the register value.

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 hw/arm/smmuv3.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 9eb56a70f3..a01031821a 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -580,15 +580,17 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
 {
     dma_addr_t addr, strtab_base;
     uint32_t log2size;
+    uint32_t idr1_sidsize;
     int strtab_size_shift;
     int ret;
 
     trace_smmuv3_find_ste(sid, s->features, s->sid_split);
     log2size = FIELD_EX32(s->strtab_base_cfg, STRTAB_BASE_CFG, LOG2SIZE);
+    idr1_sidsize = FIELD_EX32(s->idr[1], IDR1, SIDSIZE);
     /*
      * Check SID range against both guest-configured and implementation limits
      */
-    if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
+    if (sid >= (1 << MIN(log2size, idr1_sidsize))) {
         event->type = SMMU_EVT_C_BAD_STREAMID;
         return -EINVAL;
     }
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH 1/2] hw/arm/smmuv3: Check StreamIDs against SMMU_IDR1.SIDSIZE value

[Eric Auger]

Hi,

On 2/21/24 18:17, Nabih Estefan wrote:
> From: Roque Arcudia Hernandez <roqueh@google.com>
>
> Current implementation checks the StreamIDs against STRTAB_BASE_CFG.LOG2SIZE
> register field value and a constant SMMU_IDR1_SIDSIZE which is also used as
> initial value for field SMMU_IDR1.SIDSIZE.
>
> This limits the possibility of extending the SMMUv3 by inheritance and
> redefining the value of SMMU_IDR1.SIDSIZE because the check is hardcoded to the
> constant SMMU_IDR1_SIDSIZE rather than the register value.
>
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/arm/smmuv3.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 9eb56a70f3..a01031821a 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -580,15 +580,17 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>  {
>      dma_addr_t addr, strtab_base;
>      uint32_t log2size;
> +    uint32_t idr1_sidsize;
>      int strtab_size_shift;
>      int ret;
>  
>      trace_smmuv3_find_ste(sid, s->features, s->sid_split);
>      log2size = FIELD_EX32(s->strtab_base_cfg, STRTAB_BASE_CFG, LOG2SIZE);
> +    idr1_sidsize = FIELD_EX32(s->idr[1], IDR1, SIDSIZE);
>      /*
>       * Check SID range against both guest-configured and implementation limits
>       */
> -    if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
> +    if (sid >= (1 << MIN(log2size, idr1_sidsize))) {
>          event->type = SMMU_EVT_C_BAD_STREAMID;
>          return -EINVAL;
>      }

[PATCH 2/2] hw/arm/smmu-common: Create virtual function for implementation defined StreamID

[Nabih Estefan]

From: Roque Arcudia Hernandez <roqueh@google.com>

According to the SMMU specification the StreamID construction and size is
IMPLEMENTATION DEFINED, the size being between 0 and 32 bits.

This patch creates virtual functions get_sid and get_iommu_mr to allow different
implementations of how the StreamID is constructed via inheritance. The default
implementation of these functions will match the current ones where the BDF is
used directly as StreamID.

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 hw/arm/smmu-common.c         | 12 ++++++++++++
 include/hw/arm/smmu-common.h | 16 +++++++++++-----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 4caedb4998..14b3240a88 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -621,6 +621,11 @@ static const PCIIOMMUOps smmu_ops = {
 };
 
 IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
+{
+    return ARM_SMMU_GET_CLASS(s)->get_iommu_mr(s, sid);
+}
+
+static IOMMUMemoryRegion *smmu_base_iommu_mr(SMMUState *s, uint32_t sid)
 {
     uint8_t bus_n, devfn;
     SMMUPciBus *smmu_bus;
@@ -659,6 +664,11 @@ void smmu_inv_notifiers_all(SMMUState *s)
     }
 }
 
+static uint32_t smmu_base_get_sid(SMMUDevice *sdev)
+{
+    return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
+}
+
 static void smmu_base_realize(DeviceState *dev, Error **errp)
 {
     SMMUState *s = ARM_SMMU(dev);
@@ -709,6 +719,8 @@ static void smmu_base_class_init(ObjectClass *klass, void *data)
     device_class_set_parent_realize(dc, smmu_base_realize,
                                     &sbc->parent_realize);
     rc->phases.hold = smmu_base_reset_hold;
+    sbc->get_sid = smmu_base_get_sid;
+    sbc->get_iommu_mr = smmu_base_iommu_mr;
 }
 
 static const TypeInfo smmu_base_info = {
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 5ec2e6c1a4..d53121fe37 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -131,6 +131,9 @@ typedef struct SMMUIOTLBKey {
     uint8_t level;
 } SMMUIOTLBKey;
 
+#define TYPE_ARM_SMMU "arm-smmu"
+OBJECT_DECLARE_TYPE(SMMUState, SMMUBaseClass, ARM_SMMU)
+
 struct SMMUState {
     /* <private> */
     SysBusDevice  dev;
@@ -147,6 +150,9 @@ struct SMMUState {
     PCIBus *primary_bus;
 };
 
+typedef uint32_t GetSidFunc(SMMUDevice *obj);
+typedef IOMMUMemoryRegion *GetIommuMr(SMMUState *s, uint32_t sid);
+
 struct SMMUBaseClass {
     /* <private> */
     SysBusDeviceClass parent_class;
@@ -154,19 +160,19 @@ struct SMMUBaseClass {
     /*< public >*/
 
     DeviceRealize parent_realize;
+    GetSidFunc *get_sid;
+    GetIommuMr *get_iommu_mr;
 
 };
 
-#define TYPE_ARM_SMMU "arm-smmu"
-OBJECT_DECLARE_TYPE(SMMUState, SMMUBaseClass, ARM_SMMU)
-
 /* Return the SMMUPciBus handle associated to a PCI bus number */
 SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num);
 
 /* Return the stream ID of an SMMU device */
-static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
+static inline uint32_t smmu_get_sid(SMMUDevice *sdev)
 {
-    return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
+    SMMUState *s = sdev->smmu;
+    return ARM_SMMU_GET_CLASS(s)->get_sid(sdev);
 }
 
 /**
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH 2/2] hw/arm/smmu-common: Create virtual function for implementation defined StreamID

[Eric Auger]

Hi,

On 2/21/24 18:17, Nabih Estefan wrote:
> From: Roque Arcudia Hernandez <roqueh@google.com>
>
> According to the SMMU specification the StreamID construction and size is
> IMPLEMENTATION DEFINED, the size being between 0 and 32 bits.
>
> This patch creates virtual functions get_sid and get_iommu_mr to allow different
> implementations of how the StreamID is constructed via inheritance. The default
> implementation of these functions will match the current ones where the BDF is
> used directly as StreamID.

The patch itself looks good to me but it lacks a concrete derived
implementation of get_sid() and get_iommu_mr(). Until you do not
upstream it I don't see the point in introducing those callbacks. Thanks
Eric
>
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>  hw/arm/smmu-common.c         | 12 ++++++++++++
>  include/hw/arm/smmu-common.h | 16 +++++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 4caedb4998..14b3240a88 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -621,6 +621,11 @@ static const PCIIOMMUOps smmu_ops = {
>  };
>  
>  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
> +{
> +    return ARM_SMMU_GET_CLASS(s)->get_iommu_mr(s, sid);
> +}
> +
> +static IOMMUMemoryRegion *smmu_base_iommu_mr(SMMUState *s, uint32_t sid)
>  {
>      uint8_t bus_n, devfn;
>      SMMUPciBus *smmu_bus;
> @@ -659,6 +664,11 @@ void smmu_inv_notifiers_all(SMMUState *s)
>      }
>  }
>  
> +static uint32_t smmu_base_get_sid(SMMUDevice *sdev)
> +{
> +    return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
> +}
> +
>  static void smmu_base_realize(DeviceState *dev, Error **errp)
>  {
>      SMMUState *s = ARM_SMMU(dev);
> @@ -709,6 +719,8 @@ static void smmu_base_class_init(ObjectClass *klass, void *data)
>      device_class_set_parent_realize(dc, smmu_base_realize,
>                                      &sbc->parent_realize);
>      rc->phases.hold = smmu_base_reset_hold;
> +    sbc->get_sid = smmu_base_get_sid;
> +    sbc->get_iommu_mr = smmu_base_iommu_mr;
>  }
>  
>  static const TypeInfo smmu_base_info = {
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 5ec2e6c1a4..d53121fe37 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -131,6 +131,9 @@ typedef struct SMMUIOTLBKey {
>      uint8_t level;
>  } SMMUIOTLBKey;
>  
> +#define TYPE_ARM_SMMU "arm-smmu"
> +OBJECT_DECLARE_TYPE(SMMUState, SMMUBaseClass, ARM_SMMU)
> +
>  struct SMMUState {
>      /* <private> */
>      SysBusDevice  dev;
> @@ -147,6 +150,9 @@ struct SMMUState {
>      PCIBus *primary_bus;
>  };
>  
> +typedef uint32_t GetSidFunc(SMMUDevice *obj);
> +typedef IOMMUMemoryRegion *GetIommuMr(SMMUState *s, uint32_t sid);
> +
>  struct SMMUBaseClass {
>      /* <private> */
>      SysBusDeviceClass parent_class;
> @@ -154,19 +160,19 @@ struct SMMUBaseClass {
>      /*< public >*/
>  
>      DeviceRealize parent_realize;
> +    GetSidFunc *get_sid;
> +    GetIommuMr *get_iommu_mr;
>  
>  };
>  
> -#define TYPE_ARM_SMMU "arm-smmu"
> -OBJECT_DECLARE_TYPE(SMMUState, SMMUBaseClass, ARM_SMMU)
> -
>  /* Return the SMMUPciBus handle associated to a PCI bus number */
>  SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num);
>  
>  /* Return the stream ID of an SMMU device */
> -static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
> +static inline uint32_t smmu_get_sid(SMMUDevice *sdev)
>  {
> -    return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
> +    SMMUState *s = sdev->smmu;
> +    return ARM_SMMU_GET_CLASS(s)->get_sid(sdev);
>  }
>  
>  /**

Re: [PATCH 0/2] ARM SMMUv3 StreamID Implementation

[Alex Bennée]

Nabih Estefan <nabihestefan@google.com> writes:

> This patch series modifies the ARM SMMUv3 to be able to work with an
> implementation specific StreamID that does not match exactly the PCIe BDF.
> The way to achieve this is by converting the smmu_get_sid and smmu_iommu_mr
> functions to virtual functions that can be overridden by inheritance, making
> sure the StreamID is consistently 32 bits and removing the hardcoding of the
> SMMU_IDR1.SIDSIZE to 16 bits.

I was just going through my outstanding review queue for '24 and saw
this didn't get merged. Was there a re-spin that I missed? I see Eric
left a comment on 2/2.

>
> Roque Arcudia Hernandez (2):
>   hw/arm/smmuv3: Check StreamIDs against SMMU_IDR1.SIDSIZE value
>   hw/arm/smmu-common: Create virtual function for implementation defined
>     StreamID
>
>  hw/arm/smmu-common.c         | 12 ++++++++++++
>  hw/arm/smmuv3.c              |  4 +++-
>  include/hw/arm/smmu-common.h | 16 +++++++++++-----
>  3 files changed, 26 insertions(+), 6 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro