[Qemu-devel] [PATCH] qemu: make virtio-blk PCI compliant by default

[Qemu-devel] [PATCH] qemu: make virtio-blk PCI compliant by default

[Michael S. Tsirkin]

commit bf011293faaa7f87e4de83185931e7411b794128 made virtio-blk-pci not
PCI-compliant, since it makes region 0 (which is an i/o region)
size > 256, and, since PCI 2.1, i/o regions are limited to 256 bytes size.

When the ATA serial number feature is off, which is the default,
make the device spec compliant again, by making region 0 smaller.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reported-by: Vadim Rozenfeld <vrozenfe@redhat.com>
Tested-by: Vadim Rozenfeld <vrozenfe@redhat.com>

---

Note: the companion feature in guest kernel was added by
1d589bb16b825b3a7b4edd34d997f1f1f953033d in linux 2.6.31-rc1.

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index a33eafb..3652c0a 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -27,6 +27,7 @@ typedef struct VirtIOBlock
     void *rq;
     char serial_str[BLOCK_SERIAL_STRLEN + 1];
     QEMUBH *bh;
+    size_t config_size;
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -362,7 +363,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     virtio_identify_template(&blkcfg);
     memcpy(&blkcfg.identify[VIRTIO_BLK_ID_SN], s->serial_str,
         VIRTIO_BLK_ID_SN_BYTES);
-    memcpy(config, &blkcfg, sizeof(blkcfg));
+    memcpy(config, &blkcfg, s->config_size);
 }
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
@@ -419,18 +420,21 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
     VirtIOBlock *s;
     int cylinders, heads, secs;
     static int virtio_blk_id;
-    char *ps;
+    char *ps = (char *)drive_get_serial(dinfo->bdrv);
+    size_t size = strlen(ps) ? sizeof(struct virtio_blk_config) :
+	    offsetof(struct virtio_blk_config, _blk_size);
 
     s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
-                                          sizeof(struct virtio_blk_config),
+                                          size,
                                           sizeof(VirtIOBlock));
 
+    s->config_size = size;
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
     s->bs = dinfo->bdrv;
     s->rq = NULL;
-    if (strlen(ps = (char *)drive_get_serial(s->bs)))
+    if (strlen(ps))
         strncpy(s->serial_str, ps, sizeof(s->serial_str));
     else
         snprintf(s->serial_str, sizeof(s->serial_str), "0");

-- 
1.6.2.5

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[john cooper]

Michael S. Tsirkin wrote:
> commit bf011293faaa7f87e4de83185931e7411b794128 made virtio-blk-pci not
> PCI-compliant, since it makes region 0 (which is an i/o region)
> size > 256, and, since PCI 2.1, i/o regions are limited to 256 bytes size.
> 
> When the ATA serial number feature is off, which is the default,
> make the device spec compliant again, by making region 0 smaller.

I'd hazard this is the cause of the breakage others
encountered -- even when the driver was initialized
but unused.  For some odd reason I hadn't seen nor
been able to reproduce the failure.

The mock-up of an entire ATA IDENTIFY page is really
overkill for what we're trying to accomplish here,
namely passing a 20 byte S/N from qemu to the guest.
However emulating and passing an IDENTIFY page allows
guest apps to interpret the information via an
existing interface, with the guest driver doing nothing
more than transferring the data as opaque.  During
review, other defined fields of the IDENTIFY page were
speculated to be potentially useful thus the entire
512 byte page was passed wholesale.  But it is clearly
more trouble than benefit at this point.  I'll rework
the patch or use an alternate mechanism.

-john

-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Michael S. Tsirkin]

On Tue, Sep 08, 2009 at 03:40:08AM -0400, john cooper wrote:
> Michael S. Tsirkin wrote:
> > commit bf011293faaa7f87e4de83185931e7411b794128 made virtio-blk-pci not
> > PCI-compliant, since it makes region 0 (which is an i/o region)
> > size > 256, and, since PCI 2.1, i/o regions are limited to 256 bytes size.
> > 
> > When the ATA serial number feature is off, which is the default,
> > make the device spec compliant again, by making region 0 smaller.
> 
> I'd hazard this is the cause of the breakage others
> encountered -- even when the driver was initialized
> but unused.  For some odd reason I hadn't seen nor
> been able to reproduce the failure.
> 
> The mock-up of an entire ATA IDENTIFY page is really
> overkill for what we're trying to accomplish here,
> namely passing a 20 byte S/N from qemu to the guest.
> However emulating and passing an IDENTIFY page allows
> guest apps to interpret the information via an
> existing interface, with the guest driver doing nothing
> more than transferring the data as opaque.  During
> review, other defined fields of the IDENTIFY page were
> speculated to be potentially useful thus the entire
> 512 byte page was passed wholesale.  But it is clearly
> more trouble than benefit at this point.  I'll rework
> the patch or use an alternate mechanism.

BTW, will the change you have in mind affect the guest driver as well?
If yes, maybe revert 1d589bb16b825b3a7b4edd34d997f1f1f953033d in linux
before 2.6.31 is out? Otherwise we'll be stuck supporting the bad
interface from upstream 2.6.31 in upstream qemu ...

> -john
> 
> -- 
> john.cooper@redhat.com

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Michael S. Tsirkin]

On Tue, Sep 08, 2009 at 03:40:08AM -0400, john cooper wrote:
> Michael S. Tsirkin wrote:
> > commit bf011293faaa7f87e4de83185931e7411b794128 made virtio-blk-pci not
> > PCI-compliant, since it makes region 0 (which is an i/o region)
> > size > 256, and, since PCI 2.1, i/o regions are limited to 256 bytes size.
> > 
> > When the ATA serial number feature is off, which is the default,
> > make the device spec compliant again, by making region 0 smaller.
> 
> I'd hazard this is the cause of the breakage others
> encountered -- even when the driver was initialized
> but unused.  For some odd reason I hadn't seen nor
> been able to reproduce the failure.
> 
> The mock-up of an entire ATA IDENTIFY page is really
> overkill for what we're trying to accomplish here,
> namely passing a 20 byte S/N from qemu to the guest.
> However emulating and passing an IDENTIFY page allows
> guest apps to interpret the information via an
> existing interface, with the guest driver doing nothing
> more than transferring the data as opaque.  During
> review, other defined fields of the IDENTIFY page were
> speculated to be potentially useful thus the entire
> 512 byte page was passed wholesale.  But it is clearly
> more trouble than benefit at this point.  I'll rework
> the patch or use an alternate mechanism.
> 
> -john

For now, what my patch does is fix the PCI compliance issue for everyone
who uses the default setup (without the serial #). With serial disabled,
we should not reserve any space in region 0 (not even 20 bytes).

So I propose we apply this patch for now, and then your patch only has
to handle the case of serial number enabled. Makes sense?  If yes pls
ack.


> -- 
> john.cooper@redhat.com

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[john cooper]

Michael S. Tsirkin wrote:

> For now, what my patch does is fix the PCI compliance issue for everyone
> who uses the default setup (without the serial #). With serial disabled,
> we should not reserve any space in region 0 (not even 20 bytes).
> 
> So I propose we apply this patch for now, and then your patch only has
> to handle the case of serial number enabled. Makes sense?  If yes pls
> ack.

Sorry, I intended to do so in my previous mail.
Looks fine, ACK.

-john

-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Rusty Russell]

On Tue, 8 Sep 2009 05:28:31 pm Michael S. Tsirkin wrote:
> On Tue, Sep 08, 2009 at 03:40:08AM -0400, john cooper wrote:
> > Michael S. Tsirkin wrote:
> > > commit bf011293faaa7f87e4de83185931e7411b794128 made virtio-blk-pci not
> > > PCI-compliant, since it makes region 0 (which is an i/o region)
> > > size > 256, and, since PCI 2.1, i/o regions are limited to 256 bytes size.
> > > 
> > > When the ATA serial number feature is off, which is the default,
> > > make the device spec compliant again, by making region 0 smaller.
> > 
> > I'd hazard this is the cause of the breakage others
> > encountered -- even when the driver was initialized
> > but unused.  For some odd reason I hadn't seen nor
> > been able to reproduce the failure.
> > 
> > The mock-up of an entire ATA IDENTIFY page is really
> > overkill for what we're trying to accomplish here,
> > namely passing a 20 byte S/N from qemu to the guest.
> > However emulating and passing an IDENTIFY page allows
> > guest apps to interpret the information via an
> > existing interface, with the guest driver doing nothing
> > more than transferring the data as opaque.  During
> > review, other defined fields of the IDENTIFY page were
> > speculated to be potentially useful thus the entire
> > 512 byte page was passed wholesale.  But it is clearly
> > more trouble than benefit at this point.  I'll rework
> > the patch or use an alternate mechanism.
> 
> BTW, will the change you have in mind affect the guest driver as well?
> If yes, maybe revert 1d589bb16b825b3a7b4edd34d997f1f1f953033d in linux
> before 2.6.31 is out? Otherwise we'll be stuck supporting the bad
> interface from upstream 2.6.31 in upstream qemu ...

Too late, but no big deal.  We just add another feature bit for the
new method.  We can have a command virtqueue for queries such as this.

(Interestingly, this same issue broke lguest, with the 8-bit limit on
configuration space sizes).

Thanks,
Rusty.

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[john cooper]

Rusty Russell wrote:

> Too late, but no big deal.  We just add another feature bit for the
> new method.  We can have a command virtqueue for queries such as this.
> 
> (Interestingly, this same issue broke lguest, with the 8-bit limit on
> configuration space sizes).

I believe the existing guest visible interface
is preservable as-is without introducing any
ATA specific knowledge into the driver.  A
trivial alternate R/O mapping for the page
into the guest is all that is required.

I ran out of time last week to tie this off
but hopefully will do so shortly.

Thanks,

-john

-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Rusty Russell]

