linux-kernel - Re: [PATCH v3 8/8] vfio/pci: Add the support for PCI D3cold state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 May 2022 16:43:04 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Abhishek Sahu <abhsahu@...dia.com>
Cc:     Alex Williamson <alex.williamson@...hat.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 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.

>          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..

Jason