Re: [Qemu-devel] [PATCH] RFC: vmcoreinfo device

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 26 May 2017 13:59:09 +0000
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, May 4, 2017 at 5:41 PM Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 02 May 2017 19:03:15 +0000
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >  
> > > Hi
> > >
> > > On Tue, May 2, 2017 at 11:17 AM Igor Mammedov <imammedo@redhat.com>  
> > wrote:  
> > >  
> > > > On Fri, 28 Apr 2017 14:28:38 +0000
> > > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > > >  
> > > > > Hi
> > > > >
> > > > > On Fri, Apr 28, 2017 at 6:12 PM Ladi Prosek <lprosek@redhat.com>  
> > wrote:  
> > > > >  
> > > > > > On Mon, Apr 24, 2017 at 3:03 PM, Marc-André Lureau
> > > > > > <marcandre.lureau@redhat.com> wrote:  
> > > > > > > The VM coreinfo (vmcoreinfo) device is an emulated device which
> > > > > > > exposes a 4k memory range to the guest to store various  
> > informations  
> > > > > > > useful to debug the guest OS. (it is greatly inspired by the  
> > VMGENID  
> > > > > > > device implementation)
> > > > > > >
> > > > > > > This is an early-boot alternative to the qemu-ga VMDUMP_INFO  
> > event  
> > > > > > > proposed in "[PATCH 00/21] WIP: dump: add kaslr support".
> > > > > > >
> > > > > > > If deemed more appropriate, we can consider writing to fw_cfg  
> > > > directly  
> > > > > > > instead of guest memory, now that qemu 2.9 supports it again.
> > > > > > >
> > > > > > > The proof-of-concept kernel module:
> > > > > > >  
> > https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c  
> > > > > >
> > > > > > Here's a proof-of-concept Windows driver:
> > > > > >
> > > > > >  
> > > >  
> > https://github.com/ladipro/kvm-guest-drivers-windows/tree/vmcoreinfo/vmcoreinfo  
> > > > > >
> > > > > > I just wanted to be sure that it's possible to evaluate the ADDR
> > > > > > method in Windows.
> > > > > >
> > > > > > From a practical point of view it is unfortunate that this would  
> > be a  
> > > > > > completely new device. For Windows guests it means another driver
> > > > > > binary and all the overhead associated with deploying it on VMs.  
> > Would  
> > > > > > it be too crazy to add this functionality to the pvpanic device?  
> > The  
> > > > > > mechanics could stay the same but it would be done under the  
> > existing  
> > > > > > ACPI\QEMU0001 device.
> > > > > >  
> > > > >
> > > > > pvpanic is under _SB.PCI0.ISA, that could be problematic
> > > > >
> > > > > and _STA is a name field.
> > > > >
> > > > > Someone with more experience with ACPI could tell us if that make  
> > sense  
> > > > to  
> > > > > merge both and how.
> > > > >
> > > > > Can't you handle 2 ACPI devices in the same windows driver instead?  
> > > > we use QEMU0001 to reserve IO ports for pvpanic device,
> > > > ASL wise there shouldn't problems with adding _ADDR method to it
> > > >
> > > > but then we probably should fold vmcoreinfo into pvpanic device
> > > > as well (QEMU and linux driver)
> > > >
> > > >  
> > > pvpanic is x86-only afaict.  
> > There is nothing that forces it to be x86 specific (beside being ISA
> > device),
> > ARM also can benefit from/use pvpanic if you make it pci device or just
> > plain
> > Device.
> >  
> 
> Could someone advise on a recommended solution here?
> 
> Should we make a PCI variant of pvpanic and add the proposed vmcoreinfo
> ACPI/mem to it ?
> 
> Tbh, I think I prefer to have seperate devices since they work differently
> and vmcoreinfo doesn't require any bus device.
Considering pvpanic and vmcore are closely related, I'd prefer
it being one device. But I won't insist on it.

Also instead of PCI device, I'd use plain Device as vmgenid does,
that would leave open question how to make legacy ISA based pvpanic
on top of it in case both are merged.

> 
> thanks
> 
> >  
> > > While I think vmcoreinfo would work fine with
> > > any acpi-able arch.  
> > I don't insist on it but it's worth a try, probably a lot of code could be
> > shared between both (including AML part/which makes DSDT smaller little
> > bit)
> >
> >  
> > > I think I would rather modify the windows driver to support both pvpanic  
> > &  
> > > vmcoreinfo, even if it's not typical for driver to implement several
> > > devices.
> > >  
> > > >  
> > > > > > +Storage Format:  
> > > > > > > +---------------
> > > > > > > +
> > > > > > > +The content is expected to use little-endian format.
> > > > > > > +
> > > > > > > +In order to implement an OVMF "SDT Header Probe Suppressor", the  
> > > > > > contents of  
> > > > > > > +the vmcoreinfo blob has 40 bytes of padding:
> > > > > > > +
> > > > > > > ++-----------------------------------+
> > > > > > > +| SSDT with OEM Table ID = VMCOREIN |
> > > > > > > ++-----------------------------------+
> > > > > > > +| ...                               |       TOP OF PAGE
> > > > > > > +| VCIA dword object ----------------|----->  
> > > > > > +---------------------------+  
> > > > > > > +| ...                               |       | fw-allocated  
> > array for  
> > > > > > |  
> > > > > > > +| _STA method referring to VCIA     |       | "etc/vmcoreinfo"  
> > > > > > |  
> > > > > > > +| ...                               |  
> > > > > >  +---------------------------+  
> > > > > > > +| ADDR method referring to VCIA     |       |  0: OVMF SDT  
> > Header  
> > > > probe  
> > > > > > |  
> > > > > > > +| ...                               |       |     suppressor  
> > > > > > |  
> > > > > > > ++-----------------------------------+       | 40: uint32 version  
> > > > field  
> > > > > > |  
> > > > > > > +                                            | 44: info contents  
> > > > > >  |  
> > > > > > > +                                            |     ....  
> > > > > > |  
> > > > > > > +  
> > > > > > +---------------------------+  
> > > > > > > +                                            END OF PAGE
> > > > > > > +
> > > > > > > +Version 0 content:
> > > > > > > +
> > > > > > > + uint64 paddr:
> > > > > > > +  Physical address of the Linux vmcoreinfo ELF note.  
> > > > > >
> > > > > > Or physical address of the Windows crash dump header :p
> > > > > >  
> > > > >
> > > > > Is there support for Windows crash dump in qemu?
> > > > >
> > > > >  
> > > > > > Do we want to have an additional discriminator field to tell what  
> > kind  
> > > > > > of information was written by the guest or would Windows use a
> > > > > > different version?
> > > > > >
> > > > > >  
> > > > > I guess a different version would be ok.
> > > > >
> > > > > Thanks a lot for looking at it!  
> > > >
> > > > --  
> > > Marc-André Lureau  
> >
> > --  
> Marc-André Lureau