[PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices

[PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices

[Will Deacon]

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

Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices

[Michael S. Tsirkin]

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

Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices

[Will Deacon]

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

Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices

[Benjamin Serebrin]

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

Re: [PATCH] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices

[Benjamin Serebrin via 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