Plugin virtual-to-physical translation incorrect for some IO accesses

Plugin virtual-to-physical translation incorrect for some IO accesses

[Aaron Lindsay]

Hello,

I previously supplied a patch which modified the plugin interface such
that it will return physical addresses for IO regions [0]. However, I
have now found a case where the interface does not appear to correctly
return the full physical addresses.

In particular, when in qemu_plugin_hwaddr_phys_addr() for a particular
store to IO memory (haddr->is_io==true), I find that haddr->v.io.offset
is 0x0 and mrs->mr->addr is 0x3000, meaning 0x3000 is the returned
"physical address". However, I also find that
mrs->offset_within_address_space is 0x8000007000 (and also that
0x8000007000 matches up with what an actual translation would be from
inspecting the page tables).

Would it be 'safe' to *always* begin using
mrs->offset_within_address_space as the returned physical address here
instead of `haddr->v.io.offset + mrs->mr->addr`, or is there a reason we
should not do that?

Thanks!

-Aaron

[0] https://lists.nongnu.org/archive/html/qemu-devel/2021-03/msg03137.html

Re: Plugin virtual-to-physical translation incorrect for some IO accesses

[Philippe Mathieu-Daudé]

+Peter/Paolo

On 7/6/21 10:47 PM, Aaron Lindsay wrote:
> Hello,
> 
> I previously supplied a patch which modified the plugin interface such
> that it will return physical addresses for IO regions [0]. However, I
> have now found a case where the interface does not appear to correctly
> return the full physical addresses.
> 
> In particular, when in qemu_plugin_hwaddr_phys_addr() for a particular
> store to IO memory (haddr->is_io==true), I find that haddr->v.io.offset
> is 0x0 and mrs->mr->addr is 0x3000, meaning 0x3000 is the returned
> "physical address". However, I also find that
> mrs->offset_within_address_space is 0x8000007000 (and also that
> 0x8000007000 matches up with what an actual translation would be from
> inspecting the page tables).
> 
> Would it be 'safe' to *always* begin using
> mrs->offset_within_address_space as the returned physical address here
> instead of `haddr->v.io.offset + mrs->mr->addr`, or is there a reason we
> should not do that?

'safety' is not my area, but using mrs->offset_within_address_space
sounds correct to me.

> Thanks!
> 
> -Aaron
> 
> [0] https://lists.nongnu.org/archive/html/qemu-devel/2021-03/msg03137.html
> 

Re: Plugin virtual-to-physical translation incorrect for some IO accesses

[Aaron Lindsay via]

On Jul 06 23:10, Philippe Mathieu-Daudé wrote:
> +Peter/Paolo
> 
> On 7/6/21 10:47 PM, Aaron Lindsay wrote:
> > Hello,
> > 
> > I previously supplied a patch which modified the plugin interface such
> > that it will return physical addresses for IO regions [0]. However, I
> > have now found a case where the interface does not appear to correctly
> > return the full physical addresses.
> > 
> > In particular, when in qemu_plugin_hwaddr_phys_addr() for a particular
> > store to IO memory (haddr->is_io==true), I find that haddr->v.io.offset
> > is 0x0 and mrs->mr->addr is 0x3000, meaning 0x3000 is the returned
> > "physical address". However, I also find that
> > mrs->offset_within_address_space is 0x8000007000 (and also that
> > 0x8000007000 matches up with what an actual translation would be from
> > inspecting the page tables).
> > 
> > Would it be 'safe' to *always* begin using
> > mrs->offset_within_address_space as the returned physical address here
> > instead of `haddr->v.io.offset + mrs->mr->addr`, or is there a reason we
> > should not do that?

I realized this was perhaps not clear, so for clarification, I am
imagining the formula for calculating the address would be:
`mrs->offset_within_address_space + mrs->mr->addr`. Perhaps this was a
confusing example since the offset into the region is 0x0...

> 'safety' is not my area, but using mrs->offset_within_address_space
> sounds correct to me.

