Plugin Address Translations Inconsistent/Incorrect?

Plugin Address Translations Inconsistent/Incorrect?

[Aaron Lindsay]

Hello,

I've been doing some more work with plugins and found something I didn't
expect with regards to address translation.

If I call (inside a memory callback):

`uint64_t pa = qemu_plugin_hwaddr_device_offset(hwaddr);`

I see that `pa` takes the value 0xe0e58760. If, however, I plumb
`cpu_get_phys_page_debug` through to the plugin interface and call it
like:

`pa = cpu_get_phys_page_debug(current_cpu, va);`

I see it takes the value 0x120e58760.

I notice that 0x120e58760-0xe0e58760 is exactly one gigabyte, which is
also the offset of the beginning of RAM for the 'virt' AArch64 machine
I'm using. Furthermore, I see the name of the plugin function includes
"device_offset", so perhaps this discrepancy is by design. However, it
seems awkward to not be able to get a true physical address.

I've done some digging and found that inside `qemu_ram_addr_from_host`
(called by `qemu_plugin_hwaddr_device_offset`), `block->mr->addr`
appears to hold the offset of the beginning of RAM. 

Do you think it would be reasonable to modify
`qemu_plugin_hwaddr_device_offset` to add the beginning of the RAM block
or otherwise return the true physical address (or at least expose a way
to find the beginning of it through the plugin interface)?

Thanks!

-Aaron

Re: Plugin Address Translations Inconsistent/Incorrect?

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> Hello,
>
> I've been doing some more work with plugins and found something I didn't
> expect with regards to address translation.
>
> If I call (inside a memory callback):
>
> `uint64_t pa = qemu_plugin_hwaddr_device_offset(hwaddr);`
>
> I see that `pa` takes the value 0xe0e58760. If, however, I plumb
> `cpu_get_phys_page_debug` through to the plugin interface and call it
> like:
>
> `pa = cpu_get_phys_page_debug(current_cpu, va);`
>
> I see it takes the value 0x120e58760.
>
> I notice that 0x120e58760-0xe0e58760 is exactly one gigabyte, which is
> also the offset of the beginning of RAM for the 'virt' AArch64 machine
> I'm using. Furthermore, I see the name of the plugin function includes
> "device_offset", so perhaps this discrepancy is by design. However, it
> seems awkward to not be able to get a true physical address.

It certainly is by design. The comment for the helper states:

  /*
   * The following additional queries can be run on the hwaddr structure
   * to return information about it. For non-IO accesses the device
   * offset will be into the appropriate block of RAM.
   */

> I've done some digging and found that inside `qemu_ram_addr_from_host`
> (called by `qemu_plugin_hwaddr_device_offset`), `block->mr->addr`
> appears to hold the offset of the beginning of RAM. 
>
> Do you think it would be reasonable to modify
> `qemu_plugin_hwaddr_device_offset` to add the beginning of the RAM block
> or otherwise return the true physical address (or at least expose a way
> to find the beginning of it through the plugin interface)?

Well the problem here is what is the address map? For example if you
have a secure block of RAM you might have two physical addresses which
are the same. That said with the current qemu_plugin_hwaddr_device_name
helper both will get reported as "RAM" so maybe it's not that helpful
yet.

I also worry about what happens if devices get moved around. Do you end
up with aliasing of address space have a remap of the HW.

That said I think we could add an additional helper to translate a
hwaddr to a global address space address. I'm open to suggestions of the
best way to structure this.

>
> Thanks!
>
> -Aaron


-- 
Alex Bennée

Re: Plugin Address Translations Inconsistent/Incorrect?

[Peter Maydell]

On Mon, 22 Feb 2021 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
> It certainly is by design. The comment for the helper states:
>
>   /*
>    * The following additional queries can be run on the hwaddr structure
>    * to return information about it. For non-IO accesses the device
>    * offset will be into the appropriate block of RAM.
>    */

That sounds like we're exposing ram_addrs to the plugin. Are we?
I'm not sure that's a good idea, as they're not a guest-relevant
construct.

thanks
-- PMM

Re: Plugin Address Translations Inconsistent/Incorrect?

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 22 Feb 2021 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>> It certainly is by design. The comment for the helper states:
>>
>>   /*
>>    * The following additional queries can be run on the hwaddr structure
>>    * to return information about it. For non-IO accesses the device
>>    * offset will be into the appropriate block of RAM.
>>    */
>
> That sounds like we're exposing ram_addrs to the plugin. Are we?
> I'm not sure that's a good idea, as they're not a guest-relevant
> construct.

