Re: [Qemu-devel] [PATCH v7 11/42] memory: Access MemoryRegion with MemOp

[Philippe Mathieu-Daudé]

On 8/16/19 9:30 AM, address@hidden wrote:
> Convert memory_region_dispatch_{read|write} operand "unsigned size"
> into a "MemOp op".
> 
> Signed-off-by: Tony Nguyen <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
> ---
>  include/exec/memop.h  | 20 ++++++++++++++------
>  include/exec/memory.h |  9 +++++----
>  memory.c              |  7 +++++--
>  3 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/include/exec/memop.h b/include/exec/memop.h
> index dfd76a1..0a610b7 100644
> --- a/include/exec/memop.h
> +++ b/include/exec/memop.h
> @@ -12,6 +12,8 @@
>  #ifndef MEMOP_H
>  #define MEMOP_H
>  
> +#include "qemu/host-utils.h"
> +
>  typedef enum MemOp {
>      MO_8     = 0,
>      MO_16    = 1,
> @@ -107,14 +109,20 @@ typedef enum MemOp {
>      MO_SSIZE = MO_SIZE | MO_SIGN,
>  } MemOp;
>  
> +/* MemOp to size in bytes.  */
> +static inline unsigned memop_size(MemOp op)
> +{
> +    return 1 << (op & MO_SIZE);
> +}
> +
>  /* Size in bytes to MemOp.  */
> -static inline unsigned size_memop(unsigned size)
> +static inline MemOp size_memop(unsigned size)
>  {
> -    /*
> -     * FIXME: No-op to aid conversion of
> memory_region_dispatch_{read|write}
> -     * "unsigned size" operand into a "MemOp op".
> -     */
> -    return size;
> +#ifdef CONFIG_DEBUG_TCG
> +    /* Power of 2 up to 8.  */
> +    assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);

Easier to review as:

       assert(is_power_of_2(size) && size <= 8);

(This can be cleaned later).

> +#endif
> +    return ctz32(size);
>  }
>  
>  #endif
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index bb0961d..975b86a 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, sign, and endianness of the memory operation
>   * @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, sign, and endianness of the memory operation
>   * @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..89ea4fb 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1439,9 +1439,10 @@ 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)
>  {
> +    unsigned size = memop_size(op);
>      MemTxResult r;
>  
>      if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
> @@ -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;
> -- 
> 1.8.3.1
> 
> ​
> 
>