qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
@ 2024-02-27 22:24 Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 1/5] target/arm: Add requester ID to memattrs Joe Komlodi
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Joe Komlodi @ 2024-02-27 22:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: pbonzini, peterx, david, peter.maydell, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield, komlodi

Hi all,

This adds requester IDs to ARM CPUs and adds a "user-defined" memory
attribute.

The requester ID on ARM CPUs is there because I've seen some cases where
there's an IOMMU between a CPU and memory that uses the CPU's requester
ID to look up how it should translate, such as an SMMU TBU or some other
IOMMU-like device.
For a specific downstream example I've seen, Xilinx overrides CPU
attributes with ones passed in by an object property in order to have
their IOMMUs work:
https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/accel/tcg/cputlb.c#L1127.
The object property with the memory attributes is declared here, for
reference: https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/target/arm/cpu.c#L1310.

The user-defined attribute represents optional user signals that are a
part of AMBA-AXI. As the name suggests, these are defined
per-implementation and devices that receive these have their own
interpretation of what the user-defined attribute means.

We add them in CPUs and PCI transactions, because some of their
attributes are set in functions in ways that are not user-facing. DMAs
or other devices that set attributes (using address_space_rw or some
other means), can add them on a per-device basis.

RFC because it's possible we might want this implementated in some other
way, and it touches some pretty frequently used code that I'm somewhat
familiar with, but not 100% familiar with.

Thanks,
Joe

Joe Komlodi (5):
  target/arm: Add requester ID to memattrs
  memattrs: Fix target_tlb_bit whitespace
  memattrs: Add user-defined attribute
  target/arm: Add user-defined memattrs
  hw/pci: Add user-defined memattrs

 hw/pci/pci.c                | 3 +++
 include/exec/memattrs.h     | 8 +++++---
 include/hw/pci/pci_device.h | 1 +
 target/arm/cpu.c            | 6 ++++++
 target/arm/cpu.h            | 8 ++++++++
 target/arm/ptw.c            | 9 +++++++++
 6 files changed, 32 insertions(+), 3 deletions(-)

-- 
2.44.0.rc0.258.g7320e95886-goog



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC PATCH 1/5] target/arm: Add requester ID to memattrs
  2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
