On Fri, 11 Nov 2022 at 18:25, Alex Bennée <alex.ben...@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.ben...@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
