Re: [PATCH 4/9] vpci/header: Add and remove register handlers dynamically

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 06.09.21 17:11, Jan Beulich wrote:
> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>   }
>>   REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>   
>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    /* Remove previously added registers. */
>> +    vpci_remove_device_registers(pdev);
>> +
>> +    /* It only makes sense to add registers for hwdom or guest domain. */
>> +    if ( d->domain_id >= DOMID_FIRST_RESERVED )
>> +        return 0;
>> +
>> +    if ( is_hardware_domain(d) )
>> +        rc = add_bar_handlers(pdev, true);
>> +    else
>> +        rc = add_bar_handlers(pdev, false);
>      rc = add_bar_handlers(pdev, is_hardware_domain(d));
Indeed, thank you ;)
>
>> +    if ( rc )
>> +        gprintk(XENLOG_ERR,
>> +            "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>> +            d->domain_id);
> Please use %pd and correct indentation. Logging the error code might
> also help some in diagnosing issues.
Sure, I'll change it to %pd
>   Further I'm not sure this is a
> message we want in release builds
Why not?
>   - perhaps gdprintk()?
I'll change if we decide so
>
>> +    return rc;
>> +}
>> +
>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>> +{
>> +    /* Remove previously added registers. */
>> +    vpci_remove_device_registers(pdev);
>> +    return 0;
>> +}
> Also - in how far is the goal of your work to also make vPCI work for
> x86 DomU-s? If that's not a goal
It is not, unfortunately. The goal is not to break x86 and to enable Arm
> , I'd like to ask that you limit the
> introduction of code that ends up dead there.

What's wrong with this function even if it is a one-liner?

This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers

and if I understood correctly you suggest 
vpci_bar_add_handlers/vpci_remove_device_registers?

What would we gain from that, but yet another secret knowledge that in order

to remove BAR handlers one needs to call vpci_remove_device_registers

while I would personally expect to call vpci_bar_add_handlers' counterpart,

vpci_remove_device_registers namely.

> Jan
>
Thank you,

Oleksandr