public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Anthony Liguori <aliguori@us.ibm.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Avi Kivity <avi@qumranet.com>,
	virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, Dor Laor <dor.laor@qumranet.com>
Subject: virtio config_ops refactoring
Date: Tue, 06 Nov 2007 11:48:35 -0600	[thread overview]
Message-ID: <4730A8F3.6020008@us.ibm.com> (raw)

Hi,

The current virtio config_ops do not map very well to the PCI config 
space.  Here they are:

> struct virtio_config_ops
> {
>     void *(*find)(struct virtio_device *vdev, u8 type, unsigned *len);
>     void (*get)(struct virtio_device *vdev, void *token,
>             void *buf, unsigned len);
>     void (*set)(struct virtio_device *vdev, void *token,
>             const void *buf, unsigned len);
>     u8 (*get_status)(struct virtio_device *vdev);
>     void (*set_status)(struct virtio_device *vdev, u8 status);
>     struct virtqueue *(*find_vq)(struct virtio_device *vdev,
>                      bool (*callback)(struct virtqueue *));
>     void (*del_vq)(struct virtqueue *vq);
> };

Semantically, find requires that a field have both a type and a length.  
With the exception of the VIRTQUEUE field used internally by lguest, 
type is always a unique identifier.  Since virtqueue information is not 
a required part of the config space, it seems to me that type really 
should be treated as a unique identifier.

find_vq also is curious in that it is stateful in it's enumeration.  
This adds seemingly unnecessary complexity.  The result is that in my 
PCI virtio implementation, I have to use an opaque blob that's self 
describing and variable length in the PCI config space.  This is not 
very natural at all for a PCI device!

Here is how I propose changing the config_ops:

struct virtio_config_ops
{
    bool (*get)(struct virtio_device *vdev, unsigned id, void *buf, 
unsigned len);
    bool (*set)(struct virtio_device *vdev, unsigned id, const void 
*buf, unsigned len);
    u8 (*get_status)(struct virtio_device *vdev);
    void (*set_status)(struct virtio_device *vdev, u8 status);
    struct virtqueue *(*get_vq)(struct virtio_device *vdev, unsigned 
index, bool (*callback)(struct virtqueue *));
    void (*del_vq)(struct virtqueue *vq);
};

config_ops->get() returns false if the id is invalid as does ->set().  
get_vq() returns NULL if the index is not a valid virtqueue (and we'll 
mandate that all virtqueues are in order starting at 0).

The id space is defined by the driver itself but is guaranteed to be 
non-overlapping and starting from 0.  For instance, the block device 
would have:

/* The capacity (in 512-byte sectors). */
#define VIRTIO_CONFIG_BLK_F_CAPACITY    0x00
/* The maximum segment size. */
#define VIRTIO_CONFIG_BLK_F_SIZE_MAX    0x08
/* The maximum number of segments. */
#define VIRTIO_CONFIG_BLK_F_SEG_MAX    0x12

This maps very well to the PCI config space.  For lguest, the actual 
config items would simply be packed in a single page.  Since lguest uses 
the config space for virtqueues, lguest_device_desc will have to be 
changed to also contain an array of virtqueue infos.  It doesn't prevent 
an implementation from using the id's as opaque keys though (as one 
might do for Xen since presumably the configuration data would be 
represented within xenbus).

If there's agreement that this approach is better, I'll start submitting 
patches.

Regards,

Anthony Liguori

             reply	other threads:[~2007-11-06 17:48 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-11-06 17:48 Anthony Liguori [this message]
2007-11-06 22:57 ` virtio config_ops refactoring Rusty Russell
     [not found] ` <200711070957.44165.rusty__13109.5125260346$1194390035$gmane$org@rustcorp.com.au>
2007-11-06 23:05   ` Anthony Liguori
2007-11-07  6:04     ` [PATCH] " Rusty Russell
2007-11-07 17:30       ` Anthony Liguori
2007-11-08  2:20         ` Rusty Russell
     [not found]         ` <200711081320.35844.rusty__6233.15023626692$1194488552$gmane$org@rustcorp.com.au>
2007-11-08  2:41           ` Anthony Liguori
2007-11-08 22:24             ` Rusty Russell
2007-11-08 22:33               ` Anthony Liguori
2007-11-08 22:47                 ` [Lguest] " ron minnich
2007-11-08 22:49                   ` Anthony Liguori
     [not found]                   ` <4733A6F5.3040202@qumranet.com>
2007-11-09  3:17                     ` Anthony Liguori
2007-11-09 11:54                 ` Rusty Russell
2007-11-09 23:45                   ` Anthony Liguori
2007-11-10  7:58                     ` Rusty Russell
2007-11-10 22:08                       ` Anthony Liguori

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4730A8F3.6020008@us.ibm.com \
    --to=aliguori@us.ibm.com \
    --cc=avi@qumranet.com \
    --cc=dor.laor@qumranet.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox