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

[Oleksandr <olekstysh@xxxxxxxxx>]

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