Re: [virtio-dev] Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Paolo Bonzini]

On 18/11/2016 05:07, Michael S. Tsirkin wrote:
> On Fri, Nov 18, 2016 at 12:32:06PM +0900, Namhyung Kim wrote:
>> Btw I prefer using the kvmtool for my kernel work since it's much more
>> simpler..
> 
> Up to you but then you should extend that to support 1.0 spec.
> I strongly object to adding to the list of legacy interfaces
> we need to maintain.

I object to adding paravirtualization unless there is a good reason why
the usual mechanisms for physical machines cannot be used.  The cost of
maintaining a spec, two device implementations (kvmtool+qemu) and a
driver is not small, plus it will not work on older kernels.

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Paolo Bonzini]

On 18/11/2016 04:32, Namhyung Kim wrote:
>> 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..

Yes, you can make it customizable.

>>> 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.

This is something that you have to do in the usual kernel pstore
infrastructure.  It should not be specific to virtualization.

Paolo

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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Michael S. Tsirkin]

On Fri, Nov 18, 2016 at 12:32:06PM +0900, Namhyung Kim wrote:
> Btw I prefer using the kvmtool for my kernel work since it's much more
> simpler..
> 
> Thanks,
> Namhyung

Up to you but then you should extend that to support 1.0 spec.
I strongly object to adding to the list of legacy interfaces
we need to maintain.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

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-onlybit 7: if set, operation in progress
> 
>  bit 0-6: operation status, see "Command Status 
> Definition" in
>  the ACPI spec
> 
>  0x02   read-onlywhen 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 0x if the store is empty
> 
>  0x03unused, 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 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Paolo Bonzini]

> 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-onlybit 7: if set, operation in progress

 bit 0-6: operation status, see "Command Status 
Definition" in
 the ACPI spec

 0x02   read-onlywhen 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 0x if the store is empty

 0x03unused, 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.

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_OPERATIONwrite value 3 to register at 0x00
   END_OPERATION  no-op
   CHECK_BUSY_STATUS  read register at 0x01 with mask 0x80
   GET_COMMAND_STATUS 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hi,

On Tue, Nov 15, 2016 at 11:38 PM, Paolo Bonzini  wrote:
>
>
> On 15/11/2016 15:36, Namhyung Kim wrote:
>> Hi,
>>
>> On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote:
>>>
>>>
>>> On 15/11/2016 06:06, Michael S. Tsirkin wrote:
 On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> Hi Michael,
>
> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
>>> The virtio pstore driver provides interface to the pstore subsystem so
>>> that the guest kernel's log/dump message can be saved on the host
>>> machine.  Users can access the log file directly on the host, or on the
>>> guest at the next boot using pstore filesystem.  It currently deals with
>>> kernel log (printk) buffer only, but we can extend it to have other
>>> information (like ftrace dump) later.
>>>
>>> It supports legacy PCI device using single order-2 page buffer.
>>
>> Do you mean a legacy virtio device? I don't see why
>> you would want to support pre-1.0 mode.
>> If you drop that, you can drop all cpu_to_virtio things
>> and just use __le accessors.
>
> I was thinking about the kvmtools which lacks 1.0 support AFAIK.

 Unless kvmtools wants to be left behind it has to go 1.0.
>>>
>>> And it also has to go ACPI.  Is there any reason, apart from kvmtool, to
>>> make a completely new virtio device, with no support in existing guests,
>>> rather than implement ACPI ERST?
>>
>> Well, I know nothing about ACPI.  It looks like a huge spec and I
>> don't want to dig into it just for this.
>
> ERST (error record serialization table) is a small subset of the ACPI spec.

Not sure how independent ERST is from ACPI and other specs.  It looks
like referencing UEFI spec at least.  Btw, is the ERST used for pstore
only (in Linux)?

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?

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 02:50:11PM +0900, Namhyung Kim wrote:
> On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> > > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > > [SNIP]
> > > > > +struct virtio_pstore_fileinfo {
> > > > > + __virtio64  id;
> > > > > + __virtio32  count;
> > > > > + __virtio16  type;
> > > > > + __virtio16  unused;
> > > > > + __virtio32  flags;
> > > > > + __virtio32  len;
> > > > > + __virtio64  time_sec;
> > > > > + __virtio32  time_nsec;
> > > > > + __virtio32  reserved;
> > > > > +};
> > > > > +
> > > > > +struct virtio_pstore_config {
> > > > > + __virtio32  bufsize;
> > > > > +};
> > > > > +
> > > > 
> > > > What exactly does each field mean? I'm especially
> > > > interested in time fields - maintaining a consistent
> > > > time between host and guest is not a simple problem.
> > > 
> > > These are required by pstore and will be used to create corresponding
> > > files in the pstore filesystem.  The time fields are for mtime and
> > > ctime and, I think, it's just a hint for user and doesn't require
> > > strict consistency.
> > 
> > Pls add documentation. I would just drop hints for now.
> 
> Well, I'll add docmentation.  But I think just dropping might not good
> since they all have host time and it's helpful to know their relative
> difference in guest.
> 
> Thanks,
> Namhyung

If it's part of host/guest ABI it needs to be better defined.
"It's just a hint does not need to be exact" is too vague,
we need to specify what kind of change will or will not
break guests.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Paolo Bonzini]

On 15/11/2016 15:36, Namhyung Kim wrote:
> Hi,
> 
> On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote:
>>
>>
>> On 15/11/2016 06:06, Michael S. Tsirkin wrote:
>>> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
 Hi Michael,

 On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
>> The virtio pstore driver provides interface to the pstore subsystem so
>> that the guest kernel's log/dump message can be saved on the host
>> machine.  Users can access the log file directly on the host, or on the
>> guest at the next boot using pstore filesystem.  It currently deals with
>> kernel log (printk) buffer only, but we can extend it to have other
>> information (like ftrace dump) later.
>>
>> It supports legacy PCI device using single order-2 page buffer.
>
> Do you mean a legacy virtio device? I don't see why
> you would want to support pre-1.0 mode.
> If you drop that, you can drop all cpu_to_virtio things
> and just use __le accessors.

 I was thinking about the kvmtools which lacks 1.0 support AFAIK.
>>>
>>> Unless kvmtools wants to be left behind it has to go 1.0.
>>
>> And it also has to go ACPI.  Is there any reason, apart from kvmtool, to
>> make a completely new virtio device, with no support in existing guests,
>> rather than implement ACPI ERST?
> 
> Well, I know nothing about ACPI.  It looks like a huge spec and I
> don't want to dig into it just for this.

ERST (error record serialization table) is a small subset of the ACPI spec.

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hi,

On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/11/2016 06:06, Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> >> Hi Michael,
> >>
> >> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> >>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
>  The virtio pstore driver provides interface to the pstore subsystem so
>  that the guest kernel's log/dump message can be saved on the host
>  machine.  Users can access the log file directly on the host, or on the
>  guest at the next boot using pstore filesystem.  It currently deals with
>  kernel log (printk) buffer only, but we can extend it to have other
>  information (like ftrace dump) later.
> 
>  It supports legacy PCI device using single order-2 page buffer.
> >>>
> >>> Do you mean a legacy virtio device? I don't see why
> >>> you would want to support pre-1.0 mode.
> >>> If you drop that, you can drop all cpu_to_virtio things
> >>> and just use __le accessors.
> >>
> >> I was thinking about the kvmtools which lacks 1.0 support AFAIK.
> > 
> > Unless kvmtools wants to be left behind it has to go 1.0.
> 
> And it also has to go ACPI.  Is there any reason, apart from kvmtool, to
> make a completely new virtio device, with no support in existing guests,
> rather than implement ACPI ERST?

Well, I know nothing about ACPI.  It looks like a huge spec and I
don't want to dig into it just for this.

What I want is to speed up dumping guest kernel message (especially
for ftrace dump).

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Paolo Bonzini]

On 15/11/2016 06:06, Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
>> Hi Michael,
>>
>> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
>>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
 The virtio pstore driver provides interface to the pstore subsystem so
 that the guest kernel's log/dump message can be saved on the host
 machine.  Users can access the log file directly on the host, or on the
 guest at the next boot using pstore filesystem.  It currently deals with
 kernel log (printk) buffer only, but we can extend it to have other
 information (like ftrace dump) later.

 It supports legacy PCI device using single order-2 page buffer.
