| Message ID | 1564123434793.89101@bt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Invert Endian bit in SPARCv9 MMU TTE | expand |
On 7/25/19 11:43 PM, tony.nguyen@bt.com wrote: > } MemOp; > > +/* No-op while memory_region_dispatch_[read|write] is converted to MemOp */ > +#define MEMOP_SIZE(op) (op) /* MemOp to size. */ > +#define SIZE_MEMOP(ul) (ul) /* Size to MemOp. */ > + This doesn't thrill me, because for 9 patches these macros don't do what they say on the tin, but I'll accept it. I would prefer lower-case and that the real implementation in patch 10 be inline functions with proper types instead of typeless macros. In particular, "unsigned" not "unsigned long" as you imply from "ul" here, since that's what was used ... > MemTxResult memory_region_dispatch_read(MemoryRegion *mr, > hwaddr addr, > uint64_t *pval, > - unsigned size, > + MemOp op, > MemTxAttrs attrs); ... here. With the name case change, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 7/26/19 6:36 AM, Richard Henderson wrote: > On 7/25/19 11:43 PM, tony.nguyen@bt.com wrote: >> } MemOp; >> >> +/* No-op while memory_region_dispatch_[read|write] is converted to MemOp */ >> +#define MEMOP_SIZE(op) (op) /* MemOp to size. */ >> +#define SIZE_MEMOP(ul) (ul) /* Size to MemOp. */ >> + > > This doesn't thrill me, because for 9 patches these macros don't do what they > say on the tin, but I'll accept it. > > I would prefer lower-case and that the real implementation in patch 10 be > inline functions with proper types instead of typeless macros. In particular, > "unsigned" not "unsigned long" as you imply from "ul" here, since that's what > was used ... > >> MemTxResult memory_region_dispatch_read(MemoryRegion *mr, >> hwaddr addr, >> uint64_t *pval, >> - unsigned size, >> + MemOp op, >> MemTxAttrs attrs); Actually, I thought of something that would make me happier: Do not make any change to memory_region_dispatch_{read,write} now. Let the operand continue to be "unsigned size", because it still is, because of the no-op macros. Make the change to to the proper type at the same time that you flip the switch to use the proper conversion function. This will make patch 10 about 5 lines longer, but we'll have proper typing at all points in between. r~ > > ... here. > > With the name case change, > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > > r~ >
diff --git a/include/exec/memop.h b/include/exec/memop.h index ac58066..09c8d20 100644 --- a/include/exec/memop.h +++ b/include/exec/memop.h @@ -106,4 +106,8 @@ typedef enum MemOp { MO_SSIZE = MO_SIZE | MO_SIGN, } MemOp; +/* No-op while memory_region_dispatch_[read|write] is converted to MemOp */ +#define MEMOP_SIZE(op) (op) /* MemOp to size. */ +#define SIZE_MEMOP(ul) (ul) /* Size to MemOp. */ + #endif diff --git a/include/exec/memory.h b/include/exec/memory.h index bb0961d..0ea4843 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -19,6 +19,7 @@ #include "exec/cpu-common.h" #include "exec/hwaddr.h" #include "exec/memattrs.h" +#include "exec/memop.h" #include "exec/ramlist.h" #include "qemu/queue.h" #include "qemu/int128.h" @@ -1731,13 +1732,13 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner); * @mr: #MemoryRegion to access * @addr: address within that region * @pval: pointer to uint64_t which the data is written to - * @size: size of the access in bytes + * @op: size of the access in bytes * @attrs: memory transaction attributes to use for the access */ MemTxResult memory_region_dispatch_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, - unsigned size, + MemOp op, MemTxAttrs attrs); /** * memory_region_dispatch_write: perform a write directly to the specified @@ -1746,13 +1747,13 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr, * @mr: #MemoryRegion to access * @addr: address within that region * @data: data to write - * @size: size of the access in bytes + * @op: size of the access in bytes * @attrs: memory transaction attributes to use for the access */ MemTxResult memory_region_dispatch_write(MemoryRegion *mr, hwaddr addr, uint64_t data, - unsigned size, + MemOp op, MemTxAttrs attrs); /** diff --git a/memory.c b/memory.c index 5d8c9a9..6982e19 100644 --- a/memory.c +++ b/memory.c @@ -1439,10 +1439,11 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr, MemTxResult memory_region_dispatch_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, - unsigned size, + MemOp op, MemTxAttrs attrs) { MemTxResult r; + unsigned size = MEMOP_SIZE(op); if (!memory_region_access_valid(mr, addr, size, false, attrs)) { *pval = unassigned_mem_read(mr, addr, size); @@ -1483,9 +1484,11 @@ static bool memory_region_dispatch_write_eventfds(MemoryRegion *mr, MemTxResult memory_region_dispatch_write(MemoryRegion *mr, hwaddr addr, uint64_t data, - unsigned size, + MemOp op, MemTxAttrs attrs) { + unsigned size = MEMOP_SIZE(op); + if (!memory_region_access_valid(mr, addr, size, true, attrs)) { unassigned_mem_write(mr, addr, data, size); return MEMTX_DECODE_ERROR;
Change memory_region_dispatch_{read|write} parameter "unsigned size" to "MemOp op". The endianness encoded in MemOp will enable the collapse of two byte swaps, adjust_endianness and handle_bswap, along the I/O path. Interfaces will be converted in two steps: first syntactically then semantically. The syntax change is usage of no-op MEMOP_SIZE and SIZE_MEMOP macros. Being no-op there are no logical change, and we rely on coercion between unsigned and MemOp. The semantic change is implementing MEMOP_SIZE and SIZE_MEMOP to logically convert an unsigned size to and from a size+sign+endianness encoded MemOp. Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> --- include/exec/memop.h | 4 ++++ include/exec/memory.h | 9 +++++---- memory.c | 7 +++++-- 3 files changed, 14 insertions(+), 6 deletions(-) -- 1.8.3.1