| Message ID | 1611884932-1851-25-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
On 29.01.2021 02:48, Oleksandr Tyshchenko wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -137,7 +137,13 @@ config HYPFS_CONFIG > want to hide the .config contents from dom0. > > config IOREQ_SERVER > - bool > + bool "IOREQ support (EXPERT)" if EXPERT && !X86 > + default X86 > + depends on HVM > + ---help--- > + Enables generic mechanism for providing emulated devices to the guests. > + > + If unsure, say Y. I would have given this my ack, if there wasn't this last line. For an experimental feature, I think any uncertainty should lead to a suggestion of turning it off, not on? Hence Acked-by: Jan Beulich <jbeulich@suse.com> only with this saying N instead of Y. Jan
Hi Oleksandr, On 29/01/2021 01:48, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > The purpose of this patch is to add a possibility for user > to be able to select IOREQ support on Arm (which is disabled > by default) with retaining the current behaviour on x86 > (is selected by HVM and it's prompt is not visible). > > Also make the IOREQ be depended on CONFIG_EXPERT on Arm since > it is considered as Technological Preview feature. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > Changes V5 -> V6: > - new patch > --- > --- > xen/common/Kconfig | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index fa049a6..225e57b 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -137,7 +137,13 @@ config HYPFS_CONFIG > want to hide the .config contents from dom0. > > config IOREQ_SERVER > - bool > + bool "IOREQ support (EXPERT)" if EXPERT && !X86 > + default X86 > + depends on HVM AFAICT, CONFIG_HVM will already select CONFIG_IOREQ_SERVER. So are the two lines necessary? Cheers,
On 29.01.2021 10:55, Julien Grall wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -137,7 +137,13 @@ config HYPFS_CONFIG >> want to hide the .config contents from dom0. >> >> config IOREQ_SERVER >> - bool >> + bool "IOREQ support (EXPERT)" if EXPERT && !X86 >> + default X86 >> + depends on HVM > AFAICT, CONFIG_HVM will already select CONFIG_IOREQ_SERVER. So are the > two lines necessary? I agree they may not be necessary, but as long as they don't cause any harm I thought maybe they serve a documentation purpose. Jan
On 29.01.21 12:06, Jan Beulich wrote: Hi Jan, Julien > On 29.01.2021 10:55, Julien Grall wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -137,7 +137,13 @@ config HYPFS_CONFIG >>> want to hide the .config contents from dom0. >>> >>> config IOREQ_SERVER >>> - bool >>> + bool "IOREQ support (EXPERT)" if EXPERT && !X86 >>> + default X86 >>> + depends on HVM >> AFAICT, CONFIG_HVM will already select CONFIG_IOREQ_SERVER. So are the >> two lines necessary? > I agree they may not be necessary, but as long as they don't > cause any harm I thought maybe they serve a documentation > purpose. 1. Agree that it should be "If unsure, say N." 2. Agree that two lines are not strictly needed (just rechecked). 3. Agree that two lines indicates the *real* state: - Although we managed to remove almost all (all?) HVM-ism in IOREQ common code, this feature depends on HVM anyway - And it is should enabled by default on X86, and disabled on Arm So what we should do with them (keep or remove)? > > Jan
On 29.01.2021 12:19, Oleksandr wrote: > > On 29.01.21 12:06, Jan Beulich wrote: > > Hi Jan, Julien > >> On 29.01.2021 10:55, Julien Grall wrote: >>>> --- a/xen/common/Kconfig >>>> +++ b/xen/common/Kconfig >>>> @@ -137,7 +137,13 @@ config HYPFS_CONFIG >>>> want to hide the .config contents from dom0. >>>> >>>> config IOREQ_SERVER >>>> - bool >>>> + bool "IOREQ support (EXPERT)" if EXPERT && !X86 >>>> + default X86 >>>> + depends on HVM >>> AFAICT, CONFIG_HVM will already select CONFIG_IOREQ_SERVER. So are the >>> two lines necessary? >> I agree they may not be necessary, but as long as they don't >> cause any harm I thought maybe they serve a documentation >> purpose. > 1. Agree that it should be "If unsure, say N." Faod this could be taken care of while committing. > 2. Agree that two lines are not strictly needed (just rechecked). > 3. Agree that two lines indicates the *real* state: > - Although we managed to remove almost all (all?) HVM-ism in IOREQ > common code, this feature depends on HVM anyway > - And it is should enabled by default on X86, and disabled on Arm > > So what we should do with them (keep or remove)? I'd be fine either way, with just a slight preference to keeping. Julien? Jan
On 29/01/2021 11:25, Jan Beulich wrote: > On 29.01.2021 12:19, Oleksandr wrote: >> >> On 29.01.21 12:06, Jan Beulich wrote: >> >> Hi Jan, Julien >> >>> On 29.01.2021 10:55, Julien Grall wrote: >>>>> --- a/xen/common/Kconfig >>>>> +++ b/xen/common/Kconfig >>>>> @@ -137,7 +137,13 @@ config HYPFS_CONFIG >>>>> want to hide the .config contents from dom0. >>>>> >>>>> config IOREQ_SERVER >>>>> - bool >>>>> + bool "IOREQ support (EXPERT)" if EXPERT && !X86 >>>>> + default X86 >>>>> + depends on HVM >>>> AFAICT, CONFIG_HVM will already select CONFIG_IOREQ_SERVER. So are the >>>> two lines necessary? >>> I agree they may not be necessary, but as long as they don't >>> cause any harm I thought maybe they serve a documentation >>> purpose. >> 1. Agree that it should be "If unsure, say N." > > Faod this could be taken care of while committing. > >> 2. Agree that two lines are not strictly needed (just rechecked). >> 3. Agree that two lines indicates the *real* state: >> - Although we managed to remove almost all (all?) HVM-ism in IOREQ >> common code, this feature depends on HVM anyway >> - And it is should enabled by default on X86, and disabled on Arm >> >> So what we should do with them (keep or remove)? > > I'd be fine either way, with just a slight preference to keeping. > Julien? I find a bit strange, but I am happy to keep it. Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
On 29.01.21 13:26, Julien Grall wrote: Hi all > > > On 29/01/2021 11:25, Jan Beulich wrote: >> On 29.01.2021 12:19, Oleksandr wrote: >>> >>> On 29.01.21 12:06, Jan Beulich wrote: >>> >>> Hi Jan, Julien >>> >>>> On 29.01.2021 10:55, Julien Grall wrote: >>>>>> --- a/xen/common/Kconfig >>>>>> +++ b/xen/common/Kconfig >>>>>> @@ -137,7 +137,13 @@ config HYPFS_CONFIG >>>>>> want to hide the .config contents from dom0. >>>>>> config IOREQ_SERVER >>>>>> - bool >>>>>> + bool "IOREQ support (EXPERT)" if EXPERT && !X86 >>>>>> + default X86 >>>>>> + depends on HVM >>>>> AFAICT, CONFIG_HVM will already select CONFIG_IOREQ_SERVER. So are >>>>> the >>>>> two lines necessary? >>>> I agree they may not be necessary, but as long as they don't >>>> cause any harm I thought maybe they serve a documentation >>>> purpose. >>> 1. Agree that it should be "If unsure, say N." >> >> Faod this could be taken care of while committing. >> >>> 2. Agree that two lines are not strictly needed (just rechecked). >>> 3. Agree that two lines indicates the *real* state: >>> - Although we managed to remove almost all (all?) HVM-ism in IOREQ >>> common code, this feature depends on HVM anyway >>> - And it is should enabled by default on X86, and disabled on Arm >>> >>> So what we should do with them (keep or remove)? >> >> I'd be fine either way, with just a slight preference to keeping. >> Julien? > > I find a bit strange, but I am happy to keep it. > > Acked-by: Julien Grall <jgrall@amazon.com> Thanks to both of you! I am wondering do we need to update support.md in the context of IOREQ status on Arm right now or this could be postponed?
On 29.01.2021 12:37, Oleksandr wrote: > I am wondering do we need to update support.md in the context of IOREQ > status on Arm right now or this could be postponed? I think so, yes. I have to admit I didn't even realize the whole series doesn't make an addition there. I think this wants to be part of this patch here, as without it the code can't be enabled (and hence can't be used, and hence its support status really doesn't matter [yet]). Jan
On 29.01.21 13:54, Jan Beulich wrote: Hi Jan > On 29.01.2021 12:37, Oleksandr wrote: >> I am wondering do we need to update support.md in the context of IOREQ >> status on Arm right now or this could be postponed? > I think so, yes. I have to admit I didn't even realize the whole > series doesn't make an addition there. I think this wants to be > part of this patch here, as without it the code can't be enabled > (and hence can't be used, and hence its support status really > doesn't matter [yet]). Thank you for the clarification. The only mention about IOREQ server I managed to find in support.md is "x86/Multiple IOREQ servers" with Status: Experimental. Does it match the current state on X86? I am asking because we are considering Tech Preview (which is higher than Experimental) on Arm. Now I am wondering how could that be... Or I missed something? > > Jan
On 29.01.2021 13:06, Oleksandr wrote: > On 29.01.21 13:54, Jan Beulich wrote: >> On 29.01.2021 12:37, Oleksandr wrote: >>> I am wondering do we need to update support.md in the context of IOREQ >>> status on Arm right now or this could be postponed? >> I think so, yes. I have to admit I didn't even realize the whole >> series doesn't make an addition there. I think this wants to be >> part of this patch here, as without it the code can't be enabled >> (and hence can't be used, and hence its support status really >> doesn't matter [yet]). > Thank you for the clarification. > The only mention about IOREQ server I managed to find in support.md is > "x86/Multiple IOREQ servers" > with Status: Experimental. Does it match the current state on X86? I am > asking because we are considering Tech Preview (which is higher than > Experimental) > on Arm. Now I am wondering how could that be... Or I missed something? That's a question primarily to Paul, I guess, but I wouldn't recommend you piggyback on that entry that you've found. You want IOREQ servers in general (which are an integral part of x86/HVM), and hence imo you want a separate entry. Jan
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index fa049a6..225e57b 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -137,7 +137,13 @@ config HYPFS_CONFIG want to hide the .config contents from dom0. config IOREQ_SERVER - bool + bool "IOREQ support (EXPERT)" if EXPERT && !X86 + default X86 + depends on HVM + ---help--- + Enables generic mechanism for providing emulated devices to the guests. + + If unsure, say Y. config KEXEC bool "kexec support"