[Qemu-devel] BiteSizedTasks: API conversion

[Qemu-devel] BiteSizedTasks: API conversion

[Su Hang]

Dear Thomas,
I'm tring to work on this job:
BiteSizedTasks:
    API conversion:
        Replace calls to functions named cpu_physical_memory_* with address_space_*.
When I try to replace
`cpu_physical_memory_write_rom(rom->as, rom->addr, rom->data, rom->datasize);`
with
`address_space_write_rom(rom->as, rom->addr, rom->data, rom->datasize);`
gcc complains about:
"""
/home/darcy/workstation/qemu/hw/core/loader.c: In function ‘rom_reset’:
/home/darcy/workstation/qemu/hw/core/loader.c:1098:12: error: implicit declaration of function ‘address_space_write_rom’ [-Werror=implicit-function-declaration]
            address_space_write_rom(rom->as, rom->addr, rom->data,
            ^
/home/darcy/workstation/qemu/hw/core/loader.c:1098:12: error: nested extern declaration of ‘address_space_write_rom’ [-Werror=nested-externs]
cc1: all warnings being treated as errors
/home/darcy/workstation/qemu/rules.mak:66: recipe for target 'hw/core/loader.o' failed
make: *** [hw/core/loader.o] Error 1
"""

Then I use `ag address_space_write_rom` in qemu's root directory,
it failed to find any function named `address_space_write_rom`.

By the way, sorry I don't find tool like `WikiBlame`, to help me find
who add this task. So I can't CC others but you.

Best,
Su Hang

Re: [Qemu-devel] BiteSizedTasks: API conversion

[Peter Maydell]

On 12 March 2018 at 11:24, Su Hang <suhang16@mails.ucas.ac.cn> wrote:
> Dear Thomas,
> I'm tring to work on this job:
> BiteSizedTasks:
>     API conversion:
>         Replace calls to functions named cpu_physical_memory_* with address_space_*.

This is a generic suggestion, but not all cpu_physical_memory_*
functions currently have a direct counterpart in address_space_*.
(Note also that it is not as simple as "change the function name
at the call site -- there are more parameters to the address_space_*
functions. Ideally it also requires examining each call site to
see what it should do if the memory access fails, and what
address space it should really be using. This is rather more
complicated than it appears from the fact it's on a bite-sized-task
list. Any individual conversion of a callsite is probably small
enough to be bite-sized, but you shouldn't consider "change
everywhere" to be a single small task.)

As you've found, cpu_physical_memory_write_rom() is one of those
functions that doesn't currently have an address_space_* equivalent.

There's actually a 'TODO' note in our API docs in
docs/devel/loads-stores.rst about this:

# Note that unlike ``cpu_physical_memory_write()`` this function takes
# an AddressSpace argument, but unlike ``address_space_write()`` this
# function does not take a ``MemTxAttrs`` or return a ``MemTxResult``.
#
# **TODO**: we should probably clean up this inconsistency and
# turn the function into ``address_space_write_rom`` with an API
# matching ``address_space_write``.

and I think that would be the best thing:
 (1) have address_space_write_rom() which takes the extra
 MemTxAttrs argument and returns the MemTxResult, and
 make cpu_physical_memory_write_rom() just be a trivial
 wrapper around it
 (2) then change the callers to use address_space_write_rom()
 (3) remove the cpu_physical_memory_write_rom() wrapper

Step 2 is the harder part, because really it requires looking at
the call sites to see what they should do if the memory access
fails.

thanks
-- PMM

Re: [Qemu-devel] BiteSizedTasks: API conversion

[Su Hang]

> -----Original Messages-----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> Sent Time: 2018-03-12 19:40:22 (Monday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: "thomas huth" <thuth@redhat.com>, "QEMU Developers" <qemu-devel@nongnu.org>
> Subject: Re: [Qemu-devel] BiteSizedTasks: API conversion
> 
> On 12 March 2018 at 11:24, Su Hang <suhang16@mails.ucas.ac.cn> wrote:

> list. Any individual conversion of a callsite is probably small
> enough to be bite-sized, but you shouldn't consider "change
> everywhere" to be a single small task.)

> and I think that would be the best thing:
>  (1) have address_space_write_rom() which takes the extra
>  MemTxAttrs argument and returns the MemTxResult, and
>  make cpu_physical_memory_write_rom() just be a trivial
>  wrapper around it
>  (2) then change the callers to use address_space_write_rom()
>  (3) remove the cpu_physical_memory_write_rom() wrapper
> 
> Step 2 is the harder part, because really it requires looking at
> the call sites to see what they should do if the memory access
> fails.
> 
> thanks
> -- PMM

Thanks for your suggestion, I will work on it, step by step.

Su Hang