* Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices
From: Ryan Harper @ 2010-06-21 16:43 UTC (permalink / raw)
To: john cooper; +Cc: virtualization, qemu-devel, kvm
In-Reply-To: <4C1EFDFD.5050907@redhat.com>
* john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
> Rusty Russell wrote:
> > On Sat, 19 Jun 2010 04:08:02 am Ryan Harper wrote:
> >> Create a new attribute for virtio-blk devices that will fetch the serial number
> >> of the block device. This attribute can be used by udev to create disk/by-id
> >> symlinks for devices that don't have a UUID (filesystem) associated with them.
> >>
> >> ATA_IDENTIFY strings are special in that they can be up to 20 chars long
> >> and aren't required to be NULL-terminated. The buffer is also zero-padded
> >> meaning that if the serial is 19 chars or less that we get a NULL terminated
> >> string. When copying this value into a string buffer, we must be careful to
> >> copy up to the NULL (if it present) and only 20 if it is longer and not to
> >> attempt to NULL terminate; this isn't needed.
> >>
> >> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> >> Signed-off-by: john cooper <john.cooper@redhat.com>
> >> ---
> >> drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++++
> >> 1 files changed, 32 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> >> index 258bc2a..f1ef26f 100644
> >> --- a/drivers/block/virtio_blk.c
> >> +++ b/drivers/block/virtio_blk.c
> >> @@ -281,6 +281,31 @@ static int index_to_minor(int index)
> >> return index << PART_BITS;
> >> }
> >>
> >> +/* Copy serial number from *s to *d. Copy operation terminates on either
> >> + * encountering a nul in *s or after n bytes have been copied, whichever
> >> + * occurs first. *d is not forcibly nul terminated. Return # of bytes copied.
> >> + */
> >> +static inline int serial_sysfs(char *d, char *s, int n)
> >> +{
> >> + char *di = d;
> >> +
> >> + while (*s && n--)
> >> + *d++ = *s++;
> >> + return d - di;
> >> +}
> >> +
> >> +static ssize_t virtblk_serial_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + struct gendisk *disk = dev_to_disk(dev);
> >> + char id_str[VIRTIO_BLK_ID_BYTES];
> >> +
> >> + if (IS_ERR(virtblk_get_id(disk, id_str)))
> >> + return 0;
> >
> > 0? Really? That doesn't seem very informative.
>
> Propagating a prospective error from virtblk_get_id() should
> be possible. Unsure if doing so is more useful from the
> user's perspective compared to just a nul id string.
I'm not sure we can do any thing else here; maybe printk a warning?
Documentation/filesystems/sysfs.txt says that showing attributes should
always return the number of chars put into the buffer; so when there is
an error; zero is the right value to return since we're not filling the
buffer.
>
> >> + return serial_sysfs(buf, id_str, min(VIRTIO_BLK_ID_BYTES, PAGE_SIZE));
> >
> > How about something like this:
> >
> > BUILD_BUG_ON(PAGE_SIZE < VIRTIO_BLK_ID_BYTES + 1);
>
> Agreed, that's a better wrench in the gearworks.
> Note padding buf[] by 1 isn't necessary as indicated
> below.
Yep; that's a good one to take.
>
> > /* id_str is not necessarily nul-terminated! */
> > buf[VIRTIO_BLK_ID_BYTES] = '\0';
> > return virtblk_get_id(disk, buf);
>
> The /sys file is rendered according to the length
> returned from this function and the trailing nul
> is not interpreted in this context. In fact if a
> nul is added and included in the byte count of the
> string it will appear in the /sys file.
Yeah; I like the simplicity; but we do need to know how long the string
is so we can return that value.
>
> Thanks,
>
> -john
>
>
> --
> john.cooper@redhat.com
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices
From: Ryan Harper @ 2010-06-21 16:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: kvm, john cooper, qemu-devel, virtualization
In-Reply-To: <20100621124447.GA8166@lst.de>
* Christoph Hellwig <hch@lst.de> [2010-06-21 07:46]:
> On Fri, Jun 18, 2010 at 01:38:02PM -0500, Ryan Harper wrote:
> > Create a new attribute for virtio-blk devices that will fetch the serial number
> > of the block device. This attribute can be used by udev to create disk/by-id
> > symlinks for devices that don't have a UUID (filesystem) associated with them.
> >
> > ATA_IDENTIFY strings are special in that they can be up to 20 chars long
> > and aren't required to be NULL-terminated. The buffer is also zero-padded
> > meaning that if the serial is 19 chars or less that we get a NULL terminated
> > string. When copying this value into a string buffer, we must be careful to
> > copy up to the NULL (if it present) and only 20 if it is longer and not to
> > attempt to NULL terminate; this isn't needed.
>
> Why is this virtio-blk specific? In a later mail you mention you want
> to use it for udev. So please export this from scsi/libata as well and
> we have one proper interface that we can use for all devices.
ATA and SCSI devices are already supported via ata_id and scsi_id
commands included in udev. Qemu implements the drive serial part for
them and udev creates proper disk/by-id links. This patch is about
filling the gap for virtio-blk devices which cannot work with ata_id and
scsi_id.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com
^ permalink raw reply
* Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices
From: john cooper @ 2010-06-21 17:11 UTC (permalink / raw)
To: Ryan Harper; +Cc: john.cooper, qemu-devel, kvm, virtualization
In-Reply-To: <20100621164321.GF1647@us.ibm.com>
Ryan Harper wrote:
> * john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
>> Rusty Russell wrote:
>>> On Sat, 19 Jun 2010 04:08:02 am Ryan Harper wrote:
>>>> Create a new attribute for virtio-blk devices that will fetch the serial number
>>>> of the block device. This attribute can be used by udev to create disk/by-id
>>>> symlinks for devices that don't have a UUID (filesystem) associated with them.
>>>>
>>>> ATA_IDENTIFY strings are special in that they can be up to 20 chars long
>>>> and aren't required to be NULL-terminated. The buffer is also zero-padded
>>>> meaning that if the serial is 19 chars or less that we get a NULL terminated
>>>> string. When copying this value into a string buffer, we must be careful to
>>>> copy up to the NULL (if it present) and only 20 if it is longer and not to
>>>> attempt to NULL terminate; this isn't needed.
>>>>
>>>> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
>>>> Signed-off-by: john cooper <john.cooper@redhat.com>
>>>> ---
>>>> drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++++
>>>> 1 files changed, 32 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>>> index 258bc2a..f1ef26f 100644
>>>> --- a/drivers/block/virtio_blk.c
>>>> +++ b/drivers/block/virtio_blk.c
>>>> @@ -281,6 +281,31 @@ static int index_to_minor(int index)
>>>> return index << PART_BITS;
>>>> }
>>>>
>>>> +/* Copy serial number from *s to *d. Copy operation terminates on either
>>>> + * encountering a nul in *s or after n bytes have been copied, whichever
>>>> + * occurs first. *d is not forcibly nul terminated. Return # of bytes copied.
>>>> + */
>>>> +static inline int serial_sysfs(char *d, char *s, int n)
>>>> +{
>>>> + char *di = d;
>>>> +
>>>> + while (*s && n--)
>>>> + *d++ = *s++;
>>>> + return d - di;
>>>> +}
>>>> +
>>>> +static ssize_t virtblk_serial_show(struct device *dev,
>>>> + struct device_attribute *attr, char *buf)
>>>> +{
>>>> + struct gendisk *disk = dev_to_disk(dev);
>>>> + char id_str[VIRTIO_BLK_ID_BYTES];
>>>> +
>>>> + if (IS_ERR(virtblk_get_id(disk, id_str)))
>>>> + return 0;
>>> 0? Really? That doesn't seem very informative.
>> Propagating a prospective error from virtblk_get_id() should
>> be possible. Unsure if doing so is more useful from the
>> user's perspective compared to just a nul id string.
>
> I'm not sure we can do any thing else here; maybe printk a warning?
>
> Documentation/filesystems/sysfs.txt says that showing attributes should
> always return the number of chars put into the buffer; so when there is
> an error; zero is the right value to return since we're not filling the
> buffer.
So we return a nul string in the case the qemu user
didn't specify an id string and also in the case a
legacy qemu doesn't support retrieval of an id string.
Not too much difference and if needed going forward the
error return can be elaborated.
>>> /* id_str is not necessarily nul-terminated! */
>>> buf[VIRTIO_BLK_ID_BYTES] = '\0';
>>> return virtblk_get_id(disk, buf);
>> The /sys file is rendered according to the length
>> returned from this function and the trailing nul
>> is not interpreted in this context. In fact if a
>> nul is added and included in the byte count of the
>> string it will appear in the /sys file.
>
> Yeah; I like the simplicity; but we do need to know how long the string
> is so we can return that value.
Which we're getting from serial_sysfs() without
having to accommodate an unused nul. I'd hazard the
primary reason the sysfs calling code keys off a
return of byte count vs. traversing the string itself
is due to the called function almost always having the
byte count available.
-john
--
john.cooper@redhat.com
^ permalink raw reply
* Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices
From: Rusty Russell @ 2010-06-21 23:25 UTC (permalink / raw)
To: Ryan Harper; +Cc: john cooper, qemu-devel, kvm, virtualization
In-Reply-To: <20100621164321.GF1647@us.ibm.com>
On Tue, 22 Jun 2010 02:13:21 am Ryan Harper wrote:
> * john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
> > Rusty Russell wrote:
> > > On Sat, 19 Jun 2010 04:08:02 am Ryan Harper wrote:
> > >> Create a new attribute for virtio-blk devices that will fetch the serial number
> > >> of the block device. This attribute can be used by udev to create disk/by-id
> > >> symlinks for devices that don't have a UUID (filesystem) associated with them.
> > >>
> > >> ATA_IDENTIFY strings are special in that they can be up to 20 chars long
> > >> and aren't required to be NULL-terminated. The buffer is also zero-padded
> > >> meaning that if the serial is 19 chars or less that we get a NULL terminated
> > >> string. When copying this value into a string buffer, we must be careful to
> > >> copy up to the NULL (if it present) and only 20 if it is longer and not to
> > >> attempt to NULL terminate; this isn't needed.
> > >>
> > >> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> > >> Signed-off-by: john cooper <john.cooper@redhat.com>
> > >> ---
> > >> drivers/block/virtio_blk.c | 32 ++++++++++++++++++++++++++++++++
> > >> 1 files changed, 32 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > >> index 258bc2a..f1ef26f 100644
> > >> --- a/drivers/block/virtio_blk.c
> > >> +++ b/drivers/block/virtio_blk.c
> > >> @@ -281,6 +281,31 @@ static int index_to_minor(int index)
> > >> return index << PART_BITS;
> > >> }
> > >>
> > >> +/* Copy serial number from *s to *d. Copy operation terminates on either
> > >> + * encountering a nul in *s or after n bytes have been copied, whichever
> > >> + * occurs first. *d is not forcibly nul terminated. Return # of bytes copied.
> > >> + */
> > >> +static inline int serial_sysfs(char *d, char *s, int n)
> > >> +{
> > >> + char *di = d;
> > >> +
> > >> + while (*s && n--)
> > >> + *d++ = *s++;
> > >> + return d - di;
> > >> +}
> > >> +
> > >> +static ssize_t virtblk_serial_show(struct device *dev,
> > >> + struct device_attribute *attr, char *buf)
> > >> +{
> > >> + struct gendisk *disk = dev_to_disk(dev);
> > >> + char id_str[VIRTIO_BLK_ID_BYTES];
> > >> +
> > >> + if (IS_ERR(virtblk_get_id(disk, id_str)))
> > >> + return 0;
> > >
> > > 0? Really? That doesn't seem very informative.
> >
> > Propagating a prospective error from virtblk_get_id() should
> > be possible. Unsure if doing so is more useful from the
> > user's perspective compared to just a nul id string.
>
> I'm not sure we can do any thing else here; maybe printk a warning?
>
> Documentation/filesystems/sysfs.txt says that showing attributes should
> always return the number of chars put into the buffer; so when there is
> an error; zero is the right value to return since we're not filling the
> buffer.
Ideally, the file shouldn't be set up if we don't have an ID. But we never
did add a feature bit for this :(
At a glance, we'll get -EIO if the host doesn't support it (or any other
transport error). -ENOMEM if we run out of memory.
printk is dumb, but it's nice to differentiate "host didn't supply one" vs
"something went wrong". How about return 0 on -EIO? Whatever is easiest
for udev is best here.
> > > /* id_str is not necessarily nul-terminated! */
> > > buf[VIRTIO_BLK_ID_BYTES] = '\0';
> > > return virtblk_get_id(disk, buf);
> >
> > The /sys file is rendered according to the length
> > returned from this function and the trailing nul
> > is not interpreted in this context. In fact if a
> > nul is added and included in the byte count of the
> > string it will appear in the /sys file.
>
> Yeah; I like the simplicity; but we do need to know how long the string
> is so we can return that value.
So we're looking at something like:
/* id_str is not necessarily nul-terminated! */
buf[VIRTIO_BLK_ID_BYTES] = '\0';
err = virtblk_get_id(disk, buf);
if (!err)
return strlen(buf);
if (err == -EIO) /* Unsupported? Make it empty. */
return 0;
return err;
Then, please *test*!
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices
From: john cooper @ 2010-06-22 3:40 UTC (permalink / raw)
To: Rusty Russell; +Cc: john.cooper, qemu-devel, kvm, virtualization
In-Reply-To: <201006220855.21925.rusty@rustcorp.com.au>
Rusty Russell wrote:
> On Tue, 22 Jun 2010 02:13:21 am Ryan Harper wrote:
>> * john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
>>> Rusty Russell wrote:
>>>> /* id_str is not necessarily nul-terminated! */
>>>> buf[VIRTIO_BLK_ID_BYTES] = '\0';
>>>> return virtblk_get_id(disk, buf);
>>> The /sys file is rendered according to the length
>>> returned from this function and the trailing nul
>>> is not interpreted in this context. In fact if a
>>> nul is added and included in the byte count of the
>>> string it will appear in the /sys file.
>> Yeah; I like the simplicity; but we do need to know how long the string
>> is so we can return that value.
>
> So we're looking at something like:
>
> /* id_str is not necessarily nul-terminated! */
> buf[VIRTIO_BLK_ID_BYTES] = '\0';
> err = virtblk_get_id(disk, buf);
> if (!err)
> return strlen(buf);
> if (err == -EIO) /* Unsupported? Make it empty. */
> return 0;
> return err;
In my haste reading your prior mail, I'd glossed over
the fact you were copying direct to the sysfs buf. So
in retrospect that (and the above) do make sense.
Thanks,
-john
--
john.cooper@redhat.com
^ permalink raw reply
* [PULL] virtio fixes
From: Rusty Russell @ 2010-06-23 13:20 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Miller, virtualization, linux-kernel, Michael S. Tsirkin
(There are two net fixes which depend on this which will go via DaveM, so
please pull soon).
The following changes since commit 7e27d6e778cd87b6f2415515d7127eba53fe5d02:
Linus Torvalds (1):
Linux 2.6.35-rc3
are available in the git repository at:
ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6.git virtio
Michael S. Tsirkin (2):
virtio: return ENOMEM on out of memory
virtio-pci: disable msi at startup
drivers/pci/pci.c | 1 +
drivers/virtio/virtio_pci.c | 3 +++
drivers/virtio/virtio_ring.c | 2 +-
3 files changed, 5 insertions(+), 1 deletions(-)
commit 686d363786a53ed28ee875b84ef24e6d5126ef6f
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Thu Jun 10 18:16:11 2010 +0300
virtio: return ENOMEM on out of memory
add_buf returns ring size on out of memory,
this is not what devices expect.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: stable@kernel.org # .34.x
drivers/virtio/virtio_ring.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
commit b03214d559471359e2a85ae256686381d0672f29
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Wed Jun 23 22:49:06 2010 -0600
virtio-pci: disable msi at startup
virtio-pci resets the device at startup by writing to the status
register, but this does not clear the pci config space,
specifically msi enable status which affects register
layout.
This breaks things like kdump when they try to use e.g. virtio-blk.
Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: stable@kernel.org
drivers/pci/pci.c | 1 +
drivers/virtio/virtio_pci.c | 3 +++
2 files changed, 4 insertions(+), 0 deletions(-)
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Avi Kivity @ 2010-06-23 13:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <20100610152252.GA3510@redhat.com>
On 06/10/2010 06:22 PM, Michael S. Tsirkin wrote:
> virtio-pci resets the device at startup by writing to the status
> register, but this does not clear the pci config space,
> specifically msi enable status which affects register
> layout.
>
> This breaks things like kdump when they try to use e.g. virtio-blk.
>
> Fix by forcing msi off at startup. Since pci.c already has
> a routine to do this, we export and use it instead of duplicating code.
>
>
Why doesn't a device reset result in msi being cleared?
Shouldn't a reset be equivalent to power cycling?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Michael S. Tsirkin @ 2010-06-23 13:59 UTC (permalink / raw)
To: Avi Kivity
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <4C22132F.4060307@redhat.com>
On Wed, Jun 23, 2010 at 04:59:11PM +0300, Avi Kivity wrote:
> On 06/10/2010 06:22 PM, Michael S. Tsirkin wrote:
>> virtio-pci resets the device at startup by writing to the status
>> register, but this does not clear the pci config space,
>> specifically msi enable status which affects register
>> layout.
>>
>> This breaks things like kdump when they try to use e.g. virtio-blk.
>>
>> Fix by forcing msi off at startup. Since pci.c already has
>> a routine to do this, we export and use it instead of duplicating code.
>>
>>
>
> Why doesn't a device reset result in msi being cleared?
This is not a standard function reset. This is virtio specific
command. So it only clears virtio registers.
> Shouldn't a reset be equivalent to power cycling?
If we did this, driver would need to restore registers
such as BAR etc.
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Avi Kivity @ 2010-06-23 14:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <20100623135946.GA30526@redhat.com>
On 06/23/2010 04:59 PM, Michael S. Tsirkin wrote:
>
>> Why doesn't a device reset result in msi being cleared?
>>
> This is not a standard function reset. This is virtio specific
> command. So it only clears virtio registers.
>
I see. We should implement FLR in qemu. If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.
>> Shouldn't a reset be equivalent to power cycling?
>>
> If we did this, driver would need to restore registers
> such as BAR etc.
>
We could save/restore the registers we care about.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Michael S. Tsirkin @ 2010-06-23 14:43 UTC (permalink / raw)
To: Avi Kivity
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <4C221871.6020509@redhat.com>
On Wed, Jun 23, 2010 at 05:21:37PM +0300, Avi Kivity wrote:
> On 06/23/2010 04:59 PM, Michael S. Tsirkin wrote:
>>
>>> Why doesn't a device reset result in msi being cleared?
>>>
>> This is not a standard function reset. This is virtio specific
>> command. So it only clears virtio registers.
>>
>
> I see. We should implement FLR in qemu.
We can do this. Or PM reset. however ...
> If we don't already do so, we
> should probably FLR anything that moves when a kexec kernel starts.
Probably only whatever we want to use. But whether this will make it
more, or less robust, is an open question.
>>> Shouldn't a reset be equivalent to power cycling?
>>>
>> If we did this, driver would need to restore registers
>> such as BAR etc.
>>
>
> We could save/restore the registers we care about.
It seems easier to clear registers we care about. It's also too late
now: changing behaviour will break old drivers.
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Avi Kivity @ 2010-06-23 15:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <20100623144307.GB30526@redhat.com>
On 06/23/2010 05:43 PM, Michael S. Tsirkin wrote:
>
>
>> If we don't already do so, we
>> should probably FLR anything that moves when a kexec kernel starts.
>>
> Probably only whatever we want to use. But whether this will make it
> more, or less robust, is an open question.
>
I'm thinking of a sound card left on (maybe not something you have in
kdump scenarios) or an industrial controller. Those cards have side
effects and you want to quiesce them even if you don't know what they are.
>
>>>> Shouldn't a reset be equivalent to power cycling?
>>>>
>>>>
>>> If we did this, driver would need to restore registers
>>> such as BAR etc.
>>>
>>>
>> We could save/restore the registers we care about.
>>
> It seems easier to clear registers we care about.
We know the registers we care about, we don't know the ones we don't.
I'm talking about FLRing all cards, not just those you want to use.
> It's also too late
> now: changing behaviour will break old drivers.
>
Why? the FLR is triggered by the guest kernel, so all drivers will be
aware it was FLRed.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Michael S. Tsirkin @ 2010-06-23 15:26 UTC (permalink / raw)
To: Avi Kivity
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <4C222508.2060804@redhat.com>
On Wed, Jun 23, 2010 at 06:15:20PM +0300, Avi Kivity wrote:
> On 06/23/2010 05:43 PM, Michael S. Tsirkin wrote:
>>
>>
>>> If we don't already do so, we
>>> should probably FLR anything that moves when a kexec kernel starts.
>>>
>> Probably only whatever we want to use. But whether this will make it
>> more, or less robust, is an open question.
>>
>
> I'm thinking of a sound card left on (maybe not something you have in
> kdump scenarios) or an industrial controller. Those cards have side
> effects and you want to quiesce them even if you don't know what they
> are.
clearing bus master should be enough for that.
we still run the risk of hanging the kernel if the
device is hung, though.
>>
>>>>> Shouldn't a reset be equivalent to power cycling?
>>>>>
>>>>>
>>>> If we did this, driver would need to restore registers
>>>> such as BAR etc.
>>>>
>>>>
>>> We could save/restore the registers we care about.
>>>
>> It seems easier to clear registers we care about.
>
> We know the registers we care about, we don't know the ones we don't.
If/when we use more registers, we can update driver to clear them on start.
> I'm talking about FLRing all cards, not just those you want to use.
reset using FLR/PM is complex because of the need to save/restore
config space. Doing this on a crashing kernel sounds scary.
>> It's also too late
>> now: changing behaviour will break old drivers.
>>
>
> Why? the FLR is triggered by the guest kernel, so all drivers will be
> aware it was FLRed.
Not for FLR. Too late to reset on PA write.
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Avi Kivity @ 2010-06-23 15:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <20100623152633.GC30526@redhat.com>
On 06/23/2010 06:26 PM, Michael S. Tsirkin wrote:
>
>
>>>
>>>
>>>>>> Shouldn't a reset be equivalent to power cycling?
>>>>>>
>>>>>>
>>>>>>
>>>>> If we did this, driver would need to restore registers
>>>>> such as BAR etc.
>>>>>
>>>>>
>>>>>
>>>> We could save/restore the registers we care about.
>>>>
>>>>
>>> It seems easier to clear registers we care about.
>>>
>> We know the registers we care about, we don't know the ones we don't.
>>
> If/when we use more registers, we can update driver to clear them on start.
>
The kdump kernel may not load drivers for those extra devices.
>> I'm talking about FLRing all cards, not just those you want to use.
>>
> reset using FLR/PM is complex because of the need to save/restore
> config space. Doing this on a crashing kernel sounds scary.
>
Well, you only need to save/restore for the devices you use. The rest
you reset and forget.
I don't really see why copying some config space is crazy.
>>> It's also too late
>>> now: changing behaviour will break old drivers.
>>>
>>>
>> Why? the FLR is triggered by the guest kernel, so all drivers will be
>> aware it was FLRed.
>>
> Not for FLR. Too late to reset on PA write.
>
>
What's PA write?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Michael S. Tsirkin @ 2010-06-23 15:43 UTC (permalink / raw)
To: Avi Kivity
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <4C2229DB.6060706@redhat.com>
On Wed, Jun 23, 2010 at 06:35:55PM +0300, Avi Kivity wrote:
> On 06/23/2010 06:26 PM, Michael S. Tsirkin wrote:
>>
>>
>>>>
>>>>
>>>>>>> Shouldn't a reset be equivalent to power cycling?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> If we did this, driver would need to restore registers
>>>>>> such as BAR etc.
>>>>>>
>>>>>>
>>>>>>
>>>>> We could save/restore the registers we care about.
>>>>>
>>>>>
>>>> It seems easier to clear registers we care about.
>>>>
>>> We know the registers we care about, we don't know the ones we don't.
>>>
>> If/when we use more registers, we can update driver to clear them on start.
>>
>
> The kdump kernel may not load drivers for those extra devices.
Then we don't care about clearing them?
>>> I'm talking about FLRing all cards, not just those you want to use.
>>>
>> reset using FLR/PM is complex because of the need to save/restore
>> config space. Doing this on a crashing kernel sounds scary.
>>
>
> Well, you only need to save/restore for the devices you use. The rest
> you reset and forget.
>
> I don't really see why copying some config space is crazy.
Maybe not crazy, but complex. Look at pci_restore_state.
Anyway, if kdump wants to do this, it's a question of
calling pci_reset_function.
>>>> It's also too late
>>>> now: changing behaviour will break old drivers.
>>>>
>>>>
>>> Why? the FLR is triggered by the guest kernel, so all drivers will be
>>> aware it was FLRed.
>>>
>> Not for FLR. Too late to reset on PA write.
>>
>>
>
> What's PA write?
thats how we reset virtio today: write 0 to PA register.
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Jesse Barnes @ 2010-06-23 15:51 UTC (permalink / raw)
To: Avi Kivity
Cc: Anthony Liguori, Michael S. Tsirkin, linux-pci, Matt Carlson,
linux-kernel, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <4C2229DB.6060706@redhat.com>
On Wed, 23 Jun 2010 18:35:55 +0300
Avi Kivity <avi@redhat.com> wrote:
> On 06/23/2010 06:26 PM, Michael S. Tsirkin wrote:
> >
> >
> >>>
> >>>
> >>>>>> Shouldn't a reset be equivalent to power cycling?
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>> If we did this, driver would need to restore registers
> >>>>> such as BAR etc.
> >>>>>
> >>>>>
> >>>>>
> >>>> We could save/restore the registers we care about.
> >>>>
> >>>>
> >>> It seems easier to clear registers we care about.
> >>>
> >> We know the registers we care about, we don't know the ones we don't.
> >>
> > If/when we use more registers, we can update driver to clear them on start.
> >
>
> The kdump kernel may not load drivers for those extra devices.
FLR or another type of reset also has the nice property of bringing the
device into a known state. kexec/kdump has always been vulnerable to
having devices in partial states when the new kernel loads; would be
good to make it more robust.
>
> >> I'm talking about FLRing all cards, not just those you want to use.
> >>
> > reset using FLR/PM is complex because of the need to save/restore
> > config space. Doing this on a crashing kernel sounds scary.
> >
>
> Well, you only need to save/restore for the devices you use. The rest
> you reset and forget.
>
> I don't really see why copying some config space is crazy.
We could push any needed save/restore of core settings and regs into
the PCI core like we do for PM. That would save a bunch of driver
trouble...
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup
From: Avi Kivity @ 2010-06-23 15:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, linux-pci, Matt Carlson, linux-kernel,
Jesse Barnes, virtualization, Rafael J. Wysocki, Kenji Kaneshige,
Tejun Heo, David S. Miller, Bjorn Helgaas
In-Reply-To: <20100623154311.GD30526@redhat.com>
On 06/23/2010 06:43 PM, Michael S. Tsirkin wrote:
>
>
>>> If/when we use more registers, we can update driver to clear them on start.
>>>
>>>
>> The kdump kernel may not load drivers for those extra devices.
>>
> Then we don't care about clearing them?
>
We do, if the device has a side effect (like blasting out noise through
loudspeakers).
Anyway, just a suggestion.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-23 21:24 UTC (permalink / raw)
To: virtualization; +Cc: Stefan Hajnoczi, kvm
The virtio block device holds a lock during I/O request processing.
Kicking the virtqueue while the lock is held results in long lock hold
times and increases contention for the lock.
This patch modifies virtqueue_kick() to optionally release a lock while
notifying the host. Virtio block is modified to pass in its lock. This
allows other vcpus to queue I/O requests during the time spent servicing
the virtqueue notify in the host.
The virtqueue_kick() function is modified to know about locking because
it changes the state of the virtqueue and should execute with the lock
held (it would not be correct for virtio block to release the lock
before calling virtqueue_kick()).
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
I am not yet 100% happy with this patch which aims to reduce guest CPU
consumption related to vblk->lock contention. Although this patch reduces
wait/hold times it does not affect I/O throughput or guest CPU utilization.
More investigation is required to get to the bottom of why guest CPU
utilization does not decrease when a lock bottleneck has been removed.
Performance figures:
Host: 2.6.34 upstream kernel, qemu-kvm-0.12.4 if=virtio,cache=none
Guest: 2.6.35-rc3-kvm.git upstream kernel
Storage: 12 disks as striped LVM volume
Benchmark: 4 concurrent dd bs=4k iflag=direct
Lockstat data for &vblk->lock:
test con-bounces contentions waittime-min waittime-max waittime-total
unmodified 7097 7108 0.31 956.09 161165.4
patched 11484 11550 0.30 411.80 50245.83
The maximum wait time went down by 544.29 us (-57%) and the total wait time
decreased by 69%. This shows that the virtqueue kick is indeed hogging the
lock.
The patched version actually has higher contention than the unmodified version.
I think the reason for this is that each virtqueue kick now includes a short
release and reacquire. This short release gives other vcpus a chance to
acquire the lock and progress, hence more contention but overall better wait
time numbers.
name acq-bounces acquisitions holdtime-min holdtime-max holdtime-total
unmodified 10771 5038346 0.00 3271.81 59016905.47
patched 31594 5857813 0.00 219.76 24104915.55
Here we see the full impact of this patch: hold time reduced to 219.76 us
(-93%).
Again the acquisitions have increased since we're now doing an extra
unlock+lock per virtqueue kick.
Testing, ideas, and comments appreciated.
drivers/block/virtio_blk.c | 2 +-
drivers/char/hw_random/virtio-rng.c | 2 +-
drivers/char/virtio_console.c | 6 +++---
drivers/net/virtio_net.c | 6 +++---
drivers/virtio/virtio_balloon.c | 6 +++---
drivers/virtio/virtio_ring.c | 13 +++++++++++--
include/linux/virtio.h | 3 ++-
net/9p/trans_virtio.c | 2 +-
8 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 258bc2a..de033bf 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -187,7 +187,7 @@ static void do_virtblk_request(struct request_queue *q)
}
if (issued)
- virtqueue_kick(vblk->vq);
+ virtqueue_kick(vblk->vq, &vblk->lock);
}
static void virtblk_prepare_flush(struct request_queue *q, struct request *req)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 75f1cbd..852d563 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -49,7 +49,7 @@ static void register_buffer(u8 *buf, size_t size)
if (virtqueue_add_buf(vq, &sg, 0, 1, buf) < 0)
BUG();
- virtqueue_kick(vq);
+ virtqueue_kick(vq, NULL);
}
static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 942a982..677714d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -328,7 +328,7 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf)
sg_init_one(sg, buf->buf, buf->size);
ret = virtqueue_add_buf(vq, sg, 0, 1, buf);
- virtqueue_kick(vq);
+ virtqueue_kick(vq, NULL);
return ret;
}
@@ -400,7 +400,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
sg_init_one(sg, &cpkt, sizeof(cpkt));
if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt) >= 0) {
- virtqueue_kick(vq);
+ virtqueue_kick(vq, NULL);
while (!virtqueue_get_buf(vq, &len))
cpu_relax();
}
@@ -444,7 +444,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf);
/* Tell Host to go! */
- virtqueue_kick(out_vq);
+ virtqueue_kick(out_vq, NULL);
if (ret < 0) {
in_count = 0;
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1edb7a6..6a837b3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -433,7 +433,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
} while (err > 0);
if (unlikely(vi->num > vi->max))
vi->max = vi->num;
- virtqueue_kick(vi->rvq);
+ virtqueue_kick(vi->rvq, NULL);
return !oom;
}
@@ -581,7 +581,7 @@ again:
}
return NETDEV_TX_BUSY;
}
- virtqueue_kick(vi->svq);
+ virtqueue_kick(vi->svq, NULL);
/* Don't wait up for transmitted skbs to be freed. */
skb_orphan(skb);
@@ -680,7 +680,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi) < 0);
- virtqueue_kick(vi->cvq);
+ virtqueue_kick(vi->cvq, NULL);
/*
* Spin for a response, the kick causes an ioport write, trapping
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0f1da45..c9c5c4a 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -91,7 +91,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
/* We should always be able to add one buffer to an empty queue. */
if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
BUG();
- virtqueue_kick(vq);
+ virtqueue_kick(vq, NULL);
/* When host has read buffer, this completes via balloon_ack */
wait_for_completion(&vb->acked);
@@ -223,7 +223,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
sg_init_one(&sg, vb->stats, sizeof(vb->stats));
if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
BUG();
- virtqueue_kick(vq);
+ virtqueue_kick(vq, NULL);
}
static void virtballoon_changed(struct virtio_device *vdev)
@@ -316,7 +316,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
sg_init_one(&sg, vb->stats, sizeof vb->stats);
if (virtqueue_add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0)
BUG();
- virtqueue_kick(vb->stats_vq);
+ virtqueue_kick(vb->stats_vq, NULL);
}
vb->thread = kthread_run(balloon, vb, "vballoon");
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1ca8890..163a237 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -236,7 +236,7 @@ add_head:
}
EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
-void virtqueue_kick(struct virtqueue *_vq)
+void virtqueue_kick(struct virtqueue *_vq, spinlock_t *lock)
{
struct vring_virtqueue *vq = to_vvq(_vq);
START_USE(vq);
@@ -250,10 +250,19 @@ void virtqueue_kick(struct virtqueue *_vq)
/* Need to update avail index before checking if we should notify */
virtio_mb();
- if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
+ if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) {
+ /* Release lock while doing the kick because the guest should
+ * not exit with the lock held. */
+ if (lock)
+ spin_unlock(lock);
+
/* Prod other side to tell it about changes. */
vq->notify(&vq->vq);
+ if (lock)
+ spin_lock(lock);
+ }
+
END_USE(vq);
}
EXPORT_SYMBOL_GPL(virtqueue_kick);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index aff5b4f..1561c86 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -37,6 +37,7 @@ struct virtqueue {
* Returns remaining capacity of queue (sg segments) or a negative error.
* virtqueue_kick: update after add_buf
* vq: the struct virtqueue
+ * lock: spinlock to release during kick (may be NULL).
* After one or more add_buf calls, invoke this to kick the other side.
* virtqueue_get_buf: get the next used buffer
* vq: the struct virtqueue we're talking about.
@@ -78,7 +79,7 @@ static inline int virtqueue_add_buf(struct virtqueue *vq,
return virtqueue_add_buf_gfp(vq, sg, out_num, in_num, data, GFP_ATOMIC);
}
-void virtqueue_kick(struct virtqueue *vq);
+void virtqueue_kick(struct virtqueue *vq, spinlock_t *lock);
void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index dcfbe99..ccf17dc 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -215,7 +215,7 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
return -EIO;
}
- virtqueue_kick(chan->vq);
+ virtqueue_kick(chan->vq, NULL);
P9_DPRINTK(P9_DEBUG_TRANS, "9p debug: virtio request kicked\n");
return 0;
--
1.7.1
^ permalink raw reply related
* Re: [RFC] virtio: Support releasing lock during kick
From: Anthony Liguori @ 2010-06-23 22:12 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, virtualization
In-Reply-To: <1277328242-10685-1-git-send-email-stefanha@linux.vnet.ibm.com>
On 06/23/2010 04:24 PM, Stefan Hajnoczi wrote:
> The virtio block device holds a lock during I/O request processing.
> Kicking the virtqueue while the lock is held results in long lock hold
> times and increases contention for the lock.
>
> This patch modifies virtqueue_kick() to optionally release a lock while
> notifying the host. Virtio block is modified to pass in its lock. This
> allows other vcpus to queue I/O requests during the time spent servicing
> the virtqueue notify in the host.
>
> The virtqueue_kick() function is modified to know about locking because
> it changes the state of the virtqueue and should execute with the lock
> held (it would not be correct for virtio block to release the lock
> before calling virtqueue_kick()).
>
> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
> ---
> I am not yet 100% happy with this patch which aims to reduce guest CPU
> consumption related to vblk->lock contention. Although this patch reduces
> wait/hold times it does not affect I/O throughput or guest CPU utilization.
> More investigation is required to get to the bottom of why guest CPU
> utilization does not decrease when a lock bottleneck has been removed.
>
> Performance figures:
>
> Host: 2.6.34 upstream kernel, qemu-kvm-0.12.4 if=virtio,cache=none
> Guest: 2.6.35-rc3-kvm.git upstream kernel
> Storage: 12 disks as striped LVM volume
> Benchmark: 4 concurrent dd bs=4k iflag=direct
>
> Lockstat data for&vblk->lock:
>
> test con-bounces contentions waittime-min waittime-max waittime-total
> unmodified 7097 7108 0.31 956.09 161165.4
> patched 11484 11550 0.30 411.80 50245.83
>
> The maximum wait time went down by 544.29 us (-57%) and the total wait time
> decreased by 69%. This shows that the virtqueue kick is indeed hogging the
> lock.
>
> The patched version actually has higher contention than the unmodified version.
> I think the reason for this is that each virtqueue kick now includes a short
> release and reacquire. This short release gives other vcpus a chance to
> acquire the lock and progress, hence more contention but overall better wait
> time numbers.
>
> name acq-bounces acquisitions holdtime-min holdtime-max holdtime-total
> unmodified 10771 5038346 0.00 3271.81 59016905.47
> patched 31594 5857813 0.00 219.76 24104915.55
>
> Here we see the full impact of this patch: hold time reduced to 219.76 us
> (-93%).
>
> Again the acquisitions have increased since we're now doing an extra
> unlock+lock per virtqueue kick.
>
> Testing, ideas, and comments appreciated.
>
> drivers/block/virtio_blk.c | 2 +-
> drivers/char/hw_random/virtio-rng.c | 2 +-
> drivers/char/virtio_console.c | 6 +++---
> drivers/net/virtio_net.c | 6 +++---
> drivers/virtio/virtio_balloon.c | 6 +++---
> drivers/virtio/virtio_ring.c | 13 +++++++++++--
> include/linux/virtio.h | 3 ++-
> net/9p/trans_virtio.c | 2 +-
> 8 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 258bc2a..de033bf 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -187,7 +187,7 @@ static void do_virtblk_request(struct request_queue *q)
> }
>
> if (issued)
> - virtqueue_kick(vblk->vq);
> + virtqueue_kick(vblk->vq,&vblk->lock);
> }
>
Shouldn't it be possible to just drop the lock before invoking
virtqueue_kick() and reacquire it afterwards? There's nothing in that
virtqueue_kick() path that the lock is protecting AFAICT.
Regards,
Anthony Liguori
^ permalink raw reply
* [PATCH 0/2] v2: Add 'serial' attribute to virtio-blk devices
From: Ryan Harper @ 2010-06-24 3:19 UTC (permalink / raw)
To: virtualization; +Cc: john cooper, qemu-devel, kvm
Using Rusty's suggestion I've respun the patch removing the special copy
function. I've tested this patch in a guest kernel with and without qemu
supplying serial numbers for the block devices and it's working as expected.
When qemu supplies serial numbers, the correct value is supplied to
/sys/block/vdX/serial and can be used by udev for generating disk/by-id paths
(without separate utility). When running without serial number support the path
still exists in sysfs but produces no output.
Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
^ permalink raw reply
* [PATCH 1/2] v2 Add 'serial' attribute to virtio-blk devices
From: Ryan Harper @ 2010-06-24 3:19 UTC (permalink / raw)
To: virtualization; +Cc: john cooper, qemu-devel, kvm
In-Reply-To: <1277349598-24559-1-git-send-email-ryanh@us.ibm.com>
Create a new attribute for virtio-blk devices that will fetch the serial number
of the block device. This attribute can be used by udev to create disk/by-id
symlinks for devices that don't have a UUID (filesystem) associated with them.
ATA_IDENTIFY strings are special in that they can be up to 20 chars long
and aren't required to be nul-terminated. The buffer is also zero-padded
meaning that if the serial is 19 chars or less that we get a nul-terminated
string. When copying this value into a string buffer, we must be careful to
copy up to the nul (if it present) and only 20 if it is longer and not to
attempt to nul terminate; this isn't needed.
Changes since v1:
- Added BUILD_BUG_ON() for PAGE_SIZE check
- Removed min() since BUILD_BUG_ON() handles the check
- Replaced serial_sysfs() by copying id directly to buffer
Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
Signed-off-by: john cooper <john.cooper@redhat.com>
---
drivers/block/virtio_blk.c | 28 ++++++++++++++++++++++++++++
1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 258bc2a..34411ff 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -281,6 +281,27 @@ static int index_to_minor(int index)
return index << PART_BITS;
}
+static ssize_t virtblk_serial_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+ int err;
+
+ /* sysfs gives us a PAGE_SIZE buffer */
+ BUILD_BUG_ON(PAGE_SIZE < VIRTIO_BLK_ID_BYTES);
+
+ buf[VIRTIO_BLK_ID_BYTES] = '\0';
+ err = virtblk_get_id(disk, buf);
+ if (!err)
+ return strlen(buf);
+
+ if (err == -EIO) /* Unsupported? Make it empty. */
+ return 0;
+
+ return err;
+}
+DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
+
static int __devinit virtblk_probe(struct virtio_device *vdev)
{
struct virtio_blk *vblk;
@@ -445,8 +466,15 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
add_disk(vblk->disk);
+ err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial);
+ if (err)
+ goto out_del_disk;
+
return 0;
+out_del_disk:
+ del_gendisk(vblk->disk);
+ blk_cleanup_queue(vblk->disk->queue);
out_put_disk:
put_disk(vblk->disk);
out_mempool:
--
1.6.3.3
^ permalink raw reply related
* [PATCH 2/2] Remove virtio_blk VBID ioctl
From: Ryan Harper @ 2010-06-24 3:19 UTC (permalink / raw)
To: virtualization; +Cc: john cooper, qemu-devel, kvm
In-Reply-To: <1277349598-24559-1-git-send-email-ryanh@us.ibm.com>
With the availablility of a sysfs device attribute for examining disk serial
numbers the ioctl is no longer needed. The user-space changes for this aren't
upstream yet so we don't have any users to worry about.
Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
drivers/block/virtio_blk.c | 10 ----------
1 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 34411ff..04458cd 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -225,16 +225,6 @@ static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
struct gendisk *disk = bdev->bd_disk;
struct virtio_blk *vblk = disk->private_data;
- if (cmd == 0x56424944) { /* 'VBID' */
- void __user *usr_data = (void __user *)data;
- char id_str[VIRTIO_BLK_ID_BYTES];
- int err;
-
- err = virtblk_get_id(disk, id_str);
- if (!err && copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES))
- err = -EFAULT;
- return err;
- }
/*
* Only allow the generic SCSI ioctls if the host can support it.
*/
--
1.6.3.3
^ permalink raw reply related
* Re: [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-24 5:30 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Stefan Hajnoczi, kvm, virtualization
In-Reply-To: <4C2286BE.40808@codemonkey.ws>
On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Shouldn't it be possible to just drop the lock before invoking
> virtqueue_kick() and reacquire it afterwards? There's nothing in that
> virtqueue_kick() path that the lock is protecting AFAICT.
No, that would lead to a race condition because vq->num_added is
modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
Without a lock held during virtqueue_kick() another vcpu could add
bufs while vq->num_added is used and cleared by virtqueue_kick():
void virtqueue_kick(struct virtqueue *_vq, spinlock_t *lock)
{
struct vring_virtqueue *vq = to_vvq(_vq);
START_USE(vq);
/* Descriptors and available array need to be set before we expose the
* new available array entries. */
virtio_wmb();
vq->vring.avail->idx += vq->num_added;
vq->num_added = 0;
/* Need to update avail index before checking if we should notify */
virtio_mb();
if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) {
/* Release lock while doing the kick because the guest should
* not exit with the lock held. */
if (lock)
spin_unlock(lock);
/* Prod other side to tell it about changes. */
vq->notify(&vq->vq);
if (lock)
spin_lock(lock);
}
END_USE(vq);
}
Stefan
^ permalink raw reply
* Re: [PATCH 0/2] v2: Add 'serial' attribute to virtio-blk devices
From: Rusty Russell @ 2010-06-25 3:04 UTC (permalink / raw)
To: Ryan Harper; +Cc: john cooper, qemu-devel, kvm, virtualization
In-Reply-To: <1277349598-24559-1-git-send-email-ryanh@us.ibm.com>
On Thu, 24 Jun 2010 12:49:56 pm Ryan Harper wrote:
> Using Rusty's suggestion I've respun the patch removing the special copy
> function. I've tested this patch in a guest kernel with and without qemu
> supplying serial numbers for the block devices and it's working as expected.
> When qemu supplies serial numbers, the correct value is supplied to
> /sys/block/vdX/serial and can be used by udev for generating disk/by-id paths
> (without separate utility). When running without serial number support the path
> still exists in sysfs but produces no output.
>
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
Great, both applied.
BTW, a mail without a patch doesn't need a sign off, does it?
Thanks,
Ruty.
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Rusty Russell @ 2010-06-25 3:09 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin, Stefan Hajnoczi, Anthony Liguori, kvm
In-Reply-To: <AANLkTim6CH_NruBFqK6fIkMkKpAuCIef50mHfldMtNH9@mail.gmail.com>
On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
> On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > Shouldn't it be possible to just drop the lock before invoking
> > virtqueue_kick() and reacquire it afterwards? There's nothing in that
> > virtqueue_kick() path that the lock is protecting AFAICT.
>
> No, that would lead to a race condition because vq->num_added is
> modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
> Without a lock held during virtqueue_kick() another vcpu could add
> bufs while vq->num_added is used and cleared by virtqueue_kick():
Right, this dovetails with another proposed change (was it Michael?)
where we would update the avail idx inside add_buf, rather than waiting
until kick. This means a barrier inside add_buf, but that's probably
fine.
If we do that, then we don't need a lock on virtqueue_kick.
Michael, thoughts?
Thanks,
Rusty.
^ permalink raw reply
* Re: [RFC] virtio: Support releasing lock during kick
From: Stefan Hajnoczi @ 2010-06-25 6:17 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, Stefan Hajnoczi, Anthony Liguori,
virtualization
In-Reply-To: <201006251239.23224.rusty@rustcorp.com.au>
On Fri, Jun 25, 2010 at 4:09 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Thu, 24 Jun 2010 03:00:30 pm Stefan Hajnoczi wrote:
>> On Wed, Jun 23, 2010 at 11:12 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> > Shouldn't it be possible to just drop the lock before invoking
>> > virtqueue_kick() and reacquire it afterwards? There's nothing in that
>> > virtqueue_kick() path that the lock is protecting AFAICT.
>>
>> No, that would lead to a race condition because vq->num_added is
>> modified by both virtqueue_add_buf_gfp() and virtqueue_kick().
>> Without a lock held during virtqueue_kick() another vcpu could add
>> bufs while vq->num_added is used and cleared by virtqueue_kick():
>
> Right, this dovetails with another proposed change (was it Michael?)
> where we would update the avail idx inside add_buf, rather than waiting
> until kick. This means a barrier inside add_buf, but that's probably
> fine.
>
> If we do that, then we don't need a lock on virtqueue_kick.
That would be nice, we could push the change up into just virtio-blk.
I did wonder if virtio-net can take advantage of unlocked kick, too,
but haven't investigated yet. The virtio-net kick in start_xmit()
happens with the netdev _xmit_lock held. Any ideas?
Stefan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox