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