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
+	---help---
+	  Enables generic mechanism for providing emulated devices to the guests.
+
+	  If unsure, say Y.
 
 config KEXEC
 	bool "kexec support"
-- 
2.7.4

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,

-- 
Julien Grall

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

-- 
Regards,

Oleksandr Tyshchenko

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,

-- 
Julien Grall

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?


-- 
Regards,

Oleksandr Tyshchenko

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

-- 
Regards,

Oleksandr Tyshchenko

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