I'm not sure I was as clear as I could be here, either. My primary
concern was/is correctness of the address calculation, so perhaps 'safe'
was not the right way to put this.

-Aaron

Re: Plugin virtual-to-physical translation incorrect for some IO accesses

[Philippe Mathieu-Daudé]

On 7/6/21 11:56 PM, Aaron Lindsay wrote:
> On Jul 06 23:10, Philippe Mathieu-Daudé wrote:
>> +Peter/Paolo
>>
>> On 7/6/21 10:47 PM, Aaron Lindsay wrote:
>>> Hello,
>>>
>>> I previously supplied a patch which modified the plugin interface such
>>> that it will return physical addresses for IO regions [0]. However, I
>>> have now found a case where the interface does not appear to correctly
>>> return the full physical addresses.
>>>
>>> In particular, when in qemu_plugin_hwaddr_phys_addr() for a particular
>>> store to IO memory (haddr->is_io==true), I find that haddr->v.io.offset
>>> is 0x0 and mrs->mr->addr is 0x3000, meaning 0x3000 is the returned
>>> "physical address".

v.io.offset is filled with iotlb_to_section() which use
AddressSpaceDispatch:

MemoryRegionSection *iotlb_to_section(CPUState *cpu,
                                      hwaddr index, MemTxAttrs attrs)
{
    int asidx = cpu_asidx_from_attrs(cpu, attrs);
    CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
    AddressSpaceDispatch *d = qatomic_rcu_read(&cpuas->memory_dispatch);
    MemoryRegionSection *sections = d->map.sections;

    return &sections[index & ~TARGET_PAGE_MASK];
}

IIUC AddressSpaceDispatch is already adapted from the flatview to the
CPU (AS view). So v.io.offset is relative to each CPUAddressSpace.

Assuming an ARM Cortex-M core having it's secure world mapped at
0x8000000000 and non-secure mapped at 0x0000000000, the QEMU cpu
will have 2 CPUAddressSpaces, each CPUAddressSpace root MemoryRegion
is zero-based.

IOW the iotlb_to_section() API return you the relative offset (to the
CPUAddressSpace), not absolute (based on your expected 0x8000000000).

> However, I also find that
>>> mrs->offset_within_address_space is 0x8000007000 (and also that
>>> 0x8000007000 matches up with what an actual translation would be from
>>> inspecting the page tables).
>>>
>>> Would it be 'safe' to *always* begin using
>>> mrs->offset_within_address_space as the returned physical address here
>>> instead of `haddr->v.io.offset + mrs->mr->addr`, or is there a reason we
>>> should not do that?
> 
> I realized this was perhaps not clear, so for clarification, I am
> imagining the formula for calculating the address would be:
> `mrs->offset_within_address_space + mrs->mr->addr`. Perhaps this was a
> confusing example since the offset into the region is 0x0...

Yes, however remember this won't be the absolute address from the CPU
view, but the absolute address from address space (think of physical
bus) view. For example for a PCI BAR, this won't be the physical address
mapped on the CPU view, but the physical address on the PCI bus.

>> 'safety' is not my area, but using mrs->offset_within_address_space
>> sounds correct to me.
> 
> I'm not sure I was as clear as I could be here, either. My primary
> concern was/is correctness of the address calculation, so perhaps 'safe'
> was not the right way to put this.
> 
> -Aaron
> 

Re: Plugin virtual-to-physical translation incorrect for some IO accesses

[Aaron Lindsay via]

On Jul 07 09:53, Philippe Mathieu-Daudé wrote:
> On 7/6/21 11:56 PM, Aaron Lindsay wrote:
> > On Jul 06 23:10, Philippe Mathieu-Daudé wrote:
> >> +Peter/Paolo
> >>
> >> On 7/6/21 10:47 PM, Aaron Lindsay wrote:
> >>> Hello,
> >>>
> >>> I previously supplied a patch which modified the plugin interface such
> >>> that it will return physical addresses for IO regions [0]. However, I
> >>> have now found a case where the interface does not appear to correctly
> >>> return the full physical addresses.
> >>>
> >>> In particular, when in qemu_plugin_hwaddr_phys_addr() for a particular
> >>> store to IO memory (haddr->is_io==true), I find that haddr->v.io.offset
> >>> is 0x0 and mrs->mr->addr is 0x3000, meaning 0x3000 is the returned
> >>> "physical address".
> 
> v.io.offset is filled with iotlb_to_section() which use
> AddressSpaceDispatch:
> 
> MemoryRegionSection *iotlb_to_section(CPUState *cpu,
>                                       hwaddr index, MemTxAttrs attrs)
> {
>     int asidx = cpu_asidx_from_attrs(cpu, attrs);
>     CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
>     AddressSpaceDispatch *d = qatomic_rcu_read(&cpuas->memory_dispatch);
>     MemoryRegionSection *sections = d->map.sections;
> 
>     return &sections[index & ~TARGET_PAGE_MASK];
> }
> 
> IIUC AddressSpaceDispatch is already adapted from the flatview to the
> CPU (AS view). So v.io.offset is relative to each CPUAddressSpace.
> 
> Assuming an ARM Cortex-M core having it's secure world mapped at
> 0x8000000000 and non-secure mapped at 0x0000000000, the QEMU cpu
> will have 2 CPUAddressSpaces, each CPUAddressSpace root MemoryRegion
> is zero-based.
> 
> IOW the iotlb_to_section() API return you the relative offset (to the
> CPUAddressSpace), not absolute (based on your expected 0x8000000000).
> 
> > However, I also find that
> >>> mrs->offset_within_address_space is 0x8000007000 (and also that
> >>> 0x8000007000 matches up with what an actual translation would be from
> >>> inspecting the page tables).
> >>>
> >>> Would it be 'safe' to *always* begin using
> >>> mrs->offset_within_address_space as the returned physical address here
> >>> instead of `haddr->v.io.offset + mrs->mr->addr`, or is there a reason we
> >>> should not do that?
> > 
> > I realized this was perhaps not clear, so for clarification, I am
> > imagining the formula for calculating the address would be:
> > `mrs->offset_within_address_space + mrs->mr->addr`. Perhaps this was a
> > confusing example since the offset into the region is 0x0...
> 
> Yes, however remember this won't be the absolute address from the CPU
> view, but the absolute address from address space (think of physical
> bus) view. For example for a PCI BAR, this won't be the physical address
> mapped on the CPU view, but the physical address on the PCI bus.

I believe I want the CPU view here (i.e. I want the physical address
that would have been returned from a page table walk by the CPU for this
access). Given that, I think what I'm hearing is that
mrs->offset_within_address_space is *not* what I want (even though it
appears to be in this case, since they happen to align). But also that
v.io.offset is not sufficient without first adding an offset for the
address space into which the access is being made.

Do I have that right? If so, can you point me in the right direction for
getting back to the address space correctly?

Alex, I seem to recall you mentioned maybe wanting the plugins to know
more about address spaces when I posted the original patch. At the time,
I think I understood the concern to be mostly that the plugins may want
to know which address space an access was to, not that it may be
interfering with our ability to return correct addresses (at least as
the CPU understands them). My initial thoughts are that we could adjust
the address here for the address space without necessarily reporting it.
Do you have thoughts about this?

-Aaron

Re: Plugin virtual-to-physical translation incorrect for some IO accesses

[Aaron Lindsay via]

