Re: [PATCH v5 01/20] hw: encode accessing CPU index in MemTxAttrs

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 11 Nov 2022 at 18:25, 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 so CPU accesses can
> be explicitly marked.
>
> To achieve this we create a new requester_type which indicates to
> consumers how requester_id it to be consumed. We absorb the existing
> unspecified:1 bitfield into this type and also document a potential
> machine specific encoding which will be useful to (currently)
> out-of-tree extensions.
>
> Places that checked to see if things where unspecified now instead
> check the source if what they expected.
>
> 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>

>  include/exec/memattrs.h | 68 ++++++++++++++++++++++++++++++++---------
>  hw/i386/amd_iommu.c     |  6 ++--
>  hw/i386/intel_iommu.c   |  2 +-
>  hw/misc/tz-mpc.c        |  2 +-
>  hw/misc/tz-msc.c        |  6 ++--
>  hw/pci/pci.c            |  4 +--
>  6 files changed, 60 insertions(+), 28 deletions(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..8359fc448b 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -14,7 +14,32 @@
>  #ifndef MEMATTRS_H
>  #define MEMATTRS_H
>
> -/* Every memory transaction has associated with it a set of
> +/**
> + * typedef MemTxRequesterType - source of memory transaction
> + *
> + * Every memory transaction comes from a specific place which defines
> + * how requester_id should be handled if at all.
> + *
> + * UNSPECIFIED: the default for otherwise undefined MemTxAttrs
> + * CPU: requester_id is the global cpu_index
> + *      This needs further processing if you need to work out which
> + *      socket or complex it comes from
> + * PCI: indicates the requester_id is a PCI id
> + * MACHINE: indicates a machine specific encoding
> + *          This will require further processing to decode into its
> + *          constituent parts.
> + */
> +typedef enum MemTxRequesterType {
> +    MTRT_UNSPECIFIED = 0,
> +    MTRT_CPU,
> +    MTRT_PCI,
> +    MTRT_MACHINE
> +} MemTxRequesterType;

This ends up squashing two distinct things into one field:
 (1) what are the semantics of the requester_id field?
 (2) did the caller explicitly specify their attrs, or
     are they using the legacy MEMTXATTRS_UNSPECIFIED macro?

One might reasonably be explicitly specifying tx attrs
and yet not be using any of the 3 listed requester_id field
semantics. (In fact we have various places that do
exactly that, like tulip_desc_read().)

I think that the requester_type field should just be a
discriminator field that tells you how to interpret
requester_id, and nothing more, so the values would
be "CPU", "PCI", "machine" and "none".

I'm not super-enthusiastic about "MACHINE" here, but
I guess we can see how it works out over time.

> +/**
> + * typedef MemTxAttrs - attributes of a memory transaction
> + *
> + * 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
>   * bus (such as the ARM Secure/NonSecure bit). We define them
> @@ -23,13 +48,12 @@
>   * different semantics.
>   */
>  typedef struct MemTxAttrs {
> -    /* Bus masters which don't specify any attributes will get this
> -     * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
> -     * distinguish "all attributes deliberately clear" from
> -     * "didn't specify" if necessary.
> -     */
> -    unsigned int unspecified:1;
> -    /* ARM/AMBA: TrustZone Secure access
> +    /* Requester type (e.g. CPU or PCI MSI) */
> +    MemTxRequesterType requester_type:2;
> +    /* Requester ID */
> +    unsigned int requester_id:16;
> +    /*
> +     * ARM/AMBA: TrustZone Secure access
>       * x86: System Management Mode access
>       */
>      unsigned int secure:1;
> @@ -43,8 +67,6 @@ typedef struct MemTxAttrs {
>       * (see MEMTX_ACCESS_ERROR).
>       */
>      unsigned int memory:1;
> -    /* Requester ID (for MSI for example) */
> -    unsigned int requester_id:16;
>      /* Invert endianness for this page */
>      unsigned int byte_swap:1;
>      /*
> @@ -59,12 +81,28 @@ typedef struct MemTxAttrs {
>      unsigned int target_tlb_bit2 : 1;
>  } MemTxAttrs;
>
> -/* Bus masters which don't specify any attributes will get this,
> - * which has all attribute bits clear except the topmost one
> - * (so that we can distinguish "all attributes deliberately clear"
> - * from "didn't specify" if necessary).
> +/*
> + * Bus masters which don't specify any attributes will get this which
> + * indicates none of the attributes can be used.
> + */
> +#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) \
> +                                { .requester_type = MTRT_UNSPECIFIED })

This isn't the semantics I intended for MEMTXATTRS_UNSPECIFIED --
the recipient of a MEMTXATTRS_UNSPECIFIED is still allowed to look
at the various attribute bits, which should be set to "plausible
defaults". It just means that you can tell, for instance, "non-secure
because sender specifically asked for that" from "non-secure because
sender is an old bit of non-memtxattrs-aware code that doesn't
care to specify". Almost all code that looks at attrs.secure, for
instance, does it without previously checking attrs.unspecified,
because it doesn't have to. There are only a very few places that
do need to check attrs.unspecified:
 * the two places changed in this patch, which are really trying
   to check "is this a PCI transaction with PCI requester_id"
 * tz-mpc.c, which wants to treat MEMTXATTRS_UNSPECIFIED as being
   Secure for the reasons described in the comment in
   tz_mpc_attrs_to_index()

Perhaps what we should do is address the reason that tz-mpc.c
is trying to use attrs.unspecified in a better way. Specifically,
we could add a MemTxAttrs:is_debug bit that indicates debug
writes, and arrange that code like the rom-image-loader sets
that appropriately. Then tz-mpc.c could say "pass through a
debug transaction regardless of its secure bit, but treat
real transactions the way the hardware does", instead of the
current fudge it does.

> +/*
> + * Helper for setting a basic CPU sourced transaction, it expects a
> + * CPUState *
> + */
> +#define MEMTXATTRS_CPU(cs) ((MemTxAttrs) \
> +                            {.requester_type = MTRT_CPU, \
> +                             .requester_id = cs->cpu_index})
> +
> +/*
> + * Helper for setting a basic PCI sourced transaction, it expects a
> + * PCIDevice *
>   */
> -#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
> +#define MEMTXATTRS_PCI(dev) ((MemTxAttrs) \
> +                             {.requester_type = MTRT_PCI,   \
> +                             .requester_id = pci_requester_id(dev)})