On Mon, 14 Sep 2009 09:09:59 pm Michael S. Tsirkin wrote:
> On Tue, Sep 08, 2009 at 03:40:08AM -0400, john cooper wrote:
> > Michael S. Tsirkin wrote:
> > > commit bf011293faaa7f87e4de83185931e7411b794128 made virtio-blk-pci not
> > > PCI-compliant, since it makes region 0 (which is an i/o region)
> > > size > 256, and, since PCI 2.1, i/o regions are limited to 256 bytes size.
> > > 
> > > When the ATA serial number feature is off, which is the default,
> > > make the device spec compliant again, by making region 0 smaller.
> > 
> > I'd hazard this is the cause of the breakage others
> > encountered -- even when the driver was initialized
> > but unused.  For some odd reason I hadn't seen nor
> > been able to reproduce the failure.
> > 
> > The mock-up of an entire ATA IDENTIFY page is really
> > overkill for what we're trying to accomplish here,
> > namely passing a 20 byte S/N from qemu to the guest.
> > However emulating and passing an IDENTIFY page allows
> > guest apps to interpret the information via an
> > existing interface, with the guest driver doing nothing
> > more than transferring the data as opaque.  During
> > review, other defined fields of the IDENTIFY page were
> > speculated to be potentially useful thus the entire
> > 512 byte page was passed wholesale.  But it is clearly
> > more trouble than benefit at this point.  I'll rework
> > the patch or use an alternate mechanism.
> > 
> > -john
> 
> For now, what my patch does is fix the PCI compliance issue for everyone
> who uses the default setup (without the serial #). With serial disabled,
> we should not reserve any space in region 0 (not even 20 bytes).
> 
> So I propose we apply this patch for now, and then your patch only has
> to handle the case of serial number enabled. Makes sense?  If yes pls
> ack.

lguest uses the same workaround (since we don't support that feature anyway
at the moment).

Thanks!
Rusty.

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Avi Kivity]

On 09/21/2009 06:47 PM, john cooper wrote:
> Rusty Russell wrote:
>
>    
>> Too late, but no big deal.  We just add another feature bit for the
>> new method.  We can have a command virtqueue for queries such as this.
>>
>> (Interestingly, this same issue broke lguest, with the 8-bit limit on
>> configuration space sizes).
>>      
> I believe the existing guest visible interface
> is preservable as-is without introducing any
> ATA specific knowledge into the driver.  A
> trivial alternate R/O mapping for the page
> into the guest is all that is required.
>
> I ran out of time last week to tie this off
> but hopefully will do so shortly.
>
>    

Can we just read this page as a virtqueue command instead of having it 
mapped permanently?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[john cooper]

Avi Kivity wrote:
> On 09/21/2009 06:47 PM, john cooper wrote:
>> Rusty Russell wrote:
>>
>>   
>>> Too late, but no big deal.  We just add another feature bit for the
>>> new method.  We can have a command virtqueue for queries such as this.
>>>
>>> (Interestingly, this same issue broke lguest, with the 8-bit limit on
>>> configuration space sizes).
>>>      
>> I believe the existing guest visible interface
>> is preservable as-is without introducing any
>> ATA specific knowledge into the driver.  A
>> trivial alternate R/O mapping for the page
>> into the guest is all that is required.
>>
>> I ran out of time last week to tie this off
>> but hopefully will do so shortly.
>>
>>    
> 
> Can we just read this page as a virtqueue command instead of having it
> mapped permanently?

Probably although I hadn't looked specifically
at doing so.   Mapping the data via an unused
pci bar is pretty trivial and seemed minimally
intrusive to the existing driver.

-john

-- 
john.cooper@redhat.com

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Avi Kivity]

On 09/22/2009 05:21 PM, john cooper wrote:
>> Can we just read this page as a virtqueue command instead of having it
>> mapped permanently?
>>      
> Probably although I hadn't looked specifically
> at doing so.   Mapping the data via an unused
> pci bar is pretty trivial and seemed minimally
> intrusive to the existing driver.
>    

We'll run out of bars if we expend them like that.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Michael S. Tsirkin]

On Tue, Sep 22, 2009 at 05:27:22PM +0300, Avi Kivity wrote:
> On 09/22/2009 05:21 PM, john cooper wrote:
>>> Can we just read this page as a virtqueue command instead of having it
>>> mapped permanently?
>>>      
>> Probably although I hadn't looked specifically
>> at doing so.   Mapping the data via an unused
>> pci bar is pretty trivial and seemed minimally
>> intrusive to the existing driver.
>>    
>
> We'll run out of bars if we expend them like that.

BAR1 is used for MSI-X already (if MSI-X is enabled), you can just add
your data there. I would report the offset within the BAR somewhere
in io space or in configuration space, so that we can relocate tables
later if we like without changing guests.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Avi Kivity]

On 09/22/2009 05:41 PM, Michael S. Tsirkin wrote:
>>>
>>> Probably although I hadn't looked specifically
>>> at doing so.   Mapping the data via an unused
>>> pci bar is pretty trivial and seemed minimally
>>> intrusive to the existing driver.
>>>
>>>        
>> We'll run out of bars if we expend them like that.
>>      
> BAR1 is used for MSI-X already (if MSI-X is enabled), you can just add
> your data there. I would report the offset within the BAR somewhere
> in io space or in configuration space, so that we can relocate tables
> later if we like without changing guests.
>    

I'd really like to avoid such entanglements.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[john cooper]

Avi Kivity wrote:
> On 09/22/2009 05:21 PM, john cooper wrote:
>>> Can we just read this page as a virtqueue command instead of having it
>>> mapped permanently?
>>>      
>> Probably although I hadn't looked specifically
>> at doing so.   Mapping the data via an unused
>> pci bar is pretty trivial and seemed minimally
>> intrusive to the existing driver.
>>    
> 
> We'll run out of bars if we expend them like that.

Agreed.  However my motivation here was to use a
single additional bar specifically to compensate
for the PCI spec imposed 256 byte size limitation
of the config space mapping.  As we're defining the
content/size of this area, future use to accommodate
additional data should be unrestricted.

If using an additional bar is still a concern I
can certainly have a look at accomplishing the
same via virtqueue scaffolding.

-john

-- 
john.cooper@redhat.com

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[Anthony Liguori]

john cooper wrote:
> Avi Kivity wrote:
>   
>> On 09/22/2009 05:21 PM, john cooper wrote:
>>     
>>>> Can we just read this page as a virtqueue command instead of having it
>>>> mapped permanently?
>>>>      
>>>>         
>>> Probably although I hadn't looked specifically
>>> at doing so.   Mapping the data via an unused
>>> pci bar is pretty trivial and seemed minimally
>>> intrusive to the existing driver.
>>>    
>>>       
>> We'll run out of bars if we expend them like that.
>>     
>
> Agreed.  However my motivation here was to use a
> single additional bar specifically to compensate
> for the PCI spec imposed 256 byte size limitation
> of the config space mapping.  As we're defining the
> content/size of this area, future use to accommodate
> additional data should be unrestricted.
>   

Why expose the whole ATAPI page instead of just the serial number?

I think the proper solution is to move the config to a separate bar 
that's MMIO instead of PIO.  config access is never performance 
sensitive and an MMIO bar has less restrictions on size.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] qemu: make virtio-blk PCI compliant by default

[john cooper]

Anthony Liguori wrote:
> john cooper wrote:
>> Avi Kivity wrote:
>>  
>>> On 09/22/2009 05:21 PM, john cooper wrote:
>>>    
>>>>> Can we just read this page as a virtqueue command instead of having it
>>>>> mapped permanently?
>>>>>              
>>>> Probably although I hadn't looked specifically
>>>> at doing so.   Mapping the data via an unused
>>>> pci bar is pretty trivial and seemed minimally
>>>> intrusive to the existing driver.
>>>>          
>>> We'll run out of bars if we expend them like that.
>>>     
>>
>> Agreed.  However my motivation here was to use a
>> single additional bar specifically to compensate
>> for the PCI spec imposed 256 byte size limitation
>> of the config space mapping.  As we're defining the
>> content/size of this area, future use to accommodate
>> additional data should be unrestricted.
>>   
> 
> Why expose the whole ATAPI page instead of just the serial number?

Exposing the s/n alone was what I'd done originally
which in retrospect was well contained within the
pci config area -- even if squandering the somewhat
limited space in that area.

However exposing it as an identify page allows use
of the existing HDIO_GET_IDENTITY ioctl (and existing
use of that interface, eg: "hdparm -i") as a means to
extract the s/n along with potentially useful related
data in the page.

Creation of the identify structure is within qemu
as that is where the information exists.  And this
allows the virtio_blk driver to treat the data as
opaque, simply exporting it uninterpreted via a copyout.
So the only real issue remaining is how to best saw
a hole between qemu and the guest virtio_blk driver
to transfer the identity data.

> I think the proper solution is to move the config to a separate bar 
> that's MMIO instead of PIO.  config access is never performance 
> sensitive and an MMIO bar has less restrictions on size.

That's exactly what I've done.  I'll clean up the
patch and post it here so we can have a more concrete
discussion.

-john 


-- 
john.cooper@third-harmonic.com

[Qemu-devel] [PATCH 0/2] fix virtio_blk serial pci config breakage

[john cooper]

This is a correction to the previous version
which violated the PCI config space 256 byte
limitation.

The crux of the feature is simply passing a qemu
received virtio serial number to/through the guest
driver and making it available to guest userspace
via a preexisting interface.

To accomplish this, ATA IDENTIFY data as implemented
by the HDIO_GET_IDENTITY ioctl is returned to the
caller for the virtio_blk drive of interest.  Content
of this identify structure is created by qemu and
passed wholesale to guest userspace without
interpretation by the guest driver.

The change this patch implements is passing of the
identify data through a mapping established by
PCI BAR #5 rather than the PCI config area, the
latter of which resulted in the above breakage.

virtio_blk with this patch now pulls the information
from the mapped bar area and includes minimal
scaffolding to help map bars external to virtio_pci.

-john


-- 
john.cooper@redhat.com

[Qemu-devel] [PATCH 1/2] fix virtio_blk serial pci config breakage

[john cooper]

[This patch in part replicates the first version of
this feature which was rolled-back due to breakage
it caused extending the pci config area beyond the
256 byte limit.]

Add missing logic in qemu to accept a command line
serial number parameter for virtio_blk, integrate
it into an ATA IDENTIFY structure, map the resulting
data via PCI BAR #5.

Signed-off-by: john cooper <john.cooper@redhat.com>
---


diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index dad4ef0..9a067c8 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -19,12 +19,18 @@
 # include <scsi/sg.h>
 #endif
 
+#define VBLK_IDENTIFY_SIZE      512
+#define VBLK_IDENTIFY_AMASK	(VBLK_IDENTIFY_SIZE - 1)
+#define VBLK_IDENTIFY_CFGSLOT   5 /* PCI BAR #5 maps identify/config area */
+
 typedef struct VirtIOBlock
 {
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
+    uint32_t mmio_io_addr;
+    uint16_t identify[VIRTIO_BLK_ID_LEN];
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -32,6 +38,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
     return (VirtIOBlock *)vdev;
 }
 
