diff mbox series

[v2,01/11] hw: encode accessing CPU index in MemTxAttrs

Message ID 20220926133904.3297263-2-alex.bennee@linaro.org
State New
Headers show
Series gdbstub/next (MemTxAttrs and re-factoring) | expand

Commit Message

Alex Bennée Sept. 26, 2022, 1:38 p.m. UTC
We currently have hacks across the hw/ to reference current_cpu to
work out what the current accessing CPU is. This breaks in some cases
including using gdbstub to access HW state. As we have MemTxAttrs to
describe details about the access lets extend it to mention if this is
a CPU access and which one it is.

There are a number of places we need to fix up including:

  CPU helpers directly calling address_space_*() fns
  models in hw/ fishing the data out of current_cpu
  hypervisors offloading device emulation to QEMU

I'll start addressing some of these in following patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use separate field cpu_index
  - bool for requester_is_cpu
v3
  - switch to enum MemTxRequesterType
  - move helper #define to patch
  - revert to overloading requester_id
  - mention hypervisors in commit message
  - drop cputlb tweaks, they will move to target specific code
---
 include/exec/memattrs.h | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Peter Maydell Sept. 26, 2022, 2:34 p.m. UTC | #1
On Mon, 26 Sept 2022 at 15:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We currently have hacks across the hw/ to reference current_cpu to
> work out what the current accessing CPU is. This breaks in some cases
> including using gdbstub to access HW state. As we have MemTxAttrs to
> describe details about the access lets extend it to mention if this is
> a CPU access and which one it is.
>
> There are a number of places we need to fix up including:
>
>   CPU helpers directly calling address_space_*() fns
>   models in hw/ fishing the data out of current_cpu
>   hypervisors offloading device emulation to QEMU
>
> I'll start addressing some of these in following patches.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - use separate field cpu_index
>   - bool for requester_is_cpu
> v3
>   - switch to enum MemTxRequesterType
>   - move helper #define to patch
>   - revert to overloading requester_id
>   - mention hypervisors in commit message
>   - drop cputlb tweaks, they will move to target specific code

I still don't see anything in this patchset that updates
the code which currently assumes requester_id to be a PCI
index to check that it hasn't been handed a MemTxAttrs
that uses requester_id as a CPU number.

I also still need to go and look up how hardware does this,
so please don't queue this patchset yet. In particular, we
should think about whether we want this to be:
 * a CPU number, but only set opt-in by some target archs
 * a CPU number, valid for all target archs
 * a unique-within-the-machine identifier of the transaction
   master (i.e. which can be set by DMA controllers, etc,
   not just CPUs)

I would also like some input from Edgar since I know Xilinx
have some more extensive out-of-tree uses of requester_id.
We aren't obligated to not break out-of-tree code, but that
seems like a bunch of experience and knowledge about how
real hardware works that would be useful for informing
how we design this.

thanks
-- PMM
Alex Bennée Sept. 26, 2022, 3:09 p.m. UTC | #2
Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 26 Sept 2022 at 15:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> We currently have hacks across the hw/ to reference current_cpu to
>> work out what the current accessing CPU is. This breaks in some cases
>> including using gdbstub to access HW state. As we have MemTxAttrs to
>> describe details about the access lets extend it to mention if this is
>> a CPU access and which one it is.
>>
>> There are a number of places we need to fix up including:
>>
>>   CPU helpers directly calling address_space_*() fns
>>   models in hw/ fishing the data out of current_cpu
>>   hypervisors offloading device emulation to QEMU
>>
>> I'll start addressing some of these in following patches.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - use separate field cpu_index
>>   - bool for requester_is_cpu
>> v3
>>   - switch to enum MemTxRequesterType
>>   - move helper #define to patch
>>   - revert to overloading requester_id
>>   - mention hypervisors in commit message
>>   - drop cputlb tweaks, they will move to target specific code
>
> I still don't see anything in this patchset that updates
> the code which currently assumes requester_id to be a PCI
> index to check that it hasn't been handed a MemTxAttrs
> that uses requester_id as a CPU number.

OK I'll update so all the existing cases setting requester_id also set
the type to MEMTXATTRS_MSI.

I also noticed the GIC ITS code checks requester ID. Should we assert
(or hw_error?) if it's not the case?

> I also still need to go and look up how hardware does this,
> so please don't queue this patchset yet. In particular, we
> should think about whether we want this to be:
>  * a CPU number, but only set opt-in by some target archs

Given a whole bunch of arches currently use MEMTXATTRS_UNSPECIFIED I
think for now it's worth confining to just ARM where we know we have
devices that care about the cpu_index and have tagged the various paths
with the correct data.

>  * a CPU number, valid for all target archs
>  * a unique-within-the-machine identifier of the transaction
>    master (i.e. which can be set by DMA controllers, etc,
>    not just CPUs)

That would require something to keep a map of requester_id's to
source/index right?

> I would also like some input from Edgar since I know Xilinx
> have some more extensive out-of-tree uses of requester_id.
> We aren't obligated to not break out-of-tree code, but that
> seems like a bunch of experience and knowledge about how
> real hardware works that would be useful for informing
> how we design this.

His comment against the last iteration was:

"CPU's can also have a Master-IDs (Requester IDs) which are unrelated to
the Clusters CPU index. This is used for example in the Xilinx ZynqMP
and Xilinx Versal and the XMPU (Memory Protection Units).

Anyway, I think this approach is an improvement from the current state
but would rather see a new separate field from requester_id. Overloading
requester_id will break some of our use-cases (in the Xilinx tree)...