@ 2024-02-27 22:24 ` Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 2/5] memattrs: Fix target_tlb_bit whitespace Joe Komlodi
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Joe Komlodi @ 2024-02-27 22:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: pbonzini, peterx, david, peter.maydell, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield, komlodi

I've seen a few different instances where a CPU or a memory region is
behind some sort of IOMMU, and the IOMMU translates (or denies) accesses
based on the requester ID of the CPU.

This patch only does it on ARM CPUs, because I did not see CPU-agnostic
code that added CPU attributes when creating TLBs. Similarly, we add the
requester ID during PTW, while populating the rest of the memory
attributes.

We add the requester ID during GPC and descriptor grabbing as well as
PTWs.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 target/arm/cpu.c | 4 ++++
 target/arm/cpu.h | 6 ++++++
 target/arm/ptw.c | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5fa86bc8d5..9cfbba10c2 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2402,6 +2402,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    /* For MemTxAttrs. */
+    env->requester_id = cpu->requester_id;
+
     qemu_init_vcpu(cs);
     cpu_reset(cs);
 
@@ -2439,6 +2442,7 @@ static Property arm_cpu_properties[] = {
                         mp_affinity, ARM64_AFFINITY_INVALID),
     DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
     DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1),
+    DEFINE_PROP_UINT16("requester-id", ARMCPU, requester_id, 0),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 63f31e0d98..5fc572e077 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -774,6 +774,9 @@ typedef struct CPUArchState {
     /* Linux syscall tagged address support */
     bool tagged_addr_enable;
 #endif
+
+    /* For MemTxAttrs. */
+    uint16_t requester_id;
 } CPUARMState;
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -1091,6 +1094,9 @@ struct ArchCPU {
 
     /* Generic timer counter frequency, in Hz */
     uint64_t gt_cntfrq_hz;
+
+    /* Requester ID, used in MemTxAttrs. */
+    uint16_t requester_id;
 };
 
 typedef struct ARMCPUInfo {
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 5eb3577bcd..148af3a000 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -287,6 +287,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
     MemTxAttrs attrs = {
         .secure = true,
         .space = ARMSS_Root,
+        .requester_id = env->requester_id,
     };
     ARMCPU *cpu = env_archcpu(env);
     uint64_t gpccr = env->cp15.gpccr_el3;
@@ -638,6 +639,7 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, S1Translate *ptw,
         MemTxAttrs attrs = {
             .space = ptw->out_space,
             .secure = arm_space_is_secure(ptw->out_space),
+            .requester_id = env->requester_id,
         };
         AddressSpace *as = arm_addressspace(cs, attrs);
         MemTxResult result = MEMTX_OK;
@@ -684,6 +686,7 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, S1Translate *ptw,
         MemTxAttrs attrs = {
             .space = ptw->out_space,
             .secure = arm_space_is_secure(ptw->out_space),
+            .requester_id = env->requester_id,
         };
         AddressSpace *as = arm_addressspace(cs, attrs);
         MemTxResult result = MEMTX_OK;
@@ -3306,6 +3309,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
     result->f.attrs.space = ptw->in_space;
     result->f.attrs.secure = arm_space_is_secure(ptw->in_space);
 
+    result->f.attrs.requester_id = env->requester_id;
+
     switch (mmu_idx) {
     case ARMMMUIdx_Phys_S:
     case ARMMMUIdx_Phys_NS:
-- 
2.44.0.rc0.258.g7320e95886-goog



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC PATCH 2/5] memattrs: Fix target_tlb_bit whitespace
  2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 1/5] target/arm: Add requester ID to memattrs Joe Komlodi
@ 2024-02-27 22:24 ` Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 3/5] memattrs: Add user-defined attribute Joe Komlodi
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Joe Komlodi @ 2024-02-27 22:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: pbonzini, peterx, david, peter.maydell, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield, komlodi

checkpatch.pl doesn't like these spaces around the colon, so we may as
well fix it up.

No functional change.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 include/exec/memattrs.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d04170aa27..942b721be8 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -61,9 +61,9 @@ typedef struct MemTxAttrs {
      * and has unused bits.  These fields will be read by target-specific
      * helpers using env->iotlb[mmu_idx][tlb_index()].attrs.target_tlb_bitN.
      */
-    unsigned int target_tlb_bit0 : 1;
-    unsigned int target_tlb_bit1 : 1;
-    unsigned int target_tlb_bit2 : 1;
+    unsigned int target_tlb_bit0:1;
+    unsigned int target_tlb_bit1:1;
+    unsigned int target_tlb_bit2:1;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
-- 
2.44.0.rc0.258.g7320e95886-goog



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC PATCH 3/5] memattrs: Add user-defined attribute
  2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 1/5] target/arm: Add requester ID to memattrs Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 2/5] memattrs: Fix target_tlb_bit whitespace Joe Komlodi