+/* store identify data in little endian format
+ */
+static inline void put_le16(uint16_t *p, unsigned int v)
+{
+    *p = cpu_to_le16(v);
+}
+
+/* copy to *dst from *src, nul pad dst tail as needed to len bytes
+ */
+static inline void padstr(char *dst, const char *src, int len)
+{
+    while (len--)
+        *dst++ = *src ? *src++ : '\0';
+}
+
+/* setup simulated identify data as appropriate for virtio block device
+ *
+ * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
+ */
+static inline void virtio_identify_template(VirtIOBlock *s)
+{
+    uint16_t *p = s->identify;
+    uint64_t lba_sectors;
+
+    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
+    put_le16(p + 0, 0x0);                            /* ATA device */
+    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
+    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
+    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
+    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
+    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
+    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
+    put_le16(p + 81, 0x16);
+    put_le16(p + 82, 0x400);
+    put_le16(p + 83, 0x400);
+    bdrv_get_geometry(s->bs, &lba_sectors);
+    put_le16(p + 100, lba_sectors);
+    put_le16(p + 101, lba_sectors >> 16);
+    put_le16(p + 102, lba_sectors >> 32);
+    put_le16(p + 103, lba_sectors >> 48);
+}
+
 typedef struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
@@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 {
+    VirtIOBlock *s = to_virtio_blk(vdev);
     uint32_t features = 0;
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
@@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #ifdef __linux__
     features |= (1 << VIRTIO_BLK_F_SCSI);
 #endif
+    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
+        features |= 1 << VIRTIO_BLK_F_IDENTIFY;
 
     return features;
 }
@@ -348,6 +399,60 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+/* fan-in for VBLK_IDENTIFY_CFGSLOT read operations
+ */
+static uint32_t virtio_blk_io_read(void *opaque, target_phys_addr_t addr,
+                                   uint8_t access_size)
+{
+    VirtIOBlock *s = opaque;
+    uint16_t byte_index = addr & VBLK_IDENTIFY_AMASK;
+    uint8_t *source = &((uint8_t *)s->identify)[byte_index];
+
+    switch (access_size)
+        {
+    case 1:
+    	return (*(uint8_t *)source);
+    case 2:
+    	return (*(uint16_t *)source);
+    case 4:
+    	return (*(uint32_t *)source);
+        }
+    return 0;
+}
+
+static uint32_t virtio_blk_mmio_readb(void *opaque, target_phys_addr_t addr)
+{
+    return virtio_blk_io_read(opaque, addr, 1);
+}
+
+static uint32_t virtio_blk_mmio_readw(void *opaque, target_phys_addr_t addr)
+{
+    return virtio_blk_io_read(opaque, addr, 2);
+}
+
+static uint32_t virtio_blk_mmio_readl(void *opaque, target_phys_addr_t addr)
+{
+    return virtio_blk_io_read(opaque, addr, 4);
+}
+
+static CPUReadMemoryFunc *virtio_blk_mmio_read[3] = {
+    virtio_blk_mmio_readb,
+    virtio_blk_mmio_readw,
+    virtio_blk_mmio_readl,
+};
+
+static CPUWriteMemoryFunc *virtio_blk_mmio_write[3] = {
+    NULL, NULL, NULL,
+};
+
+static void virtio_blk_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
+                           uint32_t size, int type)
+{
+    VirtIOBlock *s = (VirtIOBlock *)pci_dev;
+
+    cpu_register_physical_memory(addr, VBLK_IDENTIFY_SIZE, s->mmio_io_addr);
+}
+
 void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
 {
     VirtIOBlock *s;
@@ -360,10 +465,18 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
                                        VIRTIO_ID_BLOCK,
                                        PCI_CLASS_STORAGE_OTHER, 0x00,
-                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
+                                       sizeof(struct virtio_blk_config),
+                                       sizeof(VirtIOBlock));
     if (!s)
         return NULL;
 
+    s->mmio_io_addr =
+        cpu_register_io_memory(0, virtio_blk_mmio_read,
+                               virtio_blk_mmio_write, s);
+    pci_register_io_region(&((VirtIODevice *)s)->pci_dev,
+                           VBLK_IDENTIFY_CFGSLOT, VBLK_IDENTIFY_SIZE,
+                           PCI_ADDRESS_SPACE_MEM, virtio_blk_map);
+
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
@@ -373,6 +486,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
 
+    virtio_identify_template(s);
+    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
+        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
+
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 5ef6c36..1cd5d45 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -31,6 +31,11 @@
 #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_ID_LEN       256     /* length of identify u16 array */
+#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
+#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
 
 struct virtio_blk_config
 {
diff --git a/hw/virtio.c b/hw/virtio.c
index 78c7637..dc38f59 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -44,6 +44,8 @@
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR                  19
 
+/* The remaining space is defined by each driver as the per-driver
+ * configuration space */
 #define VIRTIO_PCI_CONFIG               20
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
diff --git a/sysemu.h b/sysemu.h
index 1f45fd6..185b4e3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -141,6 +141,8 @@ typedef enum {
     BLOCK_ERR_STOP_ANY
 } BlockInterfaceErrorAction;
 
+#define BLOCK_SERIAL_STRLEN 20
+
 typedef struct DriveInfo {
     BlockDriverState *bdrv;
     BlockInterfaceType type;
@@ -149,7 +151,7 @@ typedef struct DriveInfo {
     int used;
     int drive_opt_idx;
     BlockInterfaceErrorAction onerror;
-    char serial[21];
+    char serial[BLOCK_SERIAL_STRLEN + 1];
 } DriveInfo;
 
 #define MAX_IDE_DEVS	2


-- 
john.cooper@redhat.com

[Qemu-devel] [PATCH 2/2] fix virtio_blk serial pci config breakage

[john cooper]

Change virtblk_identify() to pull ATA identify
data from the bar #5 map vs. the pci config
area.  Add minimal support for bar mapping external
to virtio/virtio_pci.c.

Signed-off-by: john cooper <john.cooper@redhat.com>
---

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index aa1a3d5..a1687f3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,6 +8,8 @@
 
 #define PART_BITS 4
 
+#define VBLK_IDENTIFY_BAR 5	/* PCI BAR #5 maps identify/config area */
+
 static int major, index;
 
 struct virtio_blk
@@ -25,6 +27,9 @@ struct virtio_blk
 
 	mempool_t *pool;
 
+	/* maintains mapping of pci bar #5 unique to virtio_blk */
+	void __iomem *identify_ioaddr;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -171,24 +176,30 @@ static void do_virtblk_request(struct request_queue *q)
 		vblk->vq->vq_ops->kick(vblk->vq);
 }
 
-/* return ATA identify data
+/* return ATA identify data if supported by virtio_blk device
  */
 static int virtblk_identify(struct gendisk *disk, void *argp)
 {
 	struct virtio_blk *vblk = disk->private_data;
 	void *opaque;
-	int err = -ENOMEM;
+	int err, i;
+	char *p;
 
+	err = 0;
 	opaque = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
-	if (!opaque)
+	if (!opaque) {
+		err = -ENOMEM;
 		goto out;
+	}
 
-	err = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_IDENTIFY,
-		offsetof(struct virtio_blk_config, identify), opaque,
-		VIRTIO_BLK_ID_BYTES);
-
-	if (err)
+	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_IDENTIFY) ||
+	    !vblk->identify_ioaddr) {
+		err = -ENOENT;
 		goto out_kfree;
+	}
+
+	for (p = opaque, i = 0; i < VIRTIO_BLK_ID_BYTES; ++i)
+		*p++ = ioread8(vblk->identify_ioaddr + i);
 
 	if (copy_to_user(argp, opaque, VIRTIO_BLK_ID_BYTES))
 		err = -EFAULT;
@@ -383,6 +394,9 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	if (!err)
 		blk_queue_logical_block_size(vblk->disk->queue, blk_size);
 
+	vblk->identify_ioaddr = vdev->config->map ?
+		vdev->config->map(vdev, VBLK_IDENTIFY_BAR, 0) : NULL;
+
 	add_disk(vblk->disk);
 	return 0;
 
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 248e00e..abc6963 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -561,6 +561,13 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 	return err;
 }
 
+/* translate struct virtio_device to struct pci_dev, setup map for bar
+ */
+void __iomem *vp_map(struct virtio_device *vdev, int bar, unsigned long maxlen)
+{
+        return pci_iomap(to_vp_device(vdev)->pci_dev, bar, maxlen);
+}
+
 static struct virtio_config_ops virtio_pci_config_ops = {
 	.get		= vp_get,
 	.set		= vp_set,
@@ -571,6 +578,7 @@ static struct virtio_config_ops virtio_pci_config_ops = {
 	.del_vqs	= vp_del_vqs,
 	.get_features	= vp_get_features,
 	.finalize_features = vp_finalize_features,
+	.map		= vp_map, 
 };
 
 static void virtio_pci_release_dev(struct device *_d)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e547e3c..b9b3c62 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -94,6 +94,8 @@ struct virtio_config_ops {
 	void (*del_vqs)(struct virtio_device *);
 	u32 (*get_features)(struct virtio_device *vdev);
 	void (*finalize_features)(struct virtio_device *vdev);
+	void __iomem *(*map)(struct virtio_device *vdev, int bar,
+				    unsigned long maxlen);
 };
 
 /* If driver didn't advertise the feature, it will never appear. */


-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 2/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 02:10:15AM -0400, john cooper wrote:
> Change virtblk_identify() to pull ATA identify
> data from the bar #5 map vs. the pci config
> area.  Add minimal support for bar mapping external
> to virtio/virtio_pci.c.
> 
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index aa1a3d5..a1687f3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -8,6 +8,8 @@
>  
>  #define PART_BITS 4
>  
> +#define VBLK_IDENTIFY_BAR 5	/* PCI BAR #5 maps identify/config area */
> +

This constant is PCI specific.

>  static int major, index;
>  
>  struct virtio_blk
> @@ -25,6 +27,9 @@ struct virtio_blk
>  
>  	mempool_t *pool;
>  
> +	/* maintains mapping of pci bar #5 unique to virtio_blk */

This puts PCI specific stuff in virtio_blk. It would be better to
hide this in transport-specific part.

> +	void __iomem *identify_ioaddr;
> +
>  	/* What host tells us, plus 2 for header & tailer. */
>  	unsigned int sg_elems;
>  
> @@ -171,24 +176,30 @@ static void do_virtblk_request(struct request_queue *q)
>  		vblk->vq->vq_ops->kick(vblk->vq);
>  }
>  
> -/* return ATA identify data
> +/* return ATA identify data if supported by virtio_blk device
>   */
>  static int virtblk_identify(struct gendisk *disk, void *argp)
>  {
>  	struct virtio_blk *vblk = disk->private_data;
>  	void *opaque;
> -	int err = -ENOMEM;
> +	int err, i;
> +	char *p;
>  
> +	err = 0;
>  	opaque = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
> -	if (!opaque)
> +	if (!opaque) {
> +		err = -ENOMEM;
>  		goto out;
> +	}
>  
> -	err = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_IDENTIFY,
> -		offsetof(struct virtio_blk_config, identify), opaque,
> -		VIRTIO_BLK_ID_BYTES);
> -
> -	if (err)
> +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_IDENTIFY) ||
> +	    !vblk->identify_ioaddr) {
> +		err = -ENOENT;
>  		goto out_kfree;
> +	}
> +
> +	for (p = opaque, i = 0; i < VIRTIO_BLK_ID_BYTES; ++i)
> +		*p++ = ioread8(vblk->identify_ioaddr + i);
>  
>  	if (copy_to_user(argp, opaque, VIRTIO_BLK_ID_BYTES))
>  		err = -EFAULT;
> @@ -383,6 +394,9 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	if (!err)
>  		blk_queue_logical_block_size(vblk->disk->queue, blk_size);
>  
> +	vblk->identify_ioaddr = vdev->config->map ?
> +		vdev->config->map(vdev, VBLK_IDENTIFY_BAR, 0) : NULL;
> +
>  	add_disk(vblk->disk);
>  	return 0;
>  
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 248e00e..abc6963 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -561,6 +561,13 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  	return err;
>  }
>  
> +/* translate struct virtio_device to struct pci_dev, setup map for bar
> + */
> +void __iomem *vp_map(struct virtio_device *vdev, int bar, unsigned long maxlen)
> +{
> +        return pci_iomap(to_vp_device(vdev)->pci_dev, bar, maxlen);
> +}
> +

