Re: [PATCH v3 04/11] vpci/header: Add and remove register handlers dynamically

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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-dbb93a18e73f@xxxxxxxx/
[1] https://lore.kernel.org/xen-devel/YXlxmdYdwptakDDK@Air-de-Roger/