"Michael S. Tsirkin" <[email protected]> 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;
}
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
