This small series contains a fix and readability improvements for
resets.
v2:
- Split single patch into 2 patches
- Improve commit messages
v1:
https://lore.kernel.org/kvm/[email protected]/
Brett Creeley (2):
vfio/pds: Always clear the save/restore FDs on reset
vfio/pds: Refactor/simplify reset logic
drivers/vfio/pci/pds/pci_drv.c | 2 +-
drivers/vfio/pci/pds/vfio_dev.c | 14 +++++++-------
drivers/vfio/pci/pds/vfio_dev.h | 7 ++++++-
3 files changed, 14 insertions(+), 9 deletions(-)
--
2.17.1
The current logic for handling resets is more complicated than it needs
to be. The deferred_reset flag is used to indicate a reset is needed
and the deferred_reset_state is the requested, post-reset, state. The
source of the requested reset isn't immediately obvious. Improve
readability by replacing deferred_reset_state with deferred_reset_type,
which can be either PDS_VFIO_DEVICE_RESET (initiated/requested by the
DSC) or PDS_VFIO_HOST_RESET (initiated/requested by the VMM).
Signed-off-by: Brett Creeley <[email protected]>
Reviewed-by: Shannon Nelson <[email protected]>
---
drivers/vfio/pci/pds/pci_drv.c | 2 +-
drivers/vfio/pci/pds/vfio_dev.c | 10 +++++-----
drivers/vfio/pci/pds/vfio_dev.h | 7 ++++++-
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/vfio/pci/pds/pci_drv.c b/drivers/vfio/pci/pds/pci_drv.c
index a34dda516629..4ac3da7abd32 100644
--- a/drivers/vfio/pci/pds/pci_drv.c
+++ b/drivers/vfio/pci/pds/pci_drv.c
@@ -57,7 +57,7 @@ static void pds_vfio_recovery(struct pds_vfio_pci_device *pds_vfio)
if (deferred_reset_needed) {
mutex_lock(&pds_vfio->reset_mutex);
pds_vfio->deferred_reset = true;
- pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_ERROR;
+ pds_vfio->deferred_reset_type = PDS_VFIO_DEVICE_RESET;
mutex_unlock(&pds_vfio->reset_mutex);
}
}
diff --git a/drivers/vfio/pci/pds/vfio_dev.c b/drivers/vfio/pci/pds/vfio_dev.c
index a286ebcc7112..1a791bef5de1 100644
--- a/drivers/vfio/pci/pds/vfio_dev.c
+++ b/drivers/vfio/pci/pds/vfio_dev.c
@@ -34,11 +34,12 @@ void pds_vfio_state_mutex_unlock(struct pds_vfio_pci_device *pds_vfio)
pds_vfio->deferred_reset = false;
pds_vfio_put_restore_file(pds_vfio);
pds_vfio_put_save_file(pds_vfio);
- if (pds_vfio->state == VFIO_DEVICE_STATE_ERROR) {
+ if (pds_vfio->deferred_reset_type == PDS_VFIO_DEVICE_RESET) {
pds_vfio_dirty_disable(pds_vfio, false);
+ pds_vfio->state = VFIO_DEVICE_STATE_ERROR;
+ } else {
+ pds_vfio->state = VFIO_DEVICE_STATE_RUNNING;
}
- pds_vfio->state = pds_vfio->deferred_reset_state;
- pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_RUNNING;
mutex_unlock(&pds_vfio->reset_mutex);
goto again;
}
@@ -50,7 +51,7 @@ void pds_vfio_reset(struct pds_vfio_pci_device *pds_vfio)
{
mutex_lock(&pds_vfio->reset_mutex);
pds_vfio->deferred_reset = true;
- pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_RUNNING;
+ pds_vfio->deferred_reset_type = PDS_VFIO_HOST_RESET;
if (!mutex_trylock(&pds_vfio->state_mutex)) {
mutex_unlock(&pds_vfio->reset_mutex);
return;
@@ -194,7 +195,6 @@ static int pds_vfio_open_device(struct vfio_device *vdev)
return err;
pds_vfio->state = VFIO_DEVICE_STATE_RUNNING;
- pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_RUNNING;
vfio_pci_core_finish_enable(&pds_vfio->vfio_coredev);
diff --git a/drivers/vfio/pci/pds/vfio_dev.h b/drivers/vfio/pci/pds/vfio_dev.h
index e7b01080a1ec..19547fd8e956 100644
--- a/drivers/vfio/pci/pds/vfio_dev.h
+++ b/drivers/vfio/pci/pds/vfio_dev.h
@@ -10,6 +10,11 @@
#include "dirty.h"
#include "lm.h"
+enum pds_vfio_reset_type {
+ PDS_VFIO_HOST_RESET = 0,
+ PDS_VFIO_DEVICE_RESET = 1,
+};
+
struct pds_vfio_pci_device {
struct vfio_pci_core_device vfio_coredev;
@@ -20,7 +25,7 @@ struct pds_vfio_pci_device {
enum vfio_device_mig_state state;
struct mutex reset_mutex; /* protect reset_done flow */
u8 deferred_reset;
- enum vfio_device_mig_state deferred_reset_state;
+ enum pds_vfio_reset_type deferred_reset_type;
struct notifier_block nb;
int vf_id;
--
2.17.1
> From: Brett Creeley <[email protected]>
> Sent: Wednesday, February 28, 2024 8:32 AM
>
> The current logic for handling resets is more complicated than it needs
> to be. The deferred_reset flag is used to indicate a reset is needed
> and the deferred_reset_state is the requested, post-reset, state. The
> source of the requested reset isn't immediately obvious. Improve
> readability by replacing deferred_reset_state with deferred_reset_type,
> which can be either PDS_VFIO_DEVICE_RESET (initiated/requested by the
> DSC) or PDS_VFIO_HOST_RESET (initiated/requested by the VMM).
>
> Signed-off-by: Brett Creeley <[email protected]>
> Reviewed-by: Shannon Nelson <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> -----Original Message-----
> From: Brett Creeley <[email protected]>
> Sent: Wednesday, February 28, 2024 12:32 AM
> To: [email protected]; [email protected]; Shameerali Kolothum Thodi
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]
> Subject: [PATCH v2 vfio 0/2] vfio/pds: Fix and simplify resets
>
> This small series contains a fix and readability improvements for
> resets.
>
> v2:
> - Split single patch into 2 patches
> - Improve commit messages
Just a query on the reset_done handler and the deferred_reset()
logic in this driver. From a quick look, it doesn't look like you have
a condition where a copy_to/from_user() is under state_mutex. So
do you think we can get rid of the deferred_reset logic from this
driver? Please see the discussion here,
https://lore.kernel.org/kvm/[email protected]/
For HiSilicon, we do have the lock taken for PRE_COPY, but that needs fixing
and then can get rid of the deferred_reset. I will sent out a patch for
that soon.
Thanks,
Shameer
>
> v1:
> https://lore.kernel.org/kvm/20240126183225.19193-1-
> [email protected]/
>
> Brett Creeley (2):
> vfio/pds: Always clear the save/restore FDs on reset
> vfio/pds: Refactor/simplify reset logic
>
> drivers/vfio/pci/pds/pci_drv.c | 2 +-
> drivers/vfio/pci/pds/vfio_dev.c | 14 +++++++-------
> drivers/vfio/pci/pds/vfio_dev.h | 7 ++++++-
> 3 files changed, 14 insertions(+), 9 deletions(-)
>
> --
> 2.17.1
On 2/28/2024 1:05 AM, Shameerali Kolothum Thodi wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
>> -----Original Message-----
>> From: Brett Creeley <[email protected]>
>> Sent: Wednesday, February 28, 2024 12:32 AM
>> To: [email protected]; [email protected]; Shameerali Kolothum Thodi
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]; linux-
>> [email protected]
>> Cc: [email protected]; [email protected]
>> Subject: [PATCH v2 vfio 0/2] vfio/pds: Fix and simplify resets
>>
>> This small series contains a fix and readability improvements for
>> resets.
>>
>> v2:
>> - Split single patch into 2 patches
>> - Improve commit messages
>
> Just a query on the reset_done handler and the deferred_reset()
> logic in this driver. From a quick look, it doesn't look like you have
> a condition where a copy_to/from_user() is under state_mutex. So
> do you think we can get rid of the deferred_reset logic from this
> driver? Please see the discussion here,
> https://lore.kernel.org/kvm/[email protected]/
>
> For HiSilicon, we do have the lock taken for PRE_COPY, but that needs fixing
> and then can get rid of the deferred_reset. I will sent out a patch for
> that soon.
>
> Thanks,
> Shameer
Hi Shameer,
You are probably right that we can get rid of this logic, but the
current 2 patch series is very simple and I would prefer to keep it that
way. If you plan to make changes to the HiSilicon driver in the near
future, then I can use that as a reference in enhancing the pds-vfio-pci
driver.
Thanks,
Brett
>
>>
>> v1:
>> https://lore.kernel.org/kvm/20240126183225.19193-1-
>> [email protected]/
>>
>> Brett Creeley (2):
>> vfio/pds: Always clear the save/restore FDs on reset
>> vfio/pds: Refactor/simplify reset logic
>>
>> drivers/vfio/pci/pds/pci_drv.c | 2 +-
>> drivers/vfio/pci/pds/vfio_dev.c | 14 +++++++-------
>> drivers/vfio/pci/pds/vfio_dev.h | 7 ++++++-
>> 3 files changed, 14 insertions(+), 9 deletions(-)
>>
>> --
>> 2.17.1
>
On Tue, 27 Feb 2024 16:32:03 -0800
Brett Creeley <[email protected]> wrote:
> This small series contains a fix and readability improvements for
> resets.
>
> v2:
> - Split single patch into 2 patches
> - Improve commit messages
>
> v1:
> https://lore.kernel.org/kvm/[email protected]/
>
> Brett Creeley (2):
> vfio/pds: Always clear the save/restore FDs on reset
> vfio/pds: Refactor/simplify reset logic
>
> drivers/vfio/pci/pds/pci_drv.c | 2 +-
> drivers/vfio/pci/pds/vfio_dev.c | 14 +++++++-------
> drivers/vfio/pci/pds/vfio_dev.h | 7 ++++++-
> 3 files changed, 14 insertions(+), 9 deletions(-)
>
Applied to vfio next branch for v6.9. Thanks,
Alex