>>>
>>> Do you mean a legacy virtio device? I don't see why
>>> you would want to support pre-1.0 mode.
>>> If you drop that, you can drop all cpu_to_virtio things
>>> and just use __le accessors.
>>
>> I was thinking about the kvmtools which lacks 1.0 support AFAIK.
> 
> Unless kvmtools wants to be left behind it has to go 1.0.

And it also has to go ACPI.  Is there any reason, apart from kvmtool, to
make a completely new virtio device, with no support in existing guests,
rather than implement ACPI ERST?

Paolo

>>  But
>> I think it'd be better to always use __le type anyway.  Will change.
>>
>>
>>>
 It uses
 two virtqueues - one for (sync) read and another for (async) write.
 Since it cannot wait for write finished, it supports up to 128
 concurrent IO.  The buffer size is configurable now.

 Cc: Paolo Bonzini 
 Cc: Radim Krčmář 
 Cc: "Michael S. Tsirkin" 
 Cc: Anthony Liguori 
 Cc: Anton Vorontsov 
 Cc: Colin Cross 
 Cc: Kees Cook 
 Cc: Tony Luck 
 Cc: Steven Rostedt 
 Cc: Ingo Molnar 
 Cc: Minchan Kim 
 Cc: k...@vger.kernel.org
 Cc: qemu-de...@nongnu.org
 Cc: virtualization@lists.linux-foundation.org
 Signed-off-by: Namhyung Kim 
 ---
  drivers/virtio/Kconfig |  10 +
  drivers/virtio/Makefile|   1 +
  drivers/virtio/virtio_pstore.c | 417 
 +
  include/uapi/linux/Kbuild  |   1 +
  include/uapi/linux/virtio_ids.h|   1 +
  include/uapi/linux/virtio_pstore.h |  74 +++
  6 files changed, 504 insertions(+)
  create mode 100644 drivers/virtio/virtio_pstore.c
  create mode 100644 include/uapi/linux/virtio_pstore.h

 diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
 index 77590320d44c..8f0e6c796c12 100644
 --- a/drivers/virtio/Kconfig
 +++ b/drivers/virtio/Kconfig
 @@ -58,6 +58,16 @@ config VIRTIO_INPUT
  
 If unsure, say M.
  
 +config VIRTIO_PSTORE
 +  tristate "Virtio pstore driver"
 +  depends on VIRTIO
 +  depends on PSTORE
 +  ---help---
 +   This driver supports virtio pstore devices to save/restore
 +   panic and oops messages on the host.
 +
 +   If unsure, say M.
 +
   config VIRTIO_MMIO
