[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>, Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Date: Tue, 7 Sep 2021 12:16:46 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=ommb9VYJ3nFEfnB9d/KRAz9te3a2pOZjcQ4N2qrkwOA=; b=isoC+O4Br4MGmrAB1wJbozl01Ni9zZ8m7SdnqSsbnS4LdnkFwjSKImIQrEbGQk7rGx+EJz6BWwPGNGhVx5WypTnGgJzFamZyvUordEF7UK7C/ou12hIILfDO0zn1Xhi3Rts4VpFaywZ08fliJxHdUi3428JJ1sSboyNHKxNJe3SWZTSZUrBPAFNZuDu01XjE6a6MX82A/GDx5L8IuNH1+/v1TzmZe7IB64SECadEGkyzgy65qd+7bWEP2vcKZTZqGfiMGzp4lbBObJDIJoEIe+xdGEdrmKAf52zM7AgpWnlRnx0k9jGhcSPdgzN1r+AGMoip21JkbXYqmLrGkPNqEQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HaP2QAgMCTDubOczWTEKnaW5lqTzjcYi8TwhpbfZshnhrNitlrieg4079WGTO27jI5vfrZBLfk709ahprnQ4DbfpczejMVD85JnWOYXWEaYCSIYZMVLivaZFVTCQbi7ED4t8/NKf/Yec6NbNu62ozbw8SmB0emavGMcwmeN5MbC3+EbGolxvnMAa01s/B+GMwvji7WqPFJm7ZXoEiLwtIj52ImqsBIJkFCoVlXQ/d2f2phwP1PGmNsLNmCsbDC9B4IbwwjmcmjrmdrFE01gpEBL9g/+/T3DquUTZIpwrGUPiZbapJAPJBAPCvYYH5TqGzNcbMi9WWovKk7XrO7dHYw==
  • Authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=epam.com;
  • Cc: "julien@xxxxxxx" <julien@xxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, "roger.pau@xxxxxxxxxx" <roger.pau@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 07 Sep 2021 12:17:00 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHXoKusySJnLVveGkulaWKrBZdXpKuXEOQAgAFPE4CAAAkRAIAAB3eAgAALDQCAAAeQAA==
  • Thread-topic: [PATCH 4/9] vpci/header: Add and remove register handlers dynamically

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

>
> Jan
>
Thank you,

Oleksandr

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.