* [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices
@ 2016-09-12 12:10 Will Deacon
2016-09-12 15:33 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Will Deacon @ 2016-09-12 12:10 UTC (permalink / raw)
To: virtualization, kvm; +Cc: Will Deacon, Andy Lutomirski, Michael S. Tsirkin
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(-)
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)
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices
2016-09-12 12:10 [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices Will Deacon
@ 2016-09-12 15:33 ` Michael S. Tsirkin
2016-09-12 15:57 ` Will Deacon
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2016-09-12 15:33 UTC (permalink / raw)
To: Will Deacon; +Cc: Andy Lutomirski, kvm, virtualization
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices
2016-09-12 15:33 ` Michael S. Tsirkin
@ 2016-09-12 15:57 ` Will Deacon
2016-09-13 6:11 ` Benjamin Serebrin
0 siblings, 1 reply; 4+ messages in thread
From: Will Deacon @ 2016-09-12 15:57 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Andy Lutomirski, kvm, virtualization
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices
2016-09-12 15:57 ` Will Deacon
@ 2016-09-13 6:11 ` Benjamin Serebrin
0 siblings, 0 replies; 4+ messages in thread
From: Benjamin Serebrin @ 2016-09-13 6:11 UTC (permalink / raw)
To: Will Deacon; +Cc: Michael S. Tsirkin, Andy Lutomirski, kvm, virtualization
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-13 6:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-12 12:10 [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices Will Deacon
2016-09-12 15:33 ` Michael S. Tsirkin
2016-09-12 15:57 ` Will Deacon
2016-09-13 6:11 ` Benjamin Serebrin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox