Re: [PATCH 2/3] S390: Add virtio hotplug add support
On 09/13/2010 09:41 AM, Martin Schwidefsky wrote: Actually Marcelo applied it. But the natural place for it is Rusty's virtio tree. Rusty, if you want to take it, let me know and I'll drop it from kvm.git. I thought it would be in the s390 tree, which is why I didn't take it... But I'm *always* happy to let do the work! I didn't pick them up after I saw that Marcelo took them. If others want to do the work, be my guest.. I just hope that all this generosity doesn't lead to merge conflicts later, or people basing their stuff on stale code. But it isn't like this is a high churn area. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Mon, 13 Sep 2010 13:05:57 +0930 Rusty Russell wrote: > On Sun, 12 Sep 2010 06:30:43 pm Avi Kivity wrote: > > On 09/12/2010 02:42 AM, Alexander Graf wrote: > > > On 24.08.2010, at 15:48, Alexander Graf wrote: > > > > > >> The one big missing feature in s390-virtio was hotplugging. This is no > > >> more. > > >> This patch implements hotplug add support, so you can on the fly add new > > >> devices > > >> in the guest. > > >> > > >> Keep in mind that this needs a patch for qemu to actually leverage the > > >> functionality. > > >> > > >> Signed-off-by: Alexander Graf > > > ping (on the patch set)? > > > > > > > Actually Marcelo applied it. But the natural place for it is Rusty's > > virtio tree. Rusty, if you want to take it, let me know and I'll drop > > it from kvm.git. > > I thought it would be in the s390 tree, which is why I didn't take it... > > But I'm *always* happy to let do the work! I didn't pick them up after I saw that Marcelo took them. If others want to do the work, be my guest.. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Sun, 12 Sep 2010 06:30:43 pm Avi Kivity wrote: > On 09/12/2010 02:42 AM, Alexander Graf wrote: > > On 24.08.2010, at 15:48, Alexander Graf wrote: > > > >> The one big missing feature in s390-virtio was hotplugging. This is no > >> more. > >> This patch implements hotplug add support, so you can on the fly add new > >> devices > >> in the guest. > >> > >> Keep in mind that this needs a patch for qemu to actually leverage the > >> functionality. > >> > >> Signed-off-by: Alexander Graf > > ping (on the patch set)? > > > > Actually Marcelo applied it. But the natural place for it is Rusty's > virtio tree. Rusty, if you want to take it, let me know and I'll drop > it from kvm.git. I thought it would be in the s390 tree, which is why I didn't take it... But I'm *always* happy to let do the work! Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On 09/12/2010 02:42 AM, Alexander Graf wrote: On 24.08.2010, at 15:48, Alexander Graf wrote: The one big missing feature in s390-virtio was hotplugging. This is no more. This patch implements hotplug add support, so you can on the fly add new devices in the guest. Keep in mind that this needs a patch for qemu to actually leverage the functionality. Signed-off-by: Alexander Graf ping (on the patch set)? Actually Marcelo applied it. But the natural place for it is Rusty's virtio tree. Rusty, if you want to take it, let me know and I'll drop it from kvm.git. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On 24.08.2010, at 15:48, Alexander Graf wrote: > The one big missing feature in s390-virtio was hotplugging. This is no more. > This patch implements hotplug add support, so you can on the fly add new > devices > in the guest. > > Keep in mind that this needs a patch for qemu to actually leverage the > functionality. > > Signed-off-by: Alexander Graf ping (on the patch set)? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On 25.08.2010, at 10:48, Heiko Carstens wrote:
> On Wed, Aug 25, 2010 at 10:34:29AM +0200, Alexander Graf wrote:
>> On 25.08.2010, at 10:35, Heiko Carstens wrote:
>>> On Wed, Aug 25, 2010 at 10:20:03AM +0200, Alexander Graf wrote:
On 25.08.2010, at 10:16, Heiko Carstens wrote:
> On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
>> +static void hotplug_devices(struct work_struct *dummy)
>> +{
>> +unsigned int i;
>> +struct kvm_device_desc *d;
>> +struct device *dev;
>> +
>> +for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
>
> This should be
>
> for (i = 0; i + desc_size(d) <= PAGE_SIZE; i += desc_size(d)) {
>
> otherwise you might have memory accesses beyond the device page...
Oh, this is a simple copy&paste from the original search method.
Is d valid in the first part of the loop already?
>>>
>>> No, you would need to initialize it with kvm_devices if you change
>>> the loop.
>>
>> But even then it would take the size of the current entry, not the next
>> one we're actually looking for, no? Maybe it'd be easier to just convert
>> this into a while loop and explicitly open code everything.
>
> Yes indeed. It's going to be quite ugly if you want to make sure that at
> no time you're going to access memory beyond the device page. Eeek.. :)
Thinking about this a bit more ... it's no problem to access beyond the device
page. After the device page follow the rings and we always have rings because
we always have a virtio console :).
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Wed, Aug 25, 2010 at 10:34:29AM +0200, Alexander Graf wrote:
> On 25.08.2010, at 10:35, Heiko Carstens wrote:
> > On Wed, Aug 25, 2010 at 10:20:03AM +0200, Alexander Graf wrote:
> >> On 25.08.2010, at 10:16, Heiko Carstens wrote:
> >>> On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
> +static void hotplug_devices(struct work_struct *dummy)
> +{
> +unsigned int i;
> +struct kvm_device_desc *d;
> +struct device *dev;
> +
> +for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
> >>>
> >>> This should be
> >>>
> >>> for (i = 0; i + desc_size(d) <= PAGE_SIZE; i += desc_size(d)) {
> >>>
> >>> otherwise you might have memory accesses beyond the device page...
> >>
> >> Oh, this is a simple copy&paste from the original search method.
> >> Is d valid in the first part of the loop already?
> >
> > No, you would need to initialize it with kvm_devices if you change
> > the loop.
>
> But even then it would take the size of the current entry, not the next
> one we're actually looking for, no? Maybe it'd be easier to just convert
> this into a while loop and explicitly open code everything.
Yes indeed. It's going to be quite ugly if you want to make sure that at
no time you're going to access memory beyond the device page. Eeek.. :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On 25.08.2010, at 10:35, Heiko Carstens wrote:
> On Wed, Aug 25, 2010 at 10:20:03AM +0200, Alexander Graf wrote:
>> On 25.08.2010, at 10:16, Heiko Carstens wrote:
>>> On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
+static void hotplug_devices(struct work_struct *dummy)
+{
+ unsigned int i;
+ struct kvm_device_desc *d;
+ struct device *dev;
+
+ for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
>>>
>>> This should be
>>>
>>> for (i = 0; i + desc_size(d) <= PAGE_SIZE; i += desc_size(d)) {
>>>
>>> otherwise you might have memory accesses beyond the device page...
>>
>> Oh, this is a simple copy&paste from the original search method.
>> Is d valid in the first part of the loop already?
>
> No, you would need to initialize it with kvm_devices if you change
> the loop.
But even then it would take the size of the current entry, not the next one
we're actually looking for, no? Maybe it'd be easier to just convert this into
a while loop and explicitly open code everything.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Wed, Aug 25, 2010 at 10:20:03AM +0200, Alexander Graf wrote:
> On 25.08.2010, at 10:16, Heiko Carstens wrote:
> > On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
> >> +static void hotplug_devices(struct work_struct *dummy)
> >> +{
> >> + unsigned int i;
> >> + struct kvm_device_desc *d;
> >> + struct device *dev;
> >> +
> >> + for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
> >
> > This should be
> >
> > for (i = 0; i + desc_size(d) <= PAGE_SIZE; i += desc_size(d)) {
> >
> > otherwise you might have memory accesses beyond the device page...
>
> Oh, this is a simple copy&paste from the original search method.
> Is d valid in the first part of the loop already?
No, you would need to initialize it with kvm_devices if you change
the loop.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On 25.08.2010, at 10:16, Heiko Carstens wrote:
> On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
>> +static void hotplug_devices(struct work_struct *dummy)
>> +{
>> +unsigned int i;
>> +struct kvm_device_desc *d;
>> +struct device *dev;
>> +
>> +for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
>
> This should be
>
> for (i = 0; i + desc_size(d) <= PAGE_SIZE; i += desc_size(d)) {
>
> otherwise you might have memory accesses beyond the device page...
Oh, this is a simple copy&paste from the original search method. Is d valid in
the first part of the loop already?
>
>> +d = kvm_devices + i;
>> +
>> +/* end of list */
>> +if (d->type == 0)
>> +break;
>
> ...even if that should not happen if everything works.
> But let's be paranoid.
Yeah :). I like paranoid.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] S390: Add virtio hotplug add support
On Tue, Aug 24, 2010 at 03:48:51PM +0200, Alexander Graf wrote:
> +static void hotplug_devices(struct work_struct *dummy)
> +{
> + unsigned int i;
> + struct kvm_device_desc *d;
> + struct device *dev;
> +
> + for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
This should be
for (i = 0; i + desc_size(d) <= PAGE_SIZE; i += desc_size(d)) {
otherwise you might have memory accesses beyond the device page...
> + d = kvm_devices + i;
> +
> + /* end of list */
> + if (d->type == 0)
> + break;
...even if that should not happen if everything works.
But let's be paranoid.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] S390: Add virtio hotplug add support
The one big missing feature in s390-virtio was hotplugging. This is no more.
This patch implements hotplug add support, so you can on the fly add new devices
in the guest.
Keep in mind that this needs a patch for qemu to actually leverage the
functionality.
Signed-off-by: Alexander Graf
---
v1 -> v2:
- move dev_add to kvm_virtio.h
---
arch/s390/include/asm/kvm_virtio.h |1 +
drivers/s390/kvm/kvm_virtio.c | 47
2 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/arch/s390/include/asm/kvm_virtio.h
b/arch/s390/include/asm/kvm_virtio.h
index 3f5d100..72f6141 100644
--- a/arch/s390/include/asm/kvm_virtio.h
+++ b/arch/s390/include/asm/kvm_virtio.h
@@ -59,5 +59,6 @@ struct kvm_vqconfig {
#define VIRTIO_PARAM_MASK 0xff
#define VIRTIO_PARAM_VRING_INTERRUPT 0x0
#define VIRTIO_PARAM_CONFIG_CHANGED0x1
+#define VIRTIO_PARAM_DEV_ADD 0x2
#endif
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 68cef4d..5a46b8c 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -32,6 +32,7 @@
* The pointer to our (page) of device descriptions.
*/
static void *kvm_devices;
+struct work_struct hotplug_work;
struct kvm_device {
struct virtio_device vdev;
@@ -328,6 +329,47 @@ static void scan_devices(void)
}
/*
+ * match for a kvm device with a specific desc pointer
+ */
+static int match_desc(struct device *dev, void *data)
+{
+ if ((ulong)to_kvmdev(dev_to_virtio(dev))->desc == (ulong)data)
+ return 1;
+
+ return 0;
+}
+
+/*
+ * hotplug_device tries to find changes in the device page.
+ */
+static void hotplug_devices(struct work_struct *dummy)
+{
+ unsigned int i;
+ struct kvm_device_desc *d;
+ struct device *dev;
+
+ for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
+ d = kvm_devices + i;
+
+ /* end of list */
+ if (d->type == 0)
+ break;
+
+ /* device already exists */
+ dev = device_find_child(kvm_root, d, match_desc);
+ if (dev) {
+ /* XXX check for hotplug remove */
+ put_device(dev);
+ continue;
+ }
+
+ /* new device */
+ printk(KERN_INFO "Adding new virtio device %p\n", d);
+ add_kvm_device(d, i);
+ }
+}
+
+/*
* we emulate the request_irq behaviour on top of s390 extints
*/
static void kvm_extint_handler(u16 code)
@@ -357,6 +399,9 @@ static void kvm_extint_handler(u16 code)
break;
}
+ case VIRTIO_PARAM_DEV_ADD:
+ schedule_work(&hotplug_work);
+ break;
case VIRTIO_PARAM_VRING_INTERRUPT:
default:
vring_interrupt(0, vq);
@@ -390,6 +435,8 @@ static int __init kvm_devices_init(void)
kvm_devices = (void *) real_memory_size;
+ INIT_WORK(&hotplug_work, hotplug_devices);
+
ctl_set_bit(0, 9);
register_external_interrupt(0x2603, kvm_extint_handler);
--
1.6.0.2
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