@ 2024-02-27 22:24 ` Joe Komlodi
  2024-02-28 11:47   ` Alex Bennée
  2024-02-27 22:24 ` [RFC PATCH 4/5] target/arm: Add user-defined memattrs Joe Komlodi
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Joe Komlodi @ 2024-02-27 22:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: pbonzini, peterx, david, peter.maydell, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield, komlodi

These are used to represent implementation-specific data.
These are based off of AMBA-AXI user signals, but can be used in any
implementation.

The length of 4-bits is arbitrary.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 include/exec/memattrs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 942b721be8..a38645f881 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -64,6 +64,8 @@ typedef struct MemTxAttrs {
     unsigned int target_tlb_bit0:1;
     unsigned int target_tlb_bit1:1;
     unsigned int target_tlb_bit2:1;
+    /* User-defined bits represent data that is implementation defined. */
+    unsigned int user_defined:4;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
-- 
2.44.0.rc0.258.g7320e95886-goog



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC PATCH 4/5] target/arm: Add user-defined memattrs
  2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
                   ` (2 preceding siblings ...)
  2024-02-27 22:24 ` [RFC PATCH 3/5] memattrs: Add user-defined attribute Joe Komlodi
@ 2024-02-27 22:24 ` Joe Komlodi
  2024-02-27 22:24 ` [RFC PATCH 5/5] hw/pci: " Joe Komlodi
  2024-02-28 14:21 ` [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Peter Maydell
  5 siblings, 0 replies; 11+ messages in thread
From: Joe Komlodi @ 2024-02-27 22:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: pbonzini, peterx, david, peter.maydell, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield, komlodi

During transactions, these get added to memory attributes at the same
time other attributes are added.

Similar to the requester ID, these are added on PTWs, GPCs, and
descriptor grabbing as well.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 target/arm/cpu.c | 2 ++
 target/arm/cpu.h | 2 ++
 target/arm/ptw.c | 4 ++++
 3 files changed, 8 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9cfbba10c2..dcd2c16c2e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2404,6 +2404,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
     /* For MemTxAttrs. */
     env->requester_id = cpu->requester_id;
+    env->memattr_user_defined = cpu->memattr_user_defined;
 
     qemu_init_vcpu(cs);
     cpu_reset(cs);
@@ -2443,6 +2444,7 @@ static Property arm_cpu_properties[] = {
     DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
     DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1),
     DEFINE_PROP_UINT16("requester-id", ARMCPU, requester_id, 0),
+    DEFINE_PROP_UINT8("memattr-user-defined", ARMCPU, memattr_user_defined, 0),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5fc572e077..499a5b25c7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -777,6 +777,7 @@ typedef struct CPUArchState {
 
     /* For MemTxAttrs. */
     uint16_t requester_id;
+    uint8_t memattr_user_defined;
 } CPUARMState;
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -1097,6 +1098,7 @@ struct ArchCPU {
 
     /* Requester ID, used in MemTxAttrs. */
     uint16_t requester_id;
+    uint8_t memattr_user_defined;
 };
 
 typedef struct ARMCPUInfo {
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 148af3a000..b2af3d9052 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -288,6 +288,7 @@ static bool granule_protection_check(CPUARMState *env, uint64_t paddress,
         .secure = true,
         .space = ARMSS_Root,
         .requester_id = env->requester_id,
+        .user_defined = env->memattr_user_defined,
     };
     ARMCPU *cpu = env_archcpu(env);
     uint64_t gpccr = env->cp15.gpccr_el3;
@@ -640,6 +641,7 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, S1Translate *ptw,
             .space = ptw->out_space,
             .secure = arm_space_is_secure(ptw->out_space),
             .requester_id = env->requester_id,
+            .user_defined = env->memattr_user_defined,
         };
         AddressSpace *as = arm_addressspace(cs, attrs);
         MemTxResult result = MEMTX_OK;
@@ -687,6 +689,7 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, S1Translate *ptw,
             .space = ptw->out_space,
             .secure = arm_space_is_secure(ptw->out_space),
             .requester_id = env->requester_id,
+            .user_defined = env->memattr_user_defined,
         };
         AddressSpace *as = arm_addressspace(cs, attrs);
         MemTxResult result = MEMTX_OK;
@@ -3310,6 +3313,7 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
     result->f.attrs.secure = arm_space_is_secure(ptw->in_space);
 
     result->f.attrs.requester_id = env->requester_id;
+    result->f.attrs.user_defined = env->memattr_user_defined;
 
     switch (mmu_idx) {
     case ARMMMUIdx_Phys_S:
-- 
2.44.0.rc0.258.g7320e95886-goog



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC PATCH 5/5] hw/pci: Add user-defined memattrs
  2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
                   ` (3 preceding siblings ...)
  2024-02-27 22:24 ` [RFC PATCH 4/5] target/arm: Add user-defined memattrs Joe Komlodi
