[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC XEN PATCH v2 1/3] xen/vpci: Clear all vpci status of device
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@xxxxxxx> >> Signed-off-by: Huang Rui <ray.huang@xxxxxxx> >> --- >> 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. -- Best regards, Jiqian Chen.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |