Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754435AbeAJLGt (ORCPT + 1 other); Wed, 10 Jan 2018 06:06:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35650 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753542AbeAJLGs (ORCPT ); Wed, 10 Jan 2018 06:06:48 -0500 Date: Wed, 10 Jan 2018 13:06:44 +0200 From: "Michael S. Tsirkin" To: Suzuki K Poulose Cc: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org, marc.zyngier@arm.com, linux-kernel@vger.kernel.org, kristina.martsenko@arm.com, peter.maydell@linaro.org, pbonzini@redhat.com, rkrcmar@redhat.com, will.deacon@arm.com, ard.biesheuvel@linaro.org, mark.rutland@arm.com, catalin.marinas@arm.com, Jason Wang , Christoffer Dall Subject: Re: [PATCH v1 01/16] virtio: Validate queue pfn for 32bit transports Message-ID: <20180110130230-mutt-send-email-mst@kernel.org> References: <20180109190414.4017-1-suzuki.poulose@arm.com> <20180109190414.4017-2-suzuki.poulose@arm.com> <20180110012429-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 10 Jan 2018 11:06:48 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, Jan 10, 2018 at 10:54:09AM +0000, Suzuki K Poulose wrote: > On 09/01/18 23:29, Michael S. Tsirkin wrote: > > On Tue, Jan 09, 2018 at 07:03:56PM +0000, Suzuki K Poulose wrote: > > > virtio-mmio using virtio-v1 and virtio legacy pci use a 32bit PFN > > > for the queue. If the queue pfn is too large to fit in 32bits, which > > > we could hit on arm64 systems with 52bit physical addresses (even with > > > 64K page size), we simply miss out a proper link to the other side of > > > the queue. > > > > > > Add a check to validate the PFN, rather than silently breaking > > > the devices. > > > > > > Cc: "Michael S. Tsirkin" > > > Cc: Jason Wang > > > Cc: Marc Zyngier > > > Cc: Christoffer Dall > > > Signed-off-by: Suzuki K Poulose > > > > Could you guys please work on virtio 1 support in > > for virtio mmio in qemu though? > > It's not a lot of code. > > Did you mean kvmtool ? Qemu already supports virto-1. For virtio-mmio? I don't seem to see that code in hw/virtio/virtio-mmio.c For example I still see handling for VIRTIO_MMIO_QUEUE_PFN there, and no handling for VIRTIO_MMIO_QUEUE_DESC_LOW and such. What am I missing? > > > > > --- > > > drivers/virtio/virtio_mmio.c | 19 ++++++++++++++++--- > > > drivers/virtio/virtio_pci_legacy.c | 11 +++++++++-- > > > 2 files changed, 25 insertions(+), 5 deletions(-) > > > > I'd rather see this as 2 patches. > > OK, I will split them. > > > > > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > > index a9192fe4f345..47109baf37f7 100644 > > > --- a/drivers/virtio/virtio_mmio.c > > > +++ b/drivers/virtio/virtio_mmio.c > > > @@ -358,6 +358,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index, > > > struct virtqueue *vq; > > > unsigned long flags; > > > unsigned int num; > > > + u64 addr; > > > int err; > > > if (!name) > > > @@ -394,16 +395,26 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index, > > > goto error_new_virtqueue; > > > } > > > + addr = virtqueue_get_desc_addr(vq); > > > + /* > > > + * virtio-mmio v1 uses a 32bit QUEUE PFN. If we have something that > > > + * doesn't fit in 32bit, fail the setup rather than pretending to > > > + * be successful. > > > + */ > > > + if (vm_dev->version == 1 && (addr >> (PAGE_SHIFT + 32))) { > > > + dev_err(&vdev->dev, "virtio-mmio: queue address too large\n"); > > > + err = -ENOMEM; > > > + goto error_bad_pfn; > > > + } > > > + > > > > Can you please move this below to where it's actually used? > > > > The reason for keeping it here was to skip selecting the Queue number if we > have a bad PFN. Why is selecting a problem if we don't program anything? > May be it doesn't make much difference as we write PFN = 0 anyway > down. > > > > /* Activate the queue */ > > > writel(virtqueue_get_vring_size(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NUM); > > > if (vm_dev->version == 1) { > > > writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN); > > > - writel(virtqueue_get_desc_addr(vq) >> PAGE_SHIFT, > > > + writel(addr >> PAGE_SHIFT, > > > vm_dev->base + VIRTIO_MMIO_QUEUE_PFN); > > > } else { > > > - u64 addr; > > > - addr = virtqueue_get_desc_addr(vq); > > > writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW); > > > writel((u32)(addr >> 32), > > > vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH); > > > @@ -430,6 +441,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index, > > > return vq; > > > +error_bad_pfn: > > > + vring_del_virtqueue(vq); > > > error_new_virtqueue: > > > if (vm_dev->version == 1) { > > > writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN); > > > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > > > index 2780886e8ba3..099d2cfb47b3 100644 > > > --- a/drivers/virtio/virtio_pci_legacy.c > > > +++ b/drivers/virtio/virtio_pci_legacy.c > > > @@ -122,6 +122,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > struct virtqueue *vq; > > > u16 num; > > > int err; > > > + u64 q_pfn; > > > /* Select the queue we're interested in */ > > > iowrite16(index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL); > > > @@ -141,9 +142,15 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > if (!vq) > > > return ERR_PTR(-ENOMEM); > > > + q_pfn = virtqueue_get_desc_addr(vq) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT; > > > + if (q_pfn >> 32) { > > > + dev_err(&vp_dev->pci_dev->dev, "virtio-pci queue PFN too large\n"); > > > + err = -ENOMEM; > > > + goto out_deactivate; > > > > You never set up the address, it's cleaner to add another target > > and not reset it. > > Thats right. However, the only thing we do is writing PFN=0, which would be a good > thing to do to indicate the error to the host ? It isn't, a good way to indicate error is to set a bad status which happens anyway I think. Writing PFN 0 resets the device instead. > I could skip it if you think it is > not needed. > > > Thanks > Suzuki