| Message ID | 20221111182535.64844-2-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | use MemTxAttrs to avoid current_cpu in hw/ | expand |
On 11/12/22 04:25, Alex Bennée wrote: > +/* > + * 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 }) > + > +/* > + * 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_PCI(dev) ((MemTxAttrs) \ > + {.requester_type = MTRT_PCI, \ > + .requester_id = pci_requester_id(dev)}) Any reason these second two shouldn't be inlines? Anything with arguments gets better type checking that way, unless you need preprocessor magic, which is not the case here. Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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
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; + +/** + * 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 }) + +/* + * 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)}) /* 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; } 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); }
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> --- 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 v4 - merge unspecified:1 into MTRT_UNSPECIFIED - document a MTRT_MACHINE for more complex encoding - ensure existing users of requester_id set MTRT_PCI - ensure existing consumers of requester_id check type is MTRT_PCI - have MEMTXATTRS_CPU take CPUState * directly v5 - re-order so MTRT_UNSPECIFIED is zero - fix up comments referring to the difference between empty and unspecified:1 - kernel-doc annotations for typedefs - don't impose source type tz-msc during transformation - re-order bitfields so requester_type/id at top - add helper for MEMTXATTRS_PCI --- 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(-)