We currently expose qemu_ram_addr_from_host for RAM blocks. Are you
saying we should translate that to the direct physical address mapping?
If we do that for RAM should we be doing the same for IO addresses?

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: Plugin Address Translations Inconsistent/Incorrect?

[Peter Maydell]

On Tue, 23 Feb 2021 at 08:55, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 22 Feb 2021 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> It certainly is by design. The comment for the helper states:
> >>
> >>   /*
> >>    * The following additional queries can be run on the hwaddr structure
> >>    * to return information about it. For non-IO accesses the device
> >>    * offset will be into the appropriate block of RAM.
> >>    */
> >
> > That sounds like we're exposing ram_addrs to the plugin. Are we?
> > I'm not sure that's a good idea, as they're not a guest-relevant
> > construct.
>
> We currently expose qemu_ram_addr_from_host for RAM blocks. Are you
> saying we should translate that to the direct physical address mapping?
> If we do that for RAM should we be doing the same for IO addresses

This is not a fully-thought-through position, but as a general
thing I feel that the plugin API should be exposing as far as
possible concepts that relate to the guest program, not concepts
that relate to QEMU's internal implementation. One of the issues
with the -d logging is that it's very much "provide information
that's easy for QEMU to provide", and this confuses people who
just want to debug their guest. We can reasonably expect users to
understand concepts like "physical address", but it's less nice
to need them to understand "QEMU models guest RAM as a bunch of
blocks with a 'ram address' which is an offset into the stack
of blocks, and this isn't the same thing as any guest address".
It also constrains our ability to change the QEMU implementation
later if we let implementation details leak out via the plugin API.

thanks
-- PMM

Re: Plugin Address Translations Inconsistent/Incorrect?

[Aaron Lindsay via]

On Feb 22 19:30, Alex Bennée wrote:
> Aaron Lindsay <aaron@os.amperecomputing.com> writes:
> > If I call (inside a memory callback):
> >
> > `uint64_t pa = qemu_plugin_hwaddr_device_offset(hwaddr);`
> >
> > I see that `pa` takes the value 0xe0e58760. If, however, I plumb
> > `cpu_get_phys_page_debug` through to the plugin interface and call it
> > like:
> >
> > `pa = cpu_get_phys_page_debug(current_cpu, va);`
> >
> > I see it takes the value 0x120e58760.
> >
> > I notice that 0x120e58760-0xe0e58760 is exactly one gigabyte, which is
> > also the offset of the beginning of RAM for the 'virt' AArch64 machine
> > I'm using. Furthermore, I see the name of the plugin function includes
> > "device_offset", so perhaps this discrepancy is by design. However, it
> > seems awkward to not be able to get a true physical address.
> 
> It certainly is by design. The comment for the helper states:
> 
>   /*
>    * The following additional queries can be run on the hwaddr structure
>    * to return information about it. For non-IO accesses the device
>    * offset will be into the appropriate block of RAM.
>    */
> 
> > I've done some digging and found that inside `qemu_ram_addr_from_host`
> > (called by `qemu_plugin_hwaddr_device_offset`), `block->mr->addr`
> > appears to hold the offset of the beginning of RAM. 
> >
> > Do you think it would be reasonable to modify
> > `qemu_plugin_hwaddr_device_offset` to add the beginning of the RAM block
> > or otherwise return the true physical address (or at least expose a way
> > to find the beginning of it through the plugin interface)?
> 
> Well the problem here is what is the address map? For example if you
> have a secure block of RAM you might have two physical addresses which
> are the same. That said with the current qemu_plugin_hwaddr_device_name
> helper both will get reported as "RAM" so maybe it's not that helpful
> yet.

I don't think I yet understand why this is a problem. It seems to me
that the current implementation of `qemu_plugin_hwaddr_device_offset`
returns offsets which may already be ambiguous without additional
information about the underlying device/memory, and I'm not sure why
translating to full physical addresses would make that worse. It's
possible I'm not correctly interpreting your concern.

> I also worry about what happens if devices get moved around. Do you end
> up with aliasing of address space have a remap of the HW.

Would the `block->mr->addr` field I mentioned above be updated in such a
case?

> That said I think we could add an additional helper to translate a
> hwaddr to a global address space address. I'm open to suggestions of the
> best way to structure this.

Haven't put a ton of thought into it, but what about something like this
(untested):

uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
{
#ifdef CONFIG_SOFTMMU
    if (haddr) {
        if (!haddr->is_io) {
            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 block->offset + offset + block->mr->addr;
        } else {
            MemoryRegionSection *mrs = haddr->v.io.section;
            return haddr->v.io.offset + mrs->mr->addr;
        }
    }
#endif
    return 0;
}