@ 2024-02-27 22:24 ` Joe Komlodi
  2024-02-28 14:21 ` [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Peter Maydell
  5 siblings, 0 replies; 11+ messages in thread
From: Joe Komlodi @ 2024-02-27 22:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: pbonzini, peterx, david, peter.maydell, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield, komlodi

This adds user-defined bits, which users can set and use on transactions
that involve memory attributes.

We add it in the MSI function, since the attributes are initialized in
that function.
We do not add it in pci_dma_rw because the attributes are passed in.
Some users might pass in MEMTXATTRS_UNSPECIFIED, and we should respect
that instead of injecting user-defined attributes in the function.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 hw/pci/pci.c                | 3 +++
 include/hw/pci/pci_device.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6496d027ca..b0bb682f15 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -85,6 +85,8 @@ static Property pci_props[] = {
                     QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
     DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
                     QEMU_PCIE_ARI_NEXTFN_1_BITNR, false),
+    DEFINE_PROP_UINT8("memattr-user-defined", PCIDevice, memattr_user_defined,
+                      0),
     DEFINE_PROP_END_OF_LIST()
 };
 
@@ -361,6 +363,7 @@ static void pci_msi_trigger(PCIDevice *dev, MSIMessage msg)
         return;
     }
     attrs.requester_id = pci_requester_id(dev);
+    attrs.user_defined = dev->memattr_user_defined;
     address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
                          attrs, NULL);
 }
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index d3dd0f64b2..99be6d72b1 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -84,6 +84,7 @@ struct PCIDevice {
      * conventional PCI root complex, this field is meaningless.
      */
     PCIReqIDCache requester_id_cache;
+    uint8_t memattr_user_defined;
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
     AddressSpace bus_master_as;
-- 
2.44.0.rc0.258.g7320e95886-goog



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 3/5] memattrs: Add user-defined attribute
  2024-02-27 22:24 ` [RFC PATCH 3/5] memattrs: Add user-defined attribute Joe Komlodi
@ 2024-02-28 11:47   ` Alex Bennée
  0 siblings, 0 replies; 11+ messages in thread
From: Alex Bennée @ 2024-02-28 11:47 UTC (permalink / raw)
  To: Joe Komlodi
  Cc: qemu-devel, pbonzini, peterx, david, peter.maydell,
	marcel.apfelbaum, mst, philmd, roqueh, slongfield

Joe Komlodi <komlodi@google.com> writes:

> These are used to represent implementation-specific data.
> These are based off of AMBA-AXI user signals, but can be used in any
> implementation.
>
> The length of 4-bits is arbitrary.
>
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> ---
>  include/exec/memattrs.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 942b721be8..a38645f881 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -64,6 +64,8 @@ typedef struct MemTxAttrs {
>      unsigned int target_tlb_bit0:1;
>      unsigned int target_tlb_bit1:1;
>      unsigned int target_tlb_bit2:1;
> +    /* User-defined bits represent data that is implementation defined. */
> +    unsigned int user_defined:4;
>  } MemTxAttrs;
>  
>  /* Bus masters which don't specify any attributes will get this,

This reminds me of the concept of MACHINE for impdef bits I proposed in:

  Message-Id: <20221111182535.64844-1-alex.bennee@linaro.org>
  Date: Fri, 11 Nov 2022 18:25:15 +0000
  Subject: [PATCH for 8.0 v5 00/20] use MemTxAttrs to avoid current_cpu in hw/
  From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>

which I unfortunately ran out of steam on. Surveying the list I see
there are other patches for MemTxAttrs for IOMMU ids so I wonder if we
are going to run out of bits soon.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
  2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
                   ` (4 preceding siblings ...)
  2024-02-27 22:24 ` [RFC PATCH 5/5] hw/pci: " Joe Komlodi
@ 2024-02-28 14:21 ` Peter Maydell
  2024-02-29  4:52   ` Joe Komlodi
  5 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2024-02-28 14:21 UTC (permalink / raw)
  To: Joe Komlodi
  Cc: qemu-devel, pbonzini, peterx, david, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield

On Tue, 27 Feb 2024 at 22:24, Joe Komlodi <komlodi@google.com> wrote:
> This adds requester IDs to ARM CPUs and adds a "user-defined" memory
> attribute.
>
> The requester ID on ARM CPUs is there because I've seen some cases where
> there's an IOMMU between a CPU and memory that uses the CPU's requester
> ID to look up how it should translate, such as an SMMU TBU or some other
> IOMMU-like device.
> For a specific downstream example I've seen, Xilinx overrides CPU
> attributes with ones passed in by an object property in order to have
> their IOMMUs work:
> https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/accel/tcg/cputlb.c#L1127.
> The object property with the memory attributes is declared here, for
> reference: https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/target/arm/cpu.c#L1310.
>
> The user-defined attribute represents optional user signals that are a
> part of AMBA-AXI. As the name suggests, these are defined
> per-implementation and devices that receive these have their own
> interpretation of what the user-defined attribute means.
>
> We add them in CPUs and PCI transactions, because some of their
> attributes are set in functions in ways that are not user-facing. DMAs
> or other devices that set attributes (using address_space_rw or some
> other means), can add them on a per-device basis.

So as far as I can see, this patchset defines a bunch of mechanism,
but no actual users: no device looks at these new memattrs, no board
code sets the properties. I don't really want to add this without
an upstream usecase for it.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
  2024-02-28 14:21 ` [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Peter Maydell
@ 2024-02-29  4:52   ` Joe Komlodi
  2024-02-29  9:57     ` Peter Maydell
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Komlodi @ 2024-02-29  4:52 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-devel, pbonzini, peterx, david, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield

On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Feb 2024 at 22:24, Joe Komlodi <komlodi@google.com> wrote:
> > This adds requester IDs to ARM CPUs and adds a "user-defined" memory
> > attribute.
> >
> > The requester ID on ARM CPUs is there because I've seen some cases where
> > there's an IOMMU between a CPU and memory that uses the CPU's requester
> > ID to look up how it should translate, such as an SMMU TBU or some other
> > IOMMU-like device.
> > For a specific downstream example I've seen, Xilinx overrides CPU
> > attributes with ones passed in by an object property in order to have
> > their IOMMUs work:
> > https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/accel/tcg/cputlb.c#L1127.
> > The object property with the memory attributes is declared here, for
> > reference: https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/target/arm/cpu.c#L1310.
> >
> > The user-defined attribute represents optional user signals that are a
> > part of AMBA-AXI. As the name suggests, these are defined
> > per-implementation and devices that receive these have their own
> > interpretation of what the user-defined attribute means.
> >
> > We add them in CPUs and PCI transactions, because some of their
> > attributes are set in functions in ways that are not user-facing. DMAs
> > or other devices that set attributes (using address_space_rw or some
> > other means), can add them on a per-device basis.
>
> So as far as I can see, this patchset defines a bunch of mechanism,
> but no actual users: no device looks at these new memattrs, no board
> code sets the properties. I don't really want to add this without
> an upstream usecase for it.

Yeah, I believe the current use-cases for this series are mostly downstream.
It's possible that there's an upstream device that might benefit from
it, but I'm not aware of one.

Is the concern the usefulness of the series, or the worry about it bit-rotting?
If it's the latter, would a qtest be alright to make sure it doesn't rot?

Thanks,
Joe
>
> thanks
> -- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
  2024-02-29  4:52   ` Joe Komlodi
@ 2024-02-29  9:57     ` Peter Maydell
  2024-02-29 17:15       ` Joe Komlodi
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2024-02-29  9:57 UTC (permalink / raw)
  To: Joe Komlodi
  Cc: qemu-devel, pbonzini, peterx, david, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield

On Thu, 29 Feb 2024 at 04:52, Joe Komlodi <komlodi@google.com> wrote:
> On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > So as far as I can see, this patchset defines a bunch of mechanism,
> > but no actual users: no device looks at these new memattrs, no board
> > code sets the properties. I don't really want to add this without
> > an upstream usecase for it.
>
> Yeah, I believe the current use-cases for this series are mostly downstream.
> It's possible that there's an upstream device that might benefit from
> it, but I'm not aware of one.
>
> Is the concern the usefulness of the series, or the worry about it bit-rotting?
> If it's the latter, would a qtest be alright to make sure it doesn't rot?

My main issues are:
 * it's hard to review design without the uses of the code
 * it's extra complexity and maintenance burden that we don't
   need (upstream): accepting the patches gives upstream extra
   work to deal with into the future with no benefit to us
 * dead code is code that typically we would remove
 * we end up with something we can't refactor or clean up
   or change because the only reason we have it is for code
   that we don't have any visibility into: effectively it
   becomes an API for us that we can't change, which is not
   something QEMU does except for specific well defined API
   surfaces (QMP, plugins, etc)

Our usual approach is "submit the patches that add the new core
feature/mechanism along with the patches that add the new
device/board/etc that uses it". Compare the recent patches
also from Google for the ITS and SMMU that try to add hooks
that aren't needed by anything in upstream QEMU:
https://patchew.org/QEMU/20240221171716.1260192-1-nabihestefan@google.com/20240221171716.1260192-3-nabihestefan@google.com/
https://patchew.org/QEMU/20240221173325.1494895-1-nabihestefan@google.com/20240221173325.1494895-3-nabihestefan@google.com/
-- we rejected those for the same reason.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
  2024-02-29  9:57     ` Peter Maydell
@ 2024-02-29 17:15       ` Joe Komlodi
  0 siblings, 0 replies; 11+ messages in thread
From: Joe Komlodi @ 2024-02-29 17:15 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-devel, pbonzini, peterx, david, marcel.apfelbaum, mst,
	philmd, roqueh, slongfield

On Thu, Feb 29, 2024 at 1:57 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 29 Feb 2024 at 04:52, Joe Komlodi <komlodi@google.com> wrote:
> > On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > So as far as I can see, this patchset defines a bunch of mechanism,
> > > but no actual users: no device looks at these new memattrs, no board
> > > code sets the properties. I don't really want to add this without
> > > an upstream usecase for it.
> >
> > Yeah, I believe the current use-cases for this series are mostly downstream.
> > It's possible that there's an upstream device that might benefit from
> > it, but I'm not aware of one.
> >
> > Is the concern the usefulness of the series, or the worry about it bit-rotting?
> > If it's the latter, would a qtest be alright to make sure it doesn't rot?
>
> My main issues are:
>  * it's hard to review design without the uses of the code
>  * it's extra complexity and maintenance burden that we don't
>    need (upstream): accepting the patches gives upstream extra
>    work to deal with into the future with no benefit to us
>  * dead code is code that typically we would remove
>  * we end up with something we can't refactor or clean up
>    or change because the only reason we have it is for code
>    that we don't have any visibility into: effectively it
>    becomes an API for us that we can't change, which is not
>    something QEMU does except for specific well defined API
>    surfaces (QMP, plugins, etc)
>
> Our usual approach is "submit the patches that add the new core
> feature/mechanism along with the patches that add the new
> device/board/etc that uses it". Compare the recent patches
> also from Google for the ITS and SMMU that try to add hooks
> that aren't needed by anything in upstream QEMU:
> https://patchew.org/QEMU/20240221171716.1260192-1-nabihestefan@google.com/20240221171716.1260192-3-nabihestefan@google.com/
> https://patchew.org/QEMU/20240221173325.1494895-1-nabihestefan@google.com/20240221173325.1494895-3-nabihestefan@google.com/
> -- we rejected those for the same reason.

Makes sense!
Thanks for taking the time to look over them.

- Joe
>
> thanks
> -- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-02-29 17:16 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-27 22:24 [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Joe Komlodi
2024-02-27 22:24 ` [RFC PATCH 1/5] target/arm: Add requester ID to memattrs Joe Komlodi
2024-02-27 22:24 ` [RFC PATCH 2/5] memattrs: Fix target_tlb_bit whitespace Joe Komlodi
2024-02-27 22:24 ` [RFC PATCH 3/5] memattrs: Add user-defined attribute Joe Komlodi
2024-02-28 11:47   ` Alex Bennée
2024-02-27 22:24 ` [RFC PATCH 4/5] target/arm: Add user-defined memattrs Joe Komlodi
2024-02-27 22:24 ` [RFC PATCH 5/5] hw/pci: " Joe Komlodi
2024-02-28 14:21 ` [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs Peter Maydell
2024-02-29  4:52   ` Joe Komlodi
2024-02-29  9:57     ` Peter Maydell
2024-02-29 17:15       ` Joe Komlodi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).