Re: [Qemu-devel] [RFC PATCH] exec: Support non-direct memory writes in cpu_memory_rw_debug

[Andrey Smirnov]

On Thu, Jun 30, 2016 at 7:06 AM, Peter Maydell <address@hidden> wrote:
> On 28 June 2016 at 22:44, Andrey Smirnov <address@hidden> wrote:
>> Add code to support writing to memory mapped peripherals via
>> cpu_memory_rw_debug(). The code of that function already supports
>> reading from such memory regions, so this commit makes that
>> functionality "symmetric".
>>
>> One use-case for that functionality is setting various registers of a
>> non-running CPU. A concrete example would be starting QEMU emulating
>> Cortex-M with -S, connecting with GDB and modifying the value of Vector
>> Table Offset register.
>>
>> Signed-off-by: Andrey Smirnov <address@hidden>
>
>>  static uint16_t vring_used_idx(VirtQueue *vq)
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index 9f38edf..d5b4966 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -302,6 +302,6 @@ void dump_opcount_info(FILE *f, fprintf_function 
>> cpu_fprintf);
>>  #endif /* !CONFIG_USER_ONLY */
>>
>>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> -                        uint8_t *buf, int len, int is_write);
>> +                        uint8_t *buf, int len, MemTxType type);
>>
>>  #endif /* CPU_ALL_H */
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 4ab6800..8fbaf1b 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -14,6 +14,13 @@
>>  #ifndef MEMORY_H
>>  #define MEMORY_H
>>
>> +typedef enum {
>> +    MEMTX_READ,
>> +    MEMTX_WRITE,
>> +    MEMTX_PROGRAM,
>> +} MemTxType;
>
> We already have an enum for this: MMUAccessType.
> That is currently unhelpfully located in cpu-common.h, but there's a
> patch on list which should get applied soon which moves it to
> include/qom/cpu.h:
>  http://patchwork.ozlabs.org/patch/635235/
>
>
>> +
>>  #ifndef CONFIG_USER_ONLY
>>
>>  #define DIRTY_MEMORY_VGA       0
>> @@ -1240,6 +1247,7 @@ AddressSpace 
>> *address_space_init_shareable(MemoryRegion *root,
>>   */
>>  void address_space_destroy(AddressSpace *as);
>>
>> +
>>  /**
>>   * address_space_rw: read from or write to an address space.
>>   *
>> @@ -1251,11 +1259,11 @@ void address_space_destroy(AddressSpace *as);
>>   * @addr: address within that address space
>>   * @attrs: memory transaction attributes
>>   * @buf: buffer with the data transferred
>> - * @is_write: indicates the transfer direction
>> + * @type: indicates the transfer type
>>   */
>>  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
>>                               MemTxAttrs attrs, uint8_t *buf,
>> -                             int len, bool is_write);
>> +                             int len, MemTxType type);
>>
>>  /**
>>   * address_space_write: write to address space.
>> @@ -1268,10 +1276,11 @@ MemTxResult address_space_rw(AddressSpace *as, 
>> hwaddr addr,
>>   * @addr: address within that address space
>>   * @attrs: memory transaction attributes
>>   * @buf: buffer with the data transferred
>> + * @force: force writing to ROM areas
>>   */
>>  MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
>>                                  MemTxAttrs attrs,
>> -                                const uint8_t *buf, int len);
>> +                                const uint8_t *buf, int len, bool force);
>>
>>  /* address_space_ld*: load from an address space
>>   * address_space_st*: store to an address space
>
> I think this patch would be easier to review if it was
> split up, something like:
>  * a patch which just converts the is_write bool parameter to the
>    enum and updates all the callers, with no change in behaviour
>  * a patch which makes use of the ability to pass in something other
>    than 0 or 1
>  * a patch which adds and uses address_space_write_debug(),
>    or whatever API we go for
>
> The important bit is splitting the mechanical "convert bool
> to enum" part (which touches lots of files but makes no
> behaviour change) from the part which changes behaviour
> and doesn't touch many files.

OK, sounds good, will update in v2.

Andrey