[PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[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

[PATCH 2/2] kvm tools: Tune the command-line option

[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

Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[Pekka Enberg]

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)

Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[Sasha Levin]

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.

Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[Asias He]

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

Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[Sasha Levin]

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.

Re: [PATCH 1/2] kvm tools: Bring VIRTIO_BLK_F_SEG_MAX feature back to virtio blk

[Asias He]

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