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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan