Re: [PATCH v2 0/5] PCI: Implement basic PCI PM capability backing

["Cédric Le Goater" <clg@redhat.com>]

Hello Michael,

Could you please re-ack (or not) v2 ?

Thanks

C.

On 2/25/25 22:52, Alex Williamson wrote:
> v2:
> 
> Eric noted in v1 that one of the drivers had a redundant wmask setting
> since pci_pm_init() enabled writes to the power state field.  This was
> added because vfio-pci was not setting wmask for this capability but
> is allowing writes to the PM state field through to the device.  For
> vfio-pci, QEMU emulated config space is rather secondary to the config
> space through vfio.
> 
> It turns out therefore, that vfio-pci is nearly unique in not already
> managing the wmask of the PM capability state and if we embrace that
> it's the pci_pm_init() caller's responsibility to manage the remaining
> contents and write-access of the capability, then I think we also
> solve the question of migration compatibility.  The new infrastructure
> here is not changing whether any fields were previously writable, it's
> only effecting a mapping change based on the value found there.
> 
> This requires only a slight change to patch 1/, removing setting of
> the wmask, but commit log is also updated and comments added.  I also
> made the bad transition trace a little more obvious given Eric's
> comments.  Patch 2/ is also updated so that vfio-pci effects the wmask
> change locally.  The couple drivers that don't currently update wmask
> simply don't get this new BAR unmapped when not in D0 behavior.
> 
> Incorporated reviews for the unmodified patches.  Please re-review and
> report any noted issues.  Thanks,
> 
> Alex
> 
> v1:
> 
> https://lore.kernel.org/all/20250220224918.2520417-1-alex.williamson@redhat.com/
> 
> Eric recently identified an issue[1] where during graceful shutdown
> of a VM in a vIOMMU configuration, the guest driver places the device
> into the D3 power state, the vIOMMU is then disabled, triggering an
> AddressSpace update.  The device BARs are still mapped into the AS,
> but the vfio host driver refuses to DMA map the MMIO space due to the
> device power state.
> 
> The proposed solution in [1] was to skip mappings based on the
> device power state.  Here we take a different approach.  The PCI spec
> defines that devices in D1/2/3 power state should respond only to
> configuration and message requests and all other requests should be
> handled as an Unsupported Request.  In other words, the memory and
> IO BARs are not accessible except when the device is in the D0 power
> state.
> 
> To emulate this behavior, we can factor the device power state into
> the mapping state of the device BARs.  Therefore the BAR is marked
> as unmapped if either the respective command register enable bit is
> clear or the device is not in the D0 power state.
> 
> In order to implement this, the PowerState field of the PMCSR
> register becomes writable, which allows the device to appear in
> lower power states.  This also therefore implements D3 support
> (insofar as the BAR behavior) for all devices implementing the PM
> capability.  The PCI spec requires D3 support.
> 
> An aspect that needs attention here is whether this change in the
> wmask and PMCSR bits becomes a problem for migration, and how we
> might solve it.  For a guest migrating old->new, the device would
> always be in the D0 power state, but the register becomes writable.
> In the opposite direction, is it possible that a device could
> migrate in a low power state and be stuck there since the bits are
> read-only in old QEMU?  Do we need an option for this behavior and a
> machine state bump, or are there alternatives?
> 
> Thanks,
> Alex
> 
> [1]https://lore.kernel.org/all/20250219175941.135390-1-eric.auger@redhat.com/
> 
> 
> Alex Williamson (5):
>    hw/pci: Basic support for PCI power management
>    pci: Use PCI PM capability initializer
>    vfio/pci: Delete local pm_cap
>    pcie, virtio: Remove redundant pm_cap
>    hw/vfio/pci: Re-order pre-reset
> 
>   hw/net/e1000e.c                 |  3 +-
>   hw/net/eepro100.c               |  4 +-
>   hw/net/igb.c                    |  3 +-
>   hw/nvme/ctrl.c                  |  3 +-
>   hw/pci-bridge/pcie_pci_bridge.c |  3 +-
>   hw/pci/pci.c                    | 93 ++++++++++++++++++++++++++++++++-
>   hw/pci/trace-events             |  2 +
>   hw/vfio/pci.c                   | 34 ++++++------
>   hw/vfio/pci.h                   |  1 -
>   hw/virtio/virtio-pci.c          | 11 ++--
>   include/hw/pci/pci.h            |  3 ++
>   include/hw/pci/pci_device.h     |  3 ++
>   include/hw/pci/pcie.h           |  2 -
>   13 files changed, 127 insertions(+), 38 deletions(-)
>