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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

>> +    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