Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support

[Laszlo Ersek]

On 01/18/16 17:31, Andrew Jones wrote:
> On Thu, Jan 14, 2016 at 05:24:23PM +0100, Laszlo Ersek wrote:
>> On 01/14/16 09:48, Janosch Frank wrote:
>>> The dump guest memory script for extracting a Linux core from a qemu
>>> core is currently limited to amd64 and python 2.
>>>
>>> With this series we add support for python 3 (while maintaining python
>>> 2 support) and add the possibility to extract dumps from VMs with the
>>> most common architectures.
>>>
>>> This was tested on a s390 s12 guest only, I'd appreciate tests for the
>>> other architectures.
>>>
>>> Janosch Frank (5):
>>>   scripts/dump-guest-memory.py: Move constants to the top
>>>   scripts/dump-guest-memory.py: Make methods functions
>>>   scripts/dump-guest-memory.py: Improve python 3 compatibility
>>>   scripts/dump-guest-memory.py: Cleanup functions
>>>   scripts/dump-guest-memory.py: Introduce multi-arch support
>>>
>>>  scripts/dump-guest-memory.py | 717 
>>> +++++++++++++++++++++++++++----------------
>>>  1 file changed, 453 insertions(+), 264 deletions(-)
>>>
>>
>> So, I had a few notes for patches 1-4, but those are just insignificant
>> nits, so address them or not, I'm fine.
>>
>> Also, I'm not a Python programmer (you can probably tell from the
>> source). For every three lines I wrote for this script, I had to stare
>> at basic Python documentation, and PEP-8, for five minutes. :)
>>
>> Moving out a bunch of stuff to global namespace (from classes) in the
>> initial patches is fine I guess; but maybe keeping then in the class
>> helps with avoiding namespace collisions if a user loads other
>> extensions into gdb. IIRC that was my main motivation to keep those
>> things within the class. But, I don't feel strongly about this at all.
>>
>> Patch 5 is mostly over my head ("class ELF" --> Laszlo stops reading,
>> almost).
>>
>> I do notice that you import "ceil" from math, for a simple rounded-up
>> division. I think that's a bad idea (although I'm unsure about Python's
>> conversions between floating point and integers, and its floats in
>> general). Such rounding is not hard to do purely with integers; please
>> leave floating point out of the picture if possible.
>>
>> In any case, if you have kept the script working for the x86_64 target
>> (I trust you regression tested it), in patch 5, then I don't object,
>> generally speaking. I actually welcome the aarch64 addition.
>>
>> (Drew, can you perhaps check that out? IIRC you worked on the QMP
>> dump-guest-memory for aarch64.)
> 
> I gave this a test run on AArch64 (LE). It worked, thus
> 
> Tested-by: Andrew Jones <address@hidden>
> 
> 
> But the help text needs help. I'll paste the ones I think need changes
> here in order to point out my suggestions
> 
>>  raise gdb.GdbError("No valid arch type specified.\n"
>>                     "Currently supported types:"
>>                     "aarch64 be/le, X86_64, 386, s390, ppc64 be/le")
>                               ^ missing '-'                   ^ missing '-'
> 
> Actually it might be better to spell out aarch64-be, aarch64-le and
> ppc64-be, ppc64-le as well.
> 
>>  class DumpGuestMemory(gdb.Command):
>>      """Extract guest vmcore from qemu process coredump.
>>
>>  The sole argument is FILE, identifying the target file to write the
> 
> The two required arguments are FILE and ARCH. FILE identifies... ARCH
> selects the architecture for which the core will be generated.
> 
>>  guest vmcore to.
>>
>>  This GDB command reimplements the dump-guest-memory QMP command in
>>  python, using the representation of guest memory as captured in the qemu
>>  coredump. The qemu process that has been dumped must have had the
>>  command line option "-machine dump-guest-core=on".
> 
> Add one more sentence: "By default dump-guest-core is on."
> 
>>  
>>  For simplicity, the "paging", "begin" and "end" parameters of the QMP
>>  command are not supported -- no attempt is made to get the guest's
>>  internal paging structures (ie. paging=false is hard-wired), and guest
>>  memory is always fully dumped.
>>  
>>  Only x86_64 guests are supported.
> 
> aarch64-be, aarch64-le, X86_64, 386, s390, ppc64-be, ppc64-le guests are
> supported.
> 
>>  
>>  The CORE/NT_PRSTATUS and QEMU notes (that is, the VCPUs' statuses) are
>>  not written to the vmcore. Preparing these would require context that is
>>  only present in the KVM host kernel module when the guest is alive. A
>>  fake ELF note is written instead, only to keep the ELF parser of "crash"
>>  happy.
>>  
>>  Dependent on how busted the qemu process was at the time of the
>>  coredump, this command might produce unpredictable results. If qemu
>>  deliberately called abort(), or it was dumped in response to a signal at
>>  a halfway fortunate point, then its coredump should be in reasonable
>>  shape and this command should mostly work."""
> 
> 
> Additionally, as this was a pretty full rewrite of the script, then I
> think it warrants an additional Authors line under Laszlo's name.

Great points; thanks, Drew!
Laszlo

> 
> Thanks,
> drew
> 
> 
>>
>> So, for patches 1-4, with the nits fixed or not:
>>
>> Reviewed-by: Laszlo Ersek <address@hidden>
>>
>> For patch 5, *if* you remove floating point (--> math / ceil), *and* you
>> confirm that you regression-tested it for the x86_64 target (which
>> testing includes looking briefly, with the "crash" utility, at the
>> extracted kernel vmcore), then you can add my:
>>
>> Acked-by: Laszlo Ersek <address@hidden>
>>
>> Thanks
>> Laszlo
>>