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

From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"Paul Durrant" <paul@xen.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>, "Tim Deegan" <tim@xen.org>,
	"Julien Grall" <julien.grall@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH V5 04/22] xen/ioreq: Make x86's IOREQ feature common
Date: Thu, 28 Jan 2021 13:16:22 +0200	[thread overview]
Message-ID: <17d15bd5-87d3-5159-cb80-a54dbd8fb57d@gmail.com> (raw)
In-Reply-To: <1eb1247b-f455-65c8-b91c-3e8bc05186a1@suse.com>

On 28.01.21 10:06, Jan Beulich wrote:

Hi Jan

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

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

>
> Jan

-- 
Regards,

Oleksandr Tyshchenko