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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 07.09.21 15:20, Jan Beulich wrote:
> On 07.09.2021 14:16, Oleksandr Andrushchenko wrote:
>> On 07.09.21 14:49, Jan Beulich wrote:
>>> On 07.09.2021 13:10, Oleksandr Andrushchenko wrote:
>>>> 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
>>> Where / when? You add a call from vpci_assign_device(), but besides that
>>> also being dead code on x86 (for now), you can't mean that because
>>> vpci_deassign_device() also calls vpci_bar_remove_handlers().
>> You are right here and both add/remove are not used on x86 PVH Dom0.
>>
>> I am sorry for wasting your time
>>
>>>>>     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?
>>> No, that I agree wouldn't be worth it.
>>>
>>>> What would that ifdef look like,
>>>> e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?
>>> A new setting, preferably; e.g. VCPU_UNPRIVILEGED, to be "select"ed by
>>> architectures as functionality gets enabled.
>> So, as add/remove are only needed for Arm at the moment
>> you suggest I add VCPU_UNPRIVILEGED to Arm's Kconfig to enable
>> compiling vpci_bar_add_handlers/vpci_bar_remove_handlers?
>> To me this is more about vPCI's support for guests, so should we probably call it
>> VPCI_XXX instead? E.g. VPCI_HAS_GUEST_SUPPORT or something which
>> will reflect the nature of the code being gated? VCPU_UNPRIVILEGED sounds
>> like not connected to vpci to me
> And validly so - my fingers didn't type what the brain told them. I've
> meant VPCI_UNPRIVILEGED. I would also be okay with HAS_VPCI_GUEST_SUPPORT
> (i.e. not exactly what you've suggested), for example.
I'll stick to HAS_VPCI_GUEST_SUPPORT as it seems to be more descriptive
> Jan
>
Thank you,

Oleksandr