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

[Aaron Lindsay]

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