I think we need a feature bit to figure out whether
device supports this feature.

>  static struct virtio_config_ops virtio_pci_config_ops = {
>  	.get		= vp_get,
>  	.set		= vp_set,
> @@ -571,6 +578,7 @@ static struct virtio_config_ops virtio_pci_config_ops = {
>  	.del_vqs	= vp_del_vqs,
>  	.get_features	= vp_get_features,
>  	.finalize_features = vp_finalize_features,
> +	.map		= vp_map, 
>  };
>  
>  static void virtio_pci_release_dev(struct device *_d)
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index e547e3c..b9b3c62 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -94,6 +94,8 @@ struct virtio_config_ops {
>  	void (*del_vqs)(struct virtio_device *);
>  	u32 (*get_features)(struct virtio_device *vdev);
>  	void (*finalize_features)(struct virtio_device *vdev);
> +	void __iomem *(*map)(struct virtio_device *vdev, int bar,
> +				    unsigned long maxlen);


Don't we ever unmap?

>  };
>  
>  /* If driver didn't advertise the feature, it will never appear. */
> 
> 
> -- 
> john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 02:09:30AM -0400, john cooper wrote:
> This is a correction to the previous version
> which violated the PCI config space 256 byte
> limitation.
> 
> The crux of the feature is simply passing a qemu
> received virtio serial number to/through the guest
> driver and making it available to guest userspace
> via a preexisting interface.
> 
> To accomplish this, ATA IDENTIFY data as implemented
> by the HDIO_GET_IDENTITY ioctl is returned to the
> caller for the virtio_blk drive of interest.  Content
> of this identify structure is created by qemu and
> passed wholesale to guest userspace without
> interpretation by the guest driver.
> 
> The change this patch implements is passing of the
> identify data through a mapping established by
> PCI BAR #5 rather than the PCI config area, the
> latter of which resulted in the above breakage.

Using bar per feature we'll quickly run out of BARs.
We already use a BAR for MSI-X - let's add
ATA identity there?

> virtio_blk with this patch now pulls the information
> from the mapped bar area and includes minimal
> scaffolding to help map bars external to virtio_pci.
> 
> -john
> 
> 
> -- 
> john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Avi Kivity]

On 09/29/2009 08:58 AM, Michael S. Tsirkin wrote:
> Using bar per feature we'll quickly run out of BARs.
> We already use a BAR for MSI-X - let's add
> ATA identity there?
>    

Mixing unrelated features will quickly cause confusion.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 09:22:53AM +0200, Avi Kivity wrote:
> On 09/29/2009 08:58 AM, Michael S. Tsirkin wrote:
>> Using bar per feature we'll quickly run out of BARs.
>> We already use a BAR for MSI-X - let's add
>> ATA identity there?
>>    
> Mixing unrelated features will quickly cause confusion.

Note if we ever switch to 64 bit BARs, we'll only have 3 of these
available, so the ID would be using the last free one.
We can just as well plan ahead for when we'll add another feature?

I agree fixed offsets are messy though.  I previously proposed storing
the offset to the identity data in an i/o register. This will let each
implementation lay out this data in an optimal manner, without
confusion.

> -- 
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 1/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 02:09:49AM -0400, john cooper wrote:
> [This patch in part replicates the first version of
> this feature which was rolled-back due to breakage
> it caused extending the pci config area beyond the
> 256 byte limit.]

The patch does not apply on top of qemu git tree.
How should I apply it?

> Add missing logic in qemu to accept a command line
> serial number parameter for virtio_blk, integrate
> it into an ATA IDENTIFY structure, map the resulting
> data via PCI BAR #5.
> 
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
> 
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..9a067c8 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -19,12 +19,18 @@
>  # include <scsi/sg.h>
>  #endif
>  
> +#define VBLK_IDENTIFY_SIZE      512
> +#define VBLK_IDENTIFY_AMASK	(VBLK_IDENTIFY_SIZE - 1)
> +#define VBLK_IDENTIFY_CFGSLOT   5 /* PCI BAR #5 maps identify/config area */
> +
>  typedef struct VirtIOBlock
>  {
>      VirtIODevice vdev;
>      BlockDriverState *bs;
>      VirtQueue *vq;
>      void *rq;
> +    uint32_t mmio_io_addr;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
>  } VirtIOBlock;
>  
>  static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +38,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>      return (VirtIOBlock *)vdev;
>  }
>  
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
>  typedef struct VirtIOBlockReq
>  {
>      VirtIOBlock *dev;
> @@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>  
>  static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
>  {
> +    VirtIOBlock *s = to_virtio_blk(vdev);
>      uint32_t features = 0;
>  
>      features |= (1 << VIRTIO_BLK_F_SEG_MAX);
> @@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
>  #ifdef __linux__
>      features |= (1 << VIRTIO_BLK_F_SCSI);
>  #endif
> +    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
> +        features |= 1 << VIRTIO_BLK_F_IDENTIFY;
>  
>      return features;
>  }
> @@ -348,6 +399,60 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +/* fan-in for VBLK_IDENTIFY_CFGSLOT read operations
> + */
> +static uint32_t virtio_blk_io_read(void *opaque, target_phys_addr_t addr,
> +                                   uint8_t access_size)
> +{
> +    VirtIOBlock *s = opaque;
> +    uint16_t byte_index = addr & VBLK_IDENTIFY_AMASK;
> +    uint8_t *source = &((uint8_t *)s->identify)[byte_index];
> +
> +    switch (access_size)
> +        {
> +    case 1:
> +    	return (*(uint8_t *)source);
> +    case 2:
> +    	return (*(uint16_t *)source);
> +    case 4:
> +    	return (*(uint32_t *)source);
> +        }
> +    return 0;
> +}
> +
> +static uint32_t virtio_blk_mmio_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return virtio_blk_io_read(opaque, addr, 1);
> +}
> +
> +static uint32_t virtio_blk_mmio_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return virtio_blk_io_read(opaque, addr, 2);
> +}
> +
> +static uint32_t virtio_blk_mmio_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return virtio_blk_io_read(opaque, addr, 4);
> +}
> +
> +static CPUReadMemoryFunc *virtio_blk_mmio_read[3] = {
> +    virtio_blk_mmio_readb,
> +    virtio_blk_mmio_readw,
> +    virtio_blk_mmio_readl,
> +};
> +
> +static CPUWriteMemoryFunc *virtio_blk_mmio_write[3] = {
> +    NULL, NULL, NULL,
> +};
> +
> +static void virtio_blk_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
> +                           uint32_t size, int type)
> +{
> +    VirtIOBlock *s = (VirtIOBlock *)pci_dev;
> +
> +    cpu_register_physical_memory(addr, VBLK_IDENTIFY_SIZE, s->mmio_io_addr);
> +}
> +
>  void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>  {
>      VirtIOBlock *s;
> @@ -360,10 +465,18 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>                                         PCI_VENDOR_ID_REDHAT_QUMRANET,
>                                         VIRTIO_ID_BLOCK,
>                                         PCI_CLASS_STORAGE_OTHER, 0x00,
> -                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
> +                                       sizeof(struct virtio_blk_config),
> +                                       sizeof(VirtIOBlock));
>      if (!s)
>          return NULL;
>  
> +    s->mmio_io_addr =
> +        cpu_register_io_memory(0, virtio_blk_mmio_read,
> +                               virtio_blk_mmio_write, s);
> +    pci_register_io_region(&((VirtIODevice *)s)->pci_dev,
> +                           VBLK_IDENTIFY_CFGSLOT, VBLK_IDENTIFY_SIZE,
> +                           PCI_ADDRESS_SPACE_MEM, virtio_blk_map);
> +
>      s->vdev.get_config = virtio_blk_update_config;
>      s->vdev.get_features = virtio_blk_get_features;
>      s->vdev.reset = virtio_blk_reset;
> @@ -373,6 +486,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>      bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>      bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
>  
> +    virtio_identify_template(s);
> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
> +
>      s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>  
>      qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
> index 5ef6c36..1cd5d45 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -31,6 +31,11 @@
>  #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 */
> +

This bit is already used for the non-PCI-compliant feature.
We need a new one otherwise guests will be confused.

