Message-ID: <20220607155031.078fc817.alex.williamson@redhat.com>
Date:   Tue, 7 Jun 2022 15:50:31 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Abhishek Sahu <abhsahu@...dia.com>
Cc:     Jason Gunthorpe <jgg@...dia.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Yishai Hadas <yishaih@...dia.com>,
        Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>,
        Kevin Tian <kevin.tian@...el.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Max Gurtovoy <mgurtovoy@...dia.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 8/8] vfio/pci: Add the support for PCI D3cold state

On Fri, 3 Jun 2022 15:49:27 +0530
Abhishek Sahu <abhsahu@...dia.com> wrote:

> On 6/2/2022 11:14 PM, Alex Williamson wrote:
> > On Thu, 2 Jun 2022 17:22:03 +0530
> > Abhishek Sahu <abhsahu@...dia.com> wrote:
> >   
> >> On 6/1/2022 9:51 PM, Alex Williamson wrote:  
> >>> On Wed, 1 Jun 2022 15:19:07 +0530
> >>> Abhishek Sahu <abhsahu@...dia.com> wrote:
> >>>     
> >>>> On 6/1/2022 4:22 AM, Alex Williamson wrote:    
> >>>>> On Tue, 31 May 2022 16:43:04 -0300
> >>>>> Jason Gunthorpe <jgg@...dia.com> wrote:
> >>>>>       
> >>>>>> On Tue, May 31, 2022 at 05:44:11PM +0530, Abhishek Sahu wrote:      
> >>>>>>> On 5/30/2022 5:55 PM, Jason Gunthorpe wrote:        
> >>>>>>>> On Mon, May 30, 2022 at 04:45:59PM +0530, Abhishek Sahu wrote:
> >>>>>>>>         
> >>>>>>>>>  1. In real use case, config or any other ioctl should not come along
> >>>>>>>>>     with VFIO_DEVICE_FEATURE_POWER_MANAGEMENT ioctl request.
> >>>>>>>>>  
> >>>>>>>>>  2. Maintain some 'access_count' which will be incremented when we
> >>>>>>>>>     do any config space access or ioctl.        
> >>>>>>>>
> >>>>>>>> Please don't open code locks - if you need a lock then write a proper
> >>>>>>>> lock. You can use the 'try' variants to bail out in cases where that
> >>>>>>>> is appropriate.
> >>>>>>>>
> >>>>>>>> Jason        
> >>>>>>>
> >>>>>>>  Thanks Jason for providing your inputs.
> >>>>>>>
> >>>>>>>  In that case, should I introduce new rw_semaphore (For example
> >>>>>>>  power_lock) and move ‘platform_pm_engaged’ under ‘power_lock’ ?        
> >>>>>>
> >>>>>> Possibly, this is better than an atomic at least
> >>>>>>      
> >>>>>>>  1. At the beginning of config space access or ioctl, we can take the
> >>>>>>>     lock
> >>>>>>>  
> >>>>>>>      down_read(&vdev->power_lock);        
> >>>>>>
> >>>>>> You can also do down_read_trylock() here and bail out as you were
> >>>>>> suggesting with the atomic.
> >>>>>>
> >>>>>> trylock doesn't have lock odering rules because it can't sleep so it
> >>>>>> gives a bit more flexability when designing the lock ordering.
> >>>>>>
> >>>>>> Though userspace has to be able to tolerate the failure, or never make
> >>>>>> the request.
> >>>>>>      
> >>>>
> >>>>  Thanks Alex and Jason for providing your inputs.
> >>>>
> >>>>  Using down_read_trylock() along with Alex suggestion seems fine.
> >>>>  In real use case, config space access should not happen when the
> >>>>  device is in low power state so returning error should not
> >>>>  cause any issue in this case.
> >>>>    
> >>>>>>>          down_write(&vdev->power_lock);
> >>>>>>>          ...
> >>>>>>>          switch (vfio_pm.low_power_state) {
> >>>>>>>          case VFIO_DEVICE_LOW_POWER_STATE_ENTER:
> >>>>>>>                  ...
> >>>>>>>                          vfio_pci_zap_and_down_write_memory_lock(vdev);
> >>>>>>>                          vdev->power_state_d3 = true;
> >>>>>>>                          up_write(&vdev->memory_lock);
> >>>>>>>
> >>>>>>>          ...
> >>>>>>>          up_write(&vdev->power_lock);        
> >>>>>>
> >>>>>> And something checks the power lock before allowing the memor to be
> >>>>>> re-enabled?
> >>>>>>      
> >>>>>>>  4.  For ioctl access, as mentioned previously I need to add two
> >>>>>>>      callbacks functions (one for start and one for end) in the struct
> >>>>>>>      vfio_device_ops and call the same at start and end of ioctl from
> >>>>>>>      vfio_device_fops_unl_ioctl().        
> >>>>>>
> >>>>>> Not sure I followed this..      
> >>>>>
> >>>>> I'm kinda lost here too.      
> >>>>
> >>>>
> >>>>  I have summarized the things below
> >>>>
> >>>>  1. In the current patch (v3 8/8), if config space access or ioctl was
> >>>>     being made by the user when the device is already in low power state,
> >>>>     then it was waking the device. This wake up was happening with
> >>>>     pm_runtime_resume_and_get() API in vfio_pci_config_rw() and
> >>>>     vfio_device_fops_unl_ioctl() (with patch v3 7/8 in this patch series).
> >>>>
> >>>>  2. Now, it has been decided to return error instead of waking the
> >>>>     device if the device is already in low power state.
> >>>>
> >>>>  3. Initially I thought to add following code in config space path
> >>>>     (and similar in ioctl)
> >>>>
> >>>>         vfio_pci_config_rw() {
> >>>>             ...
> >>>>             down_read(&vdev->memory_lock);
> >>>>             if (vdev->platform_pm_engaged)
> >>>>             {
> >>>>                 up_read(&vdev->memory_lock);
> >>>>                 return -EIO;
> >>>>             }
> >>>>             ...
> >>>>         }
> >>>>
> >>>>      And then there was a possibility that the physical config happens
> >>>>      when the device in D3cold in case of race condition.
> >>>>
> >>>>  4.  So, I wanted to add some mechanism so that the low power entry
> >>>>      ioctl will be serialized with other ioctl or config space. With this
> >>>>      if low power entry gets scheduled first then config/other ioctls will
> >>>>      get failure, otherwise low power entry will wait.
> >>>>
> >>>>  5.  For serializing this access, I need to ensure that lock is held
> >>>>      throughout the operation. For config space I can add the code in
> >>>>      vfio_pci_config_rw(). But for ioctls, I was not sure what is the best
> >>>>      way since few ioctls (VFIO_DEVICE_FEATURE_MIGRATION,
> >>>>      VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE etc.) are being handled in the
> >>>>      vfio core layer itself.
> >>>>
> >>>>  The memory_lock and the variables to track low power in specific to
> >>>>  vfio-pci so I need some mechanism by which I add low power check for
> >>>>  each ioctl. For serialization, I need to call function implemented in
> >>>>  vfio-pci before vfio core layer makes the actual ioctl to grab the
> >>>>  locks. Similarly, I need to release the lock once vfio core layer
> >>>>  finished the actual ioctl. I have mentioned about this problem in the
> >>>>  above point (point 4 in my earlier mail).
> >>>>    
> >>>>> A couple replies back there was some concern
> >>>>> about race scenarios with multiple user threads accessing the device.
> >>>>> The ones concerning non-deterministic behavior if a user is
> >>>>> concurrently changing power state and performing other accesses are a
> >>>>> non-issue, imo.        
> >>>>
> >>>>  What does non-deterministic behavior here mean.
> >>>>  Is it for user side that user will see different result
> >>>>  (failure or success) during race condition or in the kernel side
> >>>>  (as explained in point 3 above where physical config access
> >>>>  happens when the device in D3cold) ? My concern here is for later
> >>>>  part where this config space access in D3cold can cause fatal error
> >>>>  on the system side as we have seen for memory disablement.    
> >>>
> >>> Yes, our only concern should be to prevent such an access.  The user
> >>> seeing non-deterministic behavior, such as during concurrent power
> >>> control and config space access, all combinations of success/failure
> >>> are possible, is par for the course when we decide to block accesses
> >>> across the life of the low power state.
> >>>      
> >>>>> I think our goal is only to expand the current
> >>>>> memory_lock to block accesses, including config space, while the device
> >>>>> is in low power, or some approximation bounded by the entry/exit ioctl.
> >>>>>
> >>>>> I think the remaining issues is how to do that relative to the fact
> >>>>> that config space access can change the memory enable state and would
> >>>>> therefore need to upgrade the memory_lock read-lock to a write-lock.
> >>>>> For that I think we can simply drop the read-lock, acquire the
> >>>>> write-lock, and re-test the low power state.  If it has changed, that
> >>>>> suggests the user has again raced changing power state with another
> >>>>> access and we can simply drop the lock and return -EIO.
> >>>>>       
> >>>>
> >>>>  Yes. This looks better option. So, just to confirm, I can take the
> >>>>  memory_lock read-lock at the starting of vfio_pci_config_rw() and
> >>>>  release it just before returning from vfio_pci_config_rw() and
> >>>>  for memory related config access, we will release this lock and
> >>>>  re-aquiring again write version of this. Once memory write happens,
> >>>>  then we can downgrade this write lock to read lock ?    
> >>>
> >>> We only need to lock for the device access, so if you've finished that
> >>> access after acquiring the write-lock, there'd be no point to then
> >>> downgrade that to a read-lock.  The access should be finished by that
> >>> point.
> >>>    
> >>
> >>  I was planning to take memory_lock read-lock at the beginning of
> >>  vfio_pci_config_rw() and release the same just before returning from
> >>  this function. If I don't downgrade it back to read-lock, then the
> >>  release in the end will be called for the lock which has not taken.
> >>  Also, user can specify count to any number of bytes and then the
> >>  vfio_config_do_rw() will be invoked multiple times and then in
> >>  the second call, it will be without lock.  
> > 
> > Ok, yes, I can imagine how it might result in a cleaner exit path to do
> > a downgrade_write().
> >   
> >>>>  Also, what about IOCTLs. How can I take and release memory_lock for
> >>>>  ioctl. is it okay to go with Patch 7 where we call
> >>>>  pm_runtime_resume_and_get() before each ioctl or we need to do the
> >>>>  same low power check for ioctl also ?
> >>>>  In Later case, I am not sure how should I do the implementation so
> >>>>  that all other ioctl are covered from vfio core layer itself.    
> >>>
> >>> Some ioctls clearly cannot occur while the device is in low power, such
> >>> as resets and interrupt control, but even less obvious things like
> >>> getting region info require device access.  Migration also provides a
> >>> channel to device access.  Do we want to manage a list of ioctls that
> >>> are allowed in low power, or do we only want to allow the ioctl to exit
> >>> low power?
> >>>     
> >>
> >>  In previous version of this patch, you mentioned that maintaining the
> >>  safe ioctl list will be tough to maintain. So, currently we wanted to
> >>  allow the ioctl for low power exit.  
> > 
> > Yes, I'm still conflicted in how that would work.
> >    
> >>> I'm also still curious how we're going to handle devices that cannot
> >>> return to low power such as the self-refresh mode on the GPU.  We can
> >>> potentially prevent any wake-ups from the vfio device interface, but
> >>> that doesn't preclude a wake-up via an external lspci.  I think we need
> >>> to understand how we're going to handle such devices before we can
> >>> really complete the design.  AIUI, we cannot disable the self-refresh
> >>> sleep mode without imposing unreasonable latency and memory
> >>> requirements on the guest and we cannot retrigger the self-refresh
> >>> low-power mode without non-trivial device specific code.  Thanks,
> >>>
> >>> Alex
> >>>     
> >>
> >>  I am working on adding support to notify guest through virtual PME
> >>  whenever there is any wake-up triggered by the host and the guest has
> >>  already put the device into runtime suspended state. This virtual PME
> >>  will be similar to physical PME. Normally, if PCI device need power
> >>  management transition, then it sends PME event which will be
> >>  ultimately handled by host OS. In virtual PME case, if host need power
> >>  management transition, then it sends event to guest and then guest OS
> >>  handles these virtual PME events. Following is summary:
> >>
> >>  1. Add the support for one more event like VFIO_PCI_ERR_IRQ_INDEX
> >>     named VFIO_PCI_PME_IRQ_INDEX and add the required code for this
> >>     virtual PME event.
> >>
> >>  2. From the guest side, when the PME_IRQ is enabled then we will
> >>     set event_fd for PME.
> >>
> >>  3. In the vfio driver, the PME support bits are already
> >>     virtualized and currently set to 0. We can set PME capability support
> >>     for D3cold so that in guest, it looks like
> >>
> >>      Capabilities: [60] Power Management version 3
> >>      Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
> >>             PME(D0-,D1-,D2-,D3hot-,D3cold+)
> >>
> >>  4. From the guest side, it can do PME enable (PME_En bit in Power
> >>     Management Control/Status Register) which will be again virtualized.
> >>
> >>  5. When host gets request for resuming the device other than from
> >>     low power ioctl, then device pm usage count will be incremented, the
> >>     PME status (PME_Status bit in Power Management Control/Status Register)
> >>     will be set and then we can do the event_fd signal.
> >>
> >>  6. In the PCIe, the PME events will be handled by root port. For
> >>     using low power D3cold feature, it is required to create virtual root
> >>     port in hypervisor side and when hypervisor receives this PME event,
> >>     then it can send virtual interrupt to root port.
> >>
> >>  7. If we take example of Linux kernel, then pcie_pme_irq() will
> >>     handle this and then do the runtime resume on the guest side. Also, it
> >>     will clear the PME status bit here. Then guest can put the device
> >>     again into suspended state.
> >>
> >>  8. I did prototype changes in QEMU for above logic and was getting wake-up
> >>     in the guest whenever I do lspci on the host side.
> >>
> >>  9. Since currently only nvidia GPU has this limitation to require
> >>     driver interaction each time before going into D3cold so we can allow
> >>     the reentry for other device. We can have nvidia vendor (along with
> >>     VGA/3D controller class code). In future, if any other device also has
> >>     similar requirement then we can update this list. For other device
> >>     host can put the device into D3cold in case of any wake-up.
> >>
> >>  10. In the vfio driver, we can put all these restriction for
> >>      enabling PME and return error if user tries to make low power entry
> >>      ioctl without enabling the PME related things.
> >>
> >>  11. The virtual PME can help in handling physical PME also for all
> >>      the devices. The PME logic is not dependent upon nvidia GPU
> >>      restriction. If virtual PME is enabled by hypervisor, then when
> >>      physical PME wakes the device, then it will resume on the guest side
> >>      also.  
> > 
> > So if host accesses through things like lspci are going to wake the
> > device and we can't prevent that, and the solution to that is to notify
> > the guest to put the device back to low power, then it seems a lot less
> > important to try to prevent the user from waking the device through
> > random accesses.  In that context, maybe we do simply wrap all accesses
> > with pm_runtime_get/put() put calls, which eliminates the problem of
> > maintaining a list of safe ioctls in low power.
> >   
> 
>  So wrap all access with pm_runtime_get()/put() will only be applicable
>  for IOCTLs. Correct ?
>  For config space, we can go with the approach discussed earlier in which
>  we return error ?

If we need to handle arbitrarily induced wakes from the host, it
doesn't make much sense to restrict those same sort of accesses by the
user through the vfio-device.  It also seems a lot easier to simply do
a pm_get/put() around not only ioctls, but all region accesses to avoid
the sorts of races you previously identified.  Access through mmap
should still arguably fault given that there is no discrete end to such
an access like we have for read/write operations.  Thanks,

Alex

Powered by blists - more mailing lists