>> + 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.
Thank you,
Oleksandr