> +#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
> +#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
> +#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
>  
>  struct virtio_blk_config
>  {
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 78c7637..dc38f59 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -44,6 +44,8 @@
>   * a read-and-acknowledge. */
>  #define VIRTIO_PCI_ISR                  19
>  
> +/* The remaining space is defined by each driver as the per-driver
> + * configuration space */
>  #define VIRTIO_PCI_CONFIG               20
>  
>  /* Virtio ABI version, if we increment this, we break the guest driver. */
> diff --git a/sysemu.h b/sysemu.h
> index 1f45fd6..185b4e3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -141,6 +141,8 @@ typedef enum {
>      BLOCK_ERR_STOP_ANY
>  } BlockInterfaceErrorAction;
>  
> +#define BLOCK_SERIAL_STRLEN 20
> +
>  typedef struct DriveInfo {
>      BlockDriverState *bdrv;
>      BlockInterfaceType type;
> @@ -149,7 +151,7 @@ typedef struct DriveInfo {
>      int used;
>      int drive_opt_idx;
>      BlockInterfaceErrorAction onerror;
> -    char serial[21];
> +    char serial[BLOCK_SERIAL_STRLEN + 1];
>  } DriveInfo;
>  
>  #define MAX_IDE_DEVS	2
> 
> 
> -- 
> john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Avi Kivity]

On 09/29/2009 10:54 AM, Michael S. Tsirkin wrote:
> On Tue, Sep 29, 2009 at 09:22:53AM +0200, Avi Kivity wrote:
>    
>> On 09/29/2009 08:58 AM, Michael S. Tsirkin wrote:
>>      
>>> Using bar per feature we'll quickly run out of BARs.
>>> We already use a BAR for MSI-X - let's add
>>> ATA identity there?
>>>
>>>        
>> Mixing unrelated features will quickly cause confusion.
>>      
> Note if we ever switch to 64 bit BARs, we'll only have 3 of these
> available, so the ID would be using the last free one.
> We can just as well plan ahead for when we'll add another feature?
>
> I agree fixed offsets are messy though.  I previously proposed storing
> the offset to the identity data in an i/o register. This will let each
> implementation lay out this data in an optimal manner, without
> confusion.
>    

Yes, that works much better.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

john cooper wrote:
> This is a correction to the previous version
> which violated the PCI config space 256 byte
> limitation.
>
> The crux of the feature is simply passing a qemu
> received virtio serial number to/through the guest
> driver and making it available to guest userspace
> via a preexisting interface.
>
> To accomplish this, ATA IDENTIFY data as implemented
> by the HDIO_GET_IDENTITY ioctl is returned to the
> caller for the virtio_blk drive of interest.  Content
> of this identify structure is created by qemu and
> passed wholesale to guest userspace without
> interpretation by the guest driver.
>
> The change this patch implements is passing of the
> identify data through a mapping established by
> PCI BAR #5 rather than the PCI config area, the
> latter of which resulted in the above breakage.
>   

This is a massive layering violation.  The virtio-blk ABI cannot make 
demands of the transport.

The better solution would be to move the entire virtio-pci config space 
to a separate BAR that's an MMIO region.  Then there is no practical 
limit on the size of the config area.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

Michael S. Tsirkin wrote:
> On Tue, Sep 29, 2009 at 09:22:53AM +0200, Avi Kivity wrote:
>   
>> On 09/29/2009 08:58 AM, Michael S. Tsirkin wrote:
>>     
>>> Using bar per feature we'll quickly run out of BARs.
>>> We already use a BAR for MSI-X - let's add
>>> ATA identity there?
>>>    
>>>       
>> Mixing unrelated features will quickly cause confusion.
>>     
>
> Note if we ever switch to 64 bit BARs, we'll only have 3 of these
> available, so the ID would be using the last free one.
> We can just as well plan ahead for when we'll add another feature?
>
> I agree fixed offsets are messy though.  I previously proposed storing
> the offset to the identity data in an i/o register. This will let each
> implementation lay out this data in an optimal manner, without
> confusion.
>   

1) There's no need to make the identity page separate from the config 
space.  The real problem is the config space is too small.  We can fix 
that without making any changes to virtio-blk by putting the config 
space in a separate BAR.

2) Passing an ATA identity page is goofy.  We should just pass the 
serial number and let Linux generate the identity page.  Just because 
Linux requires this as it's user space interface, that doesn't mean that 
other guests will (like Windows).  Instead of exposing an opaque blob, 
we should expose the information we need in a structured way.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 08:55:20AM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> On Tue, Sep 29, 2009 at 09:22:53AM +0200, Avi Kivity wrote:
>>   
>>> On 09/29/2009 08:58 AM, Michael S. Tsirkin wrote:
>>>     
>>>> Using bar per feature we'll quickly run out of BARs.
>>>> We already use a BAR for MSI-X - let's add
>>>> ATA identity there?
>>>>          
>>> Mixing unrelated features will quickly cause confusion.
>>>     
>>
>> Note if we ever switch to 64 bit BARs, we'll only have 3 of these
>> available, so the ID would be using the last free one.
>> We can just as well plan ahead for when we'll add another feature?
>>
>> I agree fixed offsets are messy though.  I previously proposed storing
>> the offset to the identity data in an i/o register. This will let each
>> implementation lay out this data in an optimal manner, without
>> confusion.
>>   
>
> 1) There's no need to make the identity page separate from the config  
> space.  The real problem is the config space is too small.  We can fix  
> that without making any changes to virtio-blk by putting the config  
> space in a separate BAR.

I don't think it's such a good idea.  We want to keep exposing most of
config space in i/o bar because of backwards compatibility.  And we want
to keep datapath operations such as vq kicks, in i/o space.


> 2) Passing an ATA identity page is goofy.  We should just pass the  
> serial number and let Linux generate the identity page.  Just because  
> Linux requires this as it's user space interface, that doesn't mean that  
> other guests will (like Windows).  Instead of exposing an opaque blob,  
> we should expose the information we need in a structured way.
>
> Regards,
>
> Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

Michael S. Tsirkin wrote:
> I don't think it's such a good idea.  We want to keep exposing most of
> config space in i/o bar because of backwards compatibility.  And we want
> to keep datapath operations such as vq kicks, in i/o space.
>   

We can do feature negotiation in virtio-pci.  That lets us continue 
exposing the first 246 bytes of the config space in PIO and guests can 
negotiate a second bar for access to larger config spaces.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Avi Kivity]

On 09/29/2009 03:51 PM, Anthony Liguori wrote:
>> The change this patch implements is passing of the
>> identify data through a mapping established by
>> PCI BAR #5 rather than the PCI config area, the
>> latter of which resulted in the above breakage.
>
>
> This is a massive layering violation.  The virtio-blk ABI cannot make 
> demands of the transport.

True.

>
> The better solution would be to move the entire virtio-pci config 
> space to a separate BAR that's an MMIO region.  Then there is no 
> practical limit on the size of the config area.

Don't some fast-paths accesses go through the config space?  Using mmio 
will slow them down.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Avi Kivity]

On 09/29/2009 04:14 PM, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> I don't think it's such a good idea.  We want to keep exposing most of
>> config space in i/o bar because of backwards compatibility.  And we want
>> to keep datapath operations such as vq kicks, in i/o space.
>
> We can do feature negotiation in virtio-pci.  That lets us continue 
> exposing the first 246 bytes of the config space in PIO and guests can 
> negotiate a second bar for access to larger config spaces.

Shouldn't the guest's pci layer automatically adapt to what the card 
throws at it?

Of course we need to keep migration compatibility so we need to keep pio.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 09:14:56AM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> I don't think it's such a good idea.  We want to keep exposing most of
>> config space in i/o bar because of backwards compatibility.  And we want
>> to keep datapath operations such as vq kicks, in i/o space.
>>   
>
> We can do feature negotiation in virtio-pci.  That lets us continue  
> exposing the first 246 bytes of the config space in PIO and guests can  
> negotiate a second bar for access to larger config spaces.
>
> Regards,
>
> Anthony Liguori

guests can't negotiate a bar.

What I think we want to do
- Use i/o for small fields (2-4 bytes) as we did previously
  we won't run out anytime soon
- For large fields, put them in memory BAR1 and report the
  offset in i/o space
- For huge fields, provide a mailbox in i/o space where guest
  writes an offset and reads out a value

-- 
MST

[Qemu-devel] Re: [PATCH 2/2] fix virtio_blk serial pci config breakage

[Rusty Russell]

On Tue, 29 Sep 2009 03:40:15 pm john cooper wrote:
> Change virtblk_identify() to pull ATA identify
> data from the bar #5 map vs. the pci config
> area.  Add minimal support for bar mapping external
> to virtio/virtio_pci.c.

Repeat after me: "virtio is not PCI".

See my alternate series...
Rusty.

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

Avi Kivity wrote:
> On 09/29/2009 03:51 PM, Anthony Liguori wrote:
>>> The change this patch implements is passing of the
>>> identify data through a mapping established by
>>> PCI BAR #5 rather than the PCI config area, the
>>> latter of which resulted in the above breakage.
>>
>>
>> This is a massive layering violation.  The virtio-blk ABI cannot make 
>> demands of the transport.
>
> True.
>
>>
>> The better solution would be to move the entire virtio-pci config 
>> space to a separate BAR that's an MMIO region.  Then there is no 
>> practical limit on the size of the config area.
>
> Don't some fast-paths accesses go through the config space?  Using 
> mmio will slow them down.

Not that I know of.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

Michael S. Tsirkin wrote:
> On Tue, Sep 29, 2009 at 09:14:56AM -0500, Anthony Liguori wrote:
>   
>> Michael S. Tsirkin wrote:
>>     
>>> I don't think it's such a good idea.  We want to keep exposing most of
>>> config space in i/o bar because of backwards compatibility.  And we want
>>> to keep datapath operations such as vq kicks, in i/o space.
>>>   
>>>       
>> We can do feature negotiation in virtio-pci.  That lets us continue  
>> exposing the first 246 bytes of the config space in PIO and guests can  
>> negotiate a second bar for access to larger config spaces.
>>
>> Regards,
>>
>> Anthony Liguori
>>     
>
> guests can't negotiate a bar.
>   

The host always exposes the config in two places.  config[0..235] in 
bytes 20-255 in BAR0 and the full config space in BAR2 (or BAR1 + offset 
if desired).  Old drivers will just ignore the new config location.

> What I think we want to do
> - Use i/o for small fields (2-4 bytes) as we did previously
>   we won't run out anytime soon
> - For large fields, put them in memory BAR1 and report the
>   offset in i/o space
>   

This is at best a band aid as a large number of small fields can result 
in the same problem.

> - For huge fields, provide a mailbox in i/o space where guest
>   writes an offset and reads out a value
>   

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Rusty Russell]

On Tue, 29 Sep 2009 11:25:20 pm Anthony Liguori wrote:
> 2) Passing an ATA identity page is goofy.  We should just pass the 
> serial number and let Linux generate the identity page.  Just because 
> Linux requires this as it's user space interface, that doesn't mean that 
> other guests will (like Windows).  Instead of exposing an opaque blob, 
> we should expose the information we need in a structured way.

I think John did this on my prompting, because it already existed as a
defined ABI.  If someone is going to make stuff up, isn't it better that
kvm does it (some of those fields might have useful values?) than linux?

(I don't care: my patches stick with the current scheme, but changing is
fairly easy, but needs to be decided before commit).

Thanks,
Rusty,

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

