| Message ID | 1426526422-28338-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote: > Define an API so that devices can register MemoryRegionOps whose read > and write callback functions are passed an arbitrary pointer to some > transaction attributes and can return a success-or-failure status code. > This will allow us to model devices which: > * behave differently for ARM Secure/NonSecure memory accesses > * behave differently for privileged/unprivileged accesses > * may return a transaction failure (causing a guest exception) > for erroneous accesses > The success/failure response indication is currently ignored; it is > provided so we can implement it later without having to change the > callback function API yet again in future. > +/* New-style MMIO accessors can indicate that the transaction failed. > + * This is currently ignored, but provided in the API to allow us to add > + * support later without changing the MemoryRegionOps functions yet again. > + */ > +typedef enum { > + MemTx_OK = 0, > + MemTx_DecodeError = 1, /* "nothing at that address" */ > + MemTx_SlaveError = 2, /* "device unhappy with request" (eg misalignment) */ > +} MemTxResult; So I was looking at how this would actually get plumbed through the memory subsystem code, and there are some awkwardnesses with this simple enum approach. In particular, functions like address_space_rw want to combine the error returns from several io_mem_read/write calls into a single response to return to the caller. With an enum we'd need some pretty ugly code to prioritise particular failure types, or to do something arbitrary like "return first failure code". Alternatively we could: (a) make MemTxResult a uint32_t, where all-bits zero indicates "OK" and any bit set indicates some kind of error, eg bit 0 set for "device returned an error", and bit 1 for "decode error", and higher bits available for other kinds of extra info about errors in future. Then address_space_rw just ORs together all the bits in all the return codes it receives. (b) give up and say "just use a bool" Opinions? -- PMM
On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote: > On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote: > > Define an API so that devices can register MemoryRegionOps whose read > > and write callback functions are passed an arbitrary pointer to some > > transaction attributes and can return a success-or-failure status code. > > This will allow us to model devices which: > > * behave differently for ARM Secure/NonSecure memory accesses > > * behave differently for privileged/unprivileged accesses > > * may return a transaction failure (causing a guest exception) > > for erroneous accesses > > > The success/failure response indication is currently ignored; it is > > provided so we can implement it later without having to change the > > callback function API yet again in future. > > > +/* New-style MMIO accessors can indicate that the transaction failed. > > + * This is currently ignored, but provided in the API to allow us to add > > + * support later without changing the MemoryRegionOps functions yet again. > > + */ > > +typedef enum { > > + MemTx_OK = 0, > > + MemTx_DecodeError = 1, /* "nothing at that address" */ > > + MemTx_SlaveError = 2, /* "device unhappy with request" (eg misalignment) */ > > +} MemTxResult; > > So I was looking at how this would actually get plumbed through > the memory subsystem code, and there are some awkwardnesses > with this simple enum approach. In particular, functions like > address_space_rw want to combine the error returns from > several io_mem_read/write calls into a single response to > return to the caller. With an enum we'd need some pretty > ugly code to prioritise particular failure types, or to > do something arbitrary like "return first failure code". > Alternatively we could: > (a) make MemTxResult a uint32_t, where all-bits zero indicates > "OK" and any bit set indicates some kind of error, eg > bit 0 set for "device returned an error", and bit 1 for > "decode error", and higher bits available for other kinds > of extra info about errors in future. Then address_space_rw > just ORs together all the bits in all the return codes it > receives. > (b) give up and say "just use a bool" > > Opinions? Hi Peter, Is this related to masters relying on the memory frameworks magic handling of unaliged accesses? I guess that masters that really care about accurate the error handling would need to issue transactions without relying on the intermediate "magic" that splits unaligned accesses... Anyway, I think your option a sounds the most flexible... Cheers, Edgar
On 27/03/2015 13:02, Edgar E. Iglesias wrote: > On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote: >> On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Define an API so that devices can register MemoryRegionOps whose read >>> and write callback functions are passed an arbitrary pointer to some >>> transaction attributes and can return a success-or-failure status code. >>> This will allow us to model devices which: >>> * behave differently for ARM Secure/NonSecure memory accesses >>> * behave differently for privileged/unprivileged accesses >>> * may return a transaction failure (causing a guest exception) >>> for erroneous accesses >> >>> The success/failure response indication is currently ignored; it is >>> provided so we can implement it later without having to change the >>> callback function API yet again in future. >> >>> +/* New-style MMIO accessors can indicate that the transaction failed. >>> + * This is currently ignored, but provided in the API to allow us to add >>> + * support later without changing the MemoryRegionOps functions yet again. >>> + */ >>> +typedef enum { >>> + MemTx_OK = 0, >>> + MemTx_DecodeError = 1, /* "nothing at that address" */ >>> + MemTx_SlaveError = 2, /* "device unhappy with request" (eg misalignment) */ >>> +} MemTxResult; >> >> So I was looking at how this would actually get plumbed through >> the memory subsystem code, and there are some awkwardnesses >> with this simple enum approach. In particular, functions like >> address_space_rw want to combine the error returns from >> several io_mem_read/write calls into a single response to >> return to the caller. With an enum we'd need some pretty >> ugly code to prioritise particular failure types, or to >> do something arbitrary like "return first failure code". >> Alternatively we could: >> (a) make MemTxResult a uint32_t, where all-bits zero indicates >> "OK" and any bit set indicates some kind of error, eg >> bit 0 set for "device returned an error", and bit 1 for >> "decode error", and higher bits available for other kinds >> of extra info about errors in future. Then address_space_rw >> just ORs together all the bits in all the return codes it >> receives. >> (b) give up and say "just use a bool" >> >> Opinions? > > Hi Peter, > > Is this related to masters relying on the memory frameworks magic > handling of unaliged accesses? Not necessarily, you can get the same just by doing a large write that spans multiple MemoryRegions. See the loop in address_space_rw. > Anyway, I think your option a sounds the most flexible... ACK Paolo > Cheers, > Edgar >
On 27 March 2015 at 12:02, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote: >> So I was looking at how this would actually get plumbed through >> the memory subsystem code, and there are some awkwardnesses >> with this simple enum approach. In particular, functions like >> address_space_rw want to combine the error returns from >> several io_mem_read/write calls into a single response to >> return to the caller. With an enum we'd need some pretty >> ugly code to prioritise particular failure types, or to >> do something arbitrary like "return first failure code". >> Alternatively we could: >> (a) make MemTxResult a uint32_t, where all-bits zero indicates >> "OK" and any bit set indicates some kind of error, eg >> bit 0 set for "device returned an error", and bit 1 for >> "decode error", and higher bits available for other kinds >> of extra info about errors in future. Then address_space_rw >> just ORs together all the bits in all the return codes it >> receives. >> (b) give up and say "just use a bool" > Is this related to masters relying on the memory frameworks magic > handling of unaliged accesses? Well, that, and masters that just want to say "write this entire buffer" or otherwise access at larger than the destination's access size. > I guess that masters that really care about accurate the error > handling would need to issue transactions without relying on > the intermediate "magic" that splits unaligned accesses... Yes, I think this is probably true. (I suspect we don't actually care at that level of detail.) > Anyway, I think your option a sounds the most flexible... Yes, it's the best thing I can think of currently. -- PMM
On Fri, Mar 27, 2015 at 01:10:07PM +0100, Paolo Bonzini wrote: > > > On 27/03/2015 13:02, Edgar E. Iglesias wrote: > > On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote: > >> On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote: > >>> Define an API so that devices can register MemoryRegionOps whose read > >>> and write callback functions are passed an arbitrary pointer to some > >>> transaction attributes and can return a success-or-failure status code. > >>> This will allow us to model devices which: > >>> * behave differently for ARM Secure/NonSecure memory accesses > >>> * behave differently for privileged/unprivileged accesses > >>> * may return a transaction failure (causing a guest exception) > >>> for erroneous accesses > >> > >>> The success/failure response indication is currently ignored; it is > >>> provided so we can implement it later without having to change the > >>> callback function API yet again in future. > >> > >>> +/* New-style MMIO accessors can indicate that the transaction failed. > >>> + * This is currently ignored, but provided in the API to allow us to add > >>> + * support later without changing the MemoryRegionOps functions yet again. > >>> + */ > >>> +typedef enum { > >>> + MemTx_OK = 0, > >>> + MemTx_DecodeError = 1, /* "nothing at that address" */ > >>> + MemTx_SlaveError = 2, /* "device unhappy with request" (eg misalignment) */ > >>> +} MemTxResult; > >> > >> So I was looking at how this would actually get plumbed through > >> the memory subsystem code, and there are some awkwardnesses > >> with this simple enum approach. In particular, functions like > >> address_space_rw want to combine the error returns from > >> several io_mem_read/write calls into a single response to > >> return to the caller. With an enum we'd need some pretty > >> ugly code to prioritise particular failure types, or to > >> do something arbitrary like "return first failure code". > >> Alternatively we could: > >> (a) make MemTxResult a uint32_t, where all-bits zero indicates > >> "OK" and any bit set indicates some kind of error, eg > >> bit 0 set for "device returned an error", and bit 1 for > >> "decode error", and higher bits available for other kinds > >> of extra info about errors in future. Then address_space_rw > >> just ORs together all the bits in all the return codes it > >> receives. > >> (b) give up and say "just use a bool" > >> > >> Opinions? > > > > Hi Peter, > > > > Is this related to masters relying on the memory frameworks magic > > handling of unaliged accesses? > > Not necessarily, you can get the same just by doing a large write that > spans multiple MemoryRegions. See the loop in address_space_rw. Right, this is another case of "magic" memory handling that allows masters to issue unnatural transactions and rely on the memory framework to split things up. In these cases aren't the masters trading accuracy (including error handling accuracy) for performance or model simplicity? It could maybe be useful to have a flag so masters can say one of the following (could be encoded in the memattrs): 1. Stop at first error and return. 2. Keep going after errors and give me the OR result of all errors. For 1 to be useful, I think it would have to be combined with some kind of return info that can point out where in the magic splitting of large or unaligned transactions that things went wrong. Probably overkill at the moment... Cheers, Edgar
On 27/03/2015 13:32, Edgar E. Iglesias wrote: >>> Is this related to masters relying on the memory frameworks magic >>> handling of unaliged accesses? >> >> Not necessarily, you can get the same just by doing a large write that >> spans multiple MemoryRegions. See the loop in address_space_rw. > > Right, this is another case of "magic" memory handling that allows masters > to issue unnatural transactions and rely on the memory framework to > split things up. > In these cases aren't the masters trading accuracy (including error > handling accuracy) for performance or model simplicity? Yes. There are no "natural" transactions beyond 32 or 64-bit accesses. > It could maybe be useful to have a flag so masters can say one of the > following (could be encoded in the memattrs): > 1. Stop at first error and return. > 2. Keep going after errors and give me the OR result of all errors. It could just be a length pointer in the same vein as address_space_map's. If NULL, keep going. If not NULL, stop and return. Paolo > > For 1 to be useful, I think it would have to be combined with some > kind of return info that can point out where in the magic splitting > of large or unaligned transactions that things went wrong. > > Probably overkill at the moment... > > Cheers, > Edgar >
On Fri, Mar 27, 2015 at 02:16:53PM +0100, Paolo Bonzini wrote: > > > On 27/03/2015 13:32, Edgar E. Iglesias wrote: > >>> Is this related to masters relying on the memory frameworks magic > >>> handling of unaliged accesses? > >> > >> Not necessarily, you can get the same just by doing a large write that > >> spans multiple MemoryRegions. See the loop in address_space_rw. > > > > Right, this is another case of "magic" memory handling that allows masters > > to issue unnatural transactions and rely on the memory framework to > > split things up. > > In these cases aren't the masters trading accuracy (including error > > handling accuracy) for performance or model simplicity? > > Yes. There are no "natural" transactions beyond 32 or 64-bit accesses. > > > It could maybe be useful to have a flag so masters can say one of the > > following (could be encoded in the memattrs): > > 1. Stop at first error and return. > > 2. Keep going after errors and give me the OR result of all errors. > > It could just be a length pointer in the same vein as > address_space_map's. If NULL, keep going. If not NULL, stop and return. > Good point, thanks. Cheers, Edgar
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h new file mode 100644 index 0000000..b8d7808 --- /dev/null +++ b/include/exec/memattrs.h @@ -0,0 +1,34 @@ +/* + * Memory transaction attributes + * + * Copyright (c) 2015 Linaro Limited. + * + * Authors: + * Peter Maydell <peter.maydell@linaro.org> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#ifndef MEMATTRS_H +#define MEMATTRS_H + +/* 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 + * all as non-overlapping bits in a single integer to avoid + * confusion if different parts of QEMU used the same bit for + * different semantics. + */ +typedef uint64_t 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). + */ +#define MEMTXATTRS_UNSPECIFIED (1ULL << 63) + +#endif diff --git a/include/exec/memory.h b/include/exec/memory.h index 06ffa1d..642e59f 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -28,6 +28,7 @@ #ifndef CONFIG_USER_ONLY #include "exec/hwaddr.h" #endif +#include "exec/memattrs.h" #include "qemu/queue.h" #include "qemu/int128.h" #include "qemu/notify.h" @@ -68,6 +69,16 @@ struct IOMMUTLBEntry { IOMMUAccessFlags perm; }; +/* New-style MMIO accessors can indicate that the transaction failed. + * This is currently ignored, but provided in the API to allow us to add + * support later without changing the MemoryRegionOps functions yet again. + */ +typedef enum { + MemTx_OK = 0, + MemTx_DecodeError = 1, /* "nothing at that address" */ + MemTx_SlaveError = 2, /* "device unhappy with request" (eg misalignment) */ +} MemTxResult; + /* * Memory region callbacks */ @@ -84,6 +95,17 @@ struct MemoryRegionOps { uint64_t data, unsigned size); + MemTxResult (*read_with_attrs)(void *opaque, + hwaddr addr, + uint64_t *data, + unsigned size, + MemTxAttrs attrs); + MemTxResult (*write_with_attrs)(void *opaque, + hwaddr addr, + uint64_t data, + unsigned size, + MemTxAttrs attrs); + enum device_endian endianness; /* Guest-visible constraints: */ struct { diff --git a/memory.c b/memory.c index 20f6d9e..76f95b7 100644 --- a/memory.c +++ b/memory.c @@ -373,7 +373,8 @@ static void memory_region_oldmmio_read_accessor(MemoryRegion *mr, uint64_t *value, unsigned size, unsigned shift, - uint64_t mask) + uint64_t mask, + MemTxAttrs attrs) { uint64_t tmp; @@ -387,7 +388,8 @@ static void memory_region_read_accessor(MemoryRegion *mr, uint64_t *value, unsigned size, unsigned shift, - uint64_t mask) + uint64_t mask, + MemTxAttrs attrs) { uint64_t tmp; @@ -399,12 +401,31 @@ static void memory_region_read_accessor(MemoryRegion *mr, *value |= (tmp & mask) << shift; } +static void memory_region_read_with_attrs_accessor(MemoryRegion *mr, + hwaddr addr, + uint64_t *value, + unsigned size, + unsigned shift, + uint64_t mask, + MemTxAttrs attrs) +{ + uint64_t tmp = 0; + + if (mr->flush_coalesced_mmio) { + qemu_flush_coalesced_mmio_buffer(); + } + mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs); + trace_memory_region_ops_read(mr, addr, tmp, size); + *value |= (tmp & mask) << shift; +} + static void memory_region_oldmmio_write_accessor(MemoryRegion *mr, hwaddr addr, uint64_t *value, unsigned size, unsigned shift, - uint64_t mask) + uint64_t mask, + MemTxAttrs attrs) { uint64_t tmp; @@ -418,7 +439,8 @@ static void memory_region_write_accessor(MemoryRegion *mr, uint64_t *value, unsigned size, unsigned shift, - uint64_t mask) + uint64_t mask, + MemTxAttrs attrs) { uint64_t tmp; @@ -430,6 +452,24 @@ static void memory_region_write_accessor(MemoryRegion *mr, mr->ops->write(mr->opaque, addr, tmp, size); } +static void memory_region_write_with_attrs_accessor(MemoryRegion *mr, + hwaddr addr, + uint64_t *value, + unsigned size, + unsigned shift, + uint64_t mask, + MemTxAttrs attrs) +{ + uint64_t tmp; + + if (mr->flush_coalesced_mmio) { + qemu_flush_coalesced_mmio_buffer(); + } + tmp = (*value >> shift) & mask; + trace_memory_region_ops_write(mr, addr, tmp, size); + mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs); +} + static void access_with_adjusted_size(hwaddr addr, uint64_t *value, unsigned size, @@ -440,8 +480,10 @@ static void access_with_adjusted_size(hwaddr addr, uint64_t *value, unsigned size, unsigned shift, - uint64_t mask), - MemoryRegion *mr) + uint64_t mask, + MemTxAttrs attrs), + MemoryRegion *mr, + MemTxAttrs attrs) { uint64_t access_mask; unsigned access_size; @@ -460,11 +502,11 @@ static void access_with_adjusted_size(hwaddr addr, if (memory_region_big_endian(mr)) { for (i = 0; i < size; i += access_size) { access(mr, addr + i, value, access_size, - (size - access_size - i) * 8, access_mask); + (size - access_size - i) * 8, access_mask, attrs); } } else { for (i = 0; i < size; i += access_size) { - access(mr, addr + i, value, access_size, i * 8, access_mask); + access(mr, addr + i, value, access_size, i * 8, access_mask, attrs); } } } @@ -1055,7 +1097,8 @@ bool memory_region_access_valid(MemoryRegion *mr, static uint64_t memory_region_dispatch_read1(MemoryRegion *mr, hwaddr addr, - unsigned size) + unsigned size, + MemTxAttrs attrs) { uint64_t data = 0; @@ -1063,10 +1106,18 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr, access_with_adjusted_size(addr, &data, size, mr->ops->impl.min_access_size, mr->ops->impl.max_access_size, - memory_region_read_accessor, mr); + memory_region_read_accessor, mr, + attrs); + } else if (mr->ops->read_with_attrs) { + access_with_adjusted_size(addr, &data, size, + mr->ops->impl.min_access_size, + mr->ops->impl.max_access_size, + memory_region_read_with_attrs_accessor, mr, + attrs); } else { access_with_adjusted_size(addr, &data, size, 1, 4, - memory_region_oldmmio_read_accessor, mr); + memory_region_oldmmio_read_accessor, mr, + attrs); } return data; @@ -1075,14 +1126,15 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr, static bool memory_region_dispatch_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, - unsigned size) + unsigned size, + MemTxAttrs attrs) { if (!memory_region_access_valid(mr, addr, size, false)) { *pval = unassigned_mem_read(mr, addr, size); return true; } - *pval = memory_region_dispatch_read1(mr, addr, size); + *pval = memory_region_dispatch_read1(mr, addr, size, attrs); adjust_endianness(mr, pval, size); return false; } @@ -1090,7 +1142,8 @@ static bool memory_region_dispatch_read(MemoryRegion *mr, static bool memory_region_dispatch_write(MemoryRegion *mr, hwaddr addr, uint64_t data, - unsigned size) + unsigned size, + MemTxAttrs attrs) { if (!memory_region_access_valid(mr, addr, size, true)) { unassigned_mem_write(mr, addr, data, size); @@ -1103,10 +1156,18 @@ static bool memory_region_dispatch_write(MemoryRegion *mr, access_with_adjusted_size(addr, &data, size, mr->ops->impl.min_access_size, mr->ops->impl.max_access_size, - memory_region_write_accessor, mr); + memory_region_write_accessor, mr, + attrs); + } else if (mr->ops->write_with_attrs) { + access_with_adjusted_size(addr, &data, size, + mr->ops->impl.min_access_size, + mr->ops->impl.max_access_size, + memory_region_write_with_attrs_accessor, mr, + attrs); } else { access_with_adjusted_size(addr, &data, size, 1, 4, - memory_region_oldmmio_write_accessor, mr); + memory_region_oldmmio_write_accessor, mr, + attrs); } return false; } @@ -1994,13 +2055,15 @@ void address_space_destroy(AddressSpace *as) bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned size) { - return memory_region_dispatch_read(mr, addr, pval, size); + return memory_region_dispatch_read(mr, addr, pval, size, + MEMTXATTRS_UNSPECIFIED); } bool io_mem_write(MemoryRegion *mr, hwaddr addr, uint64_t val, unsigned size) { - return memory_region_dispatch_write(mr, addr, val, size); + return memory_region_dispatch_write(mr, addr, val, size, + MEMTXATTRS_UNSPECIFIED); } typedef struct MemoryRegionList MemoryRegionList;
Define an API so that devices can register MemoryRegionOps whose read and write callback functions are passed an arbitrary pointer to some transaction attributes and can return a success-or-failure status code. This will allow us to model devices which: * behave differently for ARM Secure/NonSecure memory accesses * behave differently for privileged/unprivileged accesses * may return a transaction failure (causing a guest exception) for erroneous accesses This patch defines the new API and plumbs the attributes parameter through to the memory.c public level functions io_mem_read() and io_mem_write(), where it is currently dummied out. The success/failure response indication is currently ignored; it is provided so we can implement it later without having to change the callback function API yet again in future. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/exec/memattrs.h | 34 +++++++++++++++++ include/exec/memory.h | 22 +++++++++++ memory.c | 99 ++++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 137 insertions(+), 18 deletions(-) create mode 100644 include/exec/memattrs.h