Message ID | 20170112055027.GK4450@pxdev.xzpeter.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 12/01/2017 06:50, Peter Xu wrote: > On Wed, Jan 11, 2017 at 06:21:46PM +0100, Paolo Bonzini wrote: >> >> >> On 21/12/2016 08:58, Peter Xu wrote: >>> - mr->romd_mode ? 'R' : '-', >>> - !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W' >>> - : '-', >>> + MR_CHAR_RD(mr), >>> + MR_CHAR_WR(mr), >> >> An alternative definition could be >> >> memory_access_is_direct(mr, false) ? 'R' : '-' >> memory_access_is_direct(mr, true) ? 'W' : '-' >> >> for MR_CHAR_RD and MR_CHAR_WR. With this change, I think the small code >> duplication in the "? :" operator is tolerable and the code is clearer. > > memory_access_is_direct() will check against whether mr is RAM, is > that what we want here? In that case we'll get most of the regions as > "--" as long as they are not RAM, while in fact IMHO we should want to > know the rw permission for all cases. Indeed. My idea was that the RW permission is not well defined for non-RAM memory regions, and ROMD regions in MMIO mode shows as "--" while MMIO regions show as "RW". But perhaps it's confusing. What about writing one of "ram", "rom", "ramd", "romd", "i/o" (with I/O also covering rom_device && !romd_mode)? If you disagree, the below patch looks good. Paolo > --------8<-------- > diff --git a/include/exec/memory.h b/include/exec/memory.h > index bec9756..50974c8 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr, > MemTxAttrs attrs, uint8_t *buf, int len); > void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr); > > +static inline bool memory_region_is_readable(MemoryRegion *mr) > +{ > + return mr->rom_device ? mr->romd_mode : true; > +} > + > +static inline bool memory_region_is_writable(MemoryRegion *mr) > +{ > + return !mr->rom_device && !mr->readonly; > +} > + > +static inline bool memory_region_is_direct(MemoryRegion *mr) > +{ > + return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr); > +} > + > static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) > { > if (is_write) { > - return memory_region_is_ram(mr) && > - !mr->readonly && !memory_region_is_ram_device(mr); > + return memory_region_is_direct(mr) && memory_region_is_writable(mr); > } else { > - return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) || > - memory_region_is_romd(mr); > + return memory_region_is_direct(mr) && memory_region_is_readable(mr); > } > } > -------->8-------- > > Then, I can throw away MR_CHAR_* macros and use: > > memory_access_is_readable(mr, false) ? 'R' : '-' > memory_access_is_writable(mr, true) ? 'W' : '-' > > Do you like this approach? > > -- peterx > >
On Thu, Jan 12, 2017 at 09:54:24AM +0100, Paolo Bonzini wrote: > > > On 12/01/2017 06:50, Peter Xu wrote: > > On Wed, Jan 11, 2017 at 06:21:46PM +0100, Paolo Bonzini wrote: > >> > >> > >> On 21/12/2016 08:58, Peter Xu wrote: > >>> - mr->romd_mode ? 'R' : '-', > >>> - !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W' > >>> - : '-', > >>> + MR_CHAR_RD(mr), > >>> + MR_CHAR_WR(mr), > >> > >> An alternative definition could be > >> > >> memory_access_is_direct(mr, false) ? 'R' : '-' > >> memory_access_is_direct(mr, true) ? 'W' : '-' > >> > >> for MR_CHAR_RD and MR_CHAR_WR. With this change, I think the small code > >> duplication in the "? :" operator is tolerable and the code is clearer. > > > > memory_access_is_direct() will check against whether mr is RAM, is > > that what we want here? In that case we'll get most of the regions as > > "--" as long as they are not RAM, while in fact IMHO we should want to > > know the rw permission for all cases. > > Indeed. My idea was that the RW permission is not well defined for > non-RAM memory regions, and ROMD regions in MMIO mode shows as "--" > while MMIO regions show as "RW". But perhaps it's confusing. > > What about writing one of "ram", "rom", "ramd", "romd", "i/o" (with I/O > also covering rom_device && !romd_mode)? > > If you disagree, the below patch looks good. Yes, above suggestion makes sense to me, since after all the RW permissions are derived from the type of memory regions, and the type itself tells more things than the RW bits. So I totally agree we can replace the "RW" chars with its type directly (if no one else disagree, of course). While for below patch, do you want me to include it as well as a standalone patch, for the purpose of refactoring memory_access_is_direct()? Since IMHO it's tiny clearer and more readable than before. Thanks! > > Paolo > > > --------8<-------- > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index bec9756..50974c8 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr, > > MemTxAttrs attrs, uint8_t *buf, int len); > > void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr); > > > > +static inline bool memory_region_is_readable(MemoryRegion *mr) > > +{ > > + return mr->rom_device ? mr->romd_mode : true; > > +} > > + > > +static inline bool memory_region_is_writable(MemoryRegion *mr) > > +{ > > + return !mr->rom_device && !mr->readonly; > > +} > > + > > +static inline bool memory_region_is_direct(MemoryRegion *mr) > > +{ > > + return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr); > > +} > > + > > static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) > > { > > if (is_write) { > > - return memory_region_is_ram(mr) && > > - !mr->readonly && !memory_region_is_ram_device(mr); > > + return memory_region_is_direct(mr) && memory_region_is_writable(mr); > > } else { > > - return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) || > > - memory_region_is_romd(mr); > > + return memory_region_is_direct(mr) && memory_region_is_readable(mr); > > } > > } > > -------->8-------- > > > > Then, I can throw away MR_CHAR_* macros and use: > > > > memory_access_is_readable(mr, false) ? 'R' : '-' > > memory_access_is_writable(mr, true) ? 'W' : '-' > > > > Do you like this approach? > > > > -- peterx > > > > -- peterx
On 12/01/2017 10:46, Peter Xu wrote: > Yes, above suggestion makes sense to me, since after all the RW > permissions are derived from the type of memory regions, and the type > itself tells more things than the RW bits. So I totally agree we can > replace the "RW" chars with its type directly (if no one else > disagree, of course). > > While for below patch, do you want me to include it as well as a > standalone patch, for the purpose of refactoring > memory_access_is_direct()? Since IMHO it's tiny clearer and more > readable than before. It is more readable, but my plan was to turn these fields into a single field (with bits) to speed up memory_access_is_direct. For that we'd need to undo your change. So I'm undecided. Paolo
On Thu, Jan 12, 2017 at 12:19:21PM +0100, Paolo Bonzini wrote: > > > On 12/01/2017 10:46, Peter Xu wrote: > > Yes, above suggestion makes sense to me, since after all the RW > > permissions are derived from the type of memory regions, and the type > > itself tells more things than the RW bits. So I totally agree we can > > replace the "RW" chars with its type directly (if no one else > > disagree, of course). > > > > While for below patch, do you want me to include it as well as a > > standalone patch, for the purpose of refactoring > > memory_access_is_direct()? Since IMHO it's tiny clearer and more > > readable than before. > > It is more readable, but my plan was to turn these fields into a single > field (with bits) to speed up memory_access_is_direct. For that we'd > need to undo your change. So I'm undecided. No problem. Then I'll just ignore it and repost with above. Thanks! -- peterx
diff --git a/include/exec/memory.h b/include/exec/memory.h index bec9756..50974c8 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, uint8_t *buf, int len); void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr); +static inline bool memory_region_is_readable(MemoryRegion *mr) +{ + return mr->rom_device ? mr->romd_mode : true; +} + +static inline bool memory_region_is_writable(MemoryRegion *mr) +{ + return !mr->rom_device && !mr->readonly; +} + +static inline bool memory_region_is_direct(MemoryRegion *mr) +{ + return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr); +} + static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) { if (is_write) { - return memory_region_is_ram(mr) && - !mr->readonly && !memory_region_is_ram_device(mr); + return memory_region_is_direct(mr) && memory_region_is_writable(mr); } else { - return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) || - memory_region_is_romd(mr); + return memory_region_is_direct(mr) && memory_region_is_readable(mr); } } -------->8--------