Rusty Russell wrote:
> On Tue, 29 Sep 2009 11:25:20 pm Anthony Liguori wrote:
>   
>> 2) Passing an ATA identity page is goofy.  We should just pass the 
>> serial number and let Linux generate the identity page.  Just because 
>> Linux requires this as it's user space interface, that doesn't mean that 
>> other guests will (like Windows).  Instead of exposing an opaque blob, 
>> we should expose the information we need in a structured way.
>>     
>
> I think John did this on my prompting, because it already existed as a
> defined ABI.  If someone is going to make stuff up, isn't it better that
> kvm does it (some of those fields might have useful values?) than linux?
>   

We should expose Linux ABI quirks in the virtio ABI.  Down the road, 
someone may decide that there's a better way to expose things like S/N 
down to userspace and then in our drivers we would have to parse the ATA 
identity page in order to figure out the S/N to expose that properly 
through the new interface.  Moreover, there's probably an OS out there 
that we already have to do that for.

That's a pretty ugly thing to have to do.

> (I don't care: my patches stick with the current scheme, but changing is
> fairly easy, but needs to be decided before commit).
>   

Everything is tremendously simplified if we just expose the S/N in the 
ABI since we can avoid solving the config size limitation.  I'm a big 
fan of deferring difficult problems when there's an immediate easy 
solution :-)

> Thanks,
> Rusty,
>   

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Michael S. Tsirkin]

On Tue, Sep 29, 2009 at 12:26:13PM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> On Tue, Sep 29, 2009 at 09:14:56AM -0500, Anthony Liguori wrote:
>>   
>>> Michael S. Tsirkin wrote:
>>>     
>>>> I don't think it's such a good idea.  We want to keep exposing most of
>>>> config space in i/o bar because of backwards compatibility.  And we want
>>>> to keep datapath operations such as vq kicks, in i/o space.
>>>>         
>>> We can do feature negotiation in virtio-pci.  That lets us continue   
>>> exposing the first 246 bytes of the config space in PIO and guests 
>>> can  negotiate a second bar for access to larger config spaces.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>     
>>
>> guests can't negotiate a bar.
>>   
>
> The host always exposes the config in two places.  config[0..235] in  
> bytes 20-255 in BAR0 and the full config space in BAR2 (or BAR1 + offset  
> if desired).  Old drivers will just ignore the new config location.
>
>> What I think we want to do
>> - Use i/o for small fields (2-4 bytes) as we did previously
>>   we won't run out anytime soon
>> - For large fields, put them in memory BAR1 and report the
>>   offset in i/o space
>>   
>
> This is at best a band aid as a large number of small fields can result  
> in the same problem.

It would have to be very large :)

>> - For huge fields, provide a mailbox in i/o space where guest
>>   writes an offset and reads out a value
>>   
>
> Regards,
>
> Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[john cooper]

Anthony Liguori wrote:

> 2) Passing an ATA identity page is goofy.  We should just pass the 
> serial number and let Linux generate the identity page.

I have a hard time disagreeing with that comment.

But the rationale that got us here was from the
user's perspective this information could be
trivially exposed via a preexisting interface vs.
creating a new interface as I'd done initially.
It was also pointed out other defined fields in
the identify page exist which were potentially
useful.  So pressing a HDIO_GET_IDENTITY ioctl
into service here didn't seem unreasonable.

In terms of getting that bag of bits from qemu to
the guest userspace, having qemu cobble together
the page is arguably appropriate as that it where
the source of the information resides.  The
alternative of pushing knowledge of this structure
into the guest driver seems unnatural.  Moreover
we would still somehow need a means to get the
data from qemu to the driver even if the driver
was only packaging it up for export as an identify
page.

> Just because 
> Linux requires this as it's user space interface, that doesn't mean that 
> other guests will (like Windows).  Instead of exposing an opaque blob, 
> we should expose the information we need in a structured way.

The identify page is defined as the return of an
"identify drive" command from an ATA drive/controller.
I'd hazard it is a familiar structure in a windows
driver/userland context as well.

-john


-- 
john.cooper@third-harmonic.com

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

john cooper wrote:
> Anthony Liguori wrote:
>
>> 2) Passing an ATA identity page is goofy.  We should just pass the 
>> serial number and let Linux generate the identity page.
>
> I have a hard time disagreeing with that comment.
>
> But the rationale that got us here was from the
> user's perspective this information could be
> trivially exposed via a preexisting interface vs.
> creating a new interface as I'd done initially.
> It was also pointed out other defined fields in
> the identify page exist which were potentially
> useful.  So pressing a HDIO_GET_IDENTITY ioctl
> into service here didn't seem unreasonable.

virtio-blk isn't an ATA device so why pretend like it is?

We can present the information we think is useful to the guest in a 
concise format and then if the guest wants to pretend it's an ATA 
device, it can create the ATA identify page on the fly.

>> Just because Linux requires this as it's user space interface, that 
>> doesn't mean that other guests will (like Windows).  Instead of 
>> exposing an opaque blob, we should expose the information we need in 
>> a structured way.
>
> The identify page is defined as the return of an
> "identify drive" command from an ATA drive/controller.
> I'd hazard it is a familiar structure in a windows
> driver/userland context as well.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Rusty Russell]

On Wed, 30 Sep 2009 03:01:12 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tue, 29 Sep 2009 11:25:20 pm Anthony Liguori wrote:
> >   
> >> 2) Passing an ATA identity page is goofy.  We should just pass the 
> >> serial number and let Linux generate the identity page.  Just because 
> >> Linux requires this as it's user space interface, that doesn't mean that 
> >> other guests will (like Windows).  Instead of exposing an opaque blob, 
> >> we should expose the information we need in a structured way.
> >>     
> >
> > I think John did this on my prompting, because it already existed as a
> > defined ABI.  If someone is going to make stuff up, isn't it better that
> > kvm does it (some of those fields might have useful values?) than linux?
> 
> We should expose Linux ABI quirks in the virtio ABI.  Down the road, 
> someone may decide that there's a better way to expose things like S/N 
> down to userspace and then in our drivers we would have to parse the ATA 
> identity page in order to figure out the S/N to expose that properly 
> through the new interface.  Moreover, there's probably an OS out there 
> that we already have to do that for.
> 
> That's a pretty ugly thing to have to do.

Calling the ATA IDENTIFY command a Linux ABI quirk is disingenuous.

Not sure what the SCSI equiv is to get a serial number (VPD?) But if everyone
is happy with 20 bytes, and John wants to do SN that way, I'll take the patch.

Still, I got to learn fun stuff about the block layer!
Rusty.

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Rusty Russell]

On Wed, 30 Sep 2009 06:25:39 am Anthony Liguori wrote:
> virtio-blk isn't an ATA device so why pretend like it is?

Do you want to maintain a "vdparm" to match hdparm and sdparm?

Confused,
Rusty.

Re: [Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Jamie Lokier]

Rusty Russell wrote:
> Calling the ATA IDENTIFY command a Linux ABI quirk is disingenuous.
> 
> Not sure what the SCSI equiv is to get a serial number (VPD?) But if
> everyone is happy with 20 bytes, and John wants to do SN that way,
> I'll take the patch.

One thing I'd hope to see at some point is a "read-only" flag, set for
read-only images.  I don't know if ATA IDENTIFY has that yet, but I'm
sure the SCSI equivalent does.

-- Jamie

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Anthony Liguori]

Rusty Russell wrote:
> On Wed, 30 Sep 2009 06:25:39 am Anthony Liguori wrote:
>   
>> virtio-blk isn't an ATA device so why pretend like it is?
>>     
>
> Do you want to maintain a "vdparm" to match hdparm and sdparm?
>   

That's an argument to implement the HDIO_GET_IDENTITY ioctl in the Linux 
virtio-blk driver.  That's a totally reasonable thing to do.

However, baking something into the virtio-blk ABI because Linux requires 
an hdparm, sdparm, and potentially a vdparm doesn't seem like a good 
idea to me.  We should expose the information that makes sense for a 
virtio block device and if a guest wants to create an ATA identify 
response out of that data, more power to it.

Regards,

Anthony Liguori

> Confused,
> Rusty.
>   

Re: [Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Paul Brook]

> > But the rationale that got us here was from the
> > user's perspective this information could be
> > trivially exposed via a preexisting interface vs.
> > creating a new interface as I'd done initially.
> > It was also pointed out other defined fields in
> > the identify page exist which were potentially
> > useful.  So pressing a HDIO_GET_IDENTITY ioctl
> > into service here didn't seem unreasonable.
> 
> virtio-blk isn't an ATA device so why pretend like it is?

Agreed. If we're going to copy anything than my vote would be for SCSI. We 
already have partial support for feeding SCSI commands to a virtio device.
TBH I wouldn't have bothered with virtio-blk at all, and gone straight for 
virtio-scsi. I guess it's a bit late for that though.

Paul

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Rusty Russell]

On Wed, 30 Sep 2009 11:47:00 am Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Wed, 30 Sep 2009 06:25:39 am Anthony Liguori wrote:
> >   
> >> virtio-blk isn't an ATA device so why pretend like it is?
... 
> That's an argument to implement the HDIO_GET_IDENTITY ioctl in the Linux 
> virtio-blk driver.  That's a totally reasonable thing to do.

Your second sentence contradicts your first here.  If it's reasonable to
imitate ATA in Linux, it's reasonable to imitate ATA in virtio.

Using an existing standard should always be the default; one should need a
compelling reason *not* to do so.  In practice, (1) that code already exists
in kvm, and (2) it makes the Linux implementation trivial.

So, what are the real arguments?
(1) Some fields are redundant (eg. geometry) with existing fields already.
(2) Future fields will have to decide whether to use ATA IDENTIFY or normal
    features,
(3) We don't know enough about ATA IDENTIFY to do a decent job of filling
    it in.

I can buy (1), but I still have to deal with (2) and (3) if they're inside
the virtio_blk driver anyway.

Anything else?
Rusty.

