Re: [RFC 0/2] hw/vfio/pci: Prevent BARs from being dma mapped in d3hot state

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

On Thu, 20 Feb 2025 08:07:23 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu, 20 Feb 2025 11:45:35 +0100
> Eric Auger <eauger@redhat.com> wrote:
> 
> > Hi Alex,
> > 
> > On 2/20/25 11:31 AM, Eric Auger wrote:  
> > > 
> > > Hi Alex,
> > > 
> > > On 2/19/25 10:19 PM, Alex Williamson wrote:    
> > >> On Wed, 19 Feb 2025 11:58:44 -0700
> > >> Alex Williamson <alex.williamson@redhat.com> wrote:
> > >>    
> > >>> On Wed, 19 Feb 2025 18:58:58 +0100
> > >>> Eric Auger <eric.auger@redhat.com> wrote:
> > >>>    
> > >>>> Since kernel commit:
> > >>>> 2b2c651baf1c ("vfio/pci: Invalidate mmaps and block the access
> > >>>> in D3hot power state")
> > >>>> any attempt to do an mmap access to a BAR when the device is in d3hot
> > >>>> state will generate a fault.
> > >>>>
> > >>>> On system_powerdown, if the VFIO device is translated by an IOMMU,
> > >>>> the device is moved to D3hot state and then the vIOMMU gets disabled
> > >>>> by the guest. As a result of this later operation, the address space is
> > >>>> swapped from translated to untranslated. When re-enabling the aliased
> > >>>> regions, the RAM regions are dma-mapped again and this causes DMA_MAP
> > >>>> faults when attempting the operation on BARs.
> > >>>>
> > >>>> To avoid doing the remap on those BARs, we compute whether the
> > >>>> device is in D3hot state and if so, skip the DMA MAP.      
> > >>> Thinking on this some more, QEMU PCI code already manages the device
> > >>> BARs appearing in the address space based on the memory enable bit in
> > >>> the command register.  Should we do the same for PM state?
> > >>>
> > >>> IOW, the device going into low power state should remove the BARs from
> > >>> the AddressSpace and waking the device should re-add them.  The BAR DMA
> > >>> mapping should then always be consistent, whereas here nothing would
> > >>> remap the BARs when the device is woken.
> > >>>
> > >>> I imagine we'd need an interface to register the PM capability with the
> > >>> core QEMU PCI code, where address space updates are performed relative
> > >>> to both memory enable and power status.  There might be a way to
> > >>> implement this just for vfio-pci devices by toggling the enable state
> > >>> of the BAR mmaps relative to PM state, but doing it at the PCI core
> > >>> level seems like it'd provide behavior more true to physical hardware.    
> > >> I took a stab at this approach here, it doesn't obviously break
> > >> anything in my configs, but I haven't yet tried to reproduce this exact
> > >> scenario.
> > >>
> > >> https://gitlab.com/alex.williamson/qemu/-/tree/pci-pm-power-state    
> > 
> > it does not totally fix the issue: I now get:
> > 
> > qemu-system-x86_64: warning: vfio_container_dma_map(0x55cc25705680,
> > 0x380000000000, 0x1000000, 0x7f8762000000) = -14 (Bad address)
> > 0000:41:00.0: PCI peer-to-peer transactions on BARs are not supported.  
> 
> Hmm, I'll reproduce and debug further.  The intention here is that BARs
> for the device in D3hot would not be DMA mapped, effectively as if the
> memory enable bit in the command register were cleared, therefore I'd
> hoped the listener is not called for this range.

I forgot to mark the PM state field as writable in config space, so we
were always reading back D0 state.  Adding the following to
pci_pm_init() resolves it:

--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -445,6 +445,7 @@ int pci_pm_init(PCIDevice *d, uint8_t offset, Error **errp)
 
     d->pm_cap = cap;
     d->cap_present |= QEMU_PCI_CAP_PM;
+    pci_set_word(d->wmask + cap + PCI_PM_CTRL, PCI_PM_CTRL_STATE_MASK);
 
     return cap;
 }

Changing this might cause a problem with migration, ISTR we validate
the wmask with the source.  Anyway, I'll post the series and we can
test further and discuss it there.  Thanks,

Alex