tristate "Platform bus driver for memory mapped virtio devices"
depends on HAS_IOMEM && HAS_DMA
 diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
 index 41e30e3dc842..bee68cb26d48 100644
 --- a/drivers/virtio/Makefile
 +++ b/drivers/virtio/Makefile
 @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
 +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
 diff --git a/drivers/virtio/virtio_pstore.c 
 b/drivers/virtio/virtio_pstore.c
 new file mode 100644
 index ..0a63c7db4278
 --- /dev/null
 +++ b/drivers/virtio/virtio_pstore.c
 @@ -0,0 +1,417 @@
 +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 +
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +#define VIRT_PSTORE_ORDER2
 +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
 +#define VIRT_PSTORE_NR_REQ   128
 +
 +struct virtio_pstore {
 +  struct virtio_device*vdev;
 +  struct virtqueue*vq[2];
>>>
>>> I'd add named fields instead of an array here, vq[0]
>>> vq[1] all over the place is hard to read.
>>
>> Will change.
>>
>>>
 +  struct pstore_info   pstore;
 +  struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
 +  struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
 +  unsigned int req_id;
 +
 +  /* Waiting for host to ack */
 +  wait_queue_head_t   

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > [SNIP]
> > > > +struct virtio_pstore_fileinfo {
> > > > +   __virtio64  id;
> > > > +   __virtio32  count;
> > > > +   __virtio16  type;
> > > > +   __virtio16  unused;
> > > > +   __virtio32  flags;
> > > > +   __virtio32  len;
> > > > +   __virtio64  time_sec;
> > > > +   __virtio32  time_nsec;
> > > > +   __virtio32  reserved;
> > > > +};
> > > > +
> > > > +struct virtio_pstore_config {
> > > > +   __virtio32  bufsize;
> > > > +};
> > > > +
> > > 
> > > What exactly does each field mean? I'm especially
> > > interested in time fields - maintaining a consistent
> > > time between host and guest is not a simple problem.
> > 
> > These are required by pstore and will be used to create corresponding
> > files in the pstore filesystem.  The time fields are for mtime and
> > ctime and, I think, it's just a hint for user and doesn't require
> > strict consistency.
> 
> Pls add documentation. I would just drop hints for now.

Well, I'll add docmentation.  But I think just dropping might not good
since they all have host time and it's helpful to know their relative
difference in guest.

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> Hi Michael,
> 
> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> > > The virtio pstore driver provides interface to the pstore subsystem so
> > > that the guest kernel's log/dump message can be saved on the host
> > > machine.  Users can access the log file directly on the host, or on the
> > > guest at the next boot using pstore filesystem.  It currently deals with
> > > kernel log (printk) buffer only, but we can extend it to have other
> > > information (like ftrace dump) later.
> > > 
> > > It supports legacy PCI device using single order-2 page buffer.
> > 
> > Do you mean a legacy virtio device? I don't see why
> > you would want to support pre-1.0 mode.
> > If you drop that, you can drop all cpu_to_virtio things
> > and just use __le accessors.
> 
> I was thinking about the kvmtools which lacks 1.0 support AFAIK.

Unless kvmtools wants to be left behind it has to go 1.0.

>  But
> I think it'd be better to always use __le type anyway.  Will change.
> 
> 
> > 
> > > It uses
> > > two virtqueues - one for (sync) read and another for (async) write.
> > > Since it cannot wait for write finished, it supports up to 128
> > > concurrent IO.  The buffer size is configurable now.
> > > 
> > > Cc: Paolo Bonzini 
> > > Cc: Radim Krčmář 
> > > Cc: "Michael S. Tsirkin" 
> > > Cc: Anthony Liguori 
> > > Cc: Anton Vorontsov 
> > > Cc: Colin Cross 
> > > Cc: Kees Cook 
> > > Cc: Tony Luck 
> > > Cc: Steven Rostedt 
> > > Cc: Ingo Molnar 
> > > Cc: Minchan Kim 
> > > Cc: k...@vger.kernel.org
> > > Cc: qemu-de...@nongnu.org
> > > Cc: virtualization@lists.linux-foundation.org
> > > Signed-off-by: Namhyung Kim 
> > > ---
> > >  drivers/virtio/Kconfig |  10 +
> > >  drivers/virtio/Makefile|   1 +
> > >  drivers/virtio/virtio_pstore.c | 417 
> > > +
> > >  include/uapi/linux/Kbuild  |   1 +
> > >  include/uapi/linux/virtio_ids.h|   1 +
> > >  include/uapi/linux/virtio_pstore.h |  74 +++
> > >  6 files changed, 504 insertions(+)
> > >  create mode 100644 drivers/virtio/virtio_pstore.c
> > >  create mode 100644 include/uapi/linux/virtio_pstore.h
> > > 
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > index 77590320d44c..8f0e6c796c12 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -58,6 +58,16 @@ config VIRTIO_INPUT
> > >  
> > >If unsure, say M.
> > >  
> > > +config VIRTIO_PSTORE
> > > + tristate "Virtio pstore driver"
> > > + depends on VIRTIO
> > > + depends on PSTORE
> > > + ---help---
> > > +  This driver supports virtio pstore devices to save/restore
> > > +  panic and oops messages on the host.
> > > +
> > > +  If unsure, say M.
> > > +
> > >   config VIRTIO_MMIO
> > >   tristate "Platform bus driver for memory mapped virtio devices"
> > >   depends on HAS_IOMEM && HAS_DMA
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 41e30e3dc842..bee68cb26d48 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > >  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> > > diff --git a/drivers/virtio/virtio_pstore.c 
> > > b/drivers/virtio/virtio_pstore.c
> > > new file mode 100644
> > > index ..0a63c7db4278
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_pstore.c
> > > @@ -0,0 +1,417 @@
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +#define VIRT_PSTORE_ORDER2
> > > +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> > > +#define VIRT_PSTORE_NR_REQ   128
> > > +
> > > +struct virtio_pstore {
> > > + struct virtio_device*vdev;
> > > + struct virtqueue*vq[2];
> > 
> > I'd add named fields instead of an array here, vq[0]
> > vq[1] all over the place is hard to read.
> 
> Will change.
> 
> > 
> > > + struct pstore_info   pstore;
> > > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> > > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> > > + unsigned int req_id;
> > > +
> > > + /* Waiting for host to ack */
> > > + wait_queue_head_t   acked;
> > > + int failed;
> > > +};
> > > +
> > > +#define TYPE_TABLE_ENTRY(_entry) \
> > > + { 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hi Michael,

On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine.  Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem.  It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> > 
> > It supports legacy PCI device using single order-2 page buffer.
> 
> Do you mean a legacy virtio device? I don't see why
> you would want to support pre-1.0 mode.
> If you drop that, you can drop all cpu_to_virtio things
> and just use __le accessors.

I was thinking about the kvmtools which lacks 1.0 support AFAIK.  But
I think it'd be better to always use __le type anyway.  Will change.


> 
> > It uses
> > two virtqueues - one for (sync) read and another for (async) write.
> > Since it cannot wait for write finished, it supports up to 128
> > concurrent IO.  The buffer size is configurable now.
> > 
> > Cc: Paolo Bonzini 
> > Cc: Radim Krčmář 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Anthony Liguori 
> > Cc: Anton Vorontsov 
> > Cc: Colin Cross 
> > Cc: Kees Cook 
> > Cc: Tony Luck 
> > Cc: Steven Rostedt 
> > Cc: Ingo Molnar 
> > Cc: Minchan Kim 
> > Cc: k...@vger.kernel.org
> > Cc: qemu-de...@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim 
> > ---
> >  drivers/virtio/Kconfig |  10 +
> >  drivers/virtio/Makefile|   1 +
> >  drivers/virtio/virtio_pstore.c | 417 
> > +
> >  include/uapi/linux/Kbuild  |   1 +
> >  include/uapi/linux/virtio_ids.h|   1 +
> >  include/uapi/linux/virtio_pstore.h |  74 +++
> >  6 files changed, 504 insertions(+)
> >  create mode 100644 drivers/virtio/virtio_pstore.c
> >  create mode 100644 include/uapi/linux/virtio_pstore.h
> > 
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 77590320d44c..8f0e6c796c12 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -58,6 +58,16 @@ config VIRTIO_INPUT
> >  
> >  If unsure, say M.
> >  
> > +config VIRTIO_PSTORE
> > +   tristate "Virtio pstore driver"
> > +   depends on VIRTIO
> > +   depends on PSTORE
> > +   ---help---
> > +This driver supports virtio pstore devices to save/restore
> > +panic and oops messages on the host.
> > +
> > +If unsure, say M.
> > +
> >   config VIRTIO_MMIO
> > tristate "Platform bus driver for memory mapped virtio devices"
> > depends on HAS_IOMEM && HAS_DMA
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 41e30e3dc842..bee68cb26d48 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> >  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> > new file mode 100644
> > index ..0a63c7db4278
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pstore.c
> > @@ -0,0 +1,417 @@
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define VIRT_PSTORE_ORDER2
> > +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> > +#define VIRT_PSTORE_NR_REQ   128
> > +
> > +struct virtio_pstore {
> > +   struct virtio_device*vdev;
> > +   struct virtqueue*vq[2];
> 
> I'd add named fields instead of an array here, vq[0]
> vq[1] all over the place is hard to read.

Will change.

> 
> > +   struct pstore_info   pstore;
> > +   struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> > +   struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> > +   unsigned int req_id;
> > +
> > +   /* Waiting for host to ack */
> > +   wait_queue_head_t   acked;
> > +   int failed;
> > +};
> > +
> > +#define TYPE_TABLE_ENTRY(_entry)   \
> > +   { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> > +
> > +struct type_table {
> > +   int pstore;
> > +   u16 virtio;
> > +} type_table[] = {
> > +   TYPE_TABLE_ENTRY(DMESG),
> > +};
> > +
> > +#undef TYPE_TABLE_ENTRY
> 
> let's avoid macros for now pls. In fact, I would just open-code this
> in to_virtio_type below. We can always change our minds 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Michael S. Tsirkin]

On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
> 
> It supports legacy PCI device using single order-2 page buffer.

Do you mean a legacy virtio device? I don't see why
you would want to support pre-1.0 mode.
If you drop that, you can drop all cpu_to_virtio things
and just use __le accessors.

> It uses
> two virtqueues - one for (sync) read and another for (async) write.
> Since it cannot wait for write finished, it supports up to 128
> concurrent IO.  The buffer size is configurable now.
> 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: k...@vger.kernel.org
> Cc: qemu-de...@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 
> ---
>  drivers/virtio/Kconfig |  10 +
>  drivers/virtio/Makefile|   1 +
>  drivers/virtio/virtio_pstore.c | 417 
> +
>  include/uapi/linux/Kbuild  |   1 +
>  include/uapi/linux/virtio_ids.h|   1 +
>  include/uapi/linux/virtio_pstore.h |  74 +++
>  6 files changed, 504 insertions(+)
>  create mode 100644 drivers/virtio/virtio_pstore.c
>  create mode 100644 include/uapi/linux/virtio_pstore.h
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 77590320d44c..8f0e6c796c12 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -58,6 +58,16 @@ config VIRTIO_INPUT
>  
>If unsure, say M.
>  
> +config VIRTIO_PSTORE
> + tristate "Virtio pstore driver"
> + depends on VIRTIO
> + depends on PSTORE
> + ---help---
> +  This driver supports virtio pstore devices to save/restore
> +  panic and oops messages on the host.
> +
> +  If unsure, say M.
> +
>   config VIRTIO_MMIO
>   tristate "Platform bus driver for memory mapped virtio devices"
>   depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3dc842..bee68cb26d48 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
>  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index ..0a63c7db4278
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,417 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define VIRT_PSTORE_ORDER2
> +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> +#define VIRT_PSTORE_NR_REQ   128
> +
> +struct virtio_pstore {
> + struct virtio_device*vdev;
> + struct virtqueue*vq[2];

I'd add named fields instead of an array here, vq[0]
vq[1] all over the place is hard to read.

> + struct pstore_info   pstore;
> + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> + unsigned int req_id;
> +
> + /* Waiting for host to ack */
> + wait_queue_head_t   acked;
> + int failed;
> +};
> +
> +#define TYPE_TABLE_ENTRY(_entry) \
> + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> +
> +struct type_table {
> + int pstore;
> + u16 virtio;
> +} type_table[] = {
> + TYPE_TABLE_ENTRY(DMESG),
> +};
> +
> +#undef TYPE_TABLE_ENTRY

let's avoid macros for now pls. In fact, I would just open-code this
in to_virtio_type below. We can always change our minds later if
lots of types are added.

> +
> +

single emoty line pls

> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id 
> type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> + if (type == type_table[i].pstore)
> + return cpu_to_virtio16(vps->vdev, type_table[i].virtio);
> + }
> +
> + return cpu_to_virtio16(vps->vdev, 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hi Stefan,

On Thu, Sep 22, 2016 at 12:57:44PM +0100, Stefan Hajnoczi wrote:
> On Sun, Sep 04, 2016 at 11:38:58PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine.  Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem.  It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> > 
> > It supports legacy PCI device using a 16K buffer by default and it's
> > configurable.  It uses two virtqueues - one for (sync) read and another
> > for (async) write.  Since it cannot wait for write finished, it supports
> > up to 128 concurrent IO.
> 
> Please document the locks that this code relies on.  It is generally not
> safe to call virtqueue_*() from multiple threads.  I also wonder about
> locking for virtio_pstore->req_id and other fields.  Are locks missing
> or is something in pstore ensuring safety?

Ok, I should use atomic inc for pstore->req_id.  The open-read-close
are serialized by the read_mutex of pstore_info.  Write can happend
anytime so I gave it a dedicate queue.

Erase is a problem though, normally it's only doable after mount
operation is done so no contention to the open-read-close.  But if the
pstore_update_ms is set, timer routine can schedule a work to do the
open-read-close loop which might contend to erase.

I'm not sure how useful pstore_update_ms is and the descriptoin saids

  "milliseconds before pstore updates its content "
  "(default is -1, which means runtime updates are disabled; "
  "enabling this option is not safe, it may lead to further "
  "corruption on Oopses)")


> 
> > +static int virt_pstore_open(struct pstore_info *psi)
> > +{
> > +   struct virtio_pstore *vps = psi->data;
> > +   struct virtio_pstore_req *req;
> > +   struct virtio_pstore_res *res;
> > +   struct scatterlist sgo[1], sgi[1];
> > +   struct scatterlist *sgs[2] = { sgo, sgi };
> > +   unsigned int len;
> > +
> > +   virt_pstore_get_reqs(vps, , );
> > +
> > +   req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_OPEN);
> > +
> > +   sg_init_one(sgo, req, sizeof(*req));
> > +   sg_init_one(sgi, res, sizeof(*res));
> > +   virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > +   virtqueue_kick(vps->vq[0]);
> > +
> > +   wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], ));
> > +   return le32_to_cpu(res->ret);
> 
> This assumes the device puts compatible Linux errno values in res->ret.
> The function doesn't need to return -errno if I'm reading fs/pstore/
> code correctly.  You could return -1 on error to avoid making this
> assumption.  The same applies to other res->ret usage below.

