| Message ID | 20241210140112.43313-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | bulk: Remove legacy cpu_physical_memory_rw() API | expand |
On Tue, 10 Dec 2024 at 14:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > cpu_physical_memory_rw() API is documented as legacy > since 2017 (commit b7ecba0f6f6). Replace it by a more > recent API. Noticed while discussing with Peter: > https://lore.kernel.org/qemu-devel/e979b3ba-e701-4ac6-962a-19e4598ba947@linaro.org I'm not sure we want to do this as a bulk automated transformation, because in each case there is likely a better thing we can do with the call than to use address_space_memory. For example most of the uses in devices probably want to have the device have an AddressSpace property that the board wires up. thanks -- PMM
On Tue, 10 Dec 2024 at 15:00, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 10 Dec 2024 at 14:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > cpu_physical_memory_rw() API is documented as legacy > > since 2017 (commit b7ecba0f6f6). Replace it by a more > > recent API. Noticed while discussing with Peter: > > https://lore.kernel.org/qemu-devel/e979b3ba-e701-4ac6-962a-19e4598ba947@linaro.org > > I'm not sure we want to do this as a bulk automated > transformation, because in each case there is likely > a better thing we can do with the call than to use > address_space_memory. For example most of the uses in > devices probably want to have the device have an > AddressSpace property that the board wires up. Also, examining each use gives us an opportunity to consider the error handling (cpu_physical_memory_*() drop errors silently) and whether there's an appropriate MemTxAttrs we want to use. thanks -- PMM
On 10/12/24 16:03, Peter Maydell wrote: > On Tue, 10 Dec 2024 at 15:00, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Tue, 10 Dec 2024 at 14:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>> >>> cpu_physical_memory_rw() API is documented as legacy >>> since 2017 (commit b7ecba0f6f6). Replace it by a more >>> recent API. Noticed while discussing with Peter: >>> https://lore.kernel.org/qemu-devel/e979b3ba-e701-4ac6-962a-19e4598ba947@linaro.org >> >> I'm not sure we want to do this as a bulk automated >> transformation, because in each case there is likely >> a better thing we can do with the call than to use >> address_space_memory. For example most of the uses in >> devices probably want to have the device have an >> AddressSpace property that the board wires up. > > Also, examining each use gives us an opportunity to > consider the error handling (cpu_physical_memory_*() > drop errors silently) and whether there's an appropriate > MemTxAttrs we want to use. Yes I noticed that and agree, but do we really want to improve these devices? They have been using the legacy API for 7 years without caring much. I can repost split in 50 patches, hoping a dozen get merged directly. But then I expect discussions requiring too much unimportant work to happen, and the series being abandoned, giving this legacy API 10 more years... I'm being a bit negative so I'll post a v2, and then we can open GitLab issues for devices needing to be improved. Regards, Phil.
On Tue, 10 Dec 2024 at 15:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 10/12/24 16:03, Peter Maydell wrote: > > On Tue, 10 Dec 2024 at 15:00, Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >> On Tue, 10 Dec 2024 at 14:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >>> > >>> cpu_physical_memory_rw() API is documented as legacy > >>> since 2017 (commit b7ecba0f6f6). Replace it by a more > >>> recent API. Noticed while discussing with Peter: > >>> https://lore.kernel.org/qemu-devel/e979b3ba-e701-4ac6-962a-19e4598ba947@linaro.org > >> > >> I'm not sure we want to do this as a bulk automated > >> transformation, because in each case there is likely > >> a better thing we can do with the call than to use > >> address_space_memory. For example most of the uses in > >> devices probably want to have the device have an > >> AddressSpace property that the board wires up. > > > > Also, examining each use gives us an opportunity to > > consider the error handling (cpu_physical_memory_*() > > drop errors silently) and whether there's an appropriate > > MemTxAttrs we want to use. > > Yes I noticed that and agree, but do we really want to improve > these devices? They have been using the legacy API for 7 years > without caring much. > > I can repost split in 50 patches, hoping a dozen get merged > directly. But then I expect discussions requiring too much > unimportant work to happen, and the series being abandoned, > giving this legacy API 10 more years.. Well, it's legacy in the sense of "don't use it for new stuff", but also is it doing any harm in the old code? If we want to improve the places that use it we can, but I'm not sure I would worry about moving from one "this isn't really the ideal" to a different "this isn't really the ideal"... If the existence of the function is blocking a separate cleanup, that's a different matter, of course. thanks -- PMM
On Tue, 10 Dec 2024 at 15:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 10/12/24 16:03, Peter Maydell wrote: > > On Tue, 10 Dec 2024 at 15:00, Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >> On Tue, 10 Dec 2024 at 14:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >>> > >>> cpu_physical_memory_rw() API is documented as legacy > >>> since 2017 (commit b7ecba0f6f6). Replace it by a more > >>> recent API. Noticed while discussing with Peter: > >>> https://lore.kernel.org/qemu-devel/e979b3ba-e701-4ac6-962a-19e4598ba947@linaro.org > >> > >> I'm not sure we want to do this as a bulk automated > >> transformation, because in each case there is likely > >> a better thing we can do with the call than to use > >> address_space_memory. For example most of the uses in > >> devices probably want to have the device have an > >> AddressSpace property that the board wires up. > > > > Also, examining each use gives us an opportunity to > > consider the error handling (cpu_physical_memory_*() > > drop errors silently) and whether there's an appropriate > > MemTxAttrs we want to use. > > Yes I noticed that and agree, but do we really want to improve > these devices? They have been using the legacy API for 7 years > without caring much. > > I can repost split in 50 patches, hoping a dozen get merged > directly. But then I expect discussions requiring too much > unimportant work to happen, and the series being abandoned, > giving this legacy API 10 more years... Also, just to be clear, in the "do a mechanical transform in a single patch" it is not the "single patch" part that I'm unhappy about here -- it's the "do a mechanical transform" bit. Splitting it into 50 patches wouldn't address that. thanks -- PMM