PCI, lguest and s390 can all only support 256-byte configuration
space. So, this giant field broke just about everyone.
Unfortunately, removing it is not so simple: we don't want to break
old userspace, but we're going to want to re-use that part of the
struct.
So, modern users can #define VIRTIO_BLK_IDENTIFY_DEPRECATED to indicate
that they know it's no longer in the config struct, and can use any
new features (all new features which add a configuration field will
conflict with this deprecated one).
Signed-off-by: Rusty Russell <[email protected]>
---
drivers/block/virtio_blk.c | 20 ++++++++++++++++++--
include/linux/virtio_blk.h | 10 +++++++++-
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -1,4 +1,6 @@
//#define DEBUG
+/* We want to see new fields: we use our own struct if we fall back to this */
+#define VIRTIO_BLK_IDENTIFY_DEPRECATED
#include <linux/spinlock.h>
#include <linux/blkdev.h>
#include <linux/hdreg.h>
@@ -183,8 +185,22 @@ static void do_virtblk_request(struct re
vblk->vq->vq_ops->kick(vblk->vq);
}
-/* return ATA identify data
+/* return ATA identify data: deprecated
*/
+struct virtio_blk_config_deprecated {
+ /* The capacity (in 512-byte sectors). */
+ __u64 capacity;
+ /* The maximum segment size (if VIRTIO_BLK_F_SIZE_MAX) */
+ __u32 size_max;
+ /* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
+ __u32 seg_max;
+ /* geometry the device (if VIRTIO_BLK_F_GEOMETRY) */
+ struct virtio_blk_geometry geometry;
+ /* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
+ __u32 blk_size;
+ __u8 identify[VIRTIO_BLK_ID_BYTES];
+} __attribute__((packed));
+
static int virtblk_identify(struct gendisk *disk, void *argp)
{
struct virtio_blk *vblk = disk->private_data;
@@ -196,7 +212,7 @@ static int virtblk_identify(struct gendi
goto out;
err = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_IDENTIFY,
- offsetof(struct virtio_blk_config, identify), opaque,
+ offsetof(struct virtio_blk_config_deprecated, identify), opaque,
VIRTIO_BLK_ID_BYTES);
if (err)
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -13,7 +13,7 @@
#define VIRTIO_BLK_F_RO 5 /* Disk is read-only */
#define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/
#define VIRTIO_BLK_F_SCSI 7 /* Supports scsi command passthru */
-#define VIRTIO_BLK_F_IDENTIFY 8 /* ATA IDENTIFY supported */
+#define VIRTIO_BLK_F_IDENTIFY 8 /* ATA IDENTIFY support (deprecated) */
#define VIRTIO_BLK_F_FLUSH 9 /* Cache flush command support */
#define VIRTIO_BLK_ID_BYTES (sizeof(__u16[256])) /* IDENTIFY DATA */
@@ -33,7 +33,15 @@ struct virtio_blk_config {
} geometry;
/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
__u32 blk_size;
+#ifndef VIRTIO_BLK_IDENTIFY_DEPRECATED
+ /*
+ * New code shouldn't use this, but it's under the #ifndef so old
+ * userspace doesn't break.
+ */
__u8 identify[VIRTIO_BLK_ID_BYTES];
+#else
+ /* New fields go here: (all new extensions conflict with F_IDENTIFY) */
+#endif
} __attribute__((packed));
/*
Am Dienstag 29 September 2009 19:18:09 schrieb Rusty Russell:
> PCI, lguest and s390 can all only support 256-byte configuration
> space. So, this giant field broke just about everyone.
> Unfortunately, removing it is not so simple: we don't want to break
> old userspace, but we're going to want to re-use that part of the
> struct.
>
> So, modern users can #define VIRTIO_BLK_IDENTIFY_DEPRECATED to indicate
> that they know it's no longer in the config struct, and can use any
> new features (all new features which add a configuration field will
> conflict with this deprecated one).
Since s390 never used the giant id field, it would be ok for us just delete it
(without the #define). IIRC kvm-userspace also never used that. Since qemu
upstream seems to use that field your way seems to be the only compatible...
O dear, virtio used to look pretty ;-).
I think somewhen in the future we have to create a virtio2 that gets rid of all
the stuff that accumulated in the early phase of Linux virtualization.
Anyway, your patch was tested successfully on s390 to survive the current
userspace.
Tested-by: Christian Borntraeger <[email protected]>
Christian Borntraeger wrote:
> Am Dienstag 29 September 2009 19:18:09 schrieb Rusty Russell:
>> PCI, lguest and s390 can all only support 256-byte configuration
>> space. So, this giant field broke just about everyone.
>> Unfortunately, removing it is not so simple: we don't want to break
>> old userspace, but we're going to want to re-use that part of the
>> struct.
>>
>> So, modern users can #define VIRTIO_BLK_IDENTIFY_DEPRECATED to indicate
>> that they know it's no longer in the config struct, and can use any
>> new features (all new features which add a configuration field will
>> conflict with this deprecated one).
>
>
> Since s390 never used the giant id field, it would be ok for us just delete it
> (without the #define). IIRC kvm-userspace also never used that. Since qemu
> upstream seems to use that field your way seems to be the only compatible...
It seems simply retiring use of the VIRTIO_BLK_F_IDENTIFY
binary value should be sufficient. The patch which
implemented the cooperating logic in qemu was dropped
due to the config space issue. So the case of a virtio_blk
driver successfully negotiating the feature and trying to
access the now depreciated structure area should effectively
be a non-issue. The bit value can be reclaimed for reuse
in the future when concern over any suspected usage falls
into the noise.
-john
--
[email protected]
On Tue, 6 Oct 2009 02:50:12 am john cooper wrote:
> It seems simply retiring use of the VIRTIO_BLK_F_IDENTIFY
> binary value should be sufficient. The patch which
> implemented the cooperating logic in qemu was dropped
> due to the config space issue.
OK, I'll just remove it then.
Also, I've been convinced to just hand over the 20 byte serial number
since we're revisiting this. It's simple, and there's no clear consensus.
Thanks,
Rusty.
Rusty Russell wrote:
> Also, I've been convinced to just hand over the 20 byte serial number
> since we're revisiting this. It's simple, and there's no clear consensus.
I'm fine with this approach as it is where
I originally started with this Sisyphus Patch.
My bias toward pressing the ATA identify
interface into service here was just to reuse
an existing interface and make life easier
from the perspective of the guest.
On the subject of retrieving the S/N alone, this
is the form of the simplistic ioctl originally
used to do so:
unsigned char sn[80];
:
sn[0] = sizeof(sn);
ioctl(fd, VBLK_GET_SN, &sn);
-john
--
[email protected]