[PATCH] plugins: Expose physical addresses instead of device offsets

[PATCH] plugins: Expose physical addresses instead of device offsets

[Aaron Lindsay]

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(-)

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
-- 
2.17.1

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[Aaron Lindsay]

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?

-Aaron

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[no-reply]

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

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[Peter Maydell]

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

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[Alex Bennée]

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.

-- 
Alex Bennée

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[Aaron Lindsay]

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

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[Alex Bennée]

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


-- 
Alex Bennée

Re: [PATCH] plugins: Expose physical addresses instead of device offsets

[Aaron Lindsay via]

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