Message-ID: <20161118033206.GA15698@danjae.aot.lge.com>
Date:   Fri, 18 Nov 2016 12:32:06 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        virtio-dev@...ts.oasis-open.org, Tony Luck <tony.luck@...el.com>,
        Kees Cook <keescook@...omium.org>, KVM <kvm@...r.kernel.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Anton Vorontsov <anton@...msg.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        qemu-devel <qemu-devel@...gnu.org>,
        Minchan Kim <minchan@...nel.org>,
        Anthony Liguori <aliguori@...zon.com>,
        Colin Cross <ccross@...roid.com>,
        virtualization@...ts.linux-foundation.org,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

Hi,

Thanks for your detailed information,

On Wed, Nov 16, 2016 at 07:10:36AM -0500, Paolo Bonzini wrote:
> > Not sure how independent ERST is from ACPI and other specs.  It looks
> > like referencing UEFI spec at least.
> 
> It is just the format of error records that comes from the UEFI spec
> (include/linux/cper.h) but you can ignore it, I think.  It should be
> handled by tools on the host side.  For you, the error log address
> range contains a CPER header followed by a binary blob.  In practice,
> you only need the record length field (bytes 20-23 of the header),
> though it may be a good idea to validate the signature at the beginning
> of the header.
> 
> > Btw, is the ERST used for pstore only (in Linux)?
> 
> Yes.  It can store various records, including dmesg and MCE.
> 
> There are other examples in QEMU of interfaces with ACPI.  They all use the
> DSDT, but the logic is similar.  For example, docs/specs/acpi_mem_hotplug.txt
> documents the memory hotplug interface. In all cases, ACPI tables contain small
> programs that talk to specialized hardware registers, typically allocated to
> hard-coded I/O ports.
> 
> In your case, the registers could occupy 16 consecutive I/O ports, like the
> following:
> 
>      0x00       read/write   operation type (0=write,1=read,2=clear,3=dummy write)
> 
>      0x01       read-only    bit 7: if set, operation in progress
> 
>                              bit 0-6: operation status, see "Command Status Definition" in
>                              the ACPI spec
> 
>      0x02       read-only    when read:
> 
>                              - read a 64-bit record id from the store to memory,
>                                from the address that was last written to 0x08.
> 
>                              - if the id is valid and is not the last id in the store,
>                                write the next 64-bit record id to the same address
> 
>                              - otherwise, write the first record id to the same address,
>                                or 0xffffffffffffffff if the store is empty
> 
>      0x03                    unused, read as zero
> 
>      0x04-0x07  read/write   offset of the error record into the error log address range
> 
>      0x08-0x0b  read/write   when read, return number of stored records
> 
>                              when written, the written value is a 32-bit memory address,
>                              which points to a 64-bit location used to communicate record ids.
> 
>      0x0c-0x0f  read/write   when read, always return -1 (together with the "mask" field
>                              and READ_REGISTER, this lets ERST instructions return any value!)
> 
>                              when written, trigger the pstore operation:
> 
>                              - if the current operation is a dummy write, do nothing
> 
>                              - if the current operation is a write, write a new record, using
>                              the written value as the base of the error log address range.  The
>                              length must be parsed from the CPER header.
> 
>                              - if the current operation is a clear, read the record id
>                              from the memory location that was last written to 0x08 and do the
>                              operation.  the value written is ignored.
> 
>                              - if the current operation is a read, read the record id from the
>                              memory location that was last written to 0x08, using the written
>                              value as the base of the error log address range.
> 
> In addition, the firmware will need to reserve a few KB of RAM for the error log
> address range (I checked a real system and it reserves 8KB).  The first eight
> bytes are needed for the record identifier interface, because there's no such
> thing as 64-bit I/O ports, and the rest can be used for the actual buffer.

Is there a limit on the size?  It'd be great if it can use a few MB..

> 
> QEMU already has an interface to allocate RAM and patch the address into an
> ACPI table (bios_linker_loader_alloc).  Because this interface is actually meant
> to load data from QEMU into the firmware (using the "fw_cfg" interface), you
> would have to add a dummy 8KB file to fw_cfg using fw_cfg_add_file (for
> example "etc/erst-memory"), it can be just full of zeros.
> 
> QEMU supports two chipsets, PIIX and ICH9, and the free I/O port ranges are
> different.  You could use 0xa20 for ICH9 and 0xae20 for PIIX.
> 
> All in all, the contents of the ERST table would not be very different from a
> non-virtual system, except that on real hardware the firmware would use SMIs
> as the trap mechanism.  You almost have a one-to-one mapping between ERST
> actions and registers accesses:
> 
>    BEGIN_WRITE_OPERATION                  write value 0 to register at 0x00
>    BEGIN_READ_OPERATION                   write value 1 to register at 0x00
>    BEGIN_CLEAR_OPERATION                  write value 2 to register at 0x00
>    BEGIN_DUMMY_WRITE_OPERATION            write value 3 to register at 0x00
>    END_OPERATION                          no-op
>    CHECK_BUSY_STATUS                      read register at 0x01 with mask 0x80
>    GET_COMMAND_STATUS                     read register at 0x01 with mask 0x7f
>    SET_RECORD_OFFSET                      write register at 0x04
>    GET_RECORD_COUNT                       read register at 0x08
>    EXECUTE_OPERATION                      write ERST memory base + 8 to 0x0c
>    GET_ERROR_LOG_ADDRESS_RANGE            read register at 0x0c (with mask = ERST memory base + 8)
>    GET_ERROR_LOG_ADDRESS_RANGE_LENGTH     read register at 0x0c (with mask = 8192 - 8 = 8184)
>    GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES read register at 0x0c (with mask = 0)
> 
> Only the get/set record identifier instructions are a little harder:
> 
>    GET_RECORD_IDENTIFIER                  write ERST memory base to register at 0x08
>                                           read register at 0x02
>                                           read eight bytes at ERST memory base
> 
>    SET_RECORD_IDENTIFIER                  write ERST memory base to register at 0x08
>                                           write eight bytes at ERST memory base
> 
> On top of this, you need to add the APEI UUID (see apei_osc_setup in Linux)
> to build_q35_osc_method, and use "-M q35" when you start QEMU.  If you need
> more help just ask.  I or others can help you with the ACPI glue, then you
> can write the file backend yourself, based on your existing virtio-pstore code.
> 
> > Also I need to control pstore driver like using bigger buffer,
> > enabling specific message types and so on if ERST supports.  Is it
> > possible for ERST to provide such information?
> 
> It's the normal pstore driver, same as on a real server.  What exactly do you
> need?

Well, I don't want to send additional pstore messages to the device if
it cannot handle them properly - for example, ftrace message should not
overwrite kmsg dump.  It'd be great if device somehow could expose
acceptable message types to the driver IMHO.

Btw I prefer using the kvmtool for my kernel work since it's much more
simpler..

Thanks,
Namhyung

Powered by blists - more mailing lists