[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: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>, Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 7 Sep 2021 13:49:42 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=uHenRRTyf45+WJBF15XEKH7XjRi3lSRhsGAvltq2EWE=; b=Er8VV61mSivAevJPngEKURkhnbMXPKJeZ955bhojAnW7QPjH1QfMOV8Tkyy1E0IgfGYE9zjct8FQXwHMjtGnEdE1orhv3S7Cyq3/cJR06BITXXRZoV5aTwuLmpuN0rnN4dOKI+qbGA0Va0Wd+SsG1ytJOQoRy6VBSwiYTwfetHWW3G82N1yuS+jMzCrWQ4Zf7zpowk/rakaNXFrpkGqF8m+LOTTQvXv+R2QmgliAE1VPGSAhQsqAn9z9T0x/Ufa22iqu+jb9B0fFqrTl3uv5/ypABNgRr0LvqyRCOi7xzw/N4O67e5esQwHnPTulasZe9oFy/CqtaV6EcPZAYrr6SQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iYj0HIVfm48U+c49s1gTcyPG6K8u+GX8M50bbRawIaTf3nexRPn/W4zGLG0+a5viCkE8g7OZjoQFbGLT5Ez7SVGVT3afJVpJBKKUJvNUv22h+M8pNrzj50Veu5vU1ALhkMX9UZ6cIA0qDNqxI+RDdwto/wOnSxIKK97Uj6uoS+NF+Ht/JEEpd5o1H0OktqzDQywVKV8qsR/LZx4HCtpMREsCvBsq7x382seFYgXLMW3oP9/gKWR9r7rpUR/fn5IeuS7qkkzykUXcqF7eoHYRrWsLFUbUPj0L1mpT+da67yIpTL9AEDGPjq1P5yVuqKRs8CNbOarSI6Yu2RGhSm8kbw==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.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 11:49:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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

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

Jan




 


Rackspace

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