Ok.

> 
> > +}
> > +
> > +static int virt_pstore_close(struct pstore_info *psi)
> > +{
> > +   struct virtio_pstore *vps = psi->data;
> > +   struct virtio_pstore_req *req = >req[vps->req_id];
> > +   struct virtio_pstore_res *res = >res[vps->req_id];
> 
> Assigning >req[vps->req_id]/>res[vps->req_id] is unnecessary,
> virt_pstore_get_reqs() handles that below.

Ah, right.

> 
> > +   struct scatterlist sgo[1], sgi[1];
> > +   struct scatterlist *sgs[2] = { sgo, sgi };
> > +   unsigned int len;
> > +
> > +   virt_pstore_get_reqs(vps, , );
> > +
> > +   req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_CLOSE);
> > +
> > +   sg_init_one(sgo, req, sizeof(*req));
> > +   sg_init_one(sgi, res, sizeof(*res));
> > +   virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > +   virtqueue_kick(vps->vq[0]);
> > +
> > +   wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], ));
> > +   return le32_to_cpu(res->ret);
> > +}
> > +
> > +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
> > +   int *count, struct timespec *time,
> > +   char **buf, bool *compressed,
> > +   ssize_t *ecc_notice_size,
> > +   struct pstore_info *psi)
> > +{
> > +   struct virtio_pstore *vps = psi->data;
> > +   struct virtio_pstore_req *req;
> > +   struct virtio_pstore_res *res;
> > +   struct virtio_pstore_fileinfo info;
> > +   struct scatterlist sgo[1], sgi[3];
> > +   struct scatterlist *sgs[2] = { sgo, sgi };
> > +   unsigned int len;
> > +   unsigned int flags;
> > +   int ret;
> > +   void *bf;
> > +
> > +   virt_pstore_get_reqs(vps, , );
> > +
> > +   req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> > +
> > +   sg_init_one(sgo, req, sizeof(*req));
> > +   sg_init_table(sgi, 3);
> > +   sg_set_buf([0], res, sizeof(*res));
> > +   sg_set_buf([1], , sizeof(info));
> > +   sg_set_buf([2], psi->buf, psi->bufsize);
> > +   virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > +   virtqueue_kick(vps->vq[0]);
> > +
> > +   wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], ));
> > +   if (len < sizeof(*res) + sizeof(info))
> > +   return -1;
> > +
> > +   ret = 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Stefan Hajnoczi]

On Sun, Sep 04, 2016 at 11:38:58PM +0900, Namhyung Kim wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
> 
> It supports legacy PCI device using a 16K buffer by default and it's
> configurable.  It uses two virtqueues - one for (sync) read and another
> for (async) write.  Since it cannot wait for write finished, it supports
> up to 128 concurrent IO.

Please document the locks that this code relies on.  It is generally not
safe to call virtqueue_*() from multiple threads.  I also wonder about
locking for virtio_pstore->req_id and other fields.  Are locks missing
or is something in pstore ensuring safety?

> +static int virt_pstore_open(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, , );
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_OPEN);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], ));
> + return le32_to_cpu(res->ret);

This assumes the device puts compatible Linux errno values in res->ret.
The function doesn't need to return -errno if I'm reading fs/pstore/
code correctly.  You could return -1 on error to avoid making this
assumption.  The same applies to other res->ret usage below.

> +}
> +
> +static int virt_pstore_close(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req = >req[vps->req_id];
> + struct virtio_pstore_res *res = >res[vps->req_id];

Assigning >req[vps->req_id]/>res[vps->req_id] is unnecessary,
virt_pstore_get_reqs() handles that below.

> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, , );
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_CLOSE);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], ));
> + return le32_to_cpu(res->ret);
> +}
> +
> +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
> + int *count, struct timespec *time,
> + char **buf, bool *compressed,
> + ssize_t *ecc_notice_size,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct virtio_pstore_fileinfo info;
> + struct scatterlist sgo[1], sgi[3];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> + unsigned int flags;
> + int ret;
> + void *bf;
> +
> + virt_pstore_get_reqs(vps, , );
> +
> + req->cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_table(sgi, 3);
> + sg_set_buf([0], res, sizeof(*res));
> + sg_set_buf([1], , sizeof(info));
> + sg_set_buf([2], psi->buf, psi->bufsize);
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], ));
> + if (len < sizeof(*res) + sizeof(info))
> + return -1;
> +
> + ret = le32_to_cpu(res->ret);
> + if (ret < 0)
> + return ret;
> +
> + len = le32_to_cpu(info.len);

We trust the device but a length check would be nice here to be clear
that the memcpy() below is always safe:

if (len > psi->bufsize)
return -1;

> +
> + bf = kmalloc(len, GFP_KERNEL);
> + if (bf == NULL)
> + return -ENOMEM;

There's no point in returning -ENOMEM if we return -1 and res->ret
above.  The caller has no way of knowing the true meaning of the return
value.  I would return -1 to be clear that there are no error constants.

> +
> + *id= le64_to_cpu(info.id);
> + *type  = from_virtio_type(info.type);
> + *count = le32_to_cpu(info.count);
> +
> + flags = le32_to_cpu(info.flags);
> + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED;
> +
> + time->tv_sec  = le64_to_cpu(info.time_sec);
> + time->tv_nsec = 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hello Michael,

Thanks for your detailed review.  Btw are you ok with the overall
direction of the patch?


On Tue, Sep 13, 2016 at 06:19:41PM +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine.  Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem.  It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> > 
> > It supports legacy PCI device using single order-2 page buffer.  It uses
> > two virtqueues - one for (sync) read and another for (async) write.
> > Since it cannot wait for write finished, it supports up to 128
> > concurrent IO.  The buffer size is configurable now.
> > 
> > Cc: Paolo Bonzini 
> > Cc: Radim Krčmář 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Anthony Liguori 
> > Cc: Anton Vorontsov 
> > Cc: Colin Cross 
> > Cc: Kees Cook 
> > Cc: Tony Luck 
> > Cc: Steven Rostedt 
> > Cc: Ingo Molnar 
> > Cc: Minchan Kim 
> > Cc: k...@vger.kernel.org
> > Cc: qemu-de...@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim 
> > ---
> >  drivers/virtio/Kconfig |  10 +
> >  drivers/virtio/Makefile|   1 +
> >  drivers/virtio/virtio_pstore.c | 417 
> > +
> >  include/uapi/linux/Kbuild  |   1 +
> >  include/uapi/linux/virtio_ids.h|   1 +
> >  include/uapi/linux/virtio_pstore.h |  74 +++
> >  6 files changed, 504 insertions(+)
> >  create mode 100644 drivers/virtio/virtio_pstore.c
> >  create mode 100644 include/uapi/linux/virtio_pstore.h
> > 
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 77590320d44c..8f0e6c796c12 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -58,6 +58,16 @@ config VIRTIO_INPUT
> >  
> >  If unsure, say M.
> >  
> > +config VIRTIO_PSTORE
> > +   tristate "Virtio pstore driver"
> > +   depends on VIRTIO
> > +   depends on PSTORE
> > +   ---help---
> > +This driver supports virtio pstore devices to save/restore
> > +panic and oops messages on the host.
> > +
> > +If unsure, say M.
> > +
> >   config VIRTIO_MMIO
> > tristate "Platform bus driver for memory mapped virtio devices"
> > depends on HAS_IOMEM && HAS_DMA
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 41e30e3dc842..bee68cb26d48 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> >  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> > new file mode 100644
> > index ..0a63c7db4278
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pstore.c
> > @@ -0,0 +1,417 @@
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define VIRT_PSTORE_ORDER2
> > +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> > +#define VIRT_PSTORE_NR_REQ   128
> 
> where are these numbers from?

