On Thu, Sep 30, 2021 at 10:52:14AM +0300, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <[email protected]> > > When a PCI device gets assigned/de-assigned some work on vPCI side needs > to be done for that device. Introduce a pair of hooks so vPCI can handle > that. > > Please note, that in the current design the error path is handled by > the toolstack via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device, > so this is why it is acceptable not to de-assign devices if vPCI's > assign fails, e.g. the roll back will be handled on deassign_device when > it is called by the toolstack.
It's kind of hard to see what would need to be rolled back, as the functions are just dummies right now that don't perform any actions. I don't think the toolstack should be the one to deal with the fallout, as it could leave Xen in a broken state. The current commit message doesn't provide any information about why it has been designed this way. > > Signed-off-by: Oleksandr Andrushchenko <[email protected]> > --- > Since v2: > - define CONFIG_HAS_VPCI_GUEST_SUPPORT so dead code is not compiled > for x86 > Since v1: > - constify struct pci_dev where possible > - do not open code is_system_domain() > - extended the commit message > --- > xen/drivers/Kconfig | 4 ++++ > xen/drivers/passthrough/pci.c | 9 +++++++++ > xen/drivers/vpci/vpci.c | 23 +++++++++++++++++++++++ > xen/include/xen/vpci.h | 20 ++++++++++++++++++++ > 4 files changed, 56 insertions(+) > > diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig > index db94393f47a6..780490cf8e39 100644 > --- a/xen/drivers/Kconfig > +++ b/xen/drivers/Kconfig > @@ -15,4 +15,8 @@ source "drivers/video/Kconfig" > config HAS_VPCI > bool > > +config HAS_VPCI_GUEST_SUPPORT > + bool > + depends on HAS_VPCI I would assume this is to go away once the work is finished? I don't think it makes sense to split vPCI code between domU/dom0 on a build time basis. > + > endmenu > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 9f804a50e780..805ab86ed555 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -870,6 +870,10 @@ static int deassign_device(struct domain *d, uint16_t > seg, uint8_t bus, > if ( ret ) > goto out; > > + ret = vpci_deassign_device(d, pdev); > + if ( ret ) > + goto out; > + > if ( pdev->domain == hardware_domain ) > pdev->quarantine = false; > > @@ -1429,6 +1433,11 @@ static int assign_device(struct domain *d, u16 seg, u8 > bus, u8 devfn, u32 flag) > rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), > flag); > } > > + if ( rc ) > + goto done; > + > + rc = vpci_assign_device(d, pdev); > + > done: > if ( rc ) > printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n", > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index 1666402d55b8..0fe86cb30d23 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -86,6 +86,29 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev) > > return rc; > } > + > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > +/* Notify vPCI that device is assigned to guest. */ > +int vpci_assign_device(struct domain *d, const struct pci_dev *dev) > +{ > + /* It only makes sense to assign for hwdom or guest domain. */ > + if ( is_system_domain(d) || !has_vpci(d) ) > + return 0; > + > + return 0; > +} > + > +/* Notify vPCI that device is de-assigned from guest. */ > +int vpci_deassign_device(struct domain *d, const struct pci_dev *dev) > +{ > + /* It only makes sense to de-assign from hwdom or guest domain. */ > + if ( is_system_domain(d) || !has_vpci(d) ) > + return 0; > + > + return 0; > +} > +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */ > + > #endif /* __XEN__ */ > > static int vpci_register_cmp(const struct vpci_register *r1, > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h > index 2e910d0b1f90..ecc08f2c0f65 100644 > --- a/xen/include/xen/vpci.h > +++ b/xen/include/xen/vpci.h > @@ -242,6 +242,26 @@ static inline bool vpci_process_pending(struct vcpu *v) > } > #endif > > +#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_HAS_VPCI_GUEST_SUPPORT) You don't need to check for CONFIG_HAS_VPCI, as CONFIG_HAS_VPCI_GUEST_SUPPORT already depends on CONFIG_HAS_VPCI being set. > +/* Notify vPCI that device is assigned/de-assigned to/from guest. */ > +int __must_check vpci_assign_device(struct domain *d, > + const struct pci_dev *dev); > +int __must_check vpci_deassign_device(struct domain *d, > + const struct pci_dev *dev); > +#else > +static inline int vpci_assign_device(struct domain *d, > + const struct pci_dev *dev) > +{ > + return 0; > +}; > + > +static inline int vpci_deassign_device(struct domain *d, > + const struct pci_dev *dev) > +{ > + return 0; > +}; You need the __must_check attributes here also to match the prototypes above. Thanks, Roger.
