Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 29.09.21 17:07, Jan Beulich wrote:
> On 29.09.2021 15:49, Oleksandr Andrushchenko wrote:
>>
>> On 29.09.21 16:23, Jan Beulich wrote:
>>> On 29.09.2021 15:16, Oleksandr Andrushchenko wrote:
>>>> On 29.09.21 15:54, Jan Beulich wrote:
>>>>> On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>>>>>> On 29.09.21 12:09, Jan Beulich wrote:
>>>>>>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>>>>>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>>>>>>
>>>>>>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>>>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>>>>>>> needs to be in the common code though.
>>>>>>> I agree it wants to live in common code, but I'd still like the code to
>>>>>>> not bloat x86 binaries. Hence yes, I think there want to be
>>>>>>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>>>>>>> possible without breaking the build, respective #ifdef-s.
>>>>>> Then it needs to be defined as (xen/drivers/Kconfig):
>>>>>>
>>>>>> config HAS_VPCI_GUEST_SUPPORT
>>>>>>         # vPCI guest support is only enabled for Arm now
>>>>>>         def_bool y if ARM
>>>>>>         depends on HAS_VPCI
>>>>>>
>>>>>> Because it needs to be defined as "y" for Arm with vPCI support.
>>>>>>
>>>>>> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>>>>>>
>>>>>> but the resulting binary behaves wrong.
>>>>>>
>>>>>> Do you see this as an acceptable solution?
>>>>> Like all (or at least the majority) of other HAS_*, it ought to be
>>>>> "select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
>>>>> (I don't mind the "depends on", as long as it's clear that it exists
>>>>> solely to allow kconfig to warn about bogus "select"s.)
>>>> There is yet no Kconfig exists (even for Arm) that enables HAS_VPCI,
>>>>
>>>> thus I can't do that at the moment even if I want to. Yes, this can be deferred
>>>>
>>>> to the later stage when we enable VPCI for Arm, bit this config option is still
>>>>
>>>> considered as "common code" as the functionality being added
>>>>
>>>> to the common code is just gated with CONFIG_HAS_VPCI_GUEST_SUPPORT.
>>>>
>>>> With this defined now and not later the code seems to be complete and more clean
>>>>
>>>> as it is seen what is this configuration option and how it is enabled and used in the
>>>>
>>>> code.
>>>>
>>>> So, I see no problem if it is defined in this Kconfig and evaluates to "n" for x86
>>>>
>>>> and eventually will be "y" for Arm when it enables HAS_VPCI.
>>> I'm afraid I don't view this as a reply reflecting that you have
>>> understood what I'm asking for. The primary request is for there to
>>> not be "def_bool y" but just "bool" accompanied by a "select" in
>>> Arm's Kconfig. If HAS_VPCI doesn't exist as an option yet, simply
>>> omit the (questionable) "depends on".
>> I understood that, but was trying to make sure we don't miss
>> this option while enabling vPCI on Arm, but ok, I'll have the following:
>> config HAS_VPCI
>>       bool
>>
>> config HAS_VPCI_GUEST_SUPPORT
>>       bool
>>       depends on HAS_VPCI
>> and select it for Arm xen/arch/arm/Kconfig
> Btw you could also have
>
> config HAS_VPCI
>       bool
>
> config HAS_VPCI_GUEST_SUPPORT
>       bool
>       select HAS_VPCI
>
> which would require arm/Kconfig to only select the latter, while
> x86/Kconfig would only select the former.
I'll probably leave it as I wrote before, because it then looks like
a sub-feature enables the parent feature and doesn't seem right
Although it may still look right...
>
>>> PS: The more replies I get from you, the more annoying I find the
>>> insertion of blank lines between every two lines of text. Blank
>>> lines are usually used to separate paragraphs. If it is your mail
>>> program which inserts these, can you please try to do something
>>> about this? Thanks.
>>>
>> I first thought that this is how Thunderbird started showing
>> my replies and was also curious about the distance between the lines
>> which seemed to be as double-line, but I couldn't delete or edit
>> those. I thought this is only visible to me...
>> It came out that this was because of some Thunderbird settings which
>> made my replies with those double-liners. Hope it is fixed now.
> Indeed, thanks - I did not remove any blank lines from context above.
>
> Jan
>
>
Thank you,
Oleksandr