The key differences from `qemu_plugin_hwaddr_device_offset` are using
`qemu_ram_block_from_host` directly instead of `qemu_ram_addr_from_host` (to
get a pointer to the RAMBlock), and adding `block->mr->addr` and
`mrs->mr->addr` to the returns for RAM and IO, respectively.

-Aaron

Re: Plugin Address Translations Inconsistent/Incorrect?

[Aaron Lindsay via]

On Feb 22 15:48, Aaron Lindsay wrote:
> On Feb 22 19:30, Alex Bennée wrote:
> > Aaron Lindsay <aaron@os.amperecomputing.com> writes:
> > That said I think we could add an additional helper to translate a
> > hwaddr to a global address space address. I'm open to suggestions of the
> > best way to structure this.
> 
> Haven't put a ton of thought into it, but what about something like this
> (untested):
> 
> uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
> {
> #ifdef CONFIG_SOFTMMU
>     if (haddr) {
>         if (!haddr->is_io) {
>             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 block->offset + offset + block->mr->addr;
>         } else {
>             MemoryRegionSection *mrs = haddr->v.io.section;
>             return haddr->v.io.offset + mrs->mr->addr;
>         }
>     }
> #endif
>     return 0;
> }

This appears to successfully return correct physical addresses for RAM
at least, though I've not tested it thoroughly for MMIO yet.

If it ends up being desirable based on the discussion elsewhere on this
thread I am willing to perform more complete testing, turn this into a
patch, and submit it.

-Aaron

Re: Plugin Address Translations Inconsistent/Incorrect?

[Aaron Lindsay via]

On Feb 23 15:53, Aaron Lindsay wrote:
> On Feb 22 15:48, Aaron Lindsay wrote:
> > On Feb 22 19:30, Alex Bennée wrote:
> > > Aaron Lindsay <aaron@os.amperecomputing.com> writes:
> > > That said I think we could add an additional helper to translate a
> > > hwaddr to a global address space address. I'm open to suggestions of the
> > > best way to structure this.
> > 
> > Haven't put a ton of thought into it, but what about something like this
> > (untested):
> > 
> > uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
> > {
> > #ifdef CONFIG_SOFTMMU
> >     if (haddr) {
> >         if (!haddr->is_io) {
> >             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 block->offset + offset + block->mr->addr;
> >         } else {
> >             MemoryRegionSection *mrs = haddr->v.io.section;
> >             return haddr->v.io.offset + mrs->mr->addr;
> >         }
> >     }
> > #endif
> >     return 0;
> > }
> 
> This appears to successfully return correct physical addresses for RAM
> at least, though I've not tested it thoroughly for MMIO yet.
> 
> If it ends up being desirable based on the discussion elsewhere on this
> thread I am willing to perform more complete testing, turn this into a
> patch, and submit it.

Ping - Is this something worth me pursuing?

-Aaron

Re: Plugin Address Translations Inconsistent/Incorrect?

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> On Feb 23 15:53, Aaron Lindsay wrote:
>> On Feb 22 15:48, Aaron Lindsay wrote:
>> > On Feb 22 19:30, Alex Bennée wrote:
>> > > Aaron Lindsay <aaron@os.amperecomputing.com> writes:
>> > > That said I think we could add an additional helper to translate a
>> > > hwaddr to a global address space address. I'm open to suggestions of the
>> > > best way to structure this.
>> > 
>> > Haven't put a ton of thought into it, but what about something like this
>> > (untested):
>> > 
>> > uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
>> > {
>> > #ifdef CONFIG_SOFTMMU
>> >     if (haddr) {
>> >         if (!haddr->is_io) {
>> >             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 block->offset + offset + block->mr->addr;
>> >         } else {
>> >             MemoryRegionSection *mrs = haddr->v.io.section;
>> >             return haddr->v.io.offset + mrs->mr->addr;
>> >         }
>> >     }
>> > #endif
>> >     return 0;
>> > }
>> 
>> This appears to successfully return correct physical addresses for RAM
>> at least, though I've not tested it thoroughly for MMIO yet.
>> 
>> If it ends up being desirable based on the discussion elsewhere on this
>> thread I am willing to perform more complete testing, turn this into a
>> patch, and submit it.
>
> Ping - Is this something worth me pursuing?

Yes please. 

-- 
Alex Bennée

Re: Plugin Address Translations Inconsistent/Incorrect?