Indent looks odd.

>  /* New-style MMIO accessors can indicate that the transaction failed.
>   * A zero (MEMTX_OK) response means success; anything else is a failure
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 725f69095b..284359c16e 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -153,9 +153,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
>  {
>      MSIMessage msg = {};
> -    MemTxAttrs attrs = {
> -        .requester_id = pci_requester_id(&s->pci.dev)
> -    };
> +    MemTxAttrs attrs = MEMTXATTRS_PCI(&s->pci.dev);
>
>      if (msi_enabled(&s->pci.dev)) {
>          msg = msi_get_message(&s->pci.dev, 0);
> @@ -1356,7 +1354,7 @@ static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
>
>      trace_amdvi_mem_ir_write_req(addr, value, size);
>
> -    if (!attrs.unspecified) {
> +    if (attrs.requester_type == MTRT_PCI) {
>          /* We have explicit Source ID */
>          sid = attrs.requester_id;
>      }
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a08ee85edf..12752413eb 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3517,7 +3517,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
>      from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
>      from.data = (uint32_t) value;
>
> -    if (!attrs.unspecified) {
> +    if (attrs.requester_type == MTRT_PCI) {
>          /* We have explicit Source ID */
>          sid = attrs.requester_id;
>      }
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index 30481e1c90..4beb5daa1a 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -461,7 +461,7 @@ static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
>       * All the real during-emulation transactions from the CPU will
>       * specify attributes.
>       */
> -    return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
> +    return ((attrs.requester_type == MTRT_UNSPECIFIED) || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
>  }
>
>  static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
> diff --git a/hw/misc/tz-msc.c b/hw/misc/tz-msc.c
> index acbe94400b..e93bfc7083 100644
> --- a/hw/misc/tz-msc.c
> +++ b/hw/misc/tz-msc.c
> @@ -137,11 +137,9 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
>          return MEMTX_OK;
>      case MSCAllowSecure:
>          attrs.secure = 1;
> -        attrs.unspecified = 0;
>          break;
>      case MSCAllowNonSecure:
>          attrs.secure = 0;
> -        attrs.unspecified = 0;
>          break;
>      }
>
> @@ -179,11 +177,11 @@ static MemTxResult tz_msc_write(void *opaque, hwaddr addr, uint64_t val,
>          return MEMTX_OK;
>      case MSCAllowSecure:
>          attrs.secure = 1;
> -        attrs.unspecified = 0;
> +        attrs.requester_type = MTRT_CPU;
>          break;
>      case MSCAllowNonSecure:
>          attrs.secure = 0;
> -        attrs.unspecified = 0;
> +        attrs.requester_type = MTRT_CPU;
>          break;
>      }

Whatever we do here, the handling of read and write transactions
should be the same, not different.

Forcing the requester_type to "this is the CPU" looks odd,
because the transaction might not be from the CPU (it could
be from a DMA-capable device).

I think probably the right thing here is "set attrs.secure as
required; don't touch any other fields of the attrs".

> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2f450f6a72..1d0d8d866f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -319,9 +319,7 @@ void pci_device_deassert_intx(PCIDevice *dev)
>
>  static void pci_msi_trigger(PCIDevice *dev, MSIMessage msg)
>  {
> -    MemTxAttrs attrs = {};
> -
> -    attrs.requester_id = pci_requester_id(dev);
> +    MemTxAttrs attrs = MEMTXATTRS_PCI(dev);
>      address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
>                           attrs, NULL);
>  }
> --
> 2.34.1

thanks
-- PMM