qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-riscv] [Qemu-devel] [PATCH v5 02/15] memory: Access MemoryRegi


From: Richard Henderson
Subject: Re: [Qemu-riscv] [Qemu-devel] [PATCH v5 02/15] memory: Access MemoryRegion with MemOp
Date: Fri, 26 Jul 2019 07:04:37 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0

On 7/26/19 6:36 AM, Richard Henderson wrote:
> On 7/25/19 11:43 PM, address@hidden 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 <address@hidden>
> 
> 
> r~
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]