From mboxrd@z Thu Jan  1 00:00:00 1970
From: Namhyung Kim <namhyung@kernel.org>
Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Date: Thu, 1 Sep 2016 09:03:06 +0900
Message-ID: <20160901000306.GA9510@sejong>
References: <20160831080802.13408-1-namhyung@kernel.org>
 <20160831080802.13408-2-namhyung@kernel.org>
 <20160831175007-mutt-send-email-mst@kernel.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Cc: <virtio-dev@lists.oasis-open.org>,
	<virtualization@lists.linux-foundation.org>, <kvm@vger.kernel.org>,
	<qemu-devel@nongnu.org>, LKML <linux-kernel@vger.kernel.org>, Paolo Bonzini
	<pbonzini@redhat.com>, Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
	Anthony Liguori <aliguori@amazon.com>, Anton Vorontsov <anton@enomsg.org>,
	Colin Cross <ccross@android.com>, Kees Cook <keescook@chromium.org>, Tony
 Luck <tony.luck@intel.com>, Steven Rostedt <rostedt@goodmis.org>, Ingo Molnar
	<mingo@kernel.org>, Minchan Kim <minchan@kernel.org>, Will Deacon
	<will.deacon@arm.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Return-path: <virtio-dev-return-1282-gcvd-virtio-dev=m.gmane.org@lists.oasis-open.org>
Sender: <virtio-dev@lists.oasis-open.org>
List-Post: <mailto:virtio-dev@lists.oasis-open.org>
List-Help: <mailto:virtio-dev-help@lists.oasis-open.org>
List-Unsubscribe: <mailto:virtio-dev-unsubscribe@lists.oasis-open.org>
List-Subscribe: <mailto:virtio-dev-subscribe@lists.oasis-open.org>
In-Reply-To: <20160831175007-mutt-send-email-mst@kernel.org>
Content-Disposition: inline
List-Id: kvm.vger.kernel.org

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.
> >=20
> > 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.
> >=20
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Kr=C4=8Dm=C3=A1=C5=99 <rkrcmar@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Anthony Liguori <aliguori@amazon.com>
> > Cc: Anton Vorontsov <anton@enomsg.org>
> > Cc: Colin Cross <ccross@android.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: kvm@vger.kernel.org
> > Cc: qemu-devel@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: virtio-dev@lists.oasis-open.org
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---

[SNIP]
> > +#define TYPE=5FTABLE=5FENTRY(=5Fentry)				\
> > +	{ PSTORE=5FTYPE=5F##=5Fentry, VIRTIO=5FPSTORE=5FTYPE=5F##=5Fentry }
> > +
> > +struct type=5Ftable {
> > +	int pstore;
> > +	u16 virtio;
> > +} type=5Ftable[] =3D {
> > +	TYPE=5FTABLE=5FENTRY(DMESG),
> > +};
> > +
> > +#undef TYPE=5FTABLE=5FENTRY
> > +
> > +
> > +static u16 to=5Fvirtio=5Ftype(struct virtio=5Fpstore *vps, enum pstore=
=5Ftype=5Fid type)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i =3D 0; i < ARRAY=5FSIZE(type=5Ftable); i++) {
> > +		if (type =3D=3D type=5Ftable[i].pstore)
> > +			return cpu=5Fto=5Fvirtio16(vps->vdev, type=5Ftable[i].virtio);
>=20
> 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 =5F=5Fle16 type
(according to your comment below).

>=20
>=20
> > +	}
> > +
> > +	return cpu=5Fto=5Fvirtio16(vps->vdev, VIRTIO=5FPSTORE=5FTYPE=5FUNKNOW=
N);
> > +}
> > +
> > +static enum pstore=5Ftype=5Fid from=5Fvirtio=5Ftype(struct virtio=5Fps=
tore *vps, u16 type)

This one should be '=5F=5Fle16 type' as well.


> > +{
> > +	unsigned int i;
> > +
> > +	for (i =3D 0; i < ARRAY=5FSIZE(type=5Ftable); i++) {
> > +		if (virtio16=5Fto=5Fcpu(vps->vdev, type) =3D=3D type=5Ftable[i].virt=
io)
> > +			return type=5Ftable[i].pstore;
> > +	}
> > +
> > +	return PSTORE=5FTYPE=5FUNKNOWN;
> > +}
> > +

[SNIP]
> > +
> > +struct virtio=5Fpstore=5Freq {
> > +	=5F=5Fvirtio16		cmd;
> > +	=5F=5Fvirtio16		type;
> > +	=5F=5Fvirtio32		flags;
> > +	=5F=5Fvirtio64		id;
> > +	=5F=5Fvirtio32		count;
> > +	=5F=5Fvirtio32		reserved;
> > +};
> > +
> > +struct virtio=5Fpstore=5Fres {
> > +	=5F=5Fvirtio16		cmd;
> > +	=5F=5Fvirtio16		type;
> > +	=5F=5Fvirtio32		ret;
> > +};
>=20
> Is there a reason to support legacy endian-ness?
> If not, you can just use =5F=5Fle formats.

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

Thanks,
Namhyung

>=20
>=20
> > +struct virtio=5Fpstore=5Ffileinfo {
> > +	=5F=5Fvirtio64		id;
> > +	=5F=5Fvirtio32		count;
> > +	=5F=5Fvirtio16		type;
> > +	=5F=5Fvirtio16		unused;
> > +	=5F=5Fvirtio32		flags;
> > +	=5F=5Fvirtio32		len;
> > +	=5F=5Fvirtio64		time=5Fsec;
> > +	=5F=5Fvirtio32		time=5Fnsec;
> > +	=5F=5Fvirtio32		reserved;
> > +};
> > +
> > +struct virtio=5Fpstore=5Fconfig {
> > +	=5F=5Fvirtio32		bufsize;
> > +};
> > +
> > +#endif /* =5FLINUX=5FVIRTIO=5FPSTORE=5FH */
> > --=20
> > 2.9.3