| Message ID | 20231124104136.3263722-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, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: > When a device has been reset on dom0 side, the vpci on Xen > side won't get notification, so the cached state in vpci is > all out of date compare with the real device state. > To solve that problem, this patch add new hypercall to clear > all vpci device state. And when reset device happens on dom0 > side, dom0 can call this hypercall to notify vpci. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > xen/arch/x86/hvm/hypercall.c | 1 + > xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ > xen/drivers/pci/physdev.c | 14 ++++++++++++++ > xen/drivers/vpci/vpci.c | 9 +++++++++ > xen/include/public/physdev.h | 2 ++ > xen/include/xen/pci.h | 1 + > xen/include/xen/vpci.h | 6 ++++++ > 7 files changed, 54 insertions(+) > > diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c > index eeb73e1aa5..6ad5b4d5f1 100644 > --- a/xen/arch/x86/hvm/hypercall.c > +++ b/xen/arch/x86/hvm/hypercall.c > @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > case PHYSDEVOP_pci_mmcfg_reserved: > case PHYSDEVOP_pci_device_add: > case PHYSDEVOP_pci_device_remove: > + case PHYSDEVOP_pci_device_state_reset: > case PHYSDEVOP_dbgp_op: > if ( !is_hardware_domain(currd) ) > return -ENOSYS; > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 04d00c7c37..f871715585 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) > return ret; > } > > +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) You could use pci_sbdf_t here instead of 3 parameters. I'm however unsure whether we really need this helper just to fetch the pdev and call vpci_reset_device_state(), I think you could place this logic directly in pci_physdev_op(). Unless there are plans to use such logic outside of pci_physdev_op(). > +{ > + struct pci_dev *pdev; > + int ret = -ENODEV; Some XSM check should be introduced fro this operation, as none of the existing ones is suitable. See xsm_resource_unplug_pci() for example. xsm_resource_reset_state_pci() or some such I would assume. (adding the XSM maintainer for feedback). > + > + pcidevs_lock(); > + > + pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn)); > + if ( !pdev ) > + goto error; > + > + ret = vpci_reset_device_state(pdev); > + if (ret) > + printk(XENLOG_ERR "PCI reset device %pp state failed\n", &pdev->sbdf); > + > +error: > + pcidevs_unlock(); > + > + return ret; > +} > + > /* Caller should hold the pcidevs_lock */ > static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, > uint8_t devfn) > diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c > index 42db3e6d13..cfdb545654 100644 > --- a/xen/drivers/pci/physdev.c > +++ b/xen/drivers/pci/physdev.c > @@ -67,6 +67,20 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > break; > } > > + case PHYSDEVOP_pci_device_state_reset: { > + struct physdev_pci_device dev; > + > + if ( !is_pci_passthrough_enabled() ) > + return -EOPNOTSUPP; > + > + ret = -EFAULT; > + if ( copy_from_guest(&dev, arg, 1) != 0 ) > + break; > + > + ret = pci_reset_device_state(dev.seg, dev.bus, dev.devfn); > + break; > + } > + > default: > ret = -ENOSYS; > break; > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index 3bec9a4153..e9c3eb1cd6 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -103,6 +103,15 @@ int vpci_add_handlers(struct pci_dev *pdev) > > return rc; > } > + > +int vpci_reset_device_state(struct pci_dev *pdev) > +{ > + ASSERT(pcidevs_locked()); > + > + vpci_remove_device(pdev); > + return vpci_add_handlers(pdev); > +} > + > #endif /* __XEN__ */ > > static int vpci_register_cmp(const struct vpci_register *r1, > diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h > index f0c0d4727c..4156948903 100644 > --- a/xen/include/public/physdev.h > +++ b/xen/include/public/physdev.h > @@ -305,6 +305,8 @@ struct physdev_pci_device { > typedef struct physdev_pci_device physdev_pci_device_t; > DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t); > > +#define PHYSDEVOP_pci_device_state_reset 32 This needs some comment in order to explain the expected behaviour, and might be better placed a bit up after PHYSDEVOP_release_msix. Thanks, Roger.
Hi Roger and Daniel P. Smith, On 2023/11/28 22:08, Roger Pau Monné wrote: > On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >> When a device has been reset on dom0 side, the vpci on Xen >> side won't get notification, so the cached state in vpci is >> all out of date compare with the real device state. >> To solve that problem, this patch add new hypercall to clear >> all vpci device state. And when reset device happens on dom0 >> side, dom0 can call this hypercall to notify vpci. >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> --- >> xen/arch/x86/hvm/hypercall.c | 1 + >> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >> xen/drivers/vpci/vpci.c | 9 +++++++++ >> xen/include/public/physdev.h | 2 ++ >> xen/include/xen/pci.h | 1 + >> xen/include/xen/vpci.h | 6 ++++++ >> 7 files changed, 54 insertions(+) >> >> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c >> index eeb73e1aa5..6ad5b4d5f1 100644 >> --- a/xen/arch/x86/hvm/hypercall.c >> +++ b/xen/arch/x86/hvm/hypercall.c >> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> case PHYSDEVOP_pci_mmcfg_reserved: >> case PHYSDEVOP_pci_device_add: >> case PHYSDEVOP_pci_device_remove: >> + case PHYSDEVOP_pci_device_state_reset: >> case PHYSDEVOP_dbgp_op: >> if ( !is_hardware_domain(currd) ) >> return -ENOSYS; >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index 04d00c7c37..f871715585 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >> return ret; >> } >> >> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) > > You could use pci_sbdf_t here instead of 3 parameters. Will change in next version, thank you. > > I'm however unsure whether we really need this helper just to fetch > the pdev and call vpci_reset_device_state(), I think you could place > this logic directly in pci_physdev_op(). Unless there are plans to > use such logic outside of pci_physdev_op(). If I place the logic of pci_reset_device_state directly in pci_physdev_op. I think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I will change in next version. > >> +{ >> + struct pci_dev *pdev; >> + int ret = -ENODEV; > > Some XSM check should be introduced fro this operation, as none of the > existing ones is suitable. See xsm_resource_unplug_pci() for example. > > xsm_resource_reset_state_pci() or some such I would assume. I don't know what I should do in XSM function(assume it is xsm_resource_reset_state_pci). Hi Daniel P. Smith, could you please give me some suggestions? Just to check the XSM_PRIV action? > > (adding the XSM maintainer for feedback). > >> + >> + pcidevs_lock(); >> + >> + pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn)); >> + if ( !pdev ) >> + goto error; >> + >> + ret = vpci_reset_device_state(pdev); >> + if (ret) >> + printk(XENLOG_ERR "PCI reset device %pp state failed\n", &pdev->sbdf); >> + >> +error: >> + pcidevs_unlock(); >> + >> + return ret; >> +} >> + >> /* Caller should hold the pcidevs_lock */ >> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, >> uint8_t devfn) >> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c >> index 42db3e6d13..cfdb545654 100644 >> --- a/xen/drivers/pci/physdev.c >> +++ b/xen/drivers/pci/physdev.c >> @@ -67,6 +67,20 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> break; >> } >> >> + case PHYSDEVOP_pci_device_state_reset: { >> + struct physdev_pci_device dev; >> + >> + if ( !is_pci_passthrough_enabled() ) >> + return -EOPNOTSUPP; >> + >> + ret = -EFAULT; >> + if ( copy_from_guest(&dev, arg, 1) != 0 ) >> + break; >> + >> + ret = pci_reset_device_state(dev.seg, dev.bus, dev.devfn); >> + break; >> + } >> + >> default: >> ret = -ENOSYS; >> break; >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >> index 3bec9a4153..e9c3eb1cd6 100644 >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -103,6 +103,15 @@ int vpci_add_handlers(struct pci_dev *pdev) >> >> return rc; >> } >> + >> +int vpci_reset_device_state(struct pci_dev *pdev) >> +{ >> + ASSERT(pcidevs_locked()); >> + >> + vpci_remove_device(pdev); >> + return vpci_add_handlers(pdev); >> +} >> + >> #endif /* __XEN__ */ >> >> static int vpci_register_cmp(const struct vpci_register *r1, >> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h >> index f0c0d4727c..4156948903 100644 >> --- a/xen/include/public/physdev.h >> +++ b/xen/include/public/physdev.h >> @@ -305,6 +305,8 @@ struct physdev_pci_device { >> typedef struct physdev_pci_device physdev_pci_device_t; >> DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t); >> >> +#define PHYSDEVOP_pci_device_state_reset 32 > > This needs some comment in order to explain the expected behaviour, > and might be better placed a bit up after PHYSDEVOP_release_msix. I will add some comment and move it closer to PHYSDEVOP_release_msix in next version. Thank you. > > Thanks, Roger.
On Thu, Nov 30, 2023 at 06:22:28AM +0000, Chen, Jiqian wrote: > Hi Roger and Daniel P. Smith, > > On 2023/11/28 22:08, Roger Pau Monné wrote: > > On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: > >> When a device has been reset on dom0 side, the vpci on Xen > >> side won't get notification, so the cached state in vpci is > >> all out of date compare with the real device state. > >> To solve that problem, this patch add new hypercall to clear > >> all vpci device state. And when reset device happens on dom0 > >> side, dom0 can call this hypercall to notify vpci. > >> > >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > >> Signed-off-by: Huang Rui <ray.huang@amd.com> > >> --- > >> xen/arch/x86/hvm/hypercall.c | 1 + > >> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ > >> xen/drivers/pci/physdev.c | 14 ++++++++++++++ > >> xen/drivers/vpci/vpci.c | 9 +++++++++ > >> xen/include/public/physdev.h | 2 ++ > >> xen/include/xen/pci.h | 1 + > >> xen/include/xen/vpci.h | 6 ++++++ > >> 7 files changed, 54 insertions(+) > >> > >> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c > >> index eeb73e1aa5..6ad5b4d5f1 100644 > >> --- a/xen/arch/x86/hvm/hypercall.c > >> +++ b/xen/arch/x86/hvm/hypercall.c > >> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > >> case PHYSDEVOP_pci_mmcfg_reserved: > >> case PHYSDEVOP_pci_device_add: > >> case PHYSDEVOP_pci_device_remove: > >> + case PHYSDEVOP_pci_device_state_reset: > >> case PHYSDEVOP_dbgp_op: > >> if ( !is_hardware_domain(currd) ) > >> return -ENOSYS; > >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > >> index 04d00c7c37..f871715585 100644 > >> --- a/xen/drivers/passthrough/pci.c > >> +++ b/xen/drivers/passthrough/pci.c > >> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) > >> return ret; > >> } > >> > >> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) > > > > You could use pci_sbdf_t here instead of 3 parameters. > Will change in next version, thank you. > > > > > I'm however unsure whether we really need this helper just to fetch > > the pdev and call vpci_reset_device_state(), I think you could place > > this logic directly in pci_physdev_op(). Unless there are plans to > > use such logic outside of pci_physdev_op(). > If I place the logic of pci_reset_device_state directly in pci_physdev_op. I think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I will change in next version. Just include xen/vpci.h in drivers/pci/physdev.c AFAICT. Thanks, Roger.
On 2023/11/30 19:52, Roger Pau Monné wrote: > On Thu, Nov 30, 2023 at 06:22:28AM +0000, Chen, Jiqian wrote: >> Hi Roger and Daniel P. Smith, >> >> On 2023/11/28 22:08, Roger Pau Monné wrote: >>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>>> When a device has been reset on dom0 side, the vpci on Xen >>>> side won't get notification, so the cached state in vpci is >>>> all out of date compare with the real device state. >>>> To solve that problem, this patch add new hypercall to clear >>>> all vpci device state. And when reset device happens on dom0 >>>> side, dom0 can call this hypercall to notify vpci. >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> xen/arch/x86/hvm/hypercall.c | 1 + >>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>>> xen/include/public/physdev.h | 2 ++ >>>> xen/include/xen/pci.h | 1 + >>>> xen/include/xen/vpci.h | 6 ++++++ >>>> 7 files changed, 54 insertions(+) >>>> >>>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c >>>> index eeb73e1aa5..6ad5b4d5f1 100644 >>>> --- a/xen/arch/x86/hvm/hypercall.c >>>> +++ b/xen/arch/x86/hvm/hypercall.c >>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> case PHYSDEVOP_pci_mmcfg_reserved: >>>> case PHYSDEVOP_pci_device_add: >>>> case PHYSDEVOP_pci_device_remove: >>>> + case PHYSDEVOP_pci_device_state_reset: >>>> case PHYSDEVOP_dbgp_op: >>>> if ( !is_hardware_domain(currd) ) >>>> return -ENOSYS; >>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>> index 04d00c7c37..f871715585 100644 >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>>> return ret; >>>> } >>>> >>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >>> >>> You could use pci_sbdf_t here instead of 3 parameters. >> Will change in next version, thank you. >> >>> >>> I'm however unsure whether we really need this helper just to fetch >>> the pdev and call vpci_reset_device_state(), I think you could place >>> this logic directly in pci_physdev_op(). Unless there are plans to >>> use such logic outside of pci_physdev_op(). >> If I place the logic of pci_reset_device_state directly in pci_physdev_op. I think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I will change in next version. > > Just include xen/vpci.h in drivers/pci/physdev.c AFAICT. Ok, I will change it in next version. Thanks. > > Thanks, Roger.
On 11/30/23 01:22, Chen, Jiqian wrote: > Hi Roger and Daniel P. Smith, > > On 2023/11/28 22:08, Roger Pau Monné wrote: >> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>> When a device has been reset on dom0 side, the vpci on Xen >>> side won't get notification, so the cached state in vpci is >>> all out of date compare with the real device state. >>> To solve that problem, this patch add new hypercall to clear >>> all vpci device state. And when reset device happens on dom0 >>> side, dom0 can call this hypercall to notify vpci. >>> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> --- >>> xen/arch/x86/hvm/hypercall.c | 1 + >>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>> xen/include/public/physdev.h | 2 ++ >>> xen/include/xen/pci.h | 1 + >>> xen/include/xen/vpci.h | 6 ++++++ >>> 7 files changed, 54 insertions(+) >>> >>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c >>> index eeb73e1aa5..6ad5b4d5f1 100644 >>> --- a/xen/arch/x86/hvm/hypercall.c >>> +++ b/xen/arch/x86/hvm/hypercall.c >>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> case PHYSDEVOP_pci_mmcfg_reserved: >>> case PHYSDEVOP_pci_device_add: >>> case PHYSDEVOP_pci_device_remove: >>> + case PHYSDEVOP_pci_device_state_reset: >>> case PHYSDEVOP_dbgp_op: >>> if ( !is_hardware_domain(currd) ) >>> return -ENOSYS; >>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>> index 04d00c7c37..f871715585 100644 >>> --- a/xen/drivers/passthrough/pci.c >>> +++ b/xen/drivers/passthrough/pci.c >>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>> return ret; >>> } >>> >>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >> >> You could use pci_sbdf_t here instead of 3 parameters. > Will change in next version, thank you. > >> >> I'm however unsure whether we really need this helper just to fetch >> the pdev and call vpci_reset_device_state(), I think you could place >> this logic directly in pci_physdev_op(). Unless there are plans to >> use such logic outside of pci_physdev_op(). > If I place the logic of pci_reset_device_state directly in pci_physdev_op. I think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I will change in next version. > >> >>> +{ >>> + struct pci_dev *pdev; >>> + int ret = -ENODEV; >> >> Some XSM check should be introduced fro this operation, as none of the >> existing ones is suitable. See xsm_resource_unplug_pci() for example. >> >> xsm_resource_reset_state_pci() or some such I would assume. > I don't know what I should do in XSM function(assume it is xsm_resource_reset_state_pci). > Hi Daniel P. Smith, could you please give me some suggestions? > Just to check the XSM_PRIV action? > Roger, thank you for seeing this and adding me in! Jiqian, I just wanted to let you know I have seen your post but I have been a little tied up this week. Just with the cursory look, I think Roger is suggesting a new XSM check/hook is warranted. If you would like to attempt at writing the dummy policy side, mimic xsm_resource_plug_pci in xen/include/xsm/dummy.h and xen/include/xsm/xsm.h, then I can look at handling the FLASK portion next week and provide it to you for inclusion into the series. If you are not comfortable with it, I can look at the whole thing next week. Just let me know what you would be comfortable with. v/r, dps
On 11/30/23 07:25, Daniel P. Smith wrote: > On 11/30/23 01:22, Chen, Jiqian wrote: >> Hi Roger and Daniel P. Smith, >> >> On 2023/11/28 22:08, Roger Pau Monné wrote: >>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>>> When a device has been reset on dom0 side, the vpci on Xen >>>> side won't get notification, so the cached state in vpci is >>>> all out of date compare with the real device state. >>>> To solve that problem, this patch add new hypercall to clear >>>> all vpci device state. And when reset device happens on dom0 >>>> side, dom0 can call this hypercall to notify vpci. >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> xen/arch/x86/hvm/hypercall.c | 1 + >>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>>> xen/include/public/physdev.h | 2 ++ >>>> xen/include/xen/pci.h | 1 + >>>> xen/include/xen/vpci.h | 6 ++++++ >>>> 7 files changed, 54 insertions(+) >>>> >>>> diff --git a/xen/arch/x86/hvm/hypercall.c >>>> b/xen/arch/x86/hvm/hypercall.c >>>> index eeb73e1aa5..6ad5b4d5f1 100644 >>>> --- a/xen/arch/x86/hvm/hypercall.c >>>> +++ b/xen/arch/x86/hvm/hypercall.c >>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, >>>> XEN_GUEST_HANDLE_PARAM(void) arg) >>>> case PHYSDEVOP_pci_mmcfg_reserved: >>>> case PHYSDEVOP_pci_device_add: >>>> case PHYSDEVOP_pci_device_remove: >>>> + case PHYSDEVOP_pci_device_state_reset: >>>> case PHYSDEVOP_dbgp_op: >>>> if ( !is_hardware_domain(currd) ) >>>> return -ENOSYS; >>>> diff --git a/xen/drivers/passthrough/pci.c >>>> b/xen/drivers/passthrough/pci.c >>>> index 04d00c7c37..f871715585 100644 >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>>> return ret; >>>> } >>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >>> >>> You could use pci_sbdf_t here instead of 3 parameters. >> Will change in next version, thank you. >> >>> >>> I'm however unsure whether we really need this helper just to fetch >>> the pdev and call vpci_reset_device_state(), I think you could place >>> this logic directly in pci_physdev_op(). Unless there are plans to >>> use such logic outside of pci_physdev_op(). >> If I place the logic of pci_reset_device_state directly in >> pci_physdev_op. I think I need to declare vpci_reset_device_state in >> pci.h? If it is suitable, I will change in next version. >> >>> >>>> +{ >>>> + struct pci_dev *pdev; >>>> + int ret = -ENODEV; >>> >>> Some XSM check should be introduced fro this operation, as none of the >>> existing ones is suitable. See xsm_resource_unplug_pci() for example. >>> >>> xsm_resource_reset_state_pci() or some such I would assume. >> I don't know what I should do in XSM function(assume it is >> xsm_resource_reset_state_pci). >> Hi Daniel P. Smith, could you please give me some suggestions? >> Just to check the XSM_PRIV action? >> > > Roger, thank you for seeing this and adding me in! > > Jiqian, I just wanted to let you know I have seen your post but I have > been a little tied up this week. Just with the cursory look, I think > Roger is suggesting a new XSM check/hook is warranted. > > If you would like to attempt at writing the dummy policy side, mimic > xsm_resource_plug_pci in xen/include/xsm/dummy.h and > xen/include/xsm/xsm.h, then I can look at handling the FLASK portion > next week and provide it to you for inclusion into the series. If you > are not comfortable with it, I can look at the whole thing next week. > Just let me know what you would be comfortable with. Apologies, thinking about for a moment and was thinking the hook should be called xsm_resource_config_pci. I would reset as a config operation and there might be new ones in the future. I do not believe there is a need to have fine grain access control down to individual config operation, thus they could all be captured under this one hook. Roger, what are your thoughts about this, in particular how you see vpci evolving? v/r, dps
On 2023/11/30 20:25, Daniel P. Smith wrote: > On 11/30/23 01:22, Chen, Jiqian wrote: >> Hi Roger and Daniel P. Smith, >> >> On 2023/11/28 22:08, Roger Pau Monné wrote: >>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>>> When a device has been reset on dom0 side, the vpci on Xen >>>> side won't get notification, so the cached state in vpci is >>>> all out of date compare with the real device state. >>>> To solve that problem, this patch add new hypercall to clear >>>> all vpci device state. And when reset device happens on dom0 >>>> side, dom0 can call this hypercall to notify vpci. >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> xen/arch/x86/hvm/hypercall.c | 1 + >>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>>> xen/include/public/physdev.h | 2 ++ >>>> xen/include/xen/pci.h | 1 + >>>> xen/include/xen/vpci.h | 6 ++++++ >>>> 7 files changed, 54 insertions(+) >>>> >>>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c >>>> index eeb73e1aa5..6ad5b4d5f1 100644 >>>> --- a/xen/arch/x86/hvm/hypercall.c >>>> +++ b/xen/arch/x86/hvm/hypercall.c >>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> case PHYSDEVOP_pci_mmcfg_reserved: >>>> case PHYSDEVOP_pci_device_add: >>>> case PHYSDEVOP_pci_device_remove: >>>> + case PHYSDEVOP_pci_device_state_reset: >>>> case PHYSDEVOP_dbgp_op: >>>> if ( !is_hardware_domain(currd) ) >>>> return -ENOSYS; >>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>>> index 04d00c7c37..f871715585 100644 >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>>> return ret; >>>> } >>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >>> >>> You could use pci_sbdf_t here instead of 3 parameters. >> Will change in next version, thank you. >> >>> >>> I'm however unsure whether we really need this helper just to fetch >>> the pdev and call vpci_reset_device_state(), I think you could place >>> this logic directly in pci_physdev_op(). Unless there are plans to >>> use such logic outside of pci_physdev_op(). >> If I place the logic of pci_reset_device_state directly in pci_physdev_op. I think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I will change in next version. >> >>> >>>> +{ >>>> + struct pci_dev *pdev; >>>> + int ret = -ENODEV; >>> >>> Some XSM check should be introduced fro this operation, as none of the >>> existing ones is suitable. See xsm_resource_unplug_pci() for example. >>> >>> xsm_resource_reset_state_pci() or some such I would assume. >> I don't know what I should do in XSM function(assume it is xsm_resource_reset_state_pci). >> Hi Daniel P. Smith, could you please give me some suggestions? >> Just to check the XSM_PRIV action? >> > > Roger, thank you for seeing this and adding me in! > > Jiqian, I just wanted to let you know I have seen your post but I have been a little tied up this week. Just with the cursory look, I think Roger is suggesting a new XSM check/hook is warranted. > > If you would like to attempt at writing the dummy policy side, mimic xsm_resource_plug_pci in xen/include/xsm/dummy.h and xen/include/xsm/xsm.h, then I can look at handling the FLASK portion next week and provide it to you for inclusion into the series. If you are not comfortable with it, I can look at the whole thing next week. Just let me know what you would be comfortable with. I will refer to xsm_resource_unplug_pci and try to add dummy policy in next version. I am looking forward your review and guide. Thank you very much! > > v/r, > dps
On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote: > On 11/30/23 07:25, Daniel P. Smith wrote: > > On 11/30/23 01:22, Chen, Jiqian wrote: > > > Hi Roger and Daniel P. Smith, > > > > > > On 2023/11/28 22:08, Roger Pau Monné wrote: > > > > On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: > > > > > When a device has been reset on dom0 side, the vpci on Xen > > > > > side won't get notification, so the cached state in vpci is > > > > > all out of date compare with the real device state. > > > > > To solve that problem, this patch add new hypercall to clear > > > > > all vpci device state. And when reset device happens on dom0 > > > > > side, dom0 can call this hypercall to notify vpci. > > > > > > > > > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > > --- > > > > > xen/arch/x86/hvm/hypercall.c | 1 + > > > > > xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ > > > > > xen/drivers/pci/physdev.c | 14 ++++++++++++++ > > > > > xen/drivers/vpci/vpci.c | 9 +++++++++ > > > > > xen/include/public/physdev.h | 2 ++ > > > > > xen/include/xen/pci.h | 1 + > > > > > xen/include/xen/vpci.h | 6 ++++++ > > > > > 7 files changed, 54 insertions(+) > > > > > > > > > > diff --git a/xen/arch/x86/hvm/hypercall.c > > > > > b/xen/arch/x86/hvm/hypercall.c > > > > > index eeb73e1aa5..6ad5b4d5f1 100644 > > > > > --- a/xen/arch/x86/hvm/hypercall.c > > > > > +++ b/xen/arch/x86/hvm/hypercall.c > > > > > @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, > > > > > XEN_GUEST_HANDLE_PARAM(void) arg) > > > > > case PHYSDEVOP_pci_mmcfg_reserved: > > > > > case PHYSDEVOP_pci_device_add: > > > > > case PHYSDEVOP_pci_device_remove: > > > > > + case PHYSDEVOP_pci_device_state_reset: > > > > > case PHYSDEVOP_dbgp_op: > > > > > if ( !is_hardware_domain(currd) ) > > > > > return -ENOSYS; > > > > > diff --git a/xen/drivers/passthrough/pci.c > > > > > b/xen/drivers/passthrough/pci.c > > > > > index 04d00c7c37..f871715585 100644 > > > > > --- a/xen/drivers/passthrough/pci.c > > > > > +++ b/xen/drivers/passthrough/pci.c > > > > > @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) > > > > > return ret; > > > > > } > > > > > +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) > > > > > > > > You could use pci_sbdf_t here instead of 3 parameters. > > > Will change in next version, thank you. > > > > > > > > > > > I'm however unsure whether we really need this helper just to fetch > > > > the pdev and call vpci_reset_device_state(), I think you could place > > > > this logic directly in pci_physdev_op(). Unless there are plans to > > > > use such logic outside of pci_physdev_op(). > > > If I place the logic of pci_reset_device_state directly in > > > pci_physdev_op. I think I need to declare vpci_reset_device_state in > > > pci.h? If it is suitable, I will change in next version. > > > > > > > > > > > > +{ > > > > > + struct pci_dev *pdev; > > > > > + int ret = -ENODEV; > > > > > > > > Some XSM check should be introduced fro this operation, as none of the > > > > existing ones is suitable. See xsm_resource_unplug_pci() for example. > > > > > > > > xsm_resource_reset_state_pci() or some such I would assume. > > > I don't know what I should do in XSM function(assume it is > > > xsm_resource_reset_state_pci). > > > Hi Daniel P. Smith, could you please give me some suggestions? > > > Just to check the XSM_PRIV action? > > > > > > > Roger, thank you for seeing this and adding me in! > > > > Jiqian, I just wanted to let you know I have seen your post but I have > > been a little tied up this week. Just with the cursory look, I think > > Roger is suggesting a new XSM check/hook is warranted. > > > > If you would like to attempt at writing the dummy policy side, mimic > > xsm_resource_plug_pci in xen/include/xsm/dummy.h and > > xen/include/xsm/xsm.h, then I can look at handling the FLASK portion > > next week and provide it to you for inclusion into the series. If you > > are not comfortable with it, I can look at the whole thing next week. > > Just let me know what you would be comfortable with. > > Apologies, thinking about for a moment and was thinking the hook should be > called xsm_resource_config_pci. I would reset as a config operation and > there might be new ones in the future. I do not believe there is a need to > have fine grain access control down to individual config operation, thus > they could all be captured under this one hook. Roger, what are your > thoughts about this, in particular how you see vpci evolving? So the configuration space reset should only be done by the domain that's also capable of triggering the physical device reset, usually the hardware domain. I don't think it's possible ATM to allow a domain different than the hardware domain to perform a PCI reset, as doing it requires unmediated access to the PCI config space. So resetting the vPCI state should either be limited to the hardware domain, or to a pci reset capability explicitly (xsm_resource_reset_pci or some such?). Or maybe xsm_resource_config_pci if that denotes unmediated access to the PCI config space. Thanks, Roger.
Hi Daniel P. Smith, On 2023/11/30 22:52, Roger Pau Monné wrote: > On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote: >> On 11/30/23 07:25, Daniel P. Smith wrote: >>> On 11/30/23 01:22, Chen, Jiqian wrote: >>>> Hi Roger and Daniel P. Smith, >>>> >>>> On 2023/11/28 22:08, Roger Pau Monné wrote: >>>>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>>>>> When a device has been reset on dom0 side, the vpci on Xen >>>>>> side won't get notification, so the cached state in vpci is >>>>>> all out of date compare with the real device state. >>>>>> To solve that problem, this patch add new hypercall to clear >>>>>> all vpci device state. And when reset device happens on dom0 >>>>>> side, dom0 can call this hypercall to notify vpci. >>>>>> >>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>> --- >>>>>> xen/arch/x86/hvm/hypercall.c | 1 + >>>>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>>>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>>>>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>>>>> xen/include/public/physdev.h | 2 ++ >>>>>> xen/include/xen/pci.h | 1 + >>>>>> xen/include/xen/vpci.h | 6 ++++++ >>>>>> 7 files changed, 54 insertions(+) >>>>>> >>>>>> diff --git a/xen/arch/x86/hvm/hypercall.c >>>>>> b/xen/arch/x86/hvm/hypercall.c >>>>>> index eeb73e1aa5..6ad5b4d5f1 100644 >>>>>> --- a/xen/arch/x86/hvm/hypercall.c >>>>>> +++ b/xen/arch/x86/hvm/hypercall.c >>>>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, >>>>>> XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>> case PHYSDEVOP_pci_mmcfg_reserved: >>>>>> case PHYSDEVOP_pci_device_add: >>>>>> case PHYSDEVOP_pci_device_remove: >>>>>> + case PHYSDEVOP_pci_device_state_reset: >>>>>> case PHYSDEVOP_dbgp_op: >>>>>> if ( !is_hardware_domain(currd) ) >>>>>> return -ENOSYS; >>>>>> diff --git a/xen/drivers/passthrough/pci.c >>>>>> b/xen/drivers/passthrough/pci.c >>>>>> index 04d00c7c37..f871715585 100644 >>>>>> --- a/xen/drivers/passthrough/pci.c >>>>>> +++ b/xen/drivers/passthrough/pci.c >>>>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>>>>> return ret; >>>>>> } >>>>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >>>>> >>>>> You could use pci_sbdf_t here instead of 3 parameters. >>>> Will change in next version, thank you. >>>> >>>>> >>>>> I'm however unsure whether we really need this helper just to fetch >>>>> the pdev and call vpci_reset_device_state(), I think you could place >>>>> this logic directly in pci_physdev_op(). Unless there are plans to >>>>> use such logic outside of pci_physdev_op(). >>>> If I place the logic of pci_reset_device_state directly in >>>> pci_physdev_op. I think I need to declare vpci_reset_device_state in >>>> pci.h? If it is suitable, I will change in next version. >>>> >>>>> >>>>>> +{ >>>>>> + struct pci_dev *pdev; >>>>>> + int ret = -ENODEV; >>>>> >>>>> Some XSM check should be introduced fro this operation, as none of the >>>>> existing ones is suitable. See xsm_resource_unplug_pci() for example. >>>>> >>>>> xsm_resource_reset_state_pci() or some such I would assume. >>>> I don't know what I should do in XSM function(assume it is >>>> xsm_resource_reset_state_pci). >>>> Hi Daniel P. Smith, could you please give me some suggestions? >>>> Just to check the XSM_PRIV action? >>>> >>> >>> Roger, thank you for seeing this and adding me in! >>> >>> Jiqian, I just wanted to let you know I have seen your post but I have >>> been a little tied up this week. Just with the cursory look, I think >>> Roger is suggesting a new XSM check/hook is warranted. >>> >>> If you would like to attempt at writing the dummy policy side, mimic >>> xsm_resource_plug_pci in xen/include/xsm/dummy.h and >>> xen/include/xsm/xsm.h, then I can look at handling the FLASK portion >>> next week and provide it to you for inclusion into the series. If you >>> are not comfortable with it, I can look at the whole thing next week. >>> Just let me know what you would be comfortable with. >> >> Apologies, thinking about for a moment and was thinking the hook should be >> called xsm_resource_config_pci. I would reset as a config operation and >> there might be new ones in the future. I do not believe there is a need to >> have fine grain access control down to individual config operation, thus >> they could all be captured under this one hook. Roger, what are your >> thoughts about this, in particular how you see vpci evolving? > > So the configuration space reset should only be done by the domain > that's also capable of triggering the physical device reset, usually > the hardware domain. I don't think it's possible ATM to allow a > domain different than the hardware domain to perform a PCI reset, as > doing it requires unmediated access to the PCI config space. > > So resetting the vPCI state should either be limited to the hardware > domain, or to a pci reset capability explicitly > (xsm_resource_reset_pci or some such?). Or maybe > xsm_resource_config_pci if that denotes unmediated access to the PCI > config space. > > Thanks, Roger. Is it like below that I need to add for XSM? diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 9d7519eb89..81ceaf145f 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -937,6 +937,10 @@ int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) struct pci_dev *pdev; int ret = -ENODEV; + ret = xsm_resource_config_pci(XSM_PRIV, (seg << 16) | (bus << 8) | devfn); + if ( ret ) + return ret; + pcidevs_lock(); pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn)); diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 8671af1ba4..bcaff99b23 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -472,6 +472,13 @@ static XSM_INLINE int cf_check xsm_resource_setup_pci( return xsm_default_action(action, current->domain, NULL); } +static XSM_INLINE int cf_check xsm_resource_config_pci( + XSM_DEFAULT_ARG uint32_t machine_bdf) +{ + XSM_ASSERT_ACTION(XSM_PRIV); + return xsm_default_action(action, current->domain, NULL); +} + static XSM_INLINE int cf_check xsm_resource_setup_gsi(XSM_DEFAULT_ARG int gsi) { XSM_ASSERT_ACTION(XSM_PRIV); diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 8dad03fd3d..1cb16b00de 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -571,6 +571,12 @@ static inline int xsm_resource_setup_pci( return alternative_call(xsm_ops.resource_setup_pci, machine_bdf); } +static inline int xsm_resource_config_pci( + xsm_default_t def, uint32_t machine_bdf) +{ + return alternative_call(xsm_ops.resource_config_pci, machine_bdf); +} + static inline int xsm_resource_setup_gsi(xsm_default_t def, int gsi) { return alternative_call(xsm_ops.resource_setup_gsi, gsi); diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index e6ffa948f7..7a289ba5d8 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -91,6 +91,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = { .resource_plug_pci = xsm_resource_plug_pci, .resource_unplug_pci = xsm_resource_unplug_pci, .resource_setup_pci = xsm_resource_setup_pci, + .resource_config_pci = xsm_resource_config_pci, .resource_setup_gsi = xsm_resource_setup_gsi,
On Mon, Dec 04, 2023 at 06:57:03AM +0000, Chen, Jiqian wrote: > Hi Daniel P. Smith, > > On 2023/11/30 22:52, Roger Pau Monné wrote: > > On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote: > >> On 11/30/23 07:25, Daniel P. Smith wrote: > >>> On 11/30/23 01:22, Chen, Jiqian wrote: > >>>> Hi Roger and Daniel P. Smith, > >>>> > >>>> On 2023/11/28 22:08, Roger Pau Monné wrote: > >>>>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: > >>>>>> When a device has been reset on dom0 side, the vpci on Xen > >>>>>> side won't get notification, so the cached state in vpci is > >>>>>> all out of date compare with the real device state. > >>>>>> To solve that problem, this patch add new hypercall to clear > >>>>>> all vpci device state. And when reset device happens on dom0 > >>>>>> side, dom0 can call this hypercall to notify vpci. > >>>>>> > >>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > >>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> > >>>>>> --- > >>>>>> xen/arch/x86/hvm/hypercall.c | 1 + > >>>>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ > >>>>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ > >>>>>> xen/drivers/vpci/vpci.c | 9 +++++++++ > >>>>>> xen/include/public/physdev.h | 2 ++ > >>>>>> xen/include/xen/pci.h | 1 + > >>>>>> xen/include/xen/vpci.h | 6 ++++++ > >>>>>> 7 files changed, 54 insertions(+) > >>>>>> > >>>>>> diff --git a/xen/arch/x86/hvm/hypercall.c > >>>>>> b/xen/arch/x86/hvm/hypercall.c > >>>>>> index eeb73e1aa5..6ad5b4d5f1 100644 > >>>>>> --- a/xen/arch/x86/hvm/hypercall.c > >>>>>> +++ b/xen/arch/x86/hvm/hypercall.c > >>>>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, > >>>>>> XEN_GUEST_HANDLE_PARAM(void) arg) > >>>>>> case PHYSDEVOP_pci_mmcfg_reserved: > >>>>>> case PHYSDEVOP_pci_device_add: > >>>>>> case PHYSDEVOP_pci_device_remove: > >>>>>> + case PHYSDEVOP_pci_device_state_reset: > >>>>>> case PHYSDEVOP_dbgp_op: > >>>>>> if ( !is_hardware_domain(currd) ) > >>>>>> return -ENOSYS; > >>>>>> diff --git a/xen/drivers/passthrough/pci.c > >>>>>> b/xen/drivers/passthrough/pci.c > >>>>>> index 04d00c7c37..f871715585 100644 > >>>>>> --- a/xen/drivers/passthrough/pci.c > >>>>>> +++ b/xen/drivers/passthrough/pci.c > >>>>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) > >>>>>> return ret; > >>>>>> } > >>>>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) > >>>>> > >>>>> You could use pci_sbdf_t here instead of 3 parameters. > >>>> Will change in next version, thank you. > >>>> > >>>>> > >>>>> I'm however unsure whether we really need this helper just to fetch > >>>>> the pdev and call vpci_reset_device_state(), I think you could place > >>>>> this logic directly in pci_physdev_op(). Unless there are plans to > >>>>> use such logic outside of pci_physdev_op(). > >>>> If I place the logic of pci_reset_device_state directly in > >>>> pci_physdev_op. I think I need to declare vpci_reset_device_state in > >>>> pci.h? If it is suitable, I will change in next version. > >>>> > >>>>> > >>>>>> +{ > >>>>>> + struct pci_dev *pdev; > >>>>>> + int ret = -ENODEV; > >>>>> > >>>>> Some XSM check should be introduced fro this operation, as none of the > >>>>> existing ones is suitable. See xsm_resource_unplug_pci() for example. > >>>>> > >>>>> xsm_resource_reset_state_pci() or some such I would assume. > >>>> I don't know what I should do in XSM function(assume it is > >>>> xsm_resource_reset_state_pci). > >>>> Hi Daniel P. Smith, could you please give me some suggestions? > >>>> Just to check the XSM_PRIV action? > >>>> > >>> > >>> Roger, thank you for seeing this and adding me in! > >>> > >>> Jiqian, I just wanted to let you know I have seen your post but I have > >>> been a little tied up this week. Just with the cursory look, I think > >>> Roger is suggesting a new XSM check/hook is warranted. > >>> > >>> If you would like to attempt at writing the dummy policy side, mimic > >>> xsm_resource_plug_pci in xen/include/xsm/dummy.h and > >>> xen/include/xsm/xsm.h, then I can look at handling the FLASK portion > >>> next week and provide it to you for inclusion into the series. If you > >>> are not comfortable with it, I can look at the whole thing next week. > >>> Just let me know what you would be comfortable with. > >> > >> Apologies, thinking about for a moment and was thinking the hook should be > >> called xsm_resource_config_pci. I would reset as a config operation and > >> there might be new ones in the future. I do not believe there is a need to > >> have fine grain access control down to individual config operation, thus > >> they could all be captured under this one hook. Roger, what are your > >> thoughts about this, in particular how you see vpci evolving? > > > > So the configuration space reset should only be done by the domain > > that's also capable of triggering the physical device reset, usually > > the hardware domain. I don't think it's possible ATM to allow a > > domain different than the hardware domain to perform a PCI reset, as > > doing it requires unmediated access to the PCI config space. > > > > So resetting the vPCI state should either be limited to the hardware > > domain, or to a pci reset capability explicitly > > (xsm_resource_reset_pci or some such?). Or maybe > > xsm_resource_config_pci if that denotes unmediated access to the PCI > > config space. > > > > Thanks, Roger. > > Is it like below that I need to add for XSM? > diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c > index e6ffa948f7..7a289ba5d8 100644 > --- a/xen/xsm/dummy.c > +++ b/xen/xsm/dummy.c > @@ -91,6 +91,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = { > .resource_plug_pci = xsm_resource_plug_pci, > .resource_unplug_pci = xsm_resource_unplug_pci, > .resource_setup_pci = xsm_resource_setup_pci, > + .resource_config_pci = xsm_resource_config_pci, Now that I look at it, using the existing xsm_resource_setup_pci might be enough, no need to introduce a xsm_resource_config_pci. Thanks, Roger.
On 2023/12/4 19:10, Roger Pau Monné wrote: > On Mon, Dec 04, 2023 at 06:57:03AM +0000, Chen, Jiqian wrote: >> Hi Daniel P. Smith, >> >> On 2023/11/30 22:52, Roger Pau Monné wrote: >>> On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote: >>>> On 11/30/23 07:25, Daniel P. Smith wrote: >>>>> On 11/30/23 01:22, Chen, Jiqian wrote: >>>>>> Hi Roger and Daniel P. Smith, >>>>>> >>>>>> On 2023/11/28 22:08, Roger Pau Monné wrote: >>>>>>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>>>>>>> When a device has been reset on dom0 side, the vpci on Xen >>>>>>>> side won't get notification, so the cached state in vpci is >>>>>>>> all out of date compare with the real device state. >>>>>>>> To solve that problem, this patch add new hypercall to clear >>>>>>>> all vpci device state. And when reset device happens on dom0 >>>>>>>> side, dom0 can call this hypercall to notify vpci. >>>>>>>> >>>>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>>>> --- >>>>>>>> xen/arch/x86/hvm/hypercall.c | 1 + >>>>>>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>>>>>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>>>>>>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>>>>>>> xen/include/public/physdev.h | 2 ++ >>>>>>>> xen/include/xen/pci.h | 1 + >>>>>>>> xen/include/xen/vpci.h | 6 ++++++ >>>>>>>> 7 files changed, 54 insertions(+) >>>>>>>> >>>>>>>> diff --git a/xen/arch/x86/hvm/hypercall.c >>>>>>>> b/xen/arch/x86/hvm/hypercall.c >>>>>>>> index eeb73e1aa5..6ad5b4d5f1 100644 >>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c >>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c >>>>>>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, >>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>>>> case PHYSDEVOP_pci_mmcfg_reserved: >>>>>>>> case PHYSDEVOP_pci_device_add: >>>>>>>> case PHYSDEVOP_pci_device_remove: >>>>>>>> + case PHYSDEVOP_pci_device_state_reset: >>>>>>>> case PHYSDEVOP_dbgp_op: >>>>>>>> if ( !is_hardware_domain(currd) ) >>>>>>>> return -ENOSYS; >>>>>>>> diff --git a/xen/drivers/passthrough/pci.c >>>>>>>> b/xen/drivers/passthrough/pci.c >>>>>>>> index 04d00c7c37..f871715585 100644 >>>>>>>> --- a/xen/drivers/passthrough/pci.c >>>>>>>> +++ b/xen/drivers/passthrough/pci.c >>>>>>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>>>>>>> return ret; >>>>>>>> } >>>>>>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >>>>>>> >>>>>>> You could use pci_sbdf_t here instead of 3 parameters. >>>>>> Will change in next version, thank you. >>>>>> >>>>>>> >>>>>>> I'm however unsure whether we really need this helper just to fetch >>>>>>> the pdev and call vpci_reset_device_state(), I think you could place >>>>>>> this logic directly in pci_physdev_op(). Unless there are plans to >>>>>>> use such logic outside of pci_physdev_op(). >>>>>> If I place the logic of pci_reset_device_state directly in >>>>>> pci_physdev_op. I think I need to declare vpci_reset_device_state in >>>>>> pci.h? If it is suitable, I will change in next version. >>>>>> >>>>>>> >>>>>>>> +{ >>>>>>>> + struct pci_dev *pdev; >>>>>>>> + int ret = -ENODEV; >>>>>>> >>>>>>> Some XSM check should be introduced fro this operation, as none of the >>>>>>> existing ones is suitable. See xsm_resource_unplug_pci() for example. >>>>>>> >>>>>>> xsm_resource_reset_state_pci() or some such I would assume. >>>>>> I don't know what I should do in XSM function(assume it is >>>>>> xsm_resource_reset_state_pci). >>>>>> Hi Daniel P. Smith, could you please give me some suggestions? >>>>>> Just to check the XSM_PRIV action? >>>>>> >>>>> >>>>> Roger, thank you for seeing this and adding me in! >>>>> >>>>> Jiqian, I just wanted to let you know I have seen your post but I have >>>>> been a little tied up this week. Just with the cursory look, I think >>>>> Roger is suggesting a new XSM check/hook is warranted. >>>>> >>>>> If you would like to attempt at writing the dummy policy side, mimic >>>>> xsm_resource_plug_pci in xen/include/xsm/dummy.h and >>>>> xen/include/xsm/xsm.h, then I can look at handling the FLASK portion >>>>> next week and provide it to you for inclusion into the series. If you >>>>> are not comfortable with it, I can look at the whole thing next week. >>>>> Just let me know what you would be comfortable with. >>>> >>>> Apologies, thinking about for a moment and was thinking the hook should be >>>> called xsm_resource_config_pci. I would reset as a config operation and >>>> there might be new ones in the future. I do not believe there is a need to >>>> have fine grain access control down to individual config operation, thus >>>> they could all be captured under this one hook. Roger, what are your >>>> thoughts about this, in particular how you see vpci evolving? >>> >>> So the configuration space reset should only be done by the domain >>> that's also capable of triggering the physical device reset, usually >>> the hardware domain. I don't think it's possible ATM to allow a >>> domain different than the hardware domain to perform a PCI reset, as >>> doing it requires unmediated access to the PCI config space. >>> >>> So resetting the vPCI state should either be limited to the hardware >>> domain, or to a pci reset capability explicitly >>> (xsm_resource_reset_pci or some such?). Or maybe >>> xsm_resource_config_pci if that denotes unmediated access to the PCI >>> config space. >>> >>> Thanks, Roger. >> >> Is it like below that I need to add for XSM? >> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c >> index e6ffa948f7..7a289ba5d8 100644 >> --- a/xen/xsm/dummy.c >> +++ b/xen/xsm/dummy.c >> @@ -91,6 +91,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = { >> .resource_plug_pci = xsm_resource_plug_pci, >> .resource_unplug_pci = xsm_resource_unplug_pci, >> .resource_setup_pci = xsm_resource_setup_pci, >> + .resource_config_pci = xsm_resource_config_pci, > > Now that I look at it, using the existing xsm_resource_setup_pci might > be enough, no need to introduce a xsm_resource_config_pci. Ok, thank you. I will add xsm_resource_setup_pci check into PHYSDEVOP_pci_device_state_reset next step. > > Thanks, Roger.
V/r, Daniel P. Smith Apertus Solutions, LLC On 12/4/23 06:10, Roger Pau Monné wrote: > On Mon, Dec 04, 2023 at 06:57:03AM +0000, Chen, Jiqian wrote: >> Hi Daniel P. Smith, >> >> On 2023/11/30 22:52, Roger Pau Monné wrote: >>> On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote: >>>> On 11/30/23 07:25, Daniel P. Smith wrote: >>>>> On 11/30/23 01:22, Chen, Jiqian wrote: >>>>>> Hi Roger and Daniel P. Smith, >>>>>> >>>>>> On 2023/11/28 22:08, Roger Pau Monné wrote: >>>>>>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote: >>>>>>>> When a device has been reset on dom0 side, the vpci on Xen >>>>>>>> side won't get notification, so the cached state in vpci is >>>>>>>> all out of date compare with the real device state. >>>>>>>> To solve that problem, this patch add new hypercall to clear >>>>>>>> all vpci device state. And when reset device happens on dom0 >>>>>>>> side, dom0 can call this hypercall to notify vpci. >>>>>>>> >>>>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>>>> --- >>>>>>>> xen/arch/x86/hvm/hypercall.c | 1 + >>>>>>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++ >>>>>>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++ >>>>>>>> xen/drivers/vpci/vpci.c | 9 +++++++++ >>>>>>>> xen/include/public/physdev.h | 2 ++ >>>>>>>> xen/include/xen/pci.h | 1 + >>>>>>>> xen/include/xen/vpci.h | 6 ++++++ >>>>>>>> 7 files changed, 54 insertions(+) >>>>>>>> >>>>>>>> diff --git a/xen/arch/x86/hvm/hypercall.c >>>>>>>> b/xen/arch/x86/hvm/hypercall.c >>>>>>>> index eeb73e1aa5..6ad5b4d5f1 100644 >>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c >>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c >>>>>>>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, >>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>>>> case PHYSDEVOP_pci_mmcfg_reserved: >>>>>>>> case PHYSDEVOP_pci_device_add: >>>>>>>> case PHYSDEVOP_pci_device_remove: >>>>>>>> + case PHYSDEVOP_pci_device_state_reset: >>>>>>>> case PHYSDEVOP_dbgp_op: >>>>>>>> if ( !is_hardware_domain(currd) ) >>>>>>>> return -ENOSYS; >>>>>>>> diff --git a/xen/drivers/passthrough/pci.c >>>>>>>> b/xen/drivers/passthrough/pci.c >>>>>>>> index 04d00c7c37..f871715585 100644 >>>>>>>> --- a/xen/drivers/passthrough/pci.c >>>>>>>> +++ b/xen/drivers/passthrough/pci.c >>>>>>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) >>>>>>>> return ret; >>>>>>>> } >>>>>>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) >>>>>>> >>>>>>> You could use pci_sbdf_t here instead of 3 parameters. >>>>>> Will change in next version, thank you. >>>>>> >>>>>>> >>>>>>> I'm however unsure whether we really need this helper just to fetch >>>>>>> the pdev and call vpci_reset_device_state(), I think you could place >>>>>>> this logic directly in pci_physdev_op(). Unless there are plans to >>>>>>> use such logic outside of pci_physdev_op(). >>>>>> If I place the logic of pci_reset_device_state directly in >>>>>> pci_physdev_op. I think I need to declare vpci_reset_device_state in >>>>>> pci.h? If it is suitable, I will change in next version. >>>>>> >>>>>>> >>>>>>>> +{ >>>>>>>> + struct pci_dev *pdev; >>>>>>>> + int ret = -ENODEV; >>>>>>> >>>>>>> Some XSM check should be introduced fro this operation, as none of the >>>>>>> existing ones is suitable. See xsm_resource_unplug_pci() for example. >>>>>>> >>>>>>> xsm_resource_reset_state_pci() or some such I would assume. >>>>>> I don't know what I should do in XSM function(assume it is >>>>>> xsm_resource_reset_state_pci). >>>>>> Hi Daniel P. Smith, could you please give me some suggestions? >>>>>> Just to check the XSM_PRIV action? >>>>>> >>>>> >>>>> Roger, thank you for seeing this and adding me in! >>>>> >>>>> Jiqian, I just wanted to let you know I have seen your post but I have >>>>> been a little tied up this week. Just with the cursory look, I think >>>>> Roger is suggesting a new XSM check/hook is warranted. >>>>> >>>>> If you would like to attempt at writing the dummy policy side, mimic >>>>> xsm_resource_plug_pci in xen/include/xsm/dummy.h and >>>>> xen/include/xsm/xsm.h, then I can look at handling the FLASK portion >>>>> next week and provide it to you for inclusion into the series. If you >>>>> are not comfortable with it, I can look at the whole thing next week. >>>>> Just let me know what you would be comfortable with. >>>> >>>> Apologies, thinking about for a moment and was thinking the hook should be >>>> called xsm_resource_config_pci. I would reset as a config operation and >>>> there might be new ones in the future. I do not believe there is a need to >>>> have fine grain access control down to individual config operation, thus >>>> they could all be captured under this one hook. Roger, what are your >>>> thoughts about this, in particular how you see vpci evolving? >>> >>> So the configuration space reset should only be done by the domain >>> that's also capable of triggering the physical device reset, usually >>> the hardware domain. I don't think it's possible ATM to allow a >>> domain different than the hardware domain to perform a PCI reset, as >>> doing it requires unmediated access to the PCI config space. >>> >>> So resetting the vPCI state should either be limited to the hardware >>> domain, or to a pci reset capability explicitly >>> (xsm_resource_reset_pci or some such?). Or maybe >>> xsm_resource_config_pci if that denotes unmediated access to the PCI >>> config space. >>> >>> Thanks, Roger. >> >> Is it like below that I need to add for XSM? >> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c >> index e6ffa948f7..7a289ba5d8 100644 >> --- a/xen/xsm/dummy.c >> +++ b/xen/xsm/dummy.c >> @@ -91,6 +91,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = { >> .resource_plug_pci = xsm_resource_plug_pci, >> .resource_unplug_pci = xsm_resource_unplug_pci, >> .resource_setup_pci = xsm_resource_setup_pci, >> + .resource_config_pci = xsm_resource_config_pci, > > Now that I look at it, using the existing xsm_resource_setup_pci might > be enough, no need to introduce a xsm_resource_config_pci. Ack.
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index eeb73e1aa5..6ad5b4d5f1 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case PHYSDEVOP_pci_mmcfg_reserved: case PHYSDEVOP_pci_device_add: case PHYSDEVOP_pci_device_remove: + case PHYSDEVOP_pci_device_state_reset: case PHYSDEVOP_dbgp_op: if ( !is_hardware_domain(currd) ) return -ENOSYS; diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 04d00c7c37..f871715585 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) return ret; } +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn) +{ + struct pci_dev *pdev; + int ret = -ENODEV; + + pcidevs_lock(); + + pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn)); + if ( !pdev ) + goto error; + + ret = vpci_reset_device_state(pdev); + if (ret) + printk(XENLOG_ERR "PCI reset device %pp state failed\n", &pdev->sbdf); + +error: + pcidevs_unlock(); + + return ret; +} + /* Caller should hold the pcidevs_lock */ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus, uint8_t devfn) diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c index 42db3e6d13..cfdb545654 100644 --- a/xen/drivers/pci/physdev.c +++ b/xen/drivers/pci/physdev.c @@ -67,6 +67,20 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) break; } + case PHYSDEVOP_pci_device_state_reset: { + struct physdev_pci_device dev; + + if ( !is_pci_passthrough_enabled() ) + return -EOPNOTSUPP; + + ret = -EFAULT; + if ( copy_from_guest(&dev, arg, 1) != 0 ) + break; + + ret = pci_reset_device_state(dev.seg, dev.bus, dev.devfn); + break; + } + default: ret = -ENOSYS; break; diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 3bec9a4153..e9c3eb1cd6 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -103,6 +103,15 @@ int vpci_add_handlers(struct pci_dev *pdev) return rc; } + +int vpci_reset_device_state(struct pci_dev *pdev) +{ + ASSERT(pcidevs_locked()); + + vpci_remove_device(pdev); + return vpci_add_handlers(pdev); +} + #endif /* __XEN__ */ static int vpci_register_cmp(const struct vpci_register *r1, diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h index f0c0d4727c..4156948903 100644 --- a/xen/include/public/physdev.h +++ b/xen/include/public/physdev.h @@ -305,6 +305,8 @@ struct physdev_pci_device { typedef struct physdev_pci_device physdev_pci_device_t; DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t); +#define PHYSDEVOP_pci_device_state_reset 32 + #define PHYSDEVOP_DBGP_RESET_PREPARE 1 #define PHYSDEVOP_DBGP_RESET_DONE 2 diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 50d7dfb2a2..a0c28c5e6c 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -186,6 +186,7 @@ const unsigned long *pci_get_ro_map(u16 seg); int pci_add_device(u16 seg, u8 bus, u8 devfn, const struct pci_dev_info *info, nodeid_t node); int pci_remove_device(u16 seg, u8 bus, u8 devfn); +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn); int pci_ro_device(int seg, int bus, int devfn); int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn); struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf); diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index d743d96a10..e2d6cd5fa3 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -30,6 +30,7 @@ int __must_check vpci_add_handlers(struct pci_dev *pdev); /* Remove all handlers and free vpci related structures. */ void vpci_remove_device(struct pci_dev *pdev); +int vpci_reset_device_state(struct pci_dev *pdev); /* Add/remove a register handler. */ int __must_check vpci_add_register(struct vpci *vpci, @@ -242,6 +243,11 @@ static inline int vpci_add_handlers(struct pci_dev *pdev) static inline void vpci_remove_device(struct pci_dev *pdev) { } +static inline int vpci_reset_device_state(struct pci_dev *pdev) +{ + return 0; +} + static inline void vpci_dump_msi(void) { } static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,