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

[Aaron Lindsay]

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