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