Re: [PATCH] uio_pci_generic does not export memory resources

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2012-06-09 at 11:28 +0200, Andreas Hartmann wrote:
> Alex Williamson wrote:
> > On Fri, 2012-06-08 at 18:16 +0200, Andreas Hartmann wrote:
> [...]
> >> Besides the problem with AMD IOMMU, which requires to unbind a whole
> >> group of devices in some cases (PCI passthrough - not PCIe), it's really
> >> cool! And it's usable now!
> > 
> > If you're feeling adventurous (and know that this may not make it
> > upstream), you can do something like this:
> 
> Hmm, even without this patch, it's not necessary to bind all devices to
> pci-stub. Just to remember:
> 
> -[0000:00]-+-14.0  Advanced Micro Devices [AMD] nee ATI SBx00 SMBus Controller                                                                                                     
>            +-14.1  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 IDE Controller                                                                                           
>            +-14.2  Advanced Micro Devices [AMD] nee ATI SBx00 Azalia (Intel HDA)                                                                                                   
>            +-14.3  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 LPC host controller                                                                                      
>            +-14.4-[06]--                                                                                                                                                           
>            \-14.5  Advanced Micro Devices [AMD] nee ATI SB7x0/SB8x0/SB9x0 USB OHCI2 Controller
> 
> Device 06:07.0 should be passed through.
> 
> The devices 14.0, 14.3 and 14.4 need not to be bound to pci-stub - the
> VM starts (and seems to work fine) anyway. Maybe because there isn't
> any driver anyway for these devices?

Right, the actual rule is that any device you want to access needs to be
bound to vfio-pci.  The remaining devices within the group can either be
bound to vfio-pci or pci-stub or no driver.  The recommendation for
avoiding that no-driver case is because if later a driver is loaded into
the kernel that would attempt to bind to any of those no-driver devices,
you end up back in the situation of the security of the group being
violated and hit the BUGON you saw previously.

> The USB device seems to be an internal device only - all of my external
> USB jacks are working fine.
> 
> The only thing I'm missing is the sound device.
> 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index ab0dba0..5c26804 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3161,11 +3161,22 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
> >         return pci_dev_get(dev);
> >  }
> >  
> > +static int pci_func_supports_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +       return 1;
> > +}
> > +
> >  static const struct pci_dev_acs_enabled {
> >         u16 vendor;
> >         u16 device;
> >         int (*acs_enabled)(struct pci_dev *dev, u16 acs_flags);
> >  } pci_dev_acs_enabled[] = {
> > +       { 0x1002, 0x4385, pci_func_supports_acs },
> > +       { 0x1002, 0x439c, pci_func_supports_acs },
> > +       { 0x1002, 0x4383, pci_func_supports_acs },
> > +       { 0x1002, 0x439d, pci_func_supports_acs },
> > +       { 0x1002, 0x4384, pci_func_supports_acs },
> > +       { 0x1002, 0x4399, pci_func_supports_acs },
> >         { 0 }
> >  };
> 
> What's the risk of this patch? Machine crash? Data loss for an active
> file in an application? Complete filesystem damage? The latter would be
> worse.

What we're trying to prevent by testing whether devices support ACS is
peer-to-peer transactions that would not be translated via the IOMMU.
For instance, imagine if your WLAN controller behind 14.4 does a DMA
write to an address that happens to be within the MMIO resources of
device 14.2, the audio device.  Instead of the transaction going up to
the IOMMU and resulting in a memory write, internal routing on device
14.x results in that transaction being redirected to the 14.2.  So
you're looking at potential data loss from the guest as well as
corrupting device state in the host.

> > Hmm, I wonder if we should make a kernel boot parameter that allows
> > whitelisting some devices.  I think it would have to taint the kernel
> > but there's probably sufficient interest for usability vs
> > supportability.
> 
> Good idea. I would print an additional big fat warning of dataloss /
> filesystem damage / crash if this could be the case.

Well, outlining the risk above makes me a little more nervous about
making such a config option, even if it taints the kernel, available so
easily... :^\  Thanks,

Alex