Re: [Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[Jamie Lokier]

Rusty Russell wrote:
> On Wed, 30 Sep 2009 11:47:00 am Anthony Liguori wrote:
> > Rusty Russell wrote:
> > > On Wed, 30 Sep 2009 06:25:39 am Anthony Liguori wrote:
> > >   
> > >> virtio-blk isn't an ATA device so why pretend like it is?
> ... 
> > That's an argument to implement the HDIO_GET_IDENTITY ioctl in the Linux 
> > virtio-blk driver.  That's a totally reasonable thing to do.
> 
> Your second sentence contradicts your first here.  If it's reasonable to
> imitate ATA in Linux, it's reasonable to imitate ATA in virtio.
> 
> Using an existing standard should always be the default; one should need a
> compelling reason *not* to do so.  In practice, (1) that code already exists
> in kvm, and (2) it makes the Linux implementation trivial.
> 
> So, what are the real arguments?
> (1) Some fields are redundant (eg. geometry) with existing fields already.
> (2) Future fields will have to decide whether to use ATA IDENTIFY or normal
>     features,
> (3) We don't know enough about ATA IDENTIFY to do a decent job of filling
>     it in.
> 
> I can buy (1), but I still have to deal with (2) and (3) if they're inside
> the virtio_blk driver anyway.
> 
> Anything else?

(4) Putting it into the virtio_blk driver means another duplicate
implementation, and it might have to be duplicated another time in the
Windows virtio_blk driver too, if there's a Windows generic equivalent
to HDIO_GET_IDENTITY (I don't know if there is though).

(5) When the host's backing driver is itself a disk implementing the
ATA command, would you ever want the guest to see all details of the
identity page as close as possible to the host device sometimes?
I.e. is virtio_blk a way to accelerate block I/O to a host device, or
a way to abstract it to the bare minimum?

(6) Why don't we use the SCSI identity page instead?  Presumably
that's already in qemu/kvm too, for SCSI and USB disks :-)

-- Jamie

Re: [Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[john cooper]

Paul Brook wrote:

> Agreed. If we're going to copy anything than my vote would be for SCSI. We 
> already have partial support for feeding SCSI commands to a virtio device.
> TBH I wouldn't have bothered with virtio-blk at all, and gone straight for 
> virtio-scsi. I guess it's a bit late for that though.

Initially the patch introduced a dedicated ioctl
in the guest driver to retrieve this data.  Review
comments from that version suggested using an
existing interface which to me seemed reasonable.

I had looked first at implementing the above via
a scsi INQUIRY cmd.   But at least at that time
this option would have been more invasive to the
existing code vs. just doing the equivalent via
an ATA HDIO_GET_IDENTITY.

-john

-- 
john.cooper@third-harmonic.com

Re: [Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[john cooper]

Jamie Lokier wrote:
> (5) When the host's backing driver is itself a disk implementing the
> ATA command, would you ever want the guest to see all details of the
> identity page as close as possible to the host device sometimes?
> I.e. is virtio_blk a way to accelerate block I/O to a host device, or
> a way to abstract it to the bare minimum?

Admittedly most of the data in the identify page
as seen by the host is fodder for the sake of
existing userspace utilities.  So I don't really
see any case where pass-through of host->guest
data is meaningful or useful.  The patch just
follows suit here with a similar mockup of this
data for the guest userland.

-john

-- 
john.cooper@third-harmonic.com

[Qemu-devel] Re: [PATCH 0/2] fix virtio_blk serial pci config breakage

[john cooper]

Rusty Russell wrote:
> Calling the ATA IDENTIFY command a Linux ABI quirk is disingenuous.

I think ATA has earned that moniker all by itself.

> Not sure what the SCSI equiv is to get a serial number (VPD?) But if everyone
> is happy with 20 bytes, and John wants to do SN that way, I'll take the patch.

I've been swayed from one side to the other of
this argument.  Still I'd begrudgingly give my
vote to the ATA option only because it reuses an
existing wart vs. introducing a new one.

Updated qemu patch follows leveraging your virtqueue
REQ_TYPE_SPECIAL scaffolding.  At this point only
the issue of this guest visible interface should
remain.

-john


-- 
john.cooper@third-harmonic.com

[Qemu-devel] [PATCH] fix virtio_blk serial pci config breakage, v2

[john cooper]

This is a re-work of the previous version where the
associated data was being funneled through a free
PCI BAR mapping.  Here a request for the identify
information results in a virtqueue command utilizing
the scaffolding introduced by Rusty's recent patch.

Signed-off-by: john cooper <john.cooper@redhat.com>
---


diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index dad4ef0..e754277 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -25,6 +25,7 @@ typedef struct VirtIOBlock
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
+    uint16_t identify[VIRTIO_BLK_ID_LEN];
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
     return (VirtIOBlock *)vdev;
 }
 
+/* store identify data in little endian format
+ */
+static inline void put_le16(uint16_t *p, unsigned int v)
+{
+    *p = cpu_to_le16(v);
+}
+
+/* copy to *dst from *src, nul pad dst tail as needed to len bytes
+ */
+static inline void padstr(char *dst, const char *src, int len)
+{
+    while (len--)
+        *dst++ = *src ? *src++ : '\0';
+}
+
+/* setup simulated identify data as appropriate for virtio block device
+ *
+ * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
+ */
+static inline void virtio_identify_template(VirtIOBlock *s)
+{
+    uint16_t *p = s->identify;
+    uint64_t lba_sectors;
+
+    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
+    put_le16(p + 0, 0x0);                            /* ATA device */
+    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
+    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
+    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
+    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
+    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
+    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
+    put_le16(p + 81, 0x16);
+    put_le16(p + 82, 0x400);
+    put_le16(p + 83, 0x400);
+    bdrv_get_geometry(s->bs, &lba_sectors);
+    put_le16(p + 100, lba_sectors);
+    put_le16(p + 101, lba_sectors >> 16);
+    put_le16(p + 102, lba_sectors >> 32);
+    put_le16(p + 103, lba_sectors >> 48);
+}
+
 typedef struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
@@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
             virtio_blk_handle_scsi(req);
+        }
+        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
+            memcpy(req->elem.in_sg[0].iov_base, s->identify,
+                req->elem.in_sg[0].iov_len);
+        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
         } else if (req->out->type & VIRTIO_BLK_T_OUT) {
             qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
                                      req->elem.out_num - 1);
@@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 {
+    VirtIOBlock *s = to_virtio_blk(vdev);
     uint32_t features = 0;
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
@@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #ifdef __linux__
     features |= (1 << VIRTIO_BLK_F_SCSI);
 #endif
+    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
+        features |= 1 << VIRTIO_BLK_F_GET_ID;
 
     return features;
 }
@@ -360,7 +411,8 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
                                        VIRTIO_ID_BLOCK,
                                        PCI_CLASS_STORAGE_OTHER, 0x00,
-                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
+                                       sizeof(struct virtio_blk_config),
+                                       sizeof(VirtIOBlock));
     if (!s)
         return NULL;
 
@@ -373,6 +425,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
 
+    virtio_identify_template(s);
+    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
+        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
+
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 5ef6c36..f508f20 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -31,6 +31,12 @@
 #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       /* obsolete */
+#define VIRTIO_BLK_F_GET_ID     10      /* ATA IDENTIFY supported */
+
+#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
+#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
+#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
 
 struct virtio_blk_config
 {
@@ -48,6 +54,8 @@ struct virtio_blk_config
 
 /* This bit says it's a scsi command, not an actual read or write. */
 #define VIRTIO_BLK_T_SCSI_CMD   2
+#define _VIRTIO_BLK_T_FLUSH	4
+#define VIRTIO_BLK_T_GET_ID	8
 
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER    0x80000000
diff --git a/hw/virtio.c b/hw/virtio.c
index 78c7637..dc38f59 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -44,6 +44,8 @@
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR                  19
 
+/* The remaining space is defined by each driver as the per-driver
+ * configuration space */
 #define VIRTIO_PCI_CONFIG               20
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
diff --git a/sysemu.h b/sysemu.h
index 1f45fd6..185b4e3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -141,6 +141,8 @@ typedef enum {
     BLOCK_ERR_STOP_ANY
 } BlockInterfaceErrorAction;
 
+#define BLOCK_SERIAL_STRLEN 20
+
 typedef struct DriveInfo {
     BlockDriverState *bdrv;
     BlockInterfaceType type;
@@ -149,7 +151,7 @@ typedef struct DriveInfo {
     int used;
     int drive_opt_idx;
     BlockInterfaceErrorAction onerror;
-    char serial[21];
+    char serial[BLOCK_SERIAL_STRLEN + 1];
 } DriveInfo;
 
 #define MAX_IDE_DEVS	2


-- 
john.cooper@third-harmonic.com

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[Michael S. Tsirkin]

On Mon, Oct 05, 2009 at 11:47:51AM -0400, john cooper wrote:
> This is a re-work of the previous version where the
> associated data was being funneled through a free
> PCI BAR mapping.  Here a request for the identify
> information results in a virtqueue command utilizing
> the scaffolding introduced by Rusty's recent patch.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>

good stuff. A couple of comments below.
Also, what's going on with text alignment here?

> ---
>
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..e754277 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -25,6 +25,7 @@ typedef struct VirtIOBlock
>     BlockDriverState *bs;
>     VirtQueue *vq;
>     void *rq;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
> } VirtIOBlock;
>
> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>     return (VirtIOBlock *)vdev;
> }
>
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);

better as sizeof s->identity

> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */

QEMU version is currently a string like "0.11.50" which is exactly 8
bytes. What if someone makes it longer?  padstr will not 0
terminate string, and only partial data will be there.
Maybe put compile assert here?

Also, identify is pre-initialized to 0, isn't it?
So just strcpy should be enough, here and elsewhere,
no need to roll our own padstr.

> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
> @@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>
>         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
>             virtio_blk_handle_scsi(req);
> +        }
> +        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {

Pls put } and else on the same line

> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
> +                req->elem.in_sg[0].iov_len);

Is this safe? Can guest make iov_len bigger than size of s->identity?

> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
>         } else if (req->out->type & VIRTIO_BLK_T_OUT) {
>             qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
>                                      req->elem.out_num - 1);
> @@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>
> static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> {
> +    VirtIOBlock *s = to_virtio_blk(vdev);
>     uint32_t features = 0;
>
>     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
> @@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> #ifdef __linux__
>     features |= (1 << VIRTIO_BLK_F_SCSI);
> #endif
> +    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
> +        features |= 1 << VIRTIO_BLK_F_GET_ID;
>     return features;
> }
> @@ -360,7 +411,8 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
>                                        VIRTIO_ID_BLOCK,
>                                        PCI_CLASS_STORAGE_OTHER, 0x00,
> -                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
> +                                       sizeof(struct virtio_blk_config),
> +                                       sizeof(VirtIOBlock));
>     if (!s)
>         return NULL;
>
> @@ -373,6 +425,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
>
> +    virtio_identify_template(s);
> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);

This can silently truncate the serial, can't it?
Maybe check and error out?

> +
>     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>
>     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
> index 5ef6c36..f508f20 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -31,6 +31,12 @@
> #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       /* obsolete */

Let's just put it in comment? It should not be used anywhere.

