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

[Will Deacon <will.deacon@arm.com>]

Hi Michael,

On Wed, Sep 14, 2016 at 03:42:25PM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 14, 2016 at 12:16:28PM +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>
> > Cc: Benjamin Serebrin <serebrin@google.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  drivers/virtio/virtio_pci_legacy.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> > index 8c4e61783441..efb3f5dff4b7 100644
> > --- a/drivers/virtio/virtio_pci_legacy.c
> > +++ b/drivers/virtio/virtio_pci_legacy.c
> > @@ -212,12 +212,17 @@ 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));
> > +	/*
> > +	 * The virtio ring base address is expressed as a 32-bit PFN, with a
> > +	 * page size of 1 << VIRTIO_PCI_QUEUE_ADDR_SHIFT.
> > +	 */
> > +	rc = dma_set_mask_and_coherent(&pci_dev->dev,
> > +				DMA_BIT_MASK(32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT));
> 
> So why not limit just the coherent mask, as I suggested in a comment on v1?

Sorry, I completely missed that suggestion. I'll roll a v3 with that
included, although I'll rejig things a bit to make the error handling
a bit more bearable.

> >  	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 %d-bit or 32-bit DMA.  Trying to continue, but this might not work.\n", 32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT);
> 
> 
> I'd rather we split this.

I'll actually just leave like it was in the first place if we're trying a
64-bit streaming mask.

Stay tuned,

Will