diff mbox series

[RFC,KERNEL,v3,1/3] xen/pci: Add xen_reset_device_state function

Message ID 20231210161519.1550860-2-Jiqian.Chen@amd.com (mailing list archive)
State Superseded
Headers show
Series Support device passthrough when dom0 is PVH on Xen | expand

Commit Message

Chen, Jiqian Dec. 10, 2023, 4:15 p.m. UTC
When device on dom0 side has been reset, the vpci on Xen side
won't get notification, so that the cached state in vpci is
all out of date with the real device state.
To solve that problem, add a new function to clear all vpci
device state when device is reset on dom0 side.

And call that function in pcistub_init_device. Because when
using "pci-assignable-add" to assign a passthrough device in
Xen, it will reset passthrough device and the vpci state will
out of date, and then device will fail to restore bar state.

Co-developed-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 drivers/xen/pci.c                  | 12 ++++++++++++
 drivers/xen/xen-pciback/pci_stub.c |  4 ++++
 include/xen/interface/physdev.h    |  8 ++++++++
 include/xen/pci.h                  |  6 ++++++
 4 files changed, 30 insertions(+)

Comments

Roger Pau Monné Dec. 12, 2023, 8:08 a.m. UTC | #1
On Mon, Dec 11, 2023 at 12:15:17AM +0800, Jiqian Chen wrote:
> When device on dom0 side has been reset, the vpci on Xen side
> won't get notification, so that the cached state in vpci is
> all out of date with the real device state.
> To solve that problem, add a new function to clear all vpci
> device state when device is reset on dom0 side.
> 
> And call that function in pcistub_init_device. Because when
> using "pci-assignable-add" to assign a passthrough device in
> Xen, it will reset passthrough device and the vpci state will
> out of date, and then device will fail to restore bar state.
> 
> Co-developed-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
>  drivers/xen/pci.c                  | 12 ++++++++++++
>  drivers/xen/xen-pciback/pci_stub.c |  4 ++++
>  include/xen/interface/physdev.h    |  8 ++++++++
>  include/xen/pci.h                  |  6 ++++++
>  4 files changed, 30 insertions(+)
> 
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 72d4e3f193af..e9b30bc09139 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -177,6 +177,18 @@ static int xen_remove_device(struct device *dev)
>  	return r;
>  }
>  
> +int xen_reset_device_state(const struct pci_dev *dev)
> +{
> +	struct physdev_pci_device device = {
> +		.seg = pci_domain_nr(dev->bus),
> +		.bus = dev->bus->number,
> +		.devfn = dev->devfn
> +	};
> +
> +	return HYPERVISOR_physdev_op(PHYSDEVOP_pci_device_state_reset, &device);
> +}
> +EXPORT_SYMBOL_GPL(xen_reset_device_state);
> +
>  static int xen_pci_notifier(struct notifier_block *nb,
>  			    unsigned long action, void *data)
>  {
> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> index e34b623e4b41..24f599eaec14 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -421,6 +421,10 @@ static int pcistub_init_device(struct pci_dev *dev)
>  	else {
>  		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
>  		__pci_reset_function_locked(dev);
> +		if (!xen_pv_domain())
> +			err = xen_reset_device_state(dev);
> +		if (err)
> +			goto config_release;

I think you are missing other instances where
__pci_reset_function_locked() is called in pci_stub.c?  See
pcistub_device_release() and pcistub_put_pci_dev().

Overall I'm not sure why the hypercall wrapper needs to live in
xen/pci.c.  I think it would be better if you could create a static
wrapper in pci_stub.c that does the call to
__pci_reset_function_locked() plus PHYSDEVOP_pci_device_state_reset.
That would make it less likely that new callers of
__pci_reset_function_locked() are introduced without noticing the need
to also call PHYSDEVOP_pci_device_state_reset.

Thanks, Roger.
Chen, Jiqian Dec. 13, 2023, 3:02 a.m. UTC | #2
On 2023/12/12 16:08, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:15:17AM +0800, Jiqian Chen wrote:
>> When device on dom0 side has been reset, the vpci on Xen side
>> won't get notification, so that the cached state in vpci is
>> all out of date with the real device state.
>> To solve that problem, add a new function to clear all vpci
>> device state when device is reset on dom0 side.
>>
>> And call that function in pcistub_init_device. Because when
>> using "pci-assignable-add" to assign a passthrough device in
>> Xen, it will reset passthrough device and the vpci state will
>> out of date, and then device will fail to restore bar state.
>>
>> Co-developed-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  drivers/xen/pci.c                  | 12 ++++++++++++
>>  drivers/xen/xen-pciback/pci_stub.c |  4 ++++
>>  include/xen/interface/physdev.h    |  8 ++++++++
>>  include/xen/pci.h                  |  6 ++++++
>>  4 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>> index 72d4e3f193af..e9b30bc09139 100644
>> --- a/drivers/xen/pci.c
>> +++ b/drivers/xen/pci.c
>> @@ -177,6 +177,18 @@ static int xen_remove_device(struct device *dev)
>>  	return r;
>>  }
>>  
>> +int xen_reset_device_state(const struct pci_dev *dev)
>> +{
>> +	struct physdev_pci_device device = {
>> +		.seg = pci_domain_nr(dev->bus),
>> +		.bus = dev->bus->number,
>> +		.devfn = dev->devfn
>> +	};
>> +
>> +	return HYPERVISOR_physdev_op(PHYSDEVOP_pci_device_state_reset, &device);
>> +}
>> +EXPORT_SYMBOL_GPL(xen_reset_device_state);
>> +
>>  static int xen_pci_notifier(struct notifier_block *nb,
>>  			    unsigned long action, void *data)
>>  {
>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
>> index e34b623e4b41..24f599eaec14 100644
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -421,6 +421,10 @@ static int pcistub_init_device(struct pci_dev *dev)
>>  	else {
>>  		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
>>  		__pci_reset_function_locked(dev);
>> +		if (!xen_pv_domain())
>> +			err = xen_reset_device_state(dev);
>> +		if (err)
>> +			goto config_release;
> 
> I think you are missing other instances where
> __pci_reset_function_locked() is called in pci_stub.c?  See
> pcistub_device_release() and pcistub_put_pci_dev().
Sorry, I didn't consider the situation to free passthrough device. You are right.

> 
> Overall I'm not sure why the hypercall wrapper needs to live in
> xen/pci.c.  
For other possible scenarios where this function may be used?

> I think it would be better if you could create a static wrapper in pci_stub.c that does the call to
> __pci_reset_function_locked() plus PHYSDEVOP_pci_device_state_reset.
> That would make it less likely that new callers of
> __pci_reset_function_locked() are introduced without noticing the need
> to also call PHYSDEVOP_pci_device_state_reset.
Ok, I will add a new function to do __pci_reset_function_locked and PHYSDEVOP_pci_device_state_reset in pci_stub.c

> 
> Thanks, Roger.
diff mbox series

Patch

diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 72d4e3f193af..e9b30bc09139 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -177,6 +177,18 @@  static int xen_remove_device(struct device *dev)
 	return r;
 }
 
+int xen_reset_device_state(const struct pci_dev *dev)
+{
+	struct physdev_pci_device device = {
+		.seg = pci_domain_nr(dev->bus),
+		.bus = dev->bus->number,
+		.devfn = dev->devfn
+	};
+
+	return HYPERVISOR_physdev_op(PHYSDEVOP_pci_device_state_reset, &device);
+}
+EXPORT_SYMBOL_GPL(xen_reset_device_state);
+
 static int xen_pci_notifier(struct notifier_block *nb,
 			    unsigned long action, void *data)
 {
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index e34b623e4b41..24f599eaec14 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -421,6 +421,10 @@  static int pcistub_init_device(struct pci_dev *dev)
 	else {
 		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
 		__pci_reset_function_locked(dev);
+		if (!xen_pv_domain())
+			err = xen_reset_device_state(dev);
+		if (err)
+			goto config_release;
 		pci_restore_state(dev);
 	}
 	/* Now disable the device (this also ensures some private device
diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
index a237af867873..bed53afc4c52 100644
--- a/include/xen/interface/physdev.h
+++ b/include/xen/interface/physdev.h
@@ -256,6 +256,14 @@  struct physdev_pci_device_add {
  */
 #define PHYSDEVOP_prepare_msix          30
 #define PHYSDEVOP_release_msix          31
+/*
+ * On PVH dom0, when device is reset, the vpci on Xen side
+ * won't get notification, so that the cached state in vpci is
+ * all out of date with the real device state. Use this to reset
+ * the vpci state of device.
+ */
+#define PHYSDEVOP_pci_device_state_reset     32
+
 struct physdev_pci_device {
     /* IN */
     uint16_t seg;
diff --git a/include/xen/pci.h b/include/xen/pci.h
index b8337cf85fd1..b2e2e856efd6 100644
--- a/include/xen/pci.h
+++ b/include/xen/pci.h
@@ -4,10 +4,16 @@ 
 #define __XEN_PCI_H__
 
 #if defined(CONFIG_XEN_DOM0)
+int xen_reset_device_state(const struct pci_dev *dev);
 int xen_find_device_domain_owner(struct pci_dev *dev);
 int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
 int xen_unregister_device_domain_owner(struct pci_dev *dev);
 #else
+static inline int xen_reset_device_state(const struct pci_dev *dev)
+{
+	return -1;
+}
+
 static inline int xen_find_device_domain_owner(struct pci_dev *dev)
 {
 	return -1;