IIRC a real GIC differentiates between the connected CPU's through
different ports, not by looking at master-ids but I'm not 100% sure..."

at the same time Richard's not keen about adding extra fields
(especially as some arches have INT32_MAX bounds for cpu_index). However
one approach would be to expand the requester_id field and you could
then expand MemTxRequesterType to and have a multiplexed type although I
admit it's hard to imagine HW that cares about both the CPU and bus id
at the same time.

>
> thanks
> -- PMM
Peter Maydell Sept. 26, 2022, 3:30 p.m. UTC | #3
On Mon, 26 Sept 2022 at 16:24, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > I still don't see anything in this patchset that updates
> > the code which currently assumes requester_id to be a PCI
> > index to check that it hasn't been handed a MemTxAttrs
> > that uses requester_id as a CPU number.
>
> OK I'll update so all the existing cases setting requester_id also set
> the type to MEMTXATTRS_MSI.

The problem is not the places that set requester_id (you can
arrange for the default to be MSI for back-compat if you don't
want to explicitly set it), but the places that *read* it.

-- PMM
Alexander Graf Sept. 26, 2022, 8:15 p.m. UTC | #4
On 26.09.22 15:38, Alex Bennée wrote:
> We currently have hacks across the hw/ to reference current_cpu to
> work out what the current accessing CPU is. This breaks in some cases
> including using gdbstub to access HW state. As we have MemTxAttrs to
> describe details about the access lets extend it to mention if this is
> a CPU access and which one it is.
>
> There are a number of places we need to fix up including:
>
>    CPU helpers directly calling address_space_*() fns
>    models in hw/ fishing the data out of current_cpu
>    hypervisors offloading device emulation to QEMU
>
> I'll start addressing some of these in following patches.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>    - use separate field cpu_index
>    - bool for requester_is_cpu
> v3
>    - switch to enum MemTxRequesterType
>    - move helper #define to patch
>    - revert to overloading requester_id
>    - mention hypervisors in commit message
>    - drop cputlb tweaks, they will move to target specific code
> ---
>   include/exec/memattrs.h | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..0fb5f29d25 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -14,6 +14,15 @@
>   #ifndef MEMATTRS_H
>   #define MEMATTRS_H
>   
> +/*
> + * Where the memory transaction comes from
> + */
> +typedef enum MemTxRequesterType {
> +    MEMTXATTRS_CPU,
> +    MEMTXATTRS_MSI,


I don't think MSI is any sensible identifier here. What you actually are 
trying to describe are bus messages that may carry additional source 
information - and what you describe as "MSI" here most likely means "no 
further information".

How about you just call it "BUS", "DMA" or literally "UNSPECIFIED" for 
now? Also, why is unspecified not part of the requester type enum?


Alex

> +} MemTxRequesterType;
> +
> +
>   /* Every memory transaction has associated with it a set of
>    * attributes. Some of these are generic (such as the ID of
>    * the bus master); some are specific to a particular kind of
> @@ -43,7 +52,9 @@ typedef struct MemTxAttrs {
>        * (see MEMTX_ACCESS_ERROR).
>        */
>       unsigned int memory:1;
> -    /* Requester ID (for MSI for example) */
> +    /* Requester type (e.g. CPU or PCI MSI) */
> +    MemTxRequesterType requester_type:2;
> +    /* Requester ID */
>       unsigned int requester_id:16;
>       /* Invert endianness for this page */
>       unsigned int byte_swap:1;
> @@ -66,6 +77,10 @@ typedef struct MemTxAttrs {
>    */
>   #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
>   
> +/* Helper for setting a basic CPU sourced transaction */
> +#define MEMTXATTRS_CPU(id) ((MemTxAttrs) \
> +                            {.requester_type = MEMTXATTRS_CPU, .requester_id = id})
> +
>   /* New-style MMIO accessors can indicate that the transaction failed.
>    * A zero (MEMTX_OK) response means success; anything else is a failure
>    * of some kind. The memory subsystem will bitwise-OR together results
diff mbox series

Patch

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..0fb5f29d25 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -14,6 +14,15 @@ 
 #ifndef MEMATTRS_H
 #define MEMATTRS_H
 
+/*
+ * Where the memory transaction comes from
+ */
+typedef enum MemTxRequesterType {
+    MEMTXATTRS_CPU,
+    MEMTXATTRS_MSI,
+} MemTxRequesterType;
+
+
 /* Every memory transaction has associated with it a set of
  * attributes. Some of these are generic (such as the ID of
  * the bus master); some are specific to a particular kind of
@@ -43,7 +52,9 @@  typedef struct MemTxAttrs {
      * (see MEMTX_ACCESS_ERROR).
      */
     unsigned int memory:1;
-    /* Requester ID (for MSI for example) */
+    /* Requester type (e.g. CPU or PCI MSI) */
+    MemTxRequesterType requester_type:2;
+    /* Requester ID */
     unsigned int requester_id:16;
     /* Invert endianness for this page */
     unsigned int byte_swap:1;
@@ -66,6 +77,10 @@  typedef struct MemTxAttrs {
  */
 #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
 
+/* Helper for setting a basic CPU sourced transaction */
+#define MEMTXATTRS_CPU(id) ((MemTxAttrs) \
+                            {.requester_type = MEMTXATTRS_CPU, .requester_id = id})
+
 /* New-style MMIO accessors can indicate that the transaction failed.
  * A zero (MEMTX_OK) response means success; anything else is a failure
  * of some kind. The memory subsystem will bitwise-OR together results