Re: [PATCH v2 3/7] ACPI ERST: support for ACPI ERST feature

[Igor Mammedov]

On Fri, 9 Apr 2021 15:54:47 +0000
Eric DeVolder <eric.devolder@oracle.com> wrote:

> Hi Igor,
> Thank you for reviewing. I've responded inline below.
> eric
> 
> ________________________________
> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Tuesday, April 6, 2021 2:31 PM
> To: Eric DeVolder <eric.devolder@oracle.com>
> Cc: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com 
> <marcel.apfelbaum@gmail.com>; pbonzini@redhat.com <pbonzini@redhat.com>; 
> rth@twiddle.net <rth@twiddle.net>; ehabkost@redhat.com <ehabkost@redhat.com>; 
> qemu-devel@nongnu.org <qemu-devel@nongnu.org>; Boris Ostrovsky 
> <boris.ostrovsky@oracle.com>; kwilk@oracle.com <kwilk@oracle.com>
> Subject: Re: [PATCH v2 3/7] ACPI ERST: support for ACPI ERST feature
> 
> On Mon,  8 Feb 2021 15:57:55 -0500
> Eric DeVolder <eric.devolder@oracle.com> wrote:
> 
> > This change implements the support for the ACPI ERST feature[1,2].
> >
> > The size of the ACPI ERST storage is declared via the QEMU
> > global parameter acpi-erst.size. The size can range from 64KiB
> > to to 64MiB. The default is 64KiB.
> >
> > The location of the ACPI ERST storage backing file is delared
> > via the QEMU global parameter acpi-erst.filename. The default
> > is acpi-erst.backing.
> >
> > [1] "Advanced Configuration and Power Interface Specification",
> >     version 6.2, May 2017.
> >     https://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
> >
> > [2] "Unified Extensible Firmware Interface Specification",
> >     version 2.8, March 2019.
> >     https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
> >
> > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>  
> 
> items 2/4/5 from v1 review still need to be addressed.
> 
> >
> > 2. patch is too big to review, please split it up in smaller chunks.
> >
> > EJD: Done.  
> 
> (separating a header and a makefile rule doesn't make much sense)
> 
> it should be split at least on part that implements device model and ACPI 
> parts
> 
> EJD: I'll rebase this patch set on qemu-6 and accommodate your suggestions 
> with how to split/organize the patch set.
> 
> [...]
> >
> > 4. Maybe instead of SYSBUS device, implement it as a PCI device and
> >    use its BAR/control registers for pstore storage and control interface.
> >    It could save you headache of picking address where to map it +
> >    it would take care of migration part automatically, as firmware
> >    would do it for you and then QEMU could pickup firmware programmed
> >    address and put it into ERST table.
> > EJD: Thanks for the idea. For now I've left it as a SYSBUS device; we can 
> > revisit as needed.  
> 
> I would really prefer to see a PCI version (current way is just a hack)
> 
> EJD: I understand, I don't like the base address problem either. Is there an 
> example PCI device that gets its base address assigned during ACPI setup that 
> I could reference and pattern this work after? I've been using SYSBUS as that 
> most closely mimics the real hardware implementations I've studied in order 
> to produce this code.
> EJD: I thought my inexperience with authoring QEMU devices was the primary 
> problem in establishing a solution for the base address. Otherwise, this 
> thing only needs a single 4KiB page (for the 2 registers + exchange buffer) 
> exposed.

I don't recall if we merged example PCI device in QEMU, but someone worked on 
it before.
Google search yields following:
 
https://github.com/grandemk/qemu_devices/commit/ba8d38a858ba63ef4d419a926f58328b9675fc98


> > 5. instead of dealing with file for storage directly, reuse hostmem backend
> >    to provide it to for your device. ex: pc-dimm. i.e. split device
> >    on frontend and backend
> >
> > EJD: I had looked into that prior to posting v1. The entire ERST storage is 
> > not memory mapped, just an exchange buffer. So the hostmem backend is not 
> > suitable for this purpose.  
> 
> Is there a compelling reason why it can't be memory mapped?
> 
> EJD: Well, this ERST device I've coded pretty much follows the ACPI ERST spec 
> verbatim. As it stands today, the spec doesn't provide a way to report the 
> total size of the persistent storage behind the interface; you know when 
> storage is full only when you receive an Out Of Storage error code upon 
> write. In a sense, that allows the size of the storage to vary greatly and be 
> implemented in any way needed (ie actual hardware, this has tended to be in 
> the 64KiB range when it is carved out of system parallel flash memory, but 
> some hardware uses serial flash as well). In virtual environments, it can be 
> of any size, and we at Oracle have intentions of heavily utilizing ACPI ERST 
> to stuff all kinds of diagnostic information into it, thus wanting the 
> storage to be very large. By not actually exposing/memory-mapping the 
> storage, the issue of where to drop it in the memory map goes away (yes a PCI 
> BAR could solve this).
> EJD: But at the end of the day, could this storage be memory mapped? I 
> suppose it could be, but then that rather circumvents the entire need for the 
> ACPI ERST interface to start with. Linux and Windows both already know how to 
> utilize ACPI ERST.

Maybe I wasn't clear on it, I did not propose to map storage into guest.
Only use MemoryRegion provided by backend inside of your device.
This way you can drop all file related code from your patch,
and just work with read/store info from/to memory directly.

[...]