Re: [PATCH V5 04/22] xen/ioreq: Make x86's IOREQ feature common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.01.2021 12:16, Oleksandr wrote:
> On 28.01.21 10:06, Jan Beulich wrote:
>> On 27.01.2021 21:14, Oleksandr wrote:
>>> On 27.01.21 18:58, Jan Beulich wrote:
>>>> On 25.01.2021 20:08, Oleksandr Tyshchenko wrote:
>>>>> --- a/xen/arch/x86/Kconfig
>>>>> +++ b/xen/arch/x86/Kconfig
>>>>> @@ -92,6 +92,7 @@ config PV_LINEAR_PT
>>>>>    
>>>>>    config HVM
>>>>>           def_bool !PV_SHIM_EXCLUSIVE
>>>>> + select IOREQ_SERVER
>>>>>           prompt "HVM support"
>>>>>           ---help---
>>>> ... the addition moved below the prompt line (could probably
>>>> be taken care of while committing, if no other need for a v6
>>>> arises).
>>> V6 is planned anyway, so will do, but ...
>>>
>>>
>>>> (Personally I think this should be
>>>>
>>>> config HVM
>>>>    bool "HVM support"
>>>>    default !PV_SHIM_EXCLUSIVE
>>> ... def_bool is changed to default by intention or this is a typo?
>> No, it's not a typo. "def_bool" is just a shorthand for "bool"
>> and "default", which is useful when there's no prompt, but
>> gets abused (in my view at least) in a number of other cases.
>> But as said ...
>>
>>>> anyway, but that's nothing you need to care about.)
>> ... here.
> 
> ok, well, thank you, but FYI playing with default instead of def_bool I 
> noticed the difference in behavior.
> 
> I don't understand why, but HVM option disappears from menuconfig 
> somehow... Or I do something completely wrong...

Hmm, this sounds odd indeed, but I confess I don't ever use
menuconfig, so I don't think I have an explanation.

>>>>> --- /dev/null
>>>>> +++ b/xen/include/asm-x86/ioreq.h
>>>>> @@ -0,0 +1,37 @@
>>>>> +/*
>>>>> + * ioreq.h: Hardware virtual machine assist interface definitions.
>>>>> + *
>>>>> + * This is a wrapper which purpose is to not include arch HVM specific 
>>>>> header
>>>>> + * from the common code.
>>>>> + *
>>>>> + * Copyright (c) 2016 Citrix Systems Inc.
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify 
>>>>> it
>>>>> + * under the terms and conditions of the GNU General Public License,
>>>>> + * version 2, as published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
>>>>> for
>>>>> + * more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License 
>>>>> along with
>>>>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>>>>> + */
>>>>> +
>>>>> +#ifndef __ASM_X86_IOREQ_H__
>>>>> +#define __ASM_X86_IOREQ_H__
>>>>> +
>>>>> +#include <asm/hvm/ioreq.h>
>>>>> +
>>>>> +#endif /* __ASM_X86_IOREQ_H__ */
>>>> Not necessarily for taking care of right away, I think in the
>>>> longer run this wants wrapping by #ifdef CONFIG_HVM, such that
>>>> in !HVM builds the dependency on the "chained" header goes
>>>> away (reducing the amount of rebuilding in incremental builds).
>>> I don't mind wrapping it right away.
>> Well, if that doesn't break the !HVM build, I'd of course
>> appreciate it. I'd expect fallout, though.
> 
> Hmm, I didn't notice a fallout with that change when CONFIG_HVM is not set
> 
> +#ifdef CONFIG_HVM
> #include <asm/hvm/ioreq.h>
> +#endif

Good.

Jan