The buffer size was chosen to be larger than default kmsg_bytes
(10240) in the pstore platform code and the request count is a
arbitrary value.

> 
> 
> > +
> > +struct virtio_pstore {
> > +   struct virtio_device*vdev;
> > +   struct virtqueue*vq[2];
> > +   struct pstore_info   pstore;
> > +   struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> > +   struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> > +   unsigned int req_id;
> > +
> > +   /* Waiting for host to ack */
> > +   wait_queue_head_t   acked;
> > +   int failed;
> > +};
> > +
> > +#define TYPE_TABLE_ENTRY(_entry)   \
> > +   { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> > +
> > +struct type_table {
> > +   int pstore;
> > +   u16 virtio;
> > +} type_table[] = {
> > +   TYPE_TABLE_ENTRY(DMESG),
> > +};
> > +
> > +#undef TYPE_TABLE_ENTRY
> 
> Let's not play preprocessor games until this becomes
> a big issue. Simple
> { PSTORE_TYPE_DMESG, VIRTIO_PSTORE_TYPE_DMESG}
> does the trick just as well for now.
> Also see below.
> 
> 
> 
> > +
> > +
> 
> single empty line pls.

Ok.

> 
> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Michael S. Tsirkin]

On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
> 
> It supports legacy PCI device using single order-2 page buffer.  It uses
> two virtqueues - one for (sync) read and another for (async) write.
> Since it cannot wait for write finished, it supports up to 128
> concurrent IO.  The buffer size is configurable now.
> 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: k...@vger.kernel.org
> Cc: qemu-de...@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 
> ---
>  drivers/virtio/Kconfig |  10 +
>  drivers/virtio/Makefile|   1 +
>  drivers/virtio/virtio_pstore.c | 417 
> +
>  include/uapi/linux/Kbuild  |   1 +
>  include/uapi/linux/virtio_ids.h|   1 +
>  include/uapi/linux/virtio_pstore.h |  74 +++
>  6 files changed, 504 insertions(+)
>  create mode 100644 drivers/virtio/virtio_pstore.c
>  create mode 100644 include/uapi/linux/virtio_pstore.h
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 77590320d44c..8f0e6c796c12 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -58,6 +58,16 @@ config VIRTIO_INPUT
>  
>If unsure, say M.
>  
> +config VIRTIO_PSTORE
> + tristate "Virtio pstore driver"
> + depends on VIRTIO
> + depends on PSTORE
> + ---help---
> +  This driver supports virtio pstore devices to save/restore
> +  panic and oops messages on the host.
> +
> +  If unsure, say M.
> +
>   config VIRTIO_MMIO
>   tristate "Platform bus driver for memory mapped virtio devices"
>   depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3dc842..bee68cb26d48 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
>  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index ..0a63c7db4278
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,417 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define VIRT_PSTORE_ORDER2
> +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> +#define VIRT_PSTORE_NR_REQ   128

where are these numbers from?


> +
> +struct virtio_pstore {
> + struct virtio_device*vdev;
> + struct virtqueue*vq[2];
> + struct pstore_info   pstore;
> + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> + unsigned int req_id;
> +
> + /* Waiting for host to ack */
> + wait_queue_head_t   acked;
> + int failed;
> +};
> +
> +#define TYPE_TABLE_ENTRY(_entry) \
> + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> +
> +struct type_table {
> + int pstore;
> + u16 virtio;
> +} type_table[] = {
> + TYPE_TABLE_ENTRY(DMESG),
> +};
> +
> +#undef TYPE_TABLE_ENTRY

Let's not play preprocessor games until this becomes
a big issue. Simple
{ PSTORE_TYPE_DMESG, VIRTIO_PSTORE_TYPE_DMESG}
does the trick just as well for now.
Also see below.



> +
> +

single empty line pls.

> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id 
> type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> + if (type == type_table[i].pstore)
> + return cpu_to_virtio16(vps->vdev, type_table[i].virtio);
> + }

Rather complex for something that always returns a single value.
why do we need a table at all?
How about a switch statement?

static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
{
switch (type) {
case PSTORE_TYPE_DMESG:
return 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Kees Cook]

On Sun, Sep 4, 2016 at 7:38 AM, Namhyung Kim  wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
>
> It supports legacy PCI device using a 16K buffer by default and it's
> configurable.  It uses two virtqueues - one for (sync) read and another
> for (async) write.  Since it cannot wait for write finished, it supports
> up to 128 concurrent IO.
>
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: Will Deacon 
> Cc: virtio-...@lists.oasis-open.org
> Cc: k...@vger.kernel.org
> Cc: qemu-de...@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 

While I can't speak to the virtio parts, the interface into pstore
looks fine to me. :)

Reviewed-by: Kees Cook 

-Kees

-- 
Kees Cook
Nexus Security
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hi Michael,

On Wed, Aug 31, 2016 at 05:54:04PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 31, 2016 at 05:08:00PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine.  Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem.  It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> > 
> > It supports legacy PCI device using single order-2 page buffer.  It uses
> > two virtqueues - one for (sync) read and another for (async) write.
> > Since it cannot wait for write finished, it supports up to 128
> > concurrent IO.  The buffer size is configurable now.
> > 
> > Cc: Paolo Bonzini 
> > Cc: Radim Krčmář 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Anthony Liguori 
> > Cc: Anton Vorontsov 
> > Cc: Colin Cross 
> > Cc: Kees Cook 
> > Cc: Tony Luck 
> > Cc: Steven Rostedt 
> > Cc: Ingo Molnar 
> > Cc: Minchan Kim 
> > Cc: Will Deacon 
> > Cc: k...@vger.kernel.org
> > Cc: qemu-de...@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: virtio-...@lists.oasis-open.org
> > Signed-off-by: Namhyung Kim 
> > ---

[SNIP]
> > +#define TYPE_TABLE_ENTRY(_entry)   \
> > +   { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> > +
> > +struct type_table {
> > +   int pstore;
> > +   u16 virtio;
> > +} type_table[] = {
> > +   TYPE_TABLE_ENTRY(DMESG),
> > +};
> > +
> > +#undef TYPE_TABLE_ENTRY
> > +
> > +
> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id 
> > type)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> > +   if (type == type_table[i].pstore)
> > +   return cpu_to_virtio16(vps->vdev, type_table[i].virtio);
> 
> Does this pass sparse checks? If yes I'm surprised - this clearly
> returns a virtio16 type.

Ah, didn't run sparse.  Will change it to return a __le16 type
(according to your comment below).

> 
> 
> > +   }
> > +
> > +   return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
> > +}
> > +
> > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 
> > type)

This one should be '__le16 type' as well.


> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> > +   if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio)
> > +   return type_table[i].pstore;
> > +   }
> > +
> > +   return PSTORE_TYPE_UNKNOWN;
> > +}
> > +

[SNIP]
> > +
> > +struct virtio_pstore_req {
> > +   __virtio16  cmd;
> > +   __virtio16  type;
> > +   __virtio32  flags;
> > +   __virtio64  id;
> > +   __virtio32  count;
> > +   __virtio32  reserved;
> > +};
> > +
> > +struct virtio_pstore_res {
> > +   __virtio16  cmd;
> > +   __virtio16  type;
> > +   __virtio32  ret;
> > +};
> 
> Is there a reason to support legacy endian-ness?
> If not, you can just use __le formats.

I just didn't know what's the preferred type.  Will change!

Thanks,
Namhyung

> 
> 
> > +struct virtio_pstore_fileinfo {
> > +   __virtio64  id;
> > +   __virtio32  count;
> > +   __virtio16  type;
> > +   __virtio16  unused;
> > +   __virtio32  flags;
> > +   __virtio32  len;
> > +   __virtio64  time_sec;
> > +   __virtio32  time_nsec;
> > +   __virtio32  reserved;
> > +};
> > +
> > +struct virtio_pstore_config {
> > +   __virtio32  bufsize;
> > +};
> > +
> > +#endif /* _LINUX_VIRTIO_PSTORE_H */
> > -- 
> > 2.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Michael S. Tsirkin]

