Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fb52e3da-4808-ade5-7872-0432e5983c9b@nvidia.com>
Date:   Mon, 21 Feb 2022 13:42:03 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [RFC PATCH v2 4/5] vfio/pci: Invalidate mmaps and block the
 access in D3hot power state
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     kvm@vger.kernel.org, Cornelia Huck <cohuck@redhat.com>,
        Max Gurtovoy <mgurtovoy@nvidia.com>,
        Yishai Hadas <yishaih@nvidia.com>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        Jason Gunthorpe <jgg@nvidia.com>, linux-kernel@vger.kernel.org
References: <20220124181726.19174-1-abhsahu@nvidia.com>
 <20220124181726.19174-5-abhsahu@nvidia.com>
 <20220217161420.7232eab6.alex.williamson@redhat.com>
From:   Abhishek Sahu <abhsahu@nvidia.com>
In-Reply-To: <20220217161420.7232eab6.alex.williamson@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NEh4TTVwcEtiTkVwWFhiM29pbTB2cmtxT1JJTjIzQk9HYk5PZXdnMWgxbkZM?=
 =?utf-8?B?dXRCMHdQYjFva0tCR0tSQ21Jekt2bWQwSnM0V3Q5TnlwRDcyOUlsaWVsbGQz?=
 =?utf-8?B?ZmpjUm9IN2U0cWZkQWs1Q0RwQUM0ekNjeDZtek04bVl5TnQ0NVlXRUVsTVFj?=
 =?utf-8?B?RU94R0dkb3VwbjBoaVkxVEZwRmUwaEVqN1VpS3EvUEluV1NpeXRqWUtYbGNO?=
 =?utf-8?B?QlBMNjI0eFJvK3lYNzlZMWEwQndvbHF4aVpHbDVndVRldVBDenduVE5ieXZm?=
 =?utf-8?B?Yi8zZldvY3lpS1RFekwxZ3BXeDBHdFo2dmNDQTVyRmpPc2dtcys1Nmp3eS90?=
 =?utf-8?B?em42WFJwQ1dXNitNa3RKcmtySDdCeHVabnZMTTJBbkl5dDZjcG5FNjJiSGdm?=
 =?utf-8?B?dklrZ09uZWFhNXZKeDQ0VlIyd09ydGphbGw2WlNBRGRmTk4xKzlzWVNPN0E0?=
 =?utf-8?B?SGlpbmcyam1jY3V0U0t2Y01iQ0ZtdTF3QXIrN2pnRDlMK2FDd3lsSkFMeG52?=
 =?utf-8?B?VDJIODNrM2FZZTc5bFkyNE9XdjJHUndFc24wYUJFdVhPZGpmMDlqQ0piOFZ4?=
 =?utf-8?B?aTNydDUvL0kvK2QvNkxmenhGR3I3MWtRNTRsS1BKR3MwU29aU2FraG5nemZZ?=
 =?utf-8?B?TlkvbjJVRGNCd3g3UVk4Q2lpSE1sTk1Gd3hCR3h2RndveGwreEZTcjBmVEE4?=
 =?utf-8?B?bDBGSzMvWDZSWkdPemcvVlFreDRma3dEYXdEeExRelZ2b2ZGUU1ES3o2UlFl?=
 =?utf-8?B?VlpucktvMGR6Zk05TnZyQVlCS0FwMU9jemVrb1RsKzNwdTl4cUUxbVBwaUxZ?=
 =?utf-8?B?M2V3UC9XR2EycFVsNFVsYjhaSkRaMFNmWi9QVGpmUUtFb2NDSnRnaXNiR2Z1?=
 =?utf-8?B?ZWpGbWw0MktFL01oTmZVRGIxWFVCUlVXcVcxQUlOajlNcDNBN2lmYjJnZ3du?=
 =?utf-8?B?eTJ4T1BXQmhPMXZKeGgrS2gzcVUvOWh1S3BXaGFaRnFvVVpvM01EdGFSTlZY?=
 =?utf-8?B?VXVKdVpvQkF5Nk83bEVCR1pJOCszUURlZjZCMThsdDFBbVBJdFVoYmViV3BP?=
 =?utf-8?B?Vzc1bm51cTJubjEwSlp1M0Y1K3BHSElkaDIrd0Y4MXhZdGhXQVM3YS9rT0tR?=
 =?utf-8?B?dTZNL1BZZk9lWjUxWHFXL3E0OHBSNlNyMURGY0tkdVIzUG9PREJXK250bHdz?=
 =?utf-8?B?eSt1VGhWaWt5Mm50WVl2akh4ZEIydUZyYW9RTjd0RjZ4R2FWeGNJTHpnUFFC?=
 =?utf-8?B?Mnp3Q01pOVZYQWlORWJFaHNiQW9iamt5MDEyTmgxN0dqdlhlRTNnS3d3R2Uv?=
 =?utf-8?B?VWQvY0VsYVZraUZqTmw2bjNucFZ4RHhFWmJBVEN3V2pGaXI5OUgySmUzbkNs?=
 =?utf-8?B?ZmZOUEVtQTg3di9tUGtzcW9xWkhFenJaSHJIbkNmcW1oZ0Z5NTYyZ1lab3dU?=
 =?utf-8?B?b2VSOWJ0QytDWk0rQm9UVlFEVG1DQ3Ntc2t2bjJyaGtMMnZheGlQUUk0Z1VY?=
 =?utf-8?B?QjM2ci80R3RMaEtvVE1ET2FqSUNiN0Y4azVSczJCMWFWVU5MUytpS0tjbkRj?=
 =?utf-8?B?MTVBSnZmM085RllaM042ZlBzekduL0dFdGYvdmUvSkxOMFVyVjlGZlZFb0V3?=
 =?utf-8?B?eDVTM1d0WXd5SW9McGdISHAxTUVsVDBoWDVuOEdkQWlnWDF4K0dTQm1IQTNZ?=
 =?utf-8?B?Z2E3ZFVDL1NpeHdYSnFScFpSOEEzd2JDWlpRbGdXeVJ2ZG5JWkVaSnVNRysw?=
 =?utf-8?B?TXdudFZsOXVSamxmM0MxOENOV05vazNRVDV0aGF4QTlOL3EyQ010d3B0dWRr?=
 =?utf-8?B?VGtuSWJBVWR0dC9od1BlWWhXeTNUa0hhaTV2aTRrWnZScjJDazFkenhVa2V2?=
 =?utf-8?B?ZEY5SXNYbm1GbG9VUzZreTcrMUZDSlg0dXBNdmdSQUNWZUttNnJHV3ptQk00?=
 =?utf-8?B?RllaM3FhQzlDYWRkNVk2QXJkbE1YQTRmT2xTbExTWEVnaWMxelp6b29ERkxC?=
 =?utf-8?B?dzU4SG95Z1F2bUVSWTdDeWZnd3hLaW1kQ21yNStsM2QrWlAxZlVwNWdZQ0FD?=
 =?utf-8?B?azVDQlZiN2tUMFBRcjFBWUtwMTdxRW5uNWUrbFNoOWcyb1RDWFlabmhwWm1q?=
 =?utf-8?B?ZURqZSs2RVR2b2Fnc3g4dU1lWFJVem01UGV5VUJqbTBSd0ZtVWZoeFVMS25n?=
 =?utf-8?Q?baiFZAngWQcKnnZvHIbL0CQ=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 06cc63ea-0c5c-4202-3ae3-08d9f511de94
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5304.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2022 08:12:15.0902
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /v66My/dlGpcSombE8H5FlnvtgEf5yUjxP/TJgPYsR19lxWaZ73PrJF8eU4i2Grk4UhZ5GDCzBFgr8C9dh13wA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR12MB2804
X-Spam-Status: No, score=-0.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/18/2022 4:44 AM, Alex Williamson wrote:
> On Mon, 24 Jan 2022 23:47:25 +0530
> Abhishek Sahu <abhsahu@nvidia.com> wrote:
> 
>> According to [PCIe v5 5.3.1.4.1] for D3hot state
>>
>>  "Configuration and Message requests are the only TLPs accepted by a
>>   Function in the D3Hot state. All other received Requests must be
>>   handled as Unsupported Requests, and all received Completions may
>>   optionally be handled as Unexpected Completions."
>>
>> Currently, if the vfio PCI device has been put into D3hot state and if
>> user makes non-config related read/write request in D3hot state, these
>> requests will be forwarded to the host and this access may cause
>> issues on a few systems.
>>
>> This patch leverages the memory-disable support added in commit
>> 'abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on
>> disabled memory")' to generate page fault on mmap access and
>> return error for the direct read/write. If the device is D3hot state,
>> then the error needs to be returned for all kinds of BAR
>> related access (memory, IO and ROM). Also, the power related structure
>> fields need to be protected so we can use the same 'memory_lock' to
>> protect these fields also. For the few cases, this 'memory_lock' will be
>> already acquired by callers so introduce a separate function
>> vfio_pci_set_power_state_locked(). The original
>> vfio_pci_set_power_state() now contains the code to do the locking
>> related operations.
>>
>> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
>> ---
>>  drivers/vfio/pci/vfio_pci_core.c | 47 +++++++++++++++++++++++++-------
>>  drivers/vfio/pci/vfio_pci_rdwr.c | 20 ++++++++++----
>>  2 files changed, 51 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index ee2fb8af57fa..38440d48973f 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -201,11 +201,12 @@ static void vfio_pci_probe_power_state(struct vfio_pci_core_device *vdev)
>>  }
>>
>>  /*
>> - * pci_set_power_state() wrapper handling devices which perform a soft reset on
>> - * D3->D0 transition.  Save state prior to D0/1/2->D3, stash it on the vdev,
>> - * restore when returned to D0.  Saved separately from pci_saved_state for use
>> - * by PM capability emulation and separately from pci_dev internal saved state
>> - * to avoid it being overwritten and consumed around other resets.
>> + * vfio_pci_set_power_state_locked() wrapper handling devices which perform a
>> + * soft reset on D3->D0 transition.  Save state prior to D0/1/2->D3, stash it
>> + * on the vdev, restore when returned to D0.  Saved separately from
>> + * pci_saved_state for use by PM capability emulation and separately from
>> + * pci_dev internal saved state to avoid it being overwritten and consumed
>> + * around other resets.
>>   *
>>   * There are few cases where the PCI power state can be changed to D0
>>   * without the involvement of this API. So, cache the power state locally
>> @@ -215,7 +216,8 @@ static void vfio_pci_probe_power_state(struct vfio_pci_core_device *vdev)
>>   * The memory taken for saving this PCI state needs to be freed to
>>   * prevent memory leak.
>>   */
>> -int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t state)
>> +static int vfio_pci_set_power_state_locked(struct vfio_pci_core_device *vdev,
>> +                                        pci_power_t state)
>>  {
>>       struct pci_dev *pdev = vdev->pdev;
>>       bool needs_restore = false, needs_save = false;
>> @@ -260,6 +262,26 @@ int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t stat
>>       return ret;
>>  }
>>
>> +/*
>> + * vfio_pci_set_power_state() takes all the required locks to protect
>> + * the access of power related variables and then invokes
>> + * vfio_pci_set_power_state_locked().
>> + */
>> +int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev,
>> +                          pci_power_t state)
>> +{
>> +     int ret;
>> +
>> +     if (state >= PCI_D3hot)
>> +             vfio_pci_zap_and_down_write_memory_lock(vdev);
>> +     else
>> +             down_write(&vdev->memory_lock);
>> +
>> +     ret = vfio_pci_set_power_state_locked(vdev, state);
>> +     up_write(&vdev->memory_lock);
>> +     return ret;
>> +}
>> +
>>  int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>>  {
>>       struct pci_dev *pdev = vdev->pdev;
>> @@ -354,7 +376,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>>        * in running the logic needed for D0 power state. The subsequent
>>        * runtime PM API's will put the device into the low power state again.
>>        */
>> -     vfio_pci_set_power_state(vdev, PCI_D0);
>> +     vfio_pci_set_power_state_locked(vdev, PCI_D0);
>>
>>       /* Stop the device from further DMA */
>>       pci_clear_master(pdev);
>> @@ -967,7 +989,7 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
>>                * interaction. Update the power state in vfio driver to perform
>>                * the logic needed for D0 power state.
>>                */
>> -             vfio_pci_set_power_state(vdev, PCI_D0);
>> +             vfio_pci_set_power_state_locked(vdev, PCI_D0);
>>               up_write(&vdev->memory_lock);
>>
>>               return ret;
>> @@ -1453,6 +1475,11 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
>>               goto up_out;
>>       }
>>
>> +     if (vdev->power_state >= PCI_D3hot) {
>> +             ret = VM_FAULT_SIGBUS;
>> +             goto up_out;
>> +     }
>> +
>>       /*
>>        * We populate the whole vma on fault, so we need to test whether
>>        * the vma has already been mapped, such as for concurrent faults
>> @@ -1902,7 +1929,7 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>>        * be able to get to D3.  Therefore first do a D0 transition
>>        * before enabling runtime PM.
>>        */
>> -     vfio_pci_set_power_state(vdev, PCI_D0);
>> +     vfio_pci_set_power_state_locked(vdev, PCI_D0);
>>       pm_runtime_allow(&pdev->dev);
>>
>>       if (!disable_idle_d3)
>> @@ -2117,7 +2144,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>>                * interaction. Update the power state in vfio driver to perform
>>                * the logic needed for D0 power state.
>>                */
>> -             vfio_pci_set_power_state(cur, PCI_D0);
>> +             vfio_pci_set_power_state_locked(cur, PCI_D0);
>>               if (cur == cur_mem)
>>                       is_mem = false;
>>               if (cur == cur_vma)
>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>> index 57d3b2cbbd8e..e97ba14c4aa0 100644
>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>> @@ -41,8 +41,13 @@
>>  static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev,         \
>>                       bool test_mem, u##size val, void __iomem *io)   \
>>  {                                                                    \
>> +     down_read(&vdev->memory_lock);                                  \
>> +     if (vdev->power_state >= PCI_D3hot) {                           \
>> +             up_read(&vdev->memory_lock);                            \
>> +             return -EIO;                                            \
>> +     }                                                               \
>> +                                                                     \
> 
> The reason that we only set test_mem for MMIO BARs is that systems are
> generally more lenient about probing unresponsive I/O port space to
> support legacy use cases.  Have you found cases where access to an I/O
> port BAR when the device is either in D3hot+ or I/O port is disabled in
> the command register triggers a system fault?  If not it seems we could
> roll the power_state check into __vfio_pci_memory_enabled(), if so then
> we probably need to improve our coverage of access to disabled I/O port
> BARs beyond only the power_state check.  Thanks,
> 
> Alex
> 

 I have not seen any system unresponsive in the systems which I am using 
 for testing these patches. If I try to access MMIO BAR or IO port while
 the device is in D3hot+, then I am getting all 0xff. Since I was not
 sure regarding the behaviour in other systems while the device is in
 D3hot+, so I did power_state check outside.

 We can start with power_state check under __vfio_pci_memory_enabled()
 and improve coverage later-on if any issue arises.

 Thanks,
 Abhishek

>>       if (test_mem) {                                                 \
>> -             down_read(&vdev->memory_lock);                          \
>>               if (!__vfio_pci_memory_enabled(vdev)) {                 \
>>                       up_read(&vdev->memory_lock);                    \
>>                       return -EIO;                                    \
>> @@ -51,8 +56,7 @@ static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev,                \
>>                                                                       \
>>       vfio_iowrite##size(val, io);                                    \
>>                                                                       \
>> -     if (test_mem)                                                   \
>> -             up_read(&vdev->memory_lock);                            \
>> +     up_read(&vdev->memory_lock);                                    \
>>                                                                       \
>>       return 0;                                                       \
>>  }
>> @@ -68,8 +72,13 @@ VFIO_IOWRITE(64)
>>  static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev,          \
>>                       bool test_mem, u##size *val, void __iomem *io)  \
>>  {                                                                    \
>> +     down_read(&vdev->memory_lock);                                  \
>> +     if (vdev->power_state >= PCI_D3hot) {                           \
>> +             up_read(&vdev->memory_lock);                            \
>> +             return -EIO;                                            \
>> +     }                                                               \
>> +                                                                     \
>>       if (test_mem) {                                                 \
>> -             down_read(&vdev->memory_lock);                          \
>>               if (!__vfio_pci_memory_enabled(vdev)) {                 \
>>                       up_read(&vdev->memory_lock);                    \
>>                       return -EIO;                                    \
>> @@ -78,8 +87,7 @@ static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev,         \
>>                                                                       \
>>       *val = vfio_ioread##size(io);                                   \
>>                                                                       \
>> -     if (test_mem)                                                   \
>> -             up_read(&vdev->memory_lock);                            \
>> +     up_read(&vdev->memory_lock);                                    \
>>                                                                       \
>>       return 0;                                                       \
>>  }
>