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