On Mon, Nov 01, 2021 at 09:18:17AM +0000, Oleksandr Andrushchenko wrote:
>
> >> + if ( rc )
> >> + gdprintk(XENLOG_ERR,
> >> + "%pp: failed to add BAR handlers for dom%pd: %d\n",
> >> + &pdev->sbdf, d, rc);
> >> + return rc;
> >> +}
> >> +
> >> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev
> >> *pdev)
> >> +{
> >> + /* Remove previously added registers. */
> >> + vpci_remove_device_registers(pdev);
> >> + return 0;
> >> +}
> >> +#endif
> >> +
> >> /*
> >> * Local variables:
> >> * mode: C
> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >> index 0fe86cb30d23..702f7b5d5dda 100644
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct
> >> pci_dev *dev)
> >> if ( is_system_domain(d) || !has_vpci(d) )
> >> return 0;
> >>
> >> - return 0;
> >> + return vpci_bar_add_handlers(d, dev);
> >> }
> >>
> >> /* Notify vPCI that device is de-assigned from guest. */
> >> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const
> >> struct pci_dev *dev)
> >> if ( is_system_domain(d) || !has_vpci(d) )
> >> return 0;
> >>
> >> - return 0;
> >> + return vpci_bar_remove_handlers(d, dev);
> > I think it would be better to use something similar to
> > REGISTER_VPCI_INIT here, otherwise this will need to be modified every
> > time a new capability is handled by Xen.
> >
> > Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
> > to be used for guest initialization?
> >
> >> }
> >> #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> >>
> >> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> >> index ecc08f2c0f65..fd822c903af5 100644
> >> --- a/xen/include/xen/vpci.h
> >> +++ b/xen/include/xen/vpci.h
> >> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev,
> >> unsigned int reg,
> >> */
> >> bool __must_check vpci_process_pending(struct vcpu *v);
> >>
> >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> >> +/* Add/remove BAR handlers for a domain. */
> >> +int vpci_bar_add_handlers(const struct domain *d,
> >> + const struct pci_dev *pdev);
> >> +int vpci_bar_remove_handlers(const struct domain *d,
> >> + const struct pci_dev *pdev);
> >> +#endif
> > This would then go away if we implement a mechanism similar to
> > REGISTER_VPCI_INIT.
> >
> > Thanks, Roger.
> Ok, so I can extend REGISTER_VPCI_INIT with an action parameter:
>
> "There are number of actions to be taken while first initializing vPCI
> for a PCI device or when the device is assigned to a guest or when it
> is de-assigned and so on.
> Every time a new action is needed during these steps we need to call some
> relevant function to handle that. Make it is easier to track the required
> steps by extending REGISTER_VPCI_INIT machinery with an action parameter
> which shows which exactly step/action is being performed."
>
> So, we have
>
> -typedef int vpci_register_init_t(struct pci_dev *dev);
> +enum VPCI_INIT_ACTION {
> + VPCI_INIT_ADD,
> + VPCI_INIT_ASSIGN,
> + VPCI_INIT_DEASSIGN,
> +};
> +
> +typedef int vpci_register_init_t(struct pci_dev *dev,
> + enum VPCI_INIT_ACTION action);
>
> and, for example,
>
> @@ -452,6 +452,9 @@ static int init_bars(struct pci_dev *pdev)
> struct vpci_bar *bars = header->bars;
> int rc;
>
> + if ( action != VPCI_INIT_ADD )
> + return 0;
> +
>
> I was thinking about adding dedicated machinery similar to REGISTER_VPCI_INIT,
> e.g. REGISTER_VPCI_{ASSIGN|DEASSIGN} + dedicated sections in the linker
> scripts,
> but it seems not worth it: these steps are only executed at device
> init/assign/deassign,
> so extending the existing approach doesn't seem to hurt performance much.
>
> Please let me know if this is what you mean, so I can re-work the relevant
> code.
I'm afraid I'm still unsure whether we need an explicit helper to
execute when assigning a device, rather than just using the current
init helpers (init_bars &c).
You said that sizing the BARs when assigning to a domU was not
possible [0], but I'm missing an explanation of why it's not possible,
as I think that won't be an issue on x86 [1].
Thanks, Roger.
[0]
https://lore.kernel.org/xen-devel/368bf4b5-f9fd-76a6-294e-dbb93a18e...@epam.com/
[1] https://lore.kernel.org/xen-devel/YXlxmdYdwptakDDK@Air-de-Roger/
