Re: [PATCH 16/22] virtio_pci: use separate notification offsets for each vq.

[Rusty Russell <rusty@rustcorp.com.au>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> By the way, Gleb pointed out that on older hosts MMIO will
> always be slower since we need to do a shadow page walk to
> translate virtual to physical address.
> Hopefully not a big concern, and after all we are still
> keeping PIO around for use by BIOS ...

Yeah, slow hosts will be slow :)

>> +	/* get offset of notification byte for this virtqueue */
>> +	off = ioread16(&vp_dev->common->queue_notify_off);
>> +	if (off * vp_dev->notify_offset_multiplier > vp_dev->notify_len) {
>
> maybe check there's no overflow too?
> if (UINT32_MAX / vp_dev->notify_offset_multiplier > off)
> 	return -EINVAL;

I think it's clearer to just do:

        if ((u64)off * vp_dev->notify_offset_multiplier > vp_dev->notify_len) {

Since off is 16 bits and notify_offset_multiplier is 32, this catches
overflow.

>> +		dev_warn(&vp_dev->pci_dev->dev,
>> +			 "bad notification offset %u for queue %u (> %u)",
>> +			 off, index, vp_dev->notify_len);
>> +		err = -EINVAL;
>> +		goto out_info;
>> +	}
>
> I don't know if you want to limit this to "0 or power of two",
> if yes you'd do
> 	if (vp_dev->notify_offset_multiplier & (vp_dev->notify_offset_multiplier - 1))
> 		return -EINVAL;
>
>> +	info->notify = vp_dev->notify_base + off * vp_dev->notify_offset_multiplier;
>>  
>>  	info->queue = alloc_virtqueue_pages(&num);
>>  	if (info->queue == NULL) {

I don't see a reason to restrict it.  It's not like you'll be
calculating this value more than once even in a micro implementation...

Also, let's add 2, since we write a word in there...

Cheers,
Rusty.

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d0318d..1c3591a 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -502,12 +502,14 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	info->msix_vector = msix_vec;
 
-	/* get offset of notification byte for this virtqueue */
+	/* get offset of notification word for this vq (shouldn't wrap) */
 	off = ioread16(&vp_dev->common->queue_notify_off);
-	if (off * vp_dev->notify_offset_multiplier + 2 > vp_dev->notify_len) {
+	if ((u64)off * vp_dev->notify_offset_multiplier + 2
+	    > vp_dev->notify_len) {
 		dev_warn(&vp_dev->pci_dev->dev,
-			 "bad notification offset %u for queue %u (> %u)",
-			 off, index, vp_dev->notify_len);
+			 "bad notification offset %u (x %u) for queue %u > %u",
+			 off, vp_dev->notify_offset_multiplier, 
+			 index, vp_dev->notify_len);
 		err = -EINVAL;
 		goto out_info;
 	}