| Message ID | 20210308201406.1240023-1-aaron@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | plugins: Expose physical addresses instead of device offsets | expand |
Patchew URL: https://patchew.org/QEMU/20210308201406.1240023-1-aaron@os.amperecomputing.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210308201406.1240023-1-aaron@os.amperecomputing.com Subject: [PATCH] plugins: Expose physical addresses instead of device offsets === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu 0436c55..229a834 master -> master - [tag update] patchew/20210308121155.2476-1-mark.cave-ayland@ilande.co.uk -> patchew/20210308121155.2476-1-mark.cave-ayland@ilande.co.uk - [tag update] patchew/20210308173244.20710-1-peter.maydell@linaro.org -> patchew/20210308173244.20710-1-peter.maydell@linaro.org * [new tag] patchew/20210308201406.1240023-1-aaron@os.amperecomputing.com -> patchew/20210308201406.1240023-1-aaron@os.amperecomputing.com Switched to a new branch 'test' fdddbf1 plugins: Expose physical addresses instead of device offsets === OUTPUT BEGIN === ERROR: line over 90 characters #96: FILE: plugins/api.c:310: + block = qemu_ram_block_from_host((void *) haddr->v.ram.hostaddr, false, &offset); total: 1 errors, 0 warnings, 68 lines checked Commit fdddbf18a9ac (plugins: Expose physical addresses instead of device offsets) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20210308201406.1240023-1-aaron@os.amperecomputing.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Mon, 8 Mar 2021 at 20:14, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > > This allows plugins to query for full virtual-to-physical address > translation for a given `qemu_plugin_hwaddr` and stops exposing the > offset within the device itself. As this change breaks the API, > QEMU_PLUGIN_VERSION is incremented. > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > --- > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index c66507fe8f..2252ecf2f0 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -47,7 +47,7 @@ typedef uint64_t qemu_plugin_id_t; > > extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; > > -#define QEMU_PLUGIN_VERSION 0 > +#define QEMU_PLUGIN_VERSION 1 > > typedef struct { > /* string describing architecture */ > @@ -328,7 +328,7 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, > * offset will be into the appropriate block of RAM. > */ > bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); > -uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); > +uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr); This should have a documentation comment, since this is the public-facing API. Also, physical addresses aren't uniquely identifying, they're only valid in the context of a particular address space (think TrustZone, for instance), so the plugin-facing API should probably have some concept of how it distinguishes "this is an access for Secure 0x4000" from "this is an access for Non-Secure 0x4000". thanks -- PMM
Aaron Lindsay <aaron@os.amperecomputing.com> writes: > Alex, > > I've now tested this change, and it is giving what appear to be valid > and correct physical addresses for both RAM and IO accesses in all the > cases I've thrown at it. My main concern with this patch at this point > is that I am concerned I may be breaking your new plugin here: > >> +++ b/contrib/plugins/hwprofile.c >> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo, >> return; >> } else { >> const char *name = qemu_plugin_hwaddr_device_name(hwaddr); >> - uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr); >> + uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr); > > How angry is the plugin going to be that these are now physical > addresses instead of offsets? I think it will be fine. It's a new plugin this cycle and it only changes the reporting.
On Mar 09 10:08, Peter Maydell wrote: > On Mon, 8 Mar 2021 at 20:14, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > > > > This allows plugins to query for full virtual-to-physical address > > translation for a given `qemu_plugin_hwaddr` and stops exposing the > > offset within the device itself. As this change breaks the API, > > QEMU_PLUGIN_VERSION is incremented. > > > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > > --- > > > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > > index c66507fe8f..2252ecf2f0 100644 > > --- a/include/qemu/qemu-plugin.h > > +++ b/include/qemu/qemu-plugin.h > > @@ -47,7 +47,7 @@ typedef uint64_t qemu_plugin_id_t; > > > > extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; > > > > -#define QEMU_PLUGIN_VERSION 0 > > +#define QEMU_PLUGIN_VERSION 1 > > > > typedef struct { > > /* string describing architecture */ > > @@ -328,7 +328,7 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, > > * offset will be into the appropriate block of RAM. > > */ > > bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); > > -uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); > > +uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr); > > > This should have a documentation comment, since this is the public-facing API. I now see I neglected to update the comment right here the function declaration, and will do so for v2. But are you asking for additional documentation beyond that change? If so, where is the right place to add this? docs/devel/tcg-plugins.rst doesn't seem to have much in the way of documentation for the actual calls. > Also, physical addresses aren't uniquely identifying, they're only valid > in the context of a particular address space (think TrustZone, for instance), > so the plugin-facing API should probably have some concept of how it > distinguishes "this is an access for Secure 0x4000" from "this is an > access for Non-Secure 0x4000". I agree it could be handy to expose address spaces in addition to the addresses themselves. Do you believe doing so would change the form of the interface in this patch, or could that be a logically separate addition? I have a secondary concern that it might be hard to expose address spaces in an architecture-agnostic yet still-helpful way, but haven't thought through that enough for it to be a firm opinion. -Aaron
Aaron Lindsay <aaron@os.amperecomputing.com> writes: > On Mar 09 10:08, Peter Maydell wrote: >> On Mon, 8 Mar 2021 at 20:14, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: >> > >> > This allows plugins to query for full virtual-to-physical address >> > translation for a given `qemu_plugin_hwaddr` and stops exposing the >> > offset within the device itself. As this change breaks the API, >> > QEMU_PLUGIN_VERSION is incremented. >> > >> > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> >> > --- >> >> >> > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> > index c66507fe8f..2252ecf2f0 100644 >> > --- a/include/qemu/qemu-plugin.h >> > +++ b/include/qemu/qemu-plugin.h >> > @@ -47,7 +47,7 @@ typedef uint64_t qemu_plugin_id_t; >> > >> > extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; >> > >> > -#define QEMU_PLUGIN_VERSION 0 >> > +#define QEMU_PLUGIN_VERSION 1 >> > >> > typedef struct { >> > /* string describing architecture */ >> > @@ -328,7 +328,7 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, >> > * offset will be into the appropriate block of RAM. >> > */ >> > bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); >> > -uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); >> > +uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr); >> >> >> This should have a documentation comment, since this is the public-facing API. > > I now see I neglected to update the comment right here the function > declaration, and will do so for v2. > > But are you asking for additional documentation beyond that change? If > so, where is the right place to add this? docs/devel/tcg-plugins.rst > doesn't seem to have much in the way of documentation for the actual > calls. The calls should be documented in @kerneldoc style comments in the main plugin header. Which reminds me I should be able to extract them into the tcg-plugins.rst document via sphinx. > >> Also, physical addresses aren't uniquely identifying, they're only valid >> in the context of a particular address space (think TrustZone, for instance), >> so the plugin-facing API should probably have some concept of how it >> distinguishes "this is an access for Secure 0x4000" from "this is an >> access for Non-Secure 0x4000". > > I agree it could be handy to expose address spaces in addition to the > addresses themselves. Do you believe doing so would change the form of > the interface in this patch, or could that be a logically separate > addition? I think information about address spaces should extracted be a separate query. > I have a secondary concern that it might be hard to expose address > spaces in an architecture-agnostic yet still-helpful way, but haven't > thought through that enough for it to be a firm opinion. Indeed - so I don't think we need to rush this without giving it some thought. As soft-freeze is only a few days away I don't think we want to rush an address space query API into 6.0. For most users I expect a assuming physical address is unique will work for the most part. But it's worth mentioning it might not be in the comment. > > -Aaron
On Mar 09 17:45, Alex Bennée wrote: > Aaron Lindsay <aaron@os.amperecomputing.com> writes: > > On Mar 09 10:08, Peter Maydell wrote: > >> On Mon, 8 Mar 2021 at 20:14, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > >> > > >> > This allows plugins to query for full virtual-to-physical address > >> > translation for a given `qemu_plugin_hwaddr` and stops exposing the > >> > offset within the device itself. As this change breaks the API, > >> > QEMU_PLUGIN_VERSION is incremented. > >> > > >> > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > >> > --- > >> > >> > >> > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > >> > index c66507fe8f..2252ecf2f0 100644 > >> > --- a/include/qemu/qemu-plugin.h > >> > +++ b/include/qemu/qemu-plugin.h > >> > @@ -47,7 +47,7 @@ typedef uint64_t qemu_plugin_id_t; > >> > > >> > extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; > >> > > >> > -#define QEMU_PLUGIN_VERSION 0 > >> > +#define QEMU_PLUGIN_VERSION 1 > >> > > >> > typedef struct { > >> > /* string describing architecture */ > >> > @@ -328,7 +328,7 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, > >> > * offset will be into the appropriate block of RAM. > >> > */ > >> > bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); > >> > -uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); > >> > +uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr); > >> > >> > >> This should have a documentation comment, since this is the public-facing API. > > > > I now see I neglected to update the comment right here the function > > declaration, and will do so for v2. > > > > But are you asking for additional documentation beyond that change? If > > so, where is the right place to add this? docs/devel/tcg-plugins.rst > > doesn't seem to have much in the way of documentation for the actual > > calls. > > The calls should be documented in @kerneldoc style comments in the main > plugin header. Which reminds me I should be able to extract them into > the tcg-plugins.rst document via sphinx. I just sent out v2, in which I took a pass at updating this documentation. Let me know what you think. -Aaron
diff --git a/contrib/plugins/hotpages.c b/contrib/plugins/hotpages.c index eacc678eac..bf53267532 100644 --- a/contrib/plugins/hotpages.c +++ b/contrib/plugins/hotpages.c @@ -122,7 +122,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo, } } else { if (hwaddr && !qemu_plugin_hwaddr_is_io(hwaddr)) { - page = (uint64_t) qemu_plugin_hwaddr_device_offset(hwaddr); + page = (uint64_t) qemu_plugin_hwaddr_phys_addr(hwaddr); } else { page = vaddr; } diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c index 6dac1d5f85..faf216ac00 100644 --- a/contrib/plugins/hwprofile.c +++ b/contrib/plugins/hwprofile.c @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo, return; } else { const char *name = qemu_plugin_hwaddr_device_name(hwaddr); - uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr); + uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr); bool is_write = qemu_plugin_mem_is_store(meminfo); DeviceCounts *counts; diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index c66507fe8f..2252ecf2f0 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -47,7 +47,7 @@ typedef uint64_t qemu_plugin_id_t; extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; -#define QEMU_PLUGIN_VERSION 0 +#define QEMU_PLUGIN_VERSION 1 typedef struct { /* string describing architecture */ @@ -328,7 +328,7 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, * offset will be into the appropriate block of RAM. */ bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr); -uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr); +uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr); /* * Returns a string representing the device. The string is valid for diff --git a/plugins/api.c b/plugins/api.c index 0b04380d57..e7352df3e3 100644 --- a/plugins/api.c +++ b/plugins/api.c @@ -40,6 +40,7 @@ #include "sysemu/sysemu.h" #include "tcg/tcg.h" #include "exec/exec-all.h" +#include "exec/ram_addr.h" #include "disas/disas.h" #include "plugin.h" #ifndef CONFIG_USER_ONLY @@ -298,19 +299,24 @@ bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr) #endif } -uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr) +uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr) { #ifdef CONFIG_SOFTMMU if (haddr) { if (!haddr->is_io) { - ram_addr_t ram_addr = qemu_ram_addr_from_host((void *) haddr->v.ram.hostaddr); - if (ram_addr == RAM_ADDR_INVALID) { + RAMBlock *block; + ram_addr_t offset; + + block = qemu_ram_block_from_host((void *) haddr->v.ram.hostaddr, false, &offset); + if (!block) { error_report("Bad ram pointer %"PRIx64"", haddr->v.ram.hostaddr); abort(); } - return ram_addr; + + return block->offset + offset + block->mr->addr; } else { - return haddr->v.io.offset; + MemoryRegionSection *mrs = haddr->v.io.section; + return haddr->v.io.offset + mrs->mr->addr; } } #endif
This allows plugins to query for full virtual-to-physical address translation for a given `qemu_plugin_hwaddr` and stops exposing the offset within the device itself. As this change breaks the API, QEMU_PLUGIN_VERSION is incremented. Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> --- contrib/plugins/hotpages.c | 2 +- contrib/plugins/hwprofile.c | 2 +- include/qemu/qemu-plugin.h | 4 ++-- plugins/api.c | 16 +++++++++++----- 4 files changed, 15 insertions(+), 9 deletions(-)