| Message ID | 1473682241-21984-1-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 12, 2016 at 01:10:41PM +0100, Will Deacon wrote: > Legacy virtio defines the virtqueue base using a 32-bit PFN field, with > a read-only register indicating a fixed page size of 4k. > > This can cause problems for DMA allocators that allocate top down from > the DMA mask, which is set to 64 bits. In this case, the addresses are > silently truncated to 44-bit, leading to IOMMU faults, failure to read > from the queue or data corruption. > > This patch restricts the DMA mask for legacy PCI virtio devices to > 44 bits, which matches the specification. > > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Will Deacon <will.deacon@arm.com> Hmm - IIUC it's actually only the case for the virtio rings themselves. The buffer addresses put in the rings are full 64 bit ones. It so happens that virtio doesn't use coherent allocs except for the rings. So I'm inclined to say the coherent mask should be set to 44, with a comment explaning that this is for the rings. In case we start using coherent allocations in virtio, it might be cleaner to relax the mask after allocating the rings, but I'm not sure that's allowed by the DMA API. thoughts? > --- > drivers/virtio/virtio_pci_legacy.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > index 8c4e61783441..f4852febd40c 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -212,12 +212,12 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev) > return -ENODEV; > } > > - rc = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(64)); > + rc = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(44)); 44 would be cleaner as 32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT. > if (rc) > rc = dma_set_mask_and_coherent(&pci_dev->dev, > DMA_BIT_MASK(32)); > if (rc) > - dev_warn(&pci_dev->dev, "Failed to enable 64-bit or 32-bit DMA. Trying to continue, but this might not work.\n"); > + dev_warn(&pci_dev->dev, "Failed to enable 44-bit or 32-bit DMA. Trying to continue, but this might not work.\n"); > > rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy"); > if (rc) > -- > 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 12, 2016 at 06:33:43PM +0300, Michael S. Tsirkin wrote: > On Mon, Sep 12, 2016 at 01:10:41PM +0100, Will Deacon wrote: > > Legacy virtio defines the virtqueue base using a 32-bit PFN field, with > > a read-only register indicating a fixed page size of 4k. > > > > This can cause problems for DMA allocators that allocate top down from > > the DMA mask, which is set to 64 bits. In this case, the addresses are > > silently truncated to 44-bit, leading to IOMMU faults, failure to read > > from the queue or data corruption. > > > > This patch restricts the DMA mask for legacy PCI virtio devices to > > 44 bits, which matches the specification. > > > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > Hmm - IIUC it's actually only the case for the virtio rings > themselves. The buffer addresses put in the rings are full 64 bit ones. I think that's right, yes. > It so happens that virtio doesn't use coherent allocs except > for the rings. > So I'm inclined to say the coherent mask should be set to 44, > with a comment explaning that this is for the rings. I can certainly add that in v2, along with your suggestion to use 32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT instead of the 44. > In case we start using coherent allocations in virtio, > it might be cleaner to relax the mask after allocating > the rings, but I'm not sure that's allowed by the DMA API. > thoughts? Hmm, that *might* work, but I could certainly imagine some DMA implementations going wrong if they assume the mask is fixed. Will -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 12, 2016 at 8:57 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Mon, Sep 12, 2016 at 06:33:43PM +0300, Michael S. Tsirkin wrote: > > On Mon, Sep 12, 2016 at 01:10:41PM +0100, Will Deacon wrote: > > > Legacy virtio defines the virtqueue base using a 32-bit PFN field, with > > > a read-only register indicating a fixed page size of 4k. > > > > > > This can cause problems for DMA allocators that allocate top down from > > > the DMA mask, which is set to 64 bits. In this case, the addresses are > > > silently truncated to 44-bit, leading to IOMMU faults, failure to read > > > from the queue or data corruption. > > > > > > This patch restricts the DMA mask for legacy PCI virtio devices to > > > 44 bits, which matches the specification. > > > > > > Cc: Andy Lutomirski <luto@kernel.org> > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > > > Hmm - IIUC it's actually only the case for the virtio rings > > themselves. The buffer addresses put in the rings are full 64 bit ones. > > I think that's right, yes. Yes. > > > > It so happens that virtio doesn't use coherent allocs except > > for the rings. > > So I'm inclined to say the coherent mask should be set to 44, > > with a comment explaning that this is for the rings. > > I can certainly add that in v2, along with your suggestion to use > 32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT instead of the 44. > > > In case we start using coherent allocations in virtio, > > it might be cleaner to relax the mask after allocating > > the rings, but I'm not sure that's allowed by the DMA API. > > thoughts? > > Hmm, that *might* work, but I could certainly imagine some DMA > implementations going wrong if they assume the mask is fixed. There's probably a hack to make the buffer addresses be 64-bit capable while letting the queue addresses be 44-bit, but is it worth the trouble? Maybe assume that guests with more than 16 TB of RAM are rare today, and catch the Virtio 1.x spec to fix the 32-bit PFN limit? Alternately, you could keep the DMA mask at 64 bits but try to allocate the rings early, and abort if they can't get a < 16 TB address. I think I prefer the 44 bit mask approach, though. > > > Will > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c index 8c4e61783441..f4852febd40c 100644 --- a/drivers/virtio/virtio_pci_legacy.c +++ b/drivers/virtio/virtio_pci_legacy.c @@ -212,12 +212,12 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev) return -ENODEV; } - rc = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(64)); + rc = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(44)); if (rc) rc = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(32)); if (rc) - dev_warn(&pci_dev->dev, "Failed to enable 64-bit or 32-bit DMA. Trying to continue, but this might not work.\n"); + dev_warn(&pci_dev->dev, "Failed to enable 44-bit or 32-bit DMA. Trying to continue, but this might not work.\n"); rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy"); if (rc)
Legacy virtio defines the virtqueue base using a 32-bit PFN field, with a read-only register indicating a fixed page size of 4k. This can cause problems for DMA allocators that allocate top down from the DMA mask, which is set to 64 bits. In this case, the addresses are silently truncated to 44-bit, leading to IOMMU faults, failure to read from the queue or data corruption. This patch restricts the DMA mask for legacy PCI virtio devices to 44 bits, which matches the specification. Cc: Andy Lutomirski <luto@kernel.org> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- drivers/virtio/virtio_pci_legacy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)