On Wed, Aug 31, 2016 at 05:08:00PM +0900, Namhyung Kim wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
> 
> It supports legacy PCI device using single order-2 page buffer.  It uses
> two virtqueues - one for (sync) read and another for (async) write.
> Since it cannot wait for write finished, it supports up to 128
> concurrent IO.  The buffer size is configurable now.
> 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: Will Deacon 
> Cc: k...@vger.kernel.org
> Cc: qemu-de...@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Cc: virtio-...@lists.oasis-open.org
> Signed-off-by: Namhyung Kim 
> ---
>  drivers/virtio/Kconfig |  10 +
>  drivers/virtio/Makefile|   1 +
>  drivers/virtio/virtio_pstore.c | 417 
> +
>  include/uapi/linux/Kbuild  |   1 +
>  include/uapi/linux/virtio_ids.h|   1 +
>  include/uapi/linux/virtio_pstore.h |  74 +++
>  6 files changed, 504 insertions(+)
>  create mode 100644 drivers/virtio/virtio_pstore.c
>  create mode 100644 include/uapi/linux/virtio_pstore.h
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 77590320d44c..8f0e6c796c12 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -58,6 +58,16 @@ config VIRTIO_INPUT
>  
>If unsure, say M.
>  
> +config VIRTIO_PSTORE
> + tristate "Virtio pstore driver"
> + depends on VIRTIO
> + depends on PSTORE
> + ---help---
> +  This driver supports virtio pstore devices to save/restore
> +  panic and oops messages on the host.
> +
> +  If unsure, say M.
> +
>   config VIRTIO_MMIO
>   tristate "Platform bus driver for memory mapped virtio devices"
>   depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3dc842..bee68cb26d48 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
>  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index ..ec41f0d2f0b7
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,417 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define VIRT_PSTORE_ORDER2
> +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> +#define VIRT_PSTORE_NR_REQ   128
> +
> +struct virtio_pstore {
> + struct virtio_device*vdev;
> + struct virtqueue*vq[2];
> + struct pstore_info   pstore;
> + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> + unsigned int req_id;
> +
> + /* Waiting for host to ack */
> + wait_queue_head_t   acked;
> + int failed;
> +};
> +
> +#define TYPE_TABLE_ENTRY(_entry) \
> + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> +
> +struct type_table {
> + int pstore;
> + u16 virtio;
> +} type_table[] = {
> + TYPE_TABLE_ENTRY(DMESG),
> +};
> +
> +#undef TYPE_TABLE_ENTRY
> +
> +
> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id 
> type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> + if (type == type_table[i].pstore)
> + return cpu_to_virtio16(vps->vdev, type_table[i].virtio);

Does this pass sparse checks? If yes I'm surprised - this clearly
returns a virtio16 type.


> + }
> +
> + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
> +}
> +
> +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 
> type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio)
> +

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

On Tue, Jul 19, 2016 at 10:43 PM, Namhyung Kim  wrote:
> The other one is the file management on the host side.  I am thinking
> of a simple way that the log file is splitted when it exceeds the half
> of the allowed max size.  It would be configurable and might allow
> unlimited logs if user requests it explicitly (if qemu guys say ok)..

On a second thought, I think that size of log file should not exceed
the pstore buffer size in order to be read back later.  When total size
of log files becomes larger then the limit, the oldest file of same
type can be deleted. This way looks simpler to manage IMHO.

Also I think the pstore id should be managed by host device rather
than guest driver to handle splitted files properly.

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

On Tue, Jul 19, 2016 at 10:43 PM, Namhyung Kim  wrote:
> Hi Kees,
>
> On Mon, Jul 18, 2016 at 10:50:06AM -0700, Kees Cook wrote:
>> On Sun, Jul 17, 2016 at 10:50 PM, Namhyung Kim  wrote:
>> > Hello,
>> >
>> > On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote:
>> >> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim  wrote:
>> > [SNIP]
>> >> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum 
>> >> > pstore_type_id type)
>> >> > +{
>> >> > +   u16 ret;
>> >> > +
>> >> > +   switch (type) {
>> >> > +   case PSTORE_TYPE_DMESG:
>> >> > +   ret = cpu_to_virtio16(vps->vdev, 
>> >> > VIRTIO_PSTORE_TYPE_DMESG);
>> >> > +   break;
>> >> > +   default:
>> >> > +   ret = cpu_to_virtio16(vps->vdev, 
>> >> > VIRTIO_PSTORE_TYPE_UNKNOWN);
>> >> > +   break;
>> >> > +   }
>> >>
>> >> I would love to see this support PSTORE_TYPE_CONSOLE too. It should be
>> >> relatively easy to add: I think it'd just be another virtio command?
>> >
>> > Do you want to append the data to the host file as guest does
>> > printk()?  I think it needs some kind of buffer management, but it's
>> > not hard to add IMHO.
>>
>> Well, with most pstore backends, the buffer size is limited, so it
>> tends to be a circular buffer of some sort. I think whatever you
>> choose to do is fine (I saw the various mentions of resource limits in
>> the qemu part of this thread), as long as the last N bytes of console
>> can be seen on the host side, where N is some portion of the memory
>> set aside for the log. (I don't mind the idea of an unlimited console
>> log either, but I suspect that will not be accepted on the qemu
>> side...)
>
> I think it needs two kinds of buffer management.
>
> The first one is the psinfo->buf (or something similar).  IIUC the
> PSTORE_TYPE_CONSOLE is different than PSTORE_TYPE_DMESG as it is
> emitted every time printk() sends messages to console.  So I think the
> it should remain in async mode due to performance reason.  To do that,
> the message should be copied to psinfo->buf and then sent via virtio.
> Then it needs to keep track of the available buffer position IMHO.

Looking at the code, I found myself confused with the PSTORE_TYPE_FTRACE
and PSTORE_TYPE_CONSOLE.  It seems that handling of
PSTORE_TYPE_CONSOLE is basically same as PSTORE_TYPE_DMESG..

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hi Kees,

On Mon, Jul 18, 2016 at 10:50:06AM -0700, Kees Cook wrote:
> On Sun, Jul 17, 2016 at 10:50 PM, Namhyung Kim  wrote:
> > Hello,
> >
> > On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote:
> >> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim  wrote:
> > [SNIP]
> >> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum 
> >> > pstore_type_id type)
> >> > +{
> >> > +   u16 ret;
> >> > +
> >> > +   switch (type) {
> >> > +   case PSTORE_TYPE_DMESG:
> >> > +   ret = cpu_to_virtio16(vps->vdev, 
> >> > VIRTIO_PSTORE_TYPE_DMESG);
> >> > +   break;
> >> > +   default:
> >> > +   ret = cpu_to_virtio16(vps->vdev, 
> >> > VIRTIO_PSTORE_TYPE_UNKNOWN);
> >> > +   break;
> >> > +   }
> >>
> >> I would love to see this support PSTORE_TYPE_CONSOLE too. It should be
> >> relatively easy to add: I think it'd just be another virtio command?
> >
> > Do you want to append the data to the host file as guest does
> > printk()?  I think it needs some kind of buffer management, but it's
> > not hard to add IMHO.
> 
> Well, with most pstore backends, the buffer size is limited, so it
> tends to be a circular buffer of some sort. I think whatever you
> choose to do is fine (I saw the various mentions of resource limits in
> the qemu part of this thread), as long as the last N bytes of console
> can be seen on the host side, where N is some portion of the memory
> set aside for the log. (I don't mind the idea of an unlimited console
> log either, but I suspect that will not be accepted on the qemu
> side...)

I think it needs two kinds of buffer management.

The first one is the psinfo->buf (or something similar).  IIUC the
PSTORE_TYPE_CONSOLE is different than PSTORE_TYPE_DMESG as it is
emitted every time printk() sends messages to console.  So I think the
it should remain in async mode due to performance reason.  To do that,
the message should be copied to psinfo->buf and then sent via virtio.
Then it needs to keep track of the available buffer position IMHO.

The other one is the file management on the host side.  I am thinking
of a simple way that the log file is splitted when it exceeds the half
of the allowed max size.  It would be configurable and might allow
unlimited logs if user requests it explicitly (if qemu guys say ok)..

Maybe we need to use 'part' or 'count' for filenames to identify
the splitted files.

> 
> > [SNIP]
> >> > +static int notrace virt_pstore_write(enum pstore_type_id type,
> >> > +enum kmsg_dump_reason reason,
> >> > +u64 *id, unsigned int part, int 
> >> > count,
> >> > +bool compressed, size_t size,
> >> > +struct pstore_info *psi)
> >> > +{
> >> > +   struct virtio_pstore *vps = psi->data;
> >> > +   struct virtio_pstore_hdr *hdr = >hdr;
> >> > +   struct scatterlist sg[2];
> >> > +   unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 
> >> > 0;
> >> > +
> >> > +   *id = vps->id++;
> >> > +
> >> > +   hdr->cmd   = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE);
> >> > +   hdr->id= cpu_to_virtio64(vps->vdev, *id);
> >> > +   hdr->flags = cpu_to_virtio32(vps->vdev, flags);
> >> > +   hdr->type  = to_virtio_type(vps, type);
> >> > +
> >> > +   sg_init_table(sg, 2);
> >> > +   sg_set_buf([0], hdr, sizeof(*hdr));
> >> > +   sg_set_buf([1], psi->buf, size);
> >> > +   virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC);
> >> > +   virtqueue_kick(vps->vq);
> >> > +
> >> > +   /* TODO: make it synchronous */
> >> > +   return 0;
> >>
> >> The down side to this being asynchronous is the lack of error
> >> reporting. Perhaps this could check hdr->type before queuing and error
> >> for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send
> >> it?
> >
> > I cannot follow, sorry.  Could you please elaborate it more?
> 
> The mention you have here of "TODO: make it synchronous" made me think
> about what effects that could have. If a pstore_write() were issued
> for a type other than DMESG, the above code would send it through
> virtio anyway. No error reporting is possible unless this is
> synchronous, but the only error here would simply be "I don't know
> what anything except DMESG is", so maybe this code could refuse to
> forward anything with type UNKNOWN in the first place. (Just an idea:
> I don't think there's anything very wrong here. It just seemed like a
> potential improvement.)

