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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
> 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?

Excess verbosity: If we have such here, why not elsewhere on error paths?
And I hope you agree things will get too verbose if we had such (about)
everywhere?

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

The comment is primarily on the earlier function, and then extends to
this one.

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

This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
dead code on x86. Hence there should be an arrangement allowing the
compiler to eliminate this dead code. Whether that's enclosing these
by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
This has a knock-on effect on other functions as you certainly realize:
The compiler seeing e.g. the 2nd argument to the add-BARs function
always being true allows it to instantiate just a clone of the original
function with the respective conditionals removed.

Jan