Re: [Qemu-devel] [PATCH] hmp: Improve 'info mtree' with optional parm for mapinfo

[Laszlo Ersek]

On 09/20/16 02:16, Thorsten Kohfeldt wrote:
> 
> Am 15.09.2016 um 11:52 schrieb Paolo Bonzini:
>>
>> On 07/09/2016 02:48, Thorsten Kohfeldt wrote:
>>> From: Thorsten Kohfeldt <address@hidden>
>>> Date: Wed, 31 Aug 2016 22:43:14 +0200
>>> Subject: [PATCH] hmp: Improve 'info mtree' with optional parm for
>>> mapinfo
>>>
>>> Motivation
>>> When 'tuning' 'quirks' for VFIO imported devices, it is not easy to
>>> directly grasp the implications of the priorisation algorithms in place
>>> for the 'layered mapping' of memory regions.
>>> Even though there are rules (documented in docs/memory.txt), once in a
>>> while one might question the correctness of the actual implementation of
>>> the rules.
>>> Particularly, I believe I have uncovered a divergence of (sub-)region
>>> priorisation/order/visibility in a corner case of importing a device
>>> (which requires a 'quirk') with mmap enabled vs. mmap disabled.
>>> This modification provides a means of visualising the ACTUAL
>>> mapping/visibility/occlusion of subregions within regions, whereas the
>>> current info mtree command only lists the tree of regions (all, visible
>>> and invisible ones).
>>> It is primarily intended to provide support for easy presentation of my
>>> cause, but I strongly believe this modification also has general purpose
>>> advantages.
>>
>> It is a useful addition, but I think a simpler presentation is also
>> fine.  What about "info mtree -f" which would visit the FlatView instead
>> of the tree?  The patch would probably be much shorter.
>>
>> Thanks,
>>
>> Paolo
> 
> Paolo,
> 
> For quite some time I had the patch in use as a direct modification of
> 'info mtree', but I felt that a general purpose use must provide an ad
> hoc user selectable presentation width parameter for the map info.
> I personally use a width of 65 or even 129 characters PREFIXING the
> tree elements which the command currently responds.
> My guess is though that most users would want a width of only 9 or 17.
> So I believe that a numerical parameter is a must.
> Visit the flat view -
> I'm not sure I understand you. Do you suggest to traverse a completely
> different data structure ?
> The purpose of the suggested visualisation is to point out where
> each of the tree's memory regions are "pinched" by other regions, so
> their "native" contents is NOT visible any more throughout the full
> region length, but (fractionally) rather another regions's content.
> Thus, I personally require to traverse exactly that tree structure.
> 
> No offence, but I would rather not want to modify the patch towards
> what I feel would be a completely different purpose.
> 
> I would appreciate if someone would review the patch in its current
> functional form to get it queued for qemu 2.8.
> My intention is to be able to rely on communication partners being
> able to reproduce findings using the new command once I start to
> attack the VFIO mmap flaw I talk about in the commit comment.
> 
> 
> @ALL
> I have provided 2 patch branches in github,
> one for qemu-2.7.0 and
> one for qemu-current-master (this needed a tiny sed-conversion, see below).
> I also placed some example info mtree mapinfo output on gist.github:
> 
> 
> # patch commit for qemu-2.7 (same patch also works for qemu-2.6):
> https://github.com/Thorsten-Kohfeldt/qemu/commit/5633a3cdbf6fd7cccd098fb83f591fbb15e8d383
> 
> # in branch:
> https://github.com/Thorsten-Kohfeldt/qemu/commits/monitor_--hmp_info_mtree_mapinfo-width
> 
> 
> # PATCH (as published in mailing list) *CONVERSION* from
> qemu-2.6/qemu-2.7 to qemu-master:
> sed s/'[.]mhandler[.]cmd = '/'.cmd        = '/ <qemu-2.6and7.patch
>>qemu-master.patch
> 
> # patch commit adapted that way for qemu-master:
> https://github.com/Thorsten-Kohfeldt/qemu/commit/60b8c1be1a0119df1f1859b0d484e06e709c2ea2
> 
> # in branch:
> https://github.com/Thorsten-Kohfeldt/qemu/commits/upstream-pullrequest-mapinfo-V1
> 
> 
> 
> # sample output info mtree 9
> https://gist.github.com/Thorsten-Kohfeldt/254b5a21fef497054959f58af53a44c9
> 
> # sample output info mtree 65
> https://gist.github.com/Thorsten-Kohfeldt/39cf5c8521c1999518b3438315e439f4

I think your current output explains, for each part (that is, each "sample" of 
user-selected size) of each MemoryRegion, whether that sample is actually 
visible in the finally rendered, flat view of the address space(s). (And, if 
not, why not.)

Whereas I think Paolo's idea is to map the flat view to begin with, and 
visually associate each interval of the guest visible physical address space 
with the one region that is ultimately accessed in that interval. This too 
would explain what the guest sees where, and why. It wouldn't give much 
information about ranges that the guest can *not* access (due to occlusion by 
other regions or otherwise), but maybe the "why not" is not so important after 
all?

Regarding the FlatView thing -- QEMU already maintains a flat rendering of the 
memory region tree, so that guest accesses to the various regions can be 
quickly dispatched to the accessors that can serve these accesses. Memory 
regions are massaged (created, deleted, enabled/disabled etc) in 
"transactions", and when the outermost transaction is committed, the flat-view 
is re-rendered (which is expensive).

(I'm not an expert on this, so my explanation may be inaccurate.)

I think it would make sense to work from the pre-rendered FlatView, if the 
information you can get out of it covers your needs.

Here's an example, from one of your sample outputs: you currently have

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    
00000000000c0000-00000000000c3fff (prio 1, RW): alias pam-ram @pc.ram 
00000000000c0000-00000000000c3fff [disabled]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    
00000000000c0000-00000000000c3fff (prio 1, RW): alias pam-pci @pc.ram 
00000000000c0000-00000000000c3fff [disabled]
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@    
00000000000c0000-00000000000c3fff (prio 1, R-): alias pam-rom @pc.ram 
00000000000c0000-00000000000c3fff
ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo    
00000000000c0000-00000000000c3fff (prio 1, RW): alias pam-pci @pci 
00000000000c0000-00000000000c3fff [disabled]

with

@: alias region mapped at sample
~: alias region mappable but disabled at sample
o: region occluded by some other region at sample

If you have an address in the c0000-c3fff range, you have to consult all four 
lines to see which region will match.

Working off of the FlatView, first I think you would find the right resolution 
for the output "for free" (you wouldn't need a user sample size -- the interval 
c0000-c3fff would neither need further subdivision nor be blurred by 
over-coarse resolution). You could represent the c0000-c3fff interval (and 
every other interval too) with a single letter, such as P, where P would stand 
for "alias pam-rom @pc.ram 00000000000c0000-00000000000c3fff".

Second, given an address in c0000-c3fff, there would be only one range to 
consult (same as QEMU itself does with FlatView), and you'd find the 
MemoryRegion visible in that range at once.

... I hope Paolo will correct me if I misunderstood his suggestion.

Thanks
Laszlo