| Message ID | 20231124103123.3263471-2-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On Fri, 24 Nov 2023, 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, this patch add a function to clear all > vpci device state when device is reset on dom0 side. > > And call that function in pcistub_init_device. Because when > we use "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. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > drivers/xen/pci.c | 12 ++++++++++++ > drivers/xen/xen-pciback/pci_stub.c | 3 +++ > include/xen/interface/physdev.h | 2 ++ > include/xen/pci.h | 6 ++++++ > 4 files changed, 23 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..5a96b6c66c07 100644 > --- a/drivers/xen/xen-pciback/pci_stub.c > +++ b/drivers/xen/xen-pciback/pci_stub.c > @@ -421,6 +421,9 @@ 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); > + err = xen_reset_device_state(dev); > + if (err) > + goto config_release; Older versions of Xen won't have the hypercall PHYSDEVOP_pci_device_state_reset implemented. I think we should do something like: if (err && xen_pvh_domain()) goto config_release; Or even: if (xen_pvh_domain()) { err = xen_reset_device_state(dev); if (err) goto config_release; } depending on whether we want to call xen_reset_device_state also for PV guests or not. I am assuming we don't want to error out on failure such as -ENOENT for PV guests. > 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..231526f80f6c 100644 > --- a/include/xen/interface/physdev.h > +++ b/include/xen/interface/physdev.h > @@ -263,6 +263,8 @@ struct physdev_pci_device { > uint8_t devfn; > }; > > +#define PHYSDEVOP_pci_device_state_reset 32 > + > #define PHYSDEVOP_DBGP_RESET_PREPARE 1 > #define PHYSDEVOP_DBGP_RESET_DONE 2 > > 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; > -- > 2.34.1 >
On 2023/11/30 11:46, Stefano Stabellini wrote: > On Fri, 24 Nov 2023, 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, this patch add a function to clear all >> vpci device state when device is reset on dom0 side. >> >> And call that function in pcistub_init_device. Because when >> we use "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. >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> --- >> drivers/xen/pci.c | 12 ++++++++++++ >> drivers/xen/xen-pciback/pci_stub.c | 3 +++ >> include/xen/interface/physdev.h | 2 ++ >> include/xen/pci.h | 6 ++++++ >> 4 files changed, 23 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..5a96b6c66c07 100644 >> --- a/drivers/xen/xen-pciback/pci_stub.c >> +++ b/drivers/xen/xen-pciback/pci_stub.c >> @@ -421,6 +421,9 @@ 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); >> + err = xen_reset_device_state(dev); >> + if (err) >> + goto config_release; > > Older versions of Xen won't have the hypercall > PHYSDEVOP_pci_device_state_reset implemented. I think we should do > something like: > > if (err && xen_pvh_domain()) > goto config_release; > > > Or even: > > if (xen_pvh_domain()) { > err = xen_reset_device_state(dev); > if (err) > goto config_release; > } > > depending on whether we want to call xen_reset_device_state also for PV > guests or not. I am assuming we don't want to error out on failure such > as -ENOENT for PV guests. Yes, only for PVH dom0, I will add the condition in next version. Thank you! > > >> 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..231526f80f6c 100644 >> --- a/include/xen/interface/physdev.h >> +++ b/include/xen/interface/physdev.h >> @@ -263,6 +263,8 @@ struct physdev_pci_device { >> uint8_t devfn; >> }; >> >> +#define PHYSDEVOP_pci_device_state_reset 32 >> + >> #define PHYSDEVOP_DBGP_RESET_PREPARE 1 >> #define PHYSDEVOP_DBGP_RESET_DONE 2 >> >> 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; >> -- >> 2.34.1 >>
On 11/30/23 02:03, Chen, Jiqian wrote: > > On 2023/11/30 11:46, Stefano Stabellini wrote: >> On Fri, 24 Nov 2023, 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, this patch add a function to clear all >>> vpci device state when device is reset on dom0 side. >>> >>> And call that function in pcistub_init_device. Because when >>> we use "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. >>> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> --- >>> drivers/xen/pci.c | 12 ++++++++++++ >>> drivers/xen/xen-pciback/pci_stub.c | 3 +++ >>> include/xen/interface/physdev.h | 2 ++ >>> include/xen/pci.h | 6 ++++++ >>> 4 files changed, 23 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..5a96b6c66c07 100644 >>> --- a/drivers/xen/xen-pciback/pci_stub.c >>> +++ b/drivers/xen/xen-pciback/pci_stub.c >>> @@ -421,6 +421,9 @@ 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); >>> + err = xen_reset_device_state(dev); >>> + if (err) >>> + goto config_release; >> >> Older versions of Xen won't have the hypercall >> PHYSDEVOP_pci_device_state_reset implemented. I think we should do >> something like: >> >> if (err && xen_pvh_domain()) >> goto config_release; >> >> >> Or even: >> >> if (xen_pvh_domain()) { >> err = xen_reset_device_state(dev); >> if (err) >> goto config_release; >> } >> >> depending on whether we want to call xen_reset_device_state also for PV >> guests or not. I am assuming we don't want to error out on failure such >> as -ENOENT for PV guests. > Yes, only for PVH dom0, I will add the condition in next version. Thank you! We will want to call xen_reset_device_state() for Arm dom0, too, so checking xen_pvh_domain() alone is not sufficient. I suggest instead to check !xen_pv_domain(). > >> >> >>> 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..231526f80f6c 100644 >>> --- a/include/xen/interface/physdev.h >>> +++ b/include/xen/interface/physdev.h >>> @@ -263,6 +263,8 @@ struct physdev_pci_device { >>> uint8_t devfn; >>> }; >>> >>> +#define PHYSDEVOP_pci_device_state_reset 32 >>> + >>> #define PHYSDEVOP_DBGP_RESET_PREPARE 1 >>> #define PHYSDEVOP_DBGP_RESET_DONE 2 >>> >>> 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; >>> -- >>> 2.34.1 >>> >
Hi Stewart, On 2023/11/30 23:03, Stewart Hildebrand wrote: > On 11/30/23 02:03, Chen, Jiqian wrote: >> >> On 2023/11/30 11:46, Stefano Stabellini wrote: >>> On Fri, 24 Nov 2023, 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, this patch add a function to clear all >>>> vpci device state when device is reset on dom0 side. >>>> >>>> And call that function in pcistub_init_device. Because when >>>> we use "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. >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> drivers/xen/pci.c | 12 ++++++++++++ >>>> drivers/xen/xen-pciback/pci_stub.c | 3 +++ >>>> include/xen/interface/physdev.h | 2 ++ >>>> include/xen/pci.h | 6 ++++++ >>>> 4 files changed, 23 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..5a96b6c66c07 100644 >>>> --- a/drivers/xen/xen-pciback/pci_stub.c >>>> +++ b/drivers/xen/xen-pciback/pci_stub.c >>>> @@ -421,6 +421,9 @@ 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); >>>> + err = xen_reset_device_state(dev); >>>> + if (err) >>>> + goto config_release; >>> >>> Older versions of Xen won't have the hypercall >>> PHYSDEVOP_pci_device_state_reset implemented. I think we should do >>> something like: >>> >>> if (err && xen_pvh_domain()) >>> goto config_release; >>> >>> >>> Or even: >>> >>> if (xen_pvh_domain()) { >>> err = xen_reset_device_state(dev); >>> if (err) >>> goto config_release; >>> } >>> >>> depending on whether we want to call xen_reset_device_state also for PV >>> guests or not. I am assuming we don't want to error out on failure such >>> as -ENOENT for PV guests. >> Yes, only for PVH dom0, I will add the condition in next version. Thank you! > > We will want to call xen_reset_device_state() for Arm dom0, too, so checking xen_pvh_domain() alone is not sufficient. I suggest instead to check !xen_pv_domain(). I am not using Arm. But is Arm dom0 not a PVH type dom0? > >> >>> >>> >>>> 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..231526f80f6c 100644 >>>> --- a/include/xen/interface/physdev.h >>>> +++ b/include/xen/interface/physdev.h >>>> @@ -263,6 +263,8 @@ struct physdev_pci_device { >>>> uint8_t devfn; >>>> }; >>>> >>>> +#define PHYSDEVOP_pci_device_state_reset 32 >>>> + >>>> #define PHYSDEVOP_DBGP_RESET_PREPARE 1 >>>> #define PHYSDEVOP_DBGP_RESET_DONE 2 >>>> >>>> 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; >>>> -- >>>> 2.34.1 >>>> >>
On 12/3/23 22:25, Chen, Jiqian wrote: > Hi Stewart, > > On 2023/11/30 23:03, Stewart Hildebrand wrote: >> On 11/30/23 02:03, Chen, Jiqian wrote: >>> >>> On 2023/11/30 11:46, Stefano Stabellini wrote: >>>> On Fri, 24 Nov 2023, 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, this patch add a function to clear all >>>>> vpci device state when device is reset on dom0 side. >>>>> >>>>> And call that function in pcistub_init_device. Because when >>>>> we use "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. >>>>> >>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>> --- >>>>> drivers/xen/pci.c | 12 ++++++++++++ >>>>> drivers/xen/xen-pciback/pci_stub.c | 3 +++ >>>>> include/xen/interface/physdev.h | 2 ++ >>>>> include/xen/pci.h | 6 ++++++ >>>>> 4 files changed, 23 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..5a96b6c66c07 100644 >>>>> --- a/drivers/xen/xen-pciback/pci_stub.c >>>>> +++ b/drivers/xen/xen-pciback/pci_stub.c >>>>> @@ -421,6 +421,9 @@ 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); >>>>> + err = xen_reset_device_state(dev); >>>>> + if (err) >>>>> + goto config_release; >>>> >>>> Older versions of Xen won't have the hypercall >>>> PHYSDEVOP_pci_device_state_reset implemented. I think we should do >>>> something like: >>>> >>>> if (err && xen_pvh_domain()) >>>> goto config_release; >>>> >>>> >>>> Or even: >>>> >>>> if (xen_pvh_domain()) { >>>> err = xen_reset_device_state(dev); >>>> if (err) >>>> goto config_release; >>>> } >>>> >>>> depending on whether we want to call xen_reset_device_state also for PV >>>> guests or not. I am assuming we don't want to error out on failure such >>>> as -ENOENT for PV guests. >>> Yes, only for PVH dom0, I will add the condition in next version. Thank you! >> >> We will want to call xen_reset_device_state() for Arm dom0, too, so checking xen_pvh_domain() alone is not sufficient. I suggest instead to check !xen_pv_domain(). > I am not using Arm. But is Arm dom0 not a PVH type dom0? Apparently not, Arm dom0 appears to be a xen_hvm_domain() from the kernel's perspective. > >> >>> >>>> >>>> >>>>> 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..231526f80f6c 100644 >>>>> --- a/include/xen/interface/physdev.h >>>>> +++ b/include/xen/interface/physdev.h >>>>> @@ -263,6 +263,8 @@ struct physdev_pci_device { >>>>> uint8_t devfn; >>>>> }; >>>>> >>>>> +#define PHYSDEVOP_pci_device_state_reset 32 >>>>> + >>>>> #define PHYSDEVOP_DBGP_RESET_PREPARE 1 >>>>> #define PHYSDEVOP_DBGP_RESET_DONE 2 >>>>> >>>>> 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; >>>>> -- >>>>> 2.34.1 >>>>> >>> >
On Fri, Nov 24 2023 at 18:31, 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, this patch add a function to clear all Please get rid of 'this patch' all over the series. # grep -r 'This patch' Documentation/process/ > vpci device state when device is reset on dom0 side. > > And call that function in pcistub_init_device. Because when > we use "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. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> This Signed-off-by chain is incorrect. Documentation/process/submitting-patches.rst has a full chapter about S-O-B and the correct usage. Thanks, tglx
On 2023/12/4 15:58, Thomas Gleixner wrote: > On Fri, Nov 24 2023 at 18:31, 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, this patch add a function to clear all > > Please get rid of 'this patch' all over the series. > > # grep -r 'This patch' Documentation/process/ Thanks. I will remove "this patch" or "I/we" in next version. > >> vpci device state when device is reset on dom0 side. >> >> And call that function in pcistub_init_device. Because when >> we use "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. >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> > > This Signed-off-by chain is incorrect. > > Documentation/process/submitting-patches.rst has a full chapter about > S-O-B and the correct usage. I am the author of this series of patches, and Huang Rui transported the v1 to upstream. And now I transport v2. I am not aware that the SOB chain is incorrect. Do you have any suggestions? > > Thanks, > > tglx
On Mon, 3 Dec 2023, Chen, Jiqian wrote: > >> vpci device state when device is reset on dom0 side. > >> > >> And call that function in pcistub_init_device. Because when > >> we use "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. > >> > >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > >> Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > This Signed-off-by chain is incorrect. > > > > Documentation/process/submitting-patches.rst has a full chapter about > > S-O-B and the correct usage. > I am the author of this series of patches, and Huang Rui transported the v1 to upstream. And now I transport v2. I am not aware that the SOB chain is incorrect. > Do you have any suggestions? I think he means that your Signed-off-by should be the second one of the two as you are the one submitting the patch to the LKML
On 2023/12/5 05:31, Stefano Stabellini wrote: > On Mon, 3 Dec 2023, Chen, Jiqian wrote: >>>> vpci device state when device is reset on dom0 side. >>>> >>>> And call that function in pcistub_init_device. Because when >>>> we use "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. >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> >>> This Signed-off-by chain is incorrect. >>> >>> Documentation/process/submitting-patches.rst has a full chapter about >>> S-O-B and the correct usage. >> I am the author of this series of patches, and Huang Rui transported the v1 to upstream. And now I transport v2. I am not aware that the SOB chain is incorrect. >> Do you have any suggestions? > > I think he means that your Signed-off-by should be the second one of the > two as you are the one submitting the patch to the LKML Got it. Thanks Stefano! I will adjust the sequence in next version.
(reducing Cc list) On 04.12.2023 22:31, Stefano Stabellini wrote: > On Mon, 3 Dec 2023, Chen, Jiqian wrote: >>>> vpci device state when device is reset on dom0 side. >>>> >>>> And call that function in pcistub_init_device. Because when >>>> we use "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. >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> >>> This Signed-off-by chain is incorrect. >>> >>> Documentation/process/submitting-patches.rst has a full chapter about >>> S-O-B and the correct usage. >> I am the author of this series of patches, and Huang Rui transported the v1 to upstream. And now I transport v2. I am not aware that the SOB chain is incorrect. >> Do you have any suggestions? > > I think he means that your Signed-off-by should be the second one of the > two as you are the one submitting the patch to the LKML But that's not really correct either, as it then doesn't represent the sequence of events. The first S-o-b normally is the (original) author's, isn't it? Jan
On Mon, Dec 04 2023 at 13:31, Stefano Stabellini wrote: > On Mon, 3 Dec 2023, Chen, Jiqian wrote: >> >> vpci device state when device is reset on dom0 side. >> >> >> >> And call that function in pcistub_init_device. Because when >> >> we use "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. >> >> >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> > >> > This Signed-off-by chain is incorrect. >> > >> > Documentation/process/submitting-patches.rst has a full chapter about >> > S-O-B and the correct usage. >> I am the author of this series of patches, and Huang Rui transported the v1 to upstream. And now I transport v2. I am not aware that the SOB chain is incorrect. >> Do you have any suggestions? > > I think he means that your Signed-off-by should be the second one of the > two as you are the one submitting the patch to the LKML No. Mailfrom: Jiqian Chen <Jiqian.Chen@amd.com> <body> Changelog-text Signed-off-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> is equally wrong because that would end up with Chen as author and Huang as first S-O-B which is required to be the author's S-O-B To make the above correct this would require: Mailfrom: Jiqian Chen <Jiqian.Chen@amd.com> <body> From: Huang Rui <ray.huang@amd.com> Changelog-text Signed-off-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> which tells that Huang is the author and Chen is the 'transporter', which unfortunately does not reflect reality. Or: Mailfrom: Jiqian Chen <Jiqian.Chen@amd.com> <body> Changelog-text Co-developed-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> which tells that Checn is the author and Huang co-developed the patch, which might be true or not. V1 which was sent by Huang has the ordering is correct: Mailfrom: Huang Rui <ray.huang@amd.com> <body> From: Jiqian Chen <Jiqian.Chen@amd.com> Changelog-text Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> Signed-off-by: Huang Rui <ray.huang@amd.com> i.e. Chen authored and Huang transported Now this V2 has not really much to do with V1 and is a new implementation to solve the problem, which was authored by Chen, so Huang is not involved at all if I understand correctly. So what does his S-O-B mean here? Nothing... It's very well documented how the whole S-O-B business works and it's not really rocket science to get it straight. It has a meaning and is not just for decoration purposes. Thanks, tglx
Hi Thomas Gleixner, On 2023/12/6 01:02, Thomas Gleixner wrote: > On Mon, Dec 04 2023 at 13:31, Stefano Stabellini wrote: >> On Mon, 3 Dec 2023, Chen, Jiqian wrote: >>>>> vpci device state when device is reset on dom0 side. >>>>> >>>>> And call that function in pcistub_init_device. Because when >>>>> we use "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. >>>>> >>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> >>>> This Signed-off-by chain is incorrect. >>>> >>>> Documentation/process/submitting-patches.rst has a full chapter about >>>> S-O-B and the correct usage. >>> I am the author of this series of patches, and Huang Rui transported the v1 to upstream. And now I transport v2. I am not aware that the SOB chain is incorrect. >>> Do you have any suggestions? >> >> I think he means that your Signed-off-by should be the second one of the >> two as you are the one submitting the patch to the LKML > > No. > > Mailfrom: Jiqian Chen <Jiqian.Chen@amd.com> > <body> > > Changelog-text > > Signed-off-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > > is equally wrong because that would end up with Chen as author and Huang > as first S-O-B which is required to be the author's S-O-B > > To make the above correct this would require: > > Mailfrom: Jiqian Chen <Jiqian.Chen@amd.com> > <body> > > From: Huang Rui <ray.huang@amd.com> > > Changelog-text > > Signed-off-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > > which tells that Huang is the author and Chen is the 'transporter', > which unfortunately does not reflect reality. > > Or: > > Mailfrom: Jiqian Chen <Jiqian.Chen@amd.com> > <body> > > Changelog-text > > Co-developed-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > > which tells that Checn is the author and Huang co-developed the > patch, which might be true or not. > > > V1 which was sent by Huang has the ordering is correct: > > Mailfrom: Huang Rui <ray.huang@amd.com> > <body> > > From: Jiqian Chen <Jiqian.Chen@amd.com> > > Changelog-text > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > > i.e. Chen authored and Huang transported > > Now this V2 has not really much to do with V1 and is a new > implementation to solve the problem, which was authored by Chen, so > Huang is not involved at all if I understand correctly. Not exactly, V2 is modified based on the V1. I am the author of this series. Huang Rui upstream the V1, and he also helped me to improve the first patch of this V2 series. Maybe in next version, below is more suitable: Co-developed-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> Which can reflect that Huang Rui is co-developer, and I am the author and the last people to send patches. > > So what does his S-O-B mean here? Nothing... > > It's very well documented how the whole S-O-B business works and it's > not really rocket science to get it straight. > > It has a meaning and is not just for decoration purposes. > > Thanks, > > tglx
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..5a96b6c66c07 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -421,6 +421,9 @@ 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); + 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..231526f80f6c 100644 --- a/include/xen/interface/physdev.h +++ b/include/xen/interface/physdev.h @@ -263,6 +263,8 @@ struct physdev_pci_device { uint8_t devfn; }; +#define PHYSDEVOP_pci_device_state_reset 32 + #define PHYSDEVOP_DBGP_RESET_PREPARE 1 #define PHYSDEVOP_DBGP_RESET_DONE 2 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;