On Jul 07 07:35, Aaron Lindsay wrote:
> On Jul 07 09:53, Philippe Mathieu-Daudé wrote:
> > On 7/6/21 11:56 PM, Aaron Lindsay wrote:
> > > On Jul 06 23:10, Philippe Mathieu-Daudé wrote:
> > >> +Peter/Paolo
> > >>
> > >> On 7/6/21 10:47 PM, Aaron Lindsay wrote:
> > >>> Hello,
> > >>>
> > >>> I previously supplied a patch which modified the plugin interface such
> > >>> that it will return physical addresses for IO regions [0]. However, I
> > >>> have now found a case where the interface does not appear to correctly
> > >>> return the full physical addresses.
> > >>>
> > >>> In particular, when in qemu_plugin_hwaddr_phys_addr() for a particular
> > >>> store to IO memory (haddr->is_io==true), I find that haddr->v.io.offset
> > >>> is 0x0 and mrs->mr->addr is 0x3000, meaning 0x3000 is the returned
> > >>> "physical address".
> > 
> > v.io.offset is filled with iotlb_to_section() which use
> > AddressSpaceDispatch:
> > 
> > MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> >                                       hwaddr index, MemTxAttrs attrs)
> > {
> >     int asidx = cpu_asidx_from_attrs(cpu, attrs);
> >     CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
> >     AddressSpaceDispatch *d = qatomic_rcu_read(&cpuas->memory_dispatch);
> >     MemoryRegionSection *sections = d->map.sections;
> > 
> >     return &sections[index & ~TARGET_PAGE_MASK];
> > }
> > 
> > IIUC AddressSpaceDispatch is already adapted from the flatview to the
> > CPU (AS view). So v.io.offset is relative to each CPUAddressSpace.

What does CPUAddressSpace represent here? In my initial reading, I
assumed there might be one CPUAddressSpace for secure and one for
non-secure in the ARM world. But from my observation so far, v.io.offset
seems to be an offset relative to the beginning of a given memory region
(i.e. one device's portion of the memory map), rather than to the
address space as a whole (in terms of S/NS).

> > Assuming an ARM Cortex-M core having it's secure world mapped at
> > 0x8000000000 and non-secure mapped at 0x0000000000, the QEMU cpu
> > will have 2 CPUAddressSpaces, each CPUAddressSpace root MemoryRegion
> > is zero-based.
> > 
> > IOW the iotlb_to_section() API return you the relative offset (to the
> > CPUAddressSpace), not absolute (based on your expected 0x8000000000).
> > 
> > > However, I also find that
> > >>> mrs->offset_within_address_space is 0x8000007000 (and also that
> > >>> 0x8000007000 matches up with what an actual translation would be from
> > >>> inspecting the page tables).
> > >>>
> > >>> Would it be 'safe' to *always* begin using
> > >>> mrs->offset_within_address_space as the returned physical address here
> > >>> instead of `haddr->v.io.offset + mrs->mr->addr`, or is there a reason we
> > >>> should not do that?
> > > 
> > > I realized this was perhaps not clear, so for clarification, I am
> > > imagining the formula for calculating the address would be:
> > > `mrs->offset_within_address_space + mrs->mr->addr`. Perhaps this was a
> > > confusing example since the offset into the region is 0x0...

Whoops, I replaced the wrong term in my clarification. What I really,
really meant was:

`mrs->offset_within_address_space + haddr->v.io.offset`

> > Yes, however remember this won't be the absolute address from the CPU
> > view, but the absolute address from address space (think of physical
> > bus) view. For example for a PCI BAR, this won't be the physical address
> > mapped on the CPU view, but the physical address on the PCI bus.
> 
> I believe I want the CPU view here (i.e. I want the physical address
> that would have been returned from a page table walk by the CPU for this
> access). Given that, I think what I'm hearing is that
> mrs->offset_within_address_space is *not* what I want (even though it
> appears to be in this case, since they happen to align). But also that
> v.io.offset is not sufficient without first adding an offset for the
> address space into which the access is being made.
> 
> Do I have that right? If so, can you point me in the right direction for
> getting back to the address space correctly?
> 
> Alex, I seem to recall you mentioned maybe wanting the plugins to know
> more about address spaces when I posted the original patch. At the time,
> I think I understood the concern to be mostly that the plugins may want
> to know which address space an access was to, not that it may be
> interfering with our ability to return correct addresses (at least as
> the CPU understands them). My initial thoughts are that we could adjust
> the address here for the address space without necessarily reporting it.
> Do you have thoughts about this?
> 
> -Aaron