On Thu, Oct 11, 2012 at 11:48:22AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@xxxxxxxxxx> writes:
> > On Mon, Oct 08, 2012 at 12:51:25PM +1030, Rusty Russell wrote:
> >> Note before anyone gets confused; we were talking about using the PCI
> >> config space to indicate what BAR(s) the virtio stuff is in. An
> >> alternative would be to simply specify a new layout format in BAR1.
> >
> > One problem we are still left with is this: device specific
> > config accesses are still non atomic.
> > This is a problem for multibyte fields such as MAC address
> > where MAC could change while we are accessing it.
>
> It's also a problem for related fields, eg. console width and height, or
> disk geometry.
>
> > I was thinking about some backwards compatible way to solve this, but if
> > we are willing to break compatiblity or use some mode switch, how about
> > we give up on virtio config space completely, and do everything besides
> > IO and ISR through guest memory?
>
> I think there's still a benefit in the simple publishing of information:
> I don't really want to add a control queue for the console.
One reason I thought using a vq is handy is because this would
let us get by with a single MSI vector. Currently we need at least 2
for config changes + a shared one for vqs.
But I don't insist.
> But
> inevitably, once-static information can change in later versions, and
> it's horrible to have config information plus a bit that says "don't use
> this, use the control queue".
>
> Here's a table from a quick audit:
>
> Driver Config Device changes Driver writes... after init?
> net Y Y N N
> block Y Y Y Y
> console Y Y N N
> rng N N N N
> balloon Y Y Y Y
> scsi Y N Y N
> 9p Y N N N
>
> For config space reads, I suggest the driver publish a generation count.
You mean device?
> For writes, the standard seems to be a commit latch. We could abuse the
> generation count for this: the driver writes to it to commit config
> changes.
I think this will work. There are a couple of things that bother me:
This assumes read accesses have no side effects, and these are sometimes handy.
Also the semantics for write aren't very clear to me.
I guess device must buffer data until generation count write?
This assumes the device has a buffer to store writes,
and it must track each byte written. I kind of dislike this
tracking of accessed bytes. Also, device would need to resolve conflicts
if any in some device specific way.
Maybe it's a good idea to make the buffer accesses explicit instead?
IOW require driver to declare intent to read/request write of a specific
config range. We could for example do it like this:
__le32 config_offset;
__le32 config_len;
__u8 config_cmd; /* write-only: 0 - read, 1 - write
config_len bytes at config_offset
from/to config space to/from device memory */
But maybe this is over-engineering?
> ie:
> /* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> struct virtio_pci_common_cfg {
> /* About the whole device. */
> __le32 device_feature_select; /* read-write */
> __le32 device_feature; /* read-only */
> __le32 guest_feature_select; /* read-write */
> __le32 guest_feature; /* read-only */
> __le32 config_gen_and_latch; /* read-write */
> __le16 msix_config; /* read-write */
> __u8 device_status; /* read-write */
> __u8 unused;
>
> /* About a specific virtqueue. */
> __le16 queue_select; /* read-write */
> __le16 queue_align; /* read-write, power of 2. */
> __le16 queue_size; /* read-write, power of 2. */
> __le16 queue_msix_vector;/* read-write */
> __le64 queue_address; /* read-write: 0xFFFFFFFFFFFFFFFF == DNE. */
> };
>
> Thoughts?
> Rusty.
> PS. Let's make all the virtio-device config LE, too...
We'll need some new API for devices then: currently we pass bytes.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
