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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 07.09.21 13:43, Jan Beulich wrote:
> 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?
Agree, will change it to gdprintk
>
>>>    - 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.
vpci_bar_add_handlers will be used by x86 PVH Dom0
>   Hence there should be an arrangement allowing the
> compiler to eliminate this dead code.

So, the only dead code for x86 here will be vpci_bar_remove_handlers. Yet.

Because I hope x86 to gain guest support for PVH Dom0 sooner or later.

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

With the above (e.g. add is going to be used, but not remove) do you

think it is worth playing with ifdef's to strip that single function and add

a piece of spaghetti code to save a bit? What would that ifdef look like,

e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?

IMO, it is cleaner to leave it as is. Yet we waste some bits for x86.

>
> Jan
>
Thank you,

Oleksandr