> +#define VIRTIO_BLK_F_GET_ID     10      /* ATA IDENTIFY supported */
> +
> +#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
> +#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
> +#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
>
> struct virtio_blk_config
> {
> @@ -48,6 +54,8 @@ struct virtio_blk_config
>
> /* This bit says it's a scsi command, not an actual read or write. */
> #define VIRTIO_BLK_T_SCSI_CMD   2
> +#define _VIRTIO_BLK_T_FLUSH	4
> +#define VIRTIO_BLK_T_GET_ID	8
>
> /* Barrier before this op. */
> #define VIRTIO_BLK_T_BARRIER    0x80000000
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 78c7637..dc38f59 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -44,6 +44,8 @@
>  * a read-and-acknowledge. */
> #define VIRTIO_PCI_ISR                  19
>
> +/* The remaining space is defined by each driver as the per-driver
> + * configuration space */
> #define VIRTIO_PCI_CONFIG               20
>
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> diff --git a/sysemu.h b/sysemu.h
> index 1f45fd6..185b4e3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -141,6 +141,8 @@ typedef enum {
>     BLOCK_ERR_STOP_ANY
> } BlockInterfaceErrorAction;
>
> +#define BLOCK_SERIAL_STRLEN 20
> +
> typedef struct DriveInfo {
>     BlockDriverState *bdrv;
>     BlockInterfaceType type;
> @@ -149,7 +151,7 @@ typedef struct DriveInfo {
>     int used;
>     int drive_opt_idx;
>     BlockInterfaceErrorAction onerror;
> -    char serial[21];
> +    char serial[BLOCK_SERIAL_STRLEN + 1];
> } DriveInfo;
>
> #define MAX_IDE_DEVS	2
>
>
> -- 
> john.cooper@third-harmonic.com

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[Michael S. Tsirkin]

On Mon, Oct 05, 2009 at 11:47:51AM -0400, john cooper wrote:
> This is a re-work of the previous version where the
> associated data was being funneled through a free
> PCI BAR mapping.  Here a request for the identify
> information results in a virtqueue command utilizing
> the scaffolding introduced by Rusty's recent patch.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>

On top of this, there should be a patch removing identity from io bar.
Right? Otherwise we'd still be non-spec-compliant when identity is set.

> ---
>
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..e754277 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -25,6 +25,7 @@ typedef struct VirtIOBlock
>     BlockDriverState *bs;
>     VirtQueue *vq;
>     void *rq;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
> } VirtIOBlock;
>
> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>     return (VirtIOBlock *)vdev;
> }
>
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
> @@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>
>         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
>             virtio_blk_handle_scsi(req);
> +        }
> +        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
> +                req->elem.in_sg[0].iov_len);
> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
>         } else if (req->out->type & VIRTIO_BLK_T_OUT) {
>             qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
>                                      req->elem.out_num - 1);
> @@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>
> static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> {
> +    VirtIOBlock *s = to_virtio_blk(vdev);
>     uint32_t features = 0;
>
>     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
> @@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> #ifdef __linux__
>     features |= (1 << VIRTIO_BLK_F_SCSI);
> #endif
> +    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
> +        features |= 1 << VIRTIO_BLK_F_GET_ID;
>
>     return features;
> }
> @@ -360,7 +411,8 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
>                                        VIRTIO_ID_BLOCK,
>                                        PCI_CLASS_STORAGE_OTHER, 0x00,
> -                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
> +                                       sizeof(struct virtio_blk_config),
> +                                       sizeof(VirtIOBlock));
>     if (!s)
>         return NULL;
>
> @@ -373,6 +425,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
>
> +    virtio_identify_template(s);
> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
> +
>     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>
>     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
> index 5ef6c36..f508f20 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -31,6 +31,12 @@
> #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       /* obsolete */
> +#define VIRTIO_BLK_F_GET_ID     10      /* ATA IDENTIFY supported */
> +
> +#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
> +#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
> +#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
>
> struct virtio_blk_config
> {
> @@ -48,6 +54,8 @@ struct virtio_blk_config
>
> /* This bit says it's a scsi command, not an actual read or write. */
> #define VIRTIO_BLK_T_SCSI_CMD   2
> +#define _VIRTIO_BLK_T_FLUSH	4
> +#define VIRTIO_BLK_T_GET_ID	8
>
> /* Barrier before this op. */
> #define VIRTIO_BLK_T_BARRIER    0x80000000
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 78c7637..dc38f59 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -44,6 +44,8 @@
>  * a read-and-acknowledge. */
> #define VIRTIO_PCI_ISR                  19
>
> +/* The remaining space is defined by each driver as the per-driver
> + * configuration space */
> #define VIRTIO_PCI_CONFIG               20
>
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> diff --git a/sysemu.h b/sysemu.h
> index 1f45fd6..185b4e3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -141,6 +141,8 @@ typedef enum {
>     BLOCK_ERR_STOP_ANY
> } BlockInterfaceErrorAction;
>
> +#define BLOCK_SERIAL_STRLEN 20
> +
> typedef struct DriveInfo {
>     BlockDriverState *bdrv;
>     BlockInterfaceType type;
> @@ -149,7 +151,7 @@ typedef struct DriveInfo {
>     int used;
>     int drive_opt_idx;
>     BlockInterfaceErrorAction onerror;
> -    char serial[21];
> +    char serial[BLOCK_SERIAL_STRLEN + 1];
> } DriveInfo;
>
> #define MAX_IDE_DEVS	2
>
>
> -- 
> john.cooper@third-harmonic.com

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[Anthony Liguori]

john cooper wrote:
> This is a re-work of the previous version where the
> associated data was being funneled through a free
> PCI BAR mapping.  Here a request for the identify
> information results in a virtqueue command utilizing
> the scaffolding introduced by Rusty's recent patch.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
>
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..e754277 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -25,6 +25,7 @@ typedef struct VirtIOBlock
>     BlockDriverState *bs;
>     VirtQueue *vq;
>     void *rq;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
> } VirtIOBlock;
>
> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>     return (VirtIOBlock *)vdev;
> }
>
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware 
> revision */
> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 
> sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support 
> IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 
> sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support 
> ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
> @@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice 
> *vdev, VirtQueue *vq)
>
>         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
>             virtio_blk_handle_scsi(req);
> +        }
> +        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {

CodingStyle.

> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
> +                req->elem.in_sg[0].iov_len);
> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);

Weird indentation.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[john cooper]

Michael S. Tsirkin wrote:
>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
> 
> QEMU version is currently a string like "0.11.50" which is exactly 8
> bytes. What if someone makes it longer?  padstr will not 0
> terminate string, and only partial data will be there.

This code treats the field similar to the logic from which
it derives (hw/ide.c) in that the field need not be nul
terminated.  Quiet truncation to 8 bytes can occur here
and in the existing usage but in a practical sense I don't
see much of a recourse.  We can flag a warning but the
data is realistically a best-effort attempt to provide
relevant information in this field.  IOW overflowing
this field probably isn't justification alone to modify
a too long qemu version string.

> Also, identify is pre-initialized to 0, isn't it?
> So just strcpy should be enough, here and elsewhere,
> no need to roll our own padstr.

Actually this is an oversight in the local padstr() which
should be padding the balance of the field with ' ' vs. '\0'.

>> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
>> +                req->elem.in_sg[0].iov_len);
> 
> Is this safe? Can guest make iov_len bigger than size of s->identity?

Good point, a malicious/buggy guest can.  The memcpy
length should be capped. 

>> +    virtio_identify_template(s);
>> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
>> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
> 
> This can silently truncate the serial, can't it?

Yes, it is the same disposition as ide/scsi's treatment
of the S/N.  My concern was of keeping the behavior
consistent.

Thanks,

-john

-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[Anthony Liguori]

john cooper wrote:
> Michael S. Tsirkin wrote:
>   
>>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
>>>       
>> QEMU version is currently a string like "0.11.50" which is exactly 8
>> bytes. What if someone makes it longer?  padstr will not 0
>> terminate string, and only partial data will be there.
>>     
>
> This code treats the field similar to the logic from which
> it derives (hw/ide.c) in that the field need not be nul
> terminated.  Quiet truncation to 8 bytes can occur here
> and in the existing usage but in a practical sense I don't
> see much of a recourse.  We can flag a warning but the
> data is realistically a best-effort attempt to provide
> relevant information in this field.  IOW overflowing
> this field probably isn't justification alone to modify
> a too long qemu version string.
>   

Hrm, we really shouldn't be exposing a version string to the guest in 
the first place.

That's a compatibility issue.

Really, I strongly dislike passing this identity page via virtio.  Why 
are we still going this route instead of just passing the S/N?

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[Michael S. Tsirkin]

On Wed, Oct 07, 2009 at 08:48:32AM -0500, Anthony Liguori wrote:
> john cooper wrote:
>> Michael S. Tsirkin wrote:
>>   
>>>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>>>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
>>>>       
>>> QEMU version is currently a string like "0.11.50" which is exactly 8
>>> bytes. What if someone makes it longer?  padstr will not 0
>>> terminate string, and only partial data will be there.
>>>     
>>
>> This code treats the field similar to the logic from which
>> it derives (hw/ide.c) in that the field need not be nul
>> terminated.  Quiet truncation to 8 bytes can occur here
>> and in the existing usage but in a practical sense I don't
>> see much of a recourse.  We can flag a warning but the
>> data is realistically a best-effort attempt to provide
>> relevant information in this field.  IOW overflowing
>> this field probably isn't justification alone to modify
>> a too long qemu version string.
>>   
>
> Hrm, we really shouldn't be exposing a version string to the guest in  
> the first place.
>
> That's a compatibility issue.

Actually, it's a good point. Otherwise e.g. the identity changes with
migration. My understanding is that this isn't the only place where
we do this?

> Really, I strongly dislike passing this identity page via virtio.  Why  
> are we still going this route instead of just passing the S/N?

No opinion on this.

> Regards,
>
> Anthony Liguori

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[Anthony Liguori]

Michael S. Tsirkin wrote:
> Actually, it's a good point. Otherwise e.g. the identity changes with
> migration. My understanding is that this isn't the only place where
> we do this?
>   

Right, we'll need to fix this in the IDE emulation.  I assume we do 
something like that in SCSI also.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2

[john cooper]

Anthony Liguori wrote:
> Really, I strongly dislike passing this identity page via virtio.  Why
> are we still going this route instead of just passing the S/N?

I believe we've accumulated enough justification to
abandon use of the ata identify interface.

Anthony Liguori wrote:
> Right, we'll need to fix this in the IDE emulation.  I assume we do
> something like that in SCSI also.

Yes, unfortunately that code is stuffing 4 chars
of QEMU_VERSION into the return of an inquiry
command.  It appears this interface as well could
use some attention.

-john

-- 
john.cooper@redhat.com