* [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk @ 2011-05-13 2:40 Asias He 2011-05-13 2:40 ` [PATCH 2/2] kvm tools: Tune the command-line option Asias He 2011-05-13 12:46 ` [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Pekka Enberg 0 siblings, 2 replies; 7+ messages in thread From: Asias He @ 2011-05-13 2:40 UTC (permalink / raw) To: Pekka Enberg Cc: Cyrill Gorcunov, Ingo Molnar, Sasha Levin, Prasad Joshi, kvm, Asias He commit b764422bb0b46b00b896f6d4538ac3d3dde9e56b (kvm tools: Add support for multiple virtio-blk) removed the VIRTIO_BLK_F_SEG_MAX publishment to guest. There is no reason we should not support it. Just bring it back. Signed-off-by: Asias He <asias.hejun@gmail.com> --- tools/kvm/virtio/blk.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-) diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c index 5085f1b..8740bc4 100644 --- a/tools/kvm/virtio/blk.c +++ b/tools/kvm/virtio/blk.c @@ -21,6 +21,10 @@ #define NUM_VIRT_QUEUES 1 #define VIRTIO_BLK_QUEUE_SIZE 128 +/* + * the header and status consume too entries + */ +#define DISK_SEG_MAX (VIRTIO_BLK_QUEUE_SIZE - 2) struct blk_dev_job { struct virt_queue *vq; @@ -278,11 +282,12 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk) blk_dev_base_addr = IOPORT_VIRTIO_BLK + new_dev_idx * IOPORT_VIRTIO_BLK_SIZE; *bdev = (struct blk_dev) { - .mutex = PTHREAD_MUTEX_INITIALIZER, - .disk = disk, - .idx = new_dev_idx, - .blk_config = (struct virtio_blk_config) { + .mutex = PTHREAD_MUTEX_INITIALIZER, + .disk = disk, + .idx = new_dev_idx, + .blk_config = (struct virtio_blk_config) { .capacity = disk->size / SECTOR_SIZE, + .seg_max = DISK_SEG_MAX, }, .pci_hdr = (struct pci_device_header) { .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET, @@ -294,6 +299,12 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk) .subsys_id = PCI_SUBSYSTEM_ID_VIRTIO_BLK, .bar[0] = blk_dev_base_addr | PCI_BASE_ADDRESS_SPACE_IO, }, + /* + * Note we don't set VIRTIO_BLK_F_GEOMETRY here so the + * guest kernel will compute disk geometry by own, the + * same applies to VIRTIO_BLK_F_BLK_SIZE + */ + .host_features = (1UL << VIRTIO_BLK_F_SEG_MAX), }; if (irq__register_device(PCI_DEVICE_ID_VIRTIO_BLK, &dev, &pin, &line) < 0) -- 1.7.5.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] kvm tools: Tune the command-line option 2011-05-13 2:40 [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Asias He @ 2011-05-13 2:40 ` Asias He 2011-05-13 12:46 ` [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Pekka Enberg 1 sibling, 0 replies; 7+ messages in thread From: Asias He @ 2011-05-13 2:40 UTC (permalink / raw) To: Pekka Enberg Cc: Cyrill Gorcunov, Ingo Molnar, Sasha Levin, Prasad Joshi, kvm, Asias He With this patch we can have -c --cpus -m --mem -d --disk -k --kernel -i --initrd which is more consistent and easy to remember. The patch also frees up -s, -g option. Ingo suggestied ''' The debug options should probably be concentrated under a --debug option anyway, to allow things like: --debug single-step,ioport Even if the debug options are kept they should be streamlined along the same pattern: >> --debug-single-step Enable single stepping >> --debug-ioport Enable ioport debugging But having a --debug option that recognizes all the debug flags would be nicer. It would also allow future enhancements to group debug features, like: --debug all # turn on everything and the kitchen sink for early hangs --debug all,-single-step # turn on everything except single-step debugging --debug nonverbose # turn on all non-noisy debug options we have Maybe even: --debug memcheck ... could run kvm under valgrind automatically - that way we can hide any secondary tool complexities from the user and turn those tools into simple debug options :-) ''' Let's do this --debug option consolidation later. Signed-off-by: Asias He <asias.hejun@gmail.com> --- tools/kvm/kvm-run.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c index 91a194e..ba8e5ce 100644 --- a/tools/kvm/kvm-run.c +++ b/tools/kvm/kvm-run.c @@ -101,16 +101,17 @@ static const struct option options[] = { OPT_GROUP("Basic options:"), OPT_INTEGER('c', "cpus", &nrcpus, "Number of CPUs"), OPT_U64('m', "mem", &ram_size, "Virtual machine memory size in MiB."), - OPT_CALLBACK('i', "image", NULL, "image", "Disk image", img_name_parser), + OPT_CALLBACK('d', "disk", NULL, "image", "Disk image", img_name_parser), OPT_STRING('\0', "console", &console, "serial or virtio", "Console to use"), - OPT_BOOLEAN('\0', "virtio-rng", &virtio_rng, + OPT_BOOLEAN('\0', "rng", &virtio_rng, "Enable virtio Random Number Generator"), + OPT_STRING('\0', "kvm-dev", &kvm_dev, "kvm-dev", "KVM device file"), OPT_GROUP("Kernel options:"), OPT_STRING('k', "kernel", &kernel_filename, "kernel", "Kernel to boot in virtual machine"), - OPT_STRING('r', "initrd", &initrd_filename, "initrd", + OPT_STRING('i', "initrd", &initrd_filename, "initrd", "Initial RAM disk image"), OPT_STRING('p', "params", &kernel_cmdline, "params", "Kernel command line arguments"), @@ -124,14 +125,14 @@ static const struct option options[] = { "Assign this address to the guest side NIC"), OPT_STRING('\0', "tapscript", &script, "Script path", "Assign a script to process created tap device"), + OPT_GROUP("Debug options:"), - OPT_STRING('d', "kvm-dev", &kvm_dev, "kvm-dev", "KVM device file"), - OPT_BOOLEAN('s', "single-step", &single_step, - "Enable single stepping"), - OPT_BOOLEAN('g', "ioport-debug", &ioport_debug, - "Enable ioport debugging"), OPT_BOOLEAN('\0', "debug", &do_debug_print, "Enable debug messages"), + OPT_BOOLEAN('\0', "debug-single-step", &single_step, + "Enable single stepping"), + OPT_BOOLEAN('\0', "debug-ioport-debug", &ioport_debug, + "Enable ioport debugging"), OPT_END() }; -- 1.7.5.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk 2011-05-13 2:40 [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Asias He 2011-05-13 2:40 ` [PATCH 2/2] kvm tools: Tune the command-line option Asias He @ 2011-05-13 12:46 ` Pekka Enberg 2011-05-13 14:07 ` Sasha Levin 1 sibling, 1 reply; 7+ messages in thread From: Pekka Enberg @ 2011-05-13 12:46 UTC (permalink / raw) To: Asias He; +Cc: Cyrill Gorcunov, Ingo Molnar, Sasha Levin, Prasad Joshi, kvm On 5/13/11 5:40 AM, Asias He wrote: > commit b764422bb0b46b00b896f6d4538ac3d3dde9e56b > (kvm tools: Add support for multiple virtio-blk) > removed the VIRTIO_BLK_F_SEG_MAX publishment to guest. > > There is no reason we should not support it. Just bring it back. > > Signed-off-by: Asias He<asias.hejun@gmail.com> Sasha? > --- > tools/kvm/virtio/blk.c | 19 +++++++++++++++---- > 1 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c > index 5085f1b..8740bc4 100644 > --- a/tools/kvm/virtio/blk.c > +++ b/tools/kvm/virtio/blk.c > @@ -21,6 +21,10 @@ > #define NUM_VIRT_QUEUES 1 > > #define VIRTIO_BLK_QUEUE_SIZE 128 > +/* > + * the header and status consume too entries > + */ > +#define DISK_SEG_MAX (VIRTIO_BLK_QUEUE_SIZE - 2) > > struct blk_dev_job { > struct virt_queue *vq; > @@ -278,11 +282,12 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk) > blk_dev_base_addr = IOPORT_VIRTIO_BLK + new_dev_idx * IOPORT_VIRTIO_BLK_SIZE; > > *bdev = (struct blk_dev) { > - .mutex = PTHREAD_MUTEX_INITIALIZER, > - .disk = disk, > - .idx = new_dev_idx, > - .blk_config = (struct virtio_blk_config) { > + .mutex = PTHREAD_MUTEX_INITIALIZER, > + .disk = disk, > + .idx = new_dev_idx, > + .blk_config = (struct virtio_blk_config) { > .capacity = disk->size / SECTOR_SIZE, > + .seg_max = DISK_SEG_MAX, > }, > .pci_hdr = (struct pci_device_header) { > .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET, > @@ -294,6 +299,12 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk) > .subsys_id = PCI_SUBSYSTEM_ID_VIRTIO_BLK, > .bar[0] = blk_dev_base_addr | PCI_BASE_ADDRESS_SPACE_IO, > }, > + /* > + * Note we don't set VIRTIO_BLK_F_GEOMETRY here so the > + * guest kernel will compute disk geometry by own, the > + * same applies to VIRTIO_BLK_F_BLK_SIZE > + */ > + .host_features = (1UL<< VIRTIO_BLK_F_SEG_MAX), > }; > > if (irq__register_device(PCI_DEVICE_ID_VIRTIO_BLK,&dev,&pin,&line)< 0) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk 2011-05-13 12:46 ` [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Pekka Enberg @ 2011-05-13 14:07 ` Sasha Levin 2011-05-13 14:34 ` Asias He 0 siblings, 1 reply; 7+ messages in thread From: Sasha Levin @ 2011-05-13 14:07 UTC (permalink / raw) To: Pekka Enberg; +Cc: Asias He, Cyrill Gorcunov, Ingo Molnar, Prasad Joshi, kvm On Fri, 2011-05-13 at 15:46 +0300, Pekka Enberg wrote: > On 5/13/11 5:40 AM, Asias He wrote: > > commit b764422bb0b46b00b896f6d4538ac3d3dde9e56b > > (kvm tools: Add support for multiple virtio-blk) > > removed the VIRTIO_BLK_F_SEG_MAX publishment to guest. > > > > There is no reason we should not support it. Just bring it back. > > > > Signed-off-by: Asias He<asias.hejun@gmail.com> > > Sasha? I'm not sure why it was removed, must have missed it when updating struct blk_dev. On the other hand, why do we need to limit max segment size? -- Sasha. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk 2011-05-13 14:07 ` Sasha Levin @ 2011-05-13 14:34 ` Asias He 2011-05-13 15:26 ` Sasha Levin 0 siblings, 1 reply; 7+ messages in thread From: Asias He @ 2011-05-13 14:34 UTC (permalink / raw) To: Sasha Levin; +Cc: Pekka Enberg, Cyrill Gorcunov, Ingo Molnar, Prasad Joshi, kvm On 05/13/2011 10:07 PM, Sasha Levin wrote: > On Fri, 2011-05-13 at 15:46 +0300, Pekka Enberg wrote: >> On 5/13/11 5:40 AM, Asias He wrote: >>> commit b764422bb0b46b00b896f6d4538ac3d3dde9e56b >>> (kvm tools: Add support for multiple virtio-blk) >>> removed the VIRTIO_BLK_F_SEG_MAX publishment to guest. >>> >>> There is no reason we should not support it. Just bring it back. >>> >>> Signed-off-by: Asias He<asias.hejun@gmail.com> >> >> Sasha? > > I'm not sure why it was removed, must have missed it when updating > struct blk_dev. > > On the other hand, why do we need to limit max segment size? That's because the number of entries in a scatter-gather operation must be less than the entries in the virt queue we can offer. -- Best Regards, Asias He ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk 2011-05-13 14:34 ` Asias He @ 2011-05-13 15:26 ` Sasha Levin 2011-05-13 16:16 ` Asias He 0 siblings, 1 reply; 7+ messages in thread From: Sasha Levin @ 2011-05-13 15:26 UTC (permalink / raw) To: Asias He; +Cc: Pekka Enberg, Cyrill Gorcunov, Ingo Molnar, Prasad Joshi, kvm On Fri, 2011-05-13 at 22:34 +0800, Asias He wrote: > On 05/13/2011 10:07 PM, Sasha Levin wrote: > > On Fri, 2011-05-13 at 15:46 +0300, Pekka Enberg wrote: > >> On 5/13/11 5:40 AM, Asias He wrote: > >>> commit b764422bb0b46b00b896f6d4538ac3d3dde9e56b > >>> (kvm tools: Add support for multiple virtio-blk) > >>> removed the VIRTIO_BLK_F_SEG_MAX publishment to guest. > >>> > >>> There is no reason we should not support it. Just bring it back. > >>> > >>> Signed-off-by: Asias He<asias.hejun@gmail.com> > >> > >> Sasha? > > > > I'm not sure why it was removed, must have missed it when updating > > struct blk_dev. > > > > On the other hand, why do we need to limit max segment size? > > That's because the number of entries in a scatter-gather operation must > be less than the entries in the virt queue we can offer. > I thought that since it's the guest who allocates vq buffers and it's the guest who does the sg ops, it won't attempt to do that. But if thats the concern, then yes - lets add it to be safe. -- Sasha. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk 2011-05-13 15:26 ` Sasha Levin @ 2011-05-13 16:16 ` Asias He 0 siblings, 0 replies; 7+ messages in thread From: Asias He @ 2011-05-13 16:16 UTC (permalink / raw) To: Sasha Levin; +Cc: Pekka Enberg, Cyrill Gorcunov, Ingo Molnar, Prasad Joshi, kvm On 05/13/2011 11:26 PM, Sasha Levin wrote: > On Fri, 2011-05-13 at 22:34 +0800, Asias He wrote: >> On 05/13/2011 10:07 PM, Sasha Levin wrote: >>> On Fri, 2011-05-13 at 15:46 +0300, Pekka Enberg wrote: >>>> On 5/13/11 5:40 AM, Asias He wrote: >>>>> commit b764422bb0b46b00b896f6d4538ac3d3dde9e56b >>>>> (kvm tools: Add support for multiple virtio-blk) >>>>> removed the VIRTIO_BLK_F_SEG_MAX publishment to guest. >>>>> >>>>> There is no reason we should not support it. Just bring it back. >>>>> >>>>> Signed-off-by: Asias He<asias.hejun@gmail.com> >>>> >>>> Sasha? >>> >>> I'm not sure why it was removed, must have missed it when updating >>> struct blk_dev. >>> >>> On the other hand, why do we need to limit max segment size? >> >> That's because the number of entries in a scatter-gather operation must >> be less than the entries in the virt queue we can offer. >> > > I thought that since it's the guest who allocates vq buffers and it's > the guest who does the sg ops, it won't attempt to do that. The size of the virt queue (number of entries) is determined by the device (host). > > But if thats the concern, then yes - lets add it to be safe. > -- Best Regards, Asias He ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-05-13 16:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-05-13 2:40 [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Asias He 2011-05-13 2:40 ` [PATCH 2/2] kvm tools: Tune the command-line option Asias He 2011-05-13 12:46 ` [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk Pekka Enberg 2011-05-13 14:07 ` Sasha Levin 2011-05-13 14:34 ` Asias He 2011-05-13 15:26 ` Sasha Levin 2011-05-13 16:16 ` Asias He
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox