| 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 |
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.
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 --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;
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(+)