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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>