[Aaron Lindsay via]

On Mar 02 16:06, Alex Bennée wrote:
> 
> Aaron Lindsay <aaron@os.amperecomputing.com> writes:
> 
> > On Feb 23 15:53, Aaron Lindsay wrote:
> >> On Feb 22 15:48, Aaron Lindsay wrote:
> >> > On Feb 22 19:30, Alex Bennée wrote:
> >> > > Aaron Lindsay <aaron@os.amperecomputing.com> writes:
> >> > > That said I think we could add an additional helper to translate a
> >> > > hwaddr to a global address space address. I'm open to suggestions of the
> >> > > best way to structure this.
> >> > 
> >> > Haven't put a ton of thought into it, but what about something like this
> >> > (untested):
> >> > 
> >> > uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
> >> > {
> >> > #ifdef CONFIG_SOFTMMU
> >> >     if (haddr) {
> >> >         if (!haddr->is_io) {
> >> >             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 block->offset + offset + block->mr->addr;
> >> >         } else {
> >> >             MemoryRegionSection *mrs = haddr->v.io.section;
> >> >             return haddr->v.io.offset + mrs->mr->addr;
> >> >         }
> >> >     }
> >> > #endif
> >> >     return 0;
> >> > }
> >> 
> >> This appears to successfully return correct physical addresses for RAM
> >> at least, though I've not tested it thoroughly for MMIO yet.
> >> 
> >> If it ends up being desirable based on the discussion elsewhere on this
> >> thread I am willing to perform more complete testing, turn this into a
> >> patch, and submit it.
> >
> > Ping - Is this something worth me pursuing?
> 
> Yes please. 

Okay, I'll work on it. Is your thinking that this would this be a
separate call as shown above, or a replacement of the existing
qemu_plugin_hwaddr_device_offset function? And, if a replacement, should
we keep the name similar to retain compatibility, or make a clean break?

It seemed like Peter may have been saying the device offset shouldn't be
exposed at all (leading me to consider full replacement), but I also
don't see a definitive resolution of that conversation.

-Aaron

Re: Plugin Address Translations Inconsistent/Incorrect?

[Alex Bennée]

Aaron Lindsay <aaron@os.amperecomputing.com> writes:

> On Mar 02 16:06, Alex Bennée wrote:
>> 
>> Aaron Lindsay <aaron@os.amperecomputing.com> writes:
>> 
>> > On Feb 23 15:53, Aaron Lindsay wrote:
>> >> On Feb 22 15:48, Aaron Lindsay wrote:
>> >> > On Feb 22 19:30, Alex Bennée wrote:
>> >> > > Aaron Lindsay <aaron@os.amperecomputing.com> writes:
>> >> > > That said I think we could add an additional helper to translate a
>> >> > > hwaddr to a global address space address. I'm open to suggestions of the
>> >> > > best way to structure this.
>> >> > 
>> >> > Haven't put a ton of thought into it, but what about something like this
>> >> > (untested):
>> >> > 
>> >> > uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr)
>> >> > {
>> >> > #ifdef CONFIG_SOFTMMU
>> >> >     if (haddr) {
>> >> >         if (!haddr->is_io) {
>> >> >             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 block->offset + offset + block->mr->addr;
>> >> >         } else {
>> >> >             MemoryRegionSection *mrs = haddr->v.io.section;
>> >> >             return haddr->v.io.offset + mrs->mr->addr;
>> >> >         }
>> >> >     }
>> >> > #endif
>> >> >     return 0;
>> >> > }
>> >> 
>> >> This appears to successfully return correct physical addresses for RAM
>> >> at least, though I've not tested it thoroughly for MMIO yet.
>> >> 
>> >> If it ends up being desirable based on the discussion elsewhere on this
>> >> thread I am willing to perform more complete testing, turn this into a
>> >> patch, and submit it.
>> >
>> > Ping - Is this something worth me pursuing?
>> 
>> Yes please. 
>
> Okay, I'll work on it. Is your thinking that this would this be a
> separate call as shown above, or a replacement of the existing
> qemu_plugin_hwaddr_device_offset function? And, if a replacement, should
> we keep the name similar to retain compatibility, or make a clean break?
>
> It seemed like Peter may have been saying the device offset shouldn't be
> exposed at all (leading me to consider full replacement), but I also
> don't see a definitive resolution of that conversation.

I think a full replacement and an increment of the minimum API version.
That way people will at least query why the plugin failed to load and
hopefully will read the release notes ;-)

>
> -Aaron


-- 
Alex Bennée