Re: [PATCH] virtio_mmio: Set DMA masks appropriately

[Russell King - ARM Linux <linux@armlinux.org.uk>]

On Tue, Jan 10, 2017 at 02:15:57PM +0100, Arnd Bergmann wrote:
> On Tuesday, January 10, 2017 12:26:01 PM CET Robin Murphy wrote:
> > @@ -548,6 +550,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >         if (vm_dev->version == 1)
> >                 writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> >  
> > +       rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> > +       if (rc)
> > +               rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> 
> You don't seem to do anything different when 64-bit DMA is unsupported.
> How do you prevent the use of kernel buffers that are above the first 4G
> here?
> 
> > +       else if (vm_dev->version == 1)
> > +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32 + PAGE_SHIFT));
> 
> Why is this limitation only for the coherent mask?

It looks wrong for two reasons:

1. It is calling dma_set_coherent_mask(), so only the coherent mask
   is being updated.  What about streaming DMA?

   Maybe include the comment from the commit you refer to (a0be1db4304f)
   which explains this, which would help reviewers understand why you're
   only changing the coherent mask.

2. It fails to check whether the coherent mask was accepted... which
   I guess is okay, as the coherent allocation mask won't be updated
   so you should get coherent memory below 4GB.  Nevertheless, drivers
   are expected to try setting a 32-bit coherent mask if setting a
   larger mask fails.  See examples in Documentation/DMA-API-HOWTO.txt.

Of course, if setting a 32-bit coherent mask fails, then the driver
should probably fail to initialise.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.