Yep, that kind of error handling should be easy.  My concern is when
write operation is failed on the host side.  We need a way to report
it back to the guest and might disallow further writes at least for
the same type.

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Kees Cook]

On Sun, Jul 17, 2016 at 10:50 PM, Namhyung Kim  wrote:
> Hello,
>
> On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote:
>> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim  wrote:
>> > The virtio pstore driver provides interface to the pstore subsystem so
>> > that the guest kernel's log/dump message can be saved on the host
>> > machine.  Users can access the log file directly on the host, or on the
>> > guest at the next boot using pstore filesystem.  It currently deals with
>> > kernel log (printk) buffer only, but we can extend it to have other
>> > information (like ftrace dump) later.
>> >
>> > It supports legacy PCI device using single order-2 page buffer.  As all
>> > operation of pstore is synchronous, it would be fine IMHO.  However I
>> > don't know how to make write operation synchronous since it's called
>> > with a spinlock held (from any context including NMI).
>> >
>> > Cc: Paolo Bonzini 
>> > Cc: Radim Kr??m 
>> > Cc: "Michael S. Tsirkin" 
>> > Cc: Anthony Liguori 
>> > Cc: Anton Vorontsov 
>> > Cc: Colin Cross 
>> > Cc: Kees Cook 
>> > Cc: Tony Luck 
>> > Cc: Steven Rostedt 
>> > Cc: Ingo Molnar 
>> > Cc: Minchan Kim 
>> > Cc: k...@vger.kernel.org
>> > Cc: qemu-de...@nongnu.org
>> > Cc: virtualization@lists.linux-foundation.org
>> > Signed-off-by: Namhyung Kim 
>>
>> This looks great to me! I'd love to use this in qemu. (Right now I go
>> through hoops to use the ramoops backend for testing.)
>>
>> Reviewed-by: Kees Cook 
>
> Thank you!
>
>>
>> Notes below...
>>
>
> [SNIP]
>> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id 
>> > type)
>> > +{
>> > +   u16 ret;
>> > +
>> > +   switch (type) {
>> > +   case PSTORE_TYPE_DMESG:
>> > +   ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG);
>> > +   break;
>> > +   default:
>> > +   ret = cpu_to_virtio16(vps->vdev, 
>> > VIRTIO_PSTORE_TYPE_UNKNOWN);
>> > +   break;
>> > +   }
>>
>> I would love to see this support PSTORE_TYPE_CONSOLE too. It should be
>> relatively easy to add: I think it'd just be another virtio command?
>
> Do you want to append the data to the host file as guest does
> printk()?  I think it needs some kind of buffer management, but it's
> not hard to add IMHO.

Well, with most pstore backends, the buffer size is limited, so it
tends to be a circular buffer of some sort. I think whatever you
choose to do is fine (I saw the various mentions of resource limits in
the qemu part of this thread), as long as the last N bytes of console
can be seen on the host side, where N is some portion of the memory
set aside for the log. (I don't mind the idea of an unlimited console
log either, but I suspect that will not be accepted on the qemu
side...)

> [SNIP]
>> > +static int notrace virt_pstore_write(enum pstore_type_id type,
>> > +enum kmsg_dump_reason reason,
>> > +u64 *id, unsigned int part, int count,
>> > +bool compressed, size_t size,
>> > +struct pstore_info *psi)
>> > +{
>> > +   struct virtio_pstore *vps = psi->data;
>> > +   struct virtio_pstore_hdr *hdr = >hdr;
>> > +   struct scatterlist sg[2];
>> > +   unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
>> > +
>> > +   *id = vps->id++;
>> > +
>> > +   hdr->cmd   = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE);
>> > +   hdr->id= cpu_to_virtio64(vps->vdev, *id);
>> > +   hdr->flags = cpu_to_virtio32(vps->vdev, flags);
>> > +   hdr->type  = to_virtio_type(vps, type);
>> > +
>> > +   sg_init_table(sg, 2);
>> > +   sg_set_buf([0], hdr, sizeof(*hdr));
>> > +   sg_set_buf([1], psi->buf, size);
>> > +   virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC);
>> > +   virtqueue_kick(vps->vq);
>> > +
>> > +   /* TODO: make it synchronous */
>> > +   return 0;
>>
>> The down side to this being asynchronous is the lack of error
>> reporting. Perhaps this could check hdr->type before queuing and error
>> for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send
>> it?
>
> I cannot follow, sorry.  Could you please elaborate it more?

The mention you have here of "TODO: make it synchronous" made me think
about what effects that could have. If a pstore_write() were issued
for a type other than DMESG, the above code would send it through
virtio anyway. No error reporting is possible unless this is
synchronous, but the only error here would simply be "I don't know
what anything except DMESG is", so maybe this code could refuse to
forward 

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

Hello,

On Mon, Jul 18, 2016 at 09:54:39AM +0200, Cornelia Huck wrote:
> On Mon, 18 Jul 2016 13:37:39 +0900
> Namhyung Kim  wrote:
> 
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine.  Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem.  It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> 
> Like the idea.

Thanks!

> 
> > 
> > It supports legacy PCI device using single order-2 page buffer.  As all
> 
> There should not be anything in there that limits this to pci, no?

Yep, there's no restriction AFAIK.  I just choose it to implement the poc
code quickly.

> 
> > operation of pstore is synchronous, it would be fine IMHO.  However I
> > don't know how to make write operation synchronous since it's called
> > with a spinlock held (from any context including NMI).
> > 
> > Cc: Paolo Bonzini 
> > Cc: Radim Kr??m 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Anthony Liguori 
> > Cc: Anton Vorontsov 
> > Cc: Colin Cross 
> > Cc: Kees Cook 
> > Cc: Tony Luck 
> > Cc: Steven Rostedt 
> > Cc: Ingo Molnar 
> > Cc: Minchan Kim 
> > Cc: k...@vger.kernel.org
> > Cc: qemu-de...@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim 
> > ---
> >  drivers/virtio/Kconfig |  10 ++
> >  drivers/virtio/Makefile|   1 +
> >  drivers/virtio/virtio_pstore.c | 317 
> > +
> >  include/uapi/linux/Kbuild  |   1 +
> >  include/uapi/linux/virtio_ids.h|   1 +
> >  include/uapi/linux/virtio_pstore.h |  53 +++
> >  6 files changed, 383 insertions(+)
> >  create mode 100644 drivers/virtio/virtio_pstore.c
> >  create mode 100644 include/uapi/linux/virtio_pstore.h
> > 
> 
> (...)
> 
> > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> > new file mode 100644
> > index ..6fe62c0f1508
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pstore.c
> > @@ -0,0 +1,317 @@
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define VIRT_PSTORE_ORDER2
> > +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> 
> It may make sense to make the size of the buffer configurable through
> the config space.

Right.  I'm considering it too, but it needs a buffer larger than
kmsg_bytes (= 10K) to work properly in the current implementation.  As
this version is just to verify the idea is sane and useful, I used a
fixed size buffer.  Will change in the next version.

> 
> (...)
> 
> > diff --git a/include/uapi/linux/virtio_ids.h 
> > b/include/uapi/linux/virtio_ids.h
> > index 77925f587b15..cba63225d85a 100644
> > --- a/include/uapi/linux/virtio_ids.h
> > +++ b/include/uapi/linux/virtio_ids.h
> > @@ -41,5 +41,6 @@
> >  #define VIRTIO_ID_CAIF12 /* Virtio caif */
> >  #define VIRTIO_ID_GPU  16 /* virtio GPU */
> >  #define VIRTIO_ID_INPUT18 /* virtio input */
> > +#define VIRTIO_ID_PSTORE   19 /* virtio pstore */
> 
> This id is already used by one of the new device types queued but not
> yet in the standard. IIRC, 22 is the next free one.

Ok, will update.

> 
> Speaking of the standard: I think it makes sense to at least reserve a
> device id for pstore, as the idea is sound. Maybe prepare a patch to
> the standard as well if you have time?

I'd love to.  As I mentioned earlier, I don't have enough knowledge in
this area.  Could you please provide some links about how can I do that?

Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Cornelia Huck]

On Mon, 18 Jul 2016 17:29:55 +0900
Namhyung Kim  wrote:

> On Mon, Jul 18, 2016 at 09:54:39AM +0200, Cornelia Huck wrote:
> > On Mon, 18 Jul 2016 13:37:39 +0900
> > Namhyung Kim  wrote:

> > > +#define VIRT_PSTORE_ORDER2
> > > +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> > 
> > It may make sense to make the size of the buffer configurable through
> > the config space.
> 
> Right.  I'm considering it too, but it needs a buffer larger than
> kmsg_bytes (= 10K) to work properly in the current implementation.  As
> this version is just to verify the idea is sane and useful, I used a
> fixed size buffer.  Will change in the next version.

Sure, that makes sense for a prototype. We can guard any config space
entry with a feature bit, but this one makes sense to add from the
beginning.

> > Speaking of the standard: I think it makes sense to at least reserve a
> > device id for pstore, as the idea is sound. Maybe prepare a patch to
> > the standard as well if you have time?
> 
> I'd love to.  As I mentioned earlier, I don't have enough knowledge in
> this area.  Could you please provide some links about how can I do that?

See the virtio page at OASIS
(https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio)
for a link to our subversion (yes...) repository. Just do two patches:
one to reserve a device id, and one that specifies how device and
driver work. (For examples, look at the proposed device types that have
been posted to the virtualization lists, e.g. virtio-crypto or
virtio-sdm). You just need to be patient, we're currently a bit
stalled...

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Cornelia Huck]

On Mon, 18 Jul 2016 13:37:39 +0900
Namhyung Kim  wrote:

> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.

Like the idea.

> 
> It supports legacy PCI device using single order-2 page buffer.  As all

There should not be anything in there that limits this to pci, no?

> operation of pstore is synchronous, it would be fine IMHO.  However I
> don't know how to make write operation synchronous since it's called
> with a spinlock held (from any context including NMI).
> 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: k...@vger.kernel.org
> Cc: qemu-de...@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 
> ---
>  drivers/virtio/Kconfig |  10 ++
>  drivers/virtio/Makefile|   1 +
>  drivers/virtio/virtio_pstore.c | 317 
> +
>  include/uapi/linux/Kbuild  |   1 +
>  include/uapi/linux/virtio_ids.h|   1 +
>  include/uapi/linux/virtio_pstore.h |  53 +++
>  6 files changed, 383 insertions(+)
>  create mode 100644 drivers/virtio/virtio_pstore.c
>  create mode 100644 include/uapi/linux/virtio_pstore.h
> 

(...)

> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index ..6fe62c0f1508
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,317 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define VIRT_PSTORE_ORDER2
> +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)

It may make sense to make the size of the buffer configurable through
the config space.

(...)

> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 77925f587b15..cba63225d85a 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
>  #define VIRTIO_ID_CAIF  12 /* Virtio caif */
>  #define VIRTIO_ID_GPU  16 /* virtio GPU */
>  #define VIRTIO_ID_INPUT18 /* virtio input */
> +#define VIRTIO_ID_PSTORE   19 /* virtio pstore */

This id is already used by one of the new device types queued but not
yet in the standard. IIRC, 22 is the next free one.

Speaking of the standard: I think it makes sense to at least reserve a
device id for pstore, as the idea is sound. Maybe prepare a patch to
the standard as well if you have time?

>  
>  #endif /* _LINUX_VIRTIO_IDS_H */

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Kees Cook]

On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim  wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine.  Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem.  It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
>
> It supports legacy PCI device using single order-2 page buffer.  As all
> operation of pstore is synchronous, it would be fine IMHO.  However I
> don't know how to make write operation synchronous since it's called
> with a spinlock held (from any context including NMI).
>
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: k...@vger.kernel.org
> Cc: qemu-de...@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 

This looks great to me! I'd love to use this in qemu. (Right now I go
through hoops to use the ramoops backend for testing.)

Reviewed-by: Kees Cook 

Notes below...

> ---
>  drivers/virtio/Kconfig |  10 ++
>  drivers/virtio/Makefile|   1 +
>  drivers/virtio/virtio_pstore.c | 317 
> +
>  include/uapi/linux/Kbuild  |   1 +
>  include/uapi/linux/virtio_ids.h|   1 +
>  include/uapi/linux/virtio_pstore.h |  53 +++
>  6 files changed, 383 insertions(+)
>  create mode 100644 drivers/virtio/virtio_pstore.c
>  create mode 100644 include/uapi/linux/virtio_pstore.h
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 77590320d44c..8f0e6c796c12 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -58,6 +58,16 @@ config VIRTIO_INPUT
>
>  If unsure, say M.
>
> +config VIRTIO_PSTORE
> +   tristate "Virtio pstore driver"
> +   depends on VIRTIO
> +   depends on PSTORE
> +   ---help---
> +This driver supports virtio pstore devices to save/restore
> +panic and oops messages on the host.
> +
> +If unsure, say M.
> +
>   config VIRTIO_MMIO
> tristate "Platform bus driver for memory mapped virtio devices"
> depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3dc842..bee68cb26d48 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
>  obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index ..6fe62c0f1508
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,317 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define VIRT_PSTORE_ORDER2
> +#define VIRT_PSTORE_BUFSIZE  (4096 << VIRT_PSTORE_ORDER)
> +
> +struct virtio_pstore {
> +   struct virtio_device*vdev;
> +   struct virtqueue*vq;
> +   struct pstore_info   pstore;
> +   struct virtio_pstore_hdr hdr;
> +   size_t   buflen;
> +   u64  id;
> +
> +   /* Waiting for host to ack */
> +   wait_queue_head_t   acked;
> +};
> +
> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id 
> type)
> +{
> +   u16 ret;
> +
> +   switch (type) {
> +   case PSTORE_TYPE_DMESG:
> +   ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG);
> +   break;
> +   default:
> +   ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
> +   break;
> +   }

I would love to see this support PSTORE_TYPE_CONSOLE too. It should be
relatively easy to add: I think it'd just be another virtio command?

> +
> +   return ret;
> +}
> +
> +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 
> type)
> +{
> +   enum pstore_type_id ret;
> +
> +   switch (virtio16_to_cpu(vps->vdev, type)) {
> +   case VIRTIO_PSTORE_TYPE_DMESG:
> +   ret = PSTORE_TYPE_DMESG;
> +   break;
> +   default:
> +   ret = PSTORE_TYPE_UNKNOWN;
> +   break;
> +   }
> +
> +   return ret;
>