Mailing List Archive

[PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The IOREQ is a common feature now and Arm will have its own
implementation.

But the name of the function is pretty generic and can be confusing
on Arm (we already have a try_handle_mmio()).

In order not to rename the function (which is used for a varying
set of purposes on x86) globally and get non-confusing variant on Arm
provide an alias ioreq_complete_mmio() to be used on common and
Arm code.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Changes RFC -> V1:
- new patch

Changes V1 -> V2:
- remove "handle"
- add Jan's A-b
---
xen/common/ioreq.c | 2 +-
xen/include/asm-x86/hvm/ioreq.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index c89df7a..29ad48e 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -200,7 +200,7 @@ bool handle_hvm_io_completion(struct vcpu *v)
break;

case HVMIO_mmio_completion:
- return handle_mmio();
+ return ioreq_complete_mmio();

case HVMIO_pio_completion:
return handle_pio(vio->io_req.addr, vio->io_req.size,
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index a3d8faa..a147856 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
#define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
#define IOREQ_STATUS_RETRY X86EMUL_RETRY

+#define ioreq_complete_mmio handle_mmio
+
#endif /* __ASM_X86_HVM_IOREQ_H__ */

/*
--
2.7.4
RE: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
> Sent: 15 October 2020 17:44
> To: xen-devel@lists.xenproject.org
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Paul Durrant <paul@xen.org>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> The IOREQ is a common feature now and Arm will have its own
> implementation.
>
> But the name of the function is pretty generic and can be confusing
> on Arm (we already have a try_handle_mmio()).
>
> In order not to rename the function (which is used for a varying
> set of purposes on x86) globally and get non-confusing variant on Arm
> provide an alias ioreq_complete_mmio() to be used on common and
> Arm code.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
>
> Changes RFC -> V1:
> - new patch
>
> Changes V1 -> V2:
> - remove "handle"
> - add Jan's A-b
> ---
> xen/common/ioreq.c | 2 +-
> xen/include/asm-x86/hvm/ioreq.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index c89df7a..29ad48e 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -200,7 +200,7 @@ bool handle_hvm_io_completion(struct vcpu *v)
> break;
>
> case HVMIO_mmio_completion:
> - return handle_mmio();
> + return ioreq_complete_mmio();
>
> case HVMIO_pio_completion:
> return handle_pio(vio->io_req.addr, vio->io_req.size,
> diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
> index a3d8faa..a147856 100644
> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>
> +#define ioreq_complete_mmio handle_mmio
> +

A #define? Really? Can we not have a static inline?

Paul

> #endif /* __ASM_X86_HVM_IOREQ_H__ */
>
> /*
> --
> 2.7.4
>
Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
On 20.10.2020 11:14, Paul Durrant wrote:
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
>> Sent: 15 October 2020 17:44
>>
>> --- a/xen/include/asm-x86/hvm/ioreq.h
>> +++ b/xen/include/asm-x86/hvm/ioreq.h
>> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>
>> +#define ioreq_complete_mmio handle_mmio
>> +
>
> A #define? Really? Can we not have a static inline?

I guess this would require further shuffling: handle_mmio() is
an inline function in hvm/emulate.h, and hvm/ioreq.h has no
need to include the former (and imo it also shouldn't have).

Jan
RE: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 20 October 2020 11:05
> To: paul@xen.org
> Cc: 'Oleksandr Tyshchenko' <olekstysh@gmail.com>; xen-devel@lists.xenproject.org; 'Oleksandr
> Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau
> Monné' <roger.pau@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Julien Grall' <julien@xen.org>; 'Stefano
> Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien.grall@arm.com>
> Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>
> On 20.10.2020 11:14, Paul Durrant wrote:
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
> >> Sent: 15 October 2020 17:44
> >>
> >> --- a/xen/include/asm-x86/hvm/ioreq.h
> >> +++ b/xen/include/asm-x86/hvm/ioreq.h
> >> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
> >> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
> >> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
> >>
> >> +#define ioreq_complete_mmio handle_mmio
> >> +
> >
> > A #define? Really? Can we not have a static inline?
>
> I guess this would require further shuffling: handle_mmio() is
> an inline function in hvm/emulate.h, and hvm/ioreq.h has no
> need to include the former (and imo it also shouldn't have).
>

I see. I think we need an x86 ioreq.c anyway, to deal with the legacy use of magic pages, so it could be dealt with there instead.

Paul

> Jan
Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
On 20.10.20 13:38, Paul Durrant wrote:

Hi Jan, Paul

Sorry for the late response.

>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 20 October 2020 11:05
>> To: paul@xen.org
>> Cc: 'Oleksandr Tyshchenko' <olekstysh@gmail.com>; xen-devel@lists.xenproject.org; 'Oleksandr
>> Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau
>> Monné' <roger.pau@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Julien Grall' <julien@xen.org>; 'Stefano
>> Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien.grall@arm.com>
>> Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>>
>> On 20.10.2020 11:14, Paul Durrant wrote:
>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
>>>> Sent: 15 October 2020 17:44
>>>>
>>>> --- a/xen/include/asm-x86/hvm/ioreq.h
>>>> +++ b/xen/include/asm-x86/hvm/ioreq.h
>>>> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>>>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>>>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>>>
>>>> +#define ioreq_complete_mmio handle_mmio
>>>> +
>>> A #define? Really? Can we not have a static inline?
>> I guess this would require further shuffling: handle_mmio() is
>> an inline function in hvm/emulate.h, and hvm/ioreq.h has no
>> need to include the former (and imo it also shouldn't have).
>>
> I see. I think we need an x86 ioreq.c anyway, to deal with the legacy use of magic pages, so it could be dealt with there instead.
I am afraid I don't entirely understand the required changes. Could you
please clarify where the "inline(?)" ioreq_complete_mmio() should
live? I included hvm/emulate.h here not for the "handle_mmio()" reason
only, but for "struct hvm_emulate_ctxt" also (see arch_io_completion()).


But, if we return x86 ioreq.c back I can move arch_io_completion() to it
as well as "non-online" ioreq_complete_mmio().
This will avoid including hvm/emulate.h here. Or I missed something?

--
Regards,

Oleksandr Tyshchenko
Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
On 10.11.2020 20:44, Oleksandr wrote:
>
> On 20.10.20 13:38, Paul Durrant wrote:
>
> Hi Jan, Paul
>
> Sorry for the late response.
>
>>> -----Original Message-----
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: 20 October 2020 11:05
>>> To: paul@xen.org
>>> Cc: 'Oleksandr Tyshchenko' <olekstysh@gmail.com>; xen-devel@lists.xenproject.org; 'Oleksandr
>>> Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau
>>> Monné' <roger.pau@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Julien Grall' <julien@xen.org>; 'Stefano
>>> Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien.grall@arm.com>
>>> Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>>>
>>> On 20.10.2020 11:14, Paul Durrant wrote:
>>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
>>>>> Sent: 15 October 2020 17:44
>>>>>
>>>>> --- a/xen/include/asm-x86/hvm/ioreq.h
>>>>> +++ b/xen/include/asm-x86/hvm/ioreq.h
>>>>> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>>>>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>>>>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>>>>
>>>>> +#define ioreq_complete_mmio handle_mmio
>>>>> +
>>>> A #define? Really? Can we not have a static inline?
>>> I guess this would require further shuffling: handle_mmio() is
>>> an inline function in hvm/emulate.h, and hvm/ioreq.h has no
>>> need to include the former (and imo it also shouldn't have).
>>>
>> I see. I think we need an x86 ioreq.c anyway, to deal with the legacy use of magic pages, so it could be dealt with there instead.
> I am afraid I don't entirely understand the required changes. Could you
> please clarify where the "inline(?)" ioreq_complete_mmio() should
> live? I included hvm/emulate.h here not for the "handle_mmio()" reason
> only, but for "struct hvm_emulate_ctxt" also (see arch_io_completion()).

I'm sorry, but in the context of this patch there's no use of any
struct hvm_emulate_ctxt instance. I'm not going to wade through 23
patches to find what you mean.

> But, if we return x86 ioreq.c back I can move arch_io_completion() to it
> as well as "non-online" ioreq_complete_mmio().
> This will avoid including hvm/emulate.h here. Or I missed something?

I suppose an out-of-line function as kind of a last resort solution
is what Paul had in mind. To be honest I'd prefer to avoid the
extra call layer though, if possible.

Jan
Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
On 11.11.20 09:27, Jan Beulich wrote:

Hi Jan

> On 10.11.2020 20:44, Oleksandr wrote:
>> On 20.10.20 13:38, Paul Durrant wrote:
>>
>> Hi Jan, Paul
>>
>> Sorry for the late response.
>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 20 October 2020 11:05
>>>> To: paul@xen.org
>>>> Cc: 'Oleksandr Tyshchenko' <olekstysh@gmail.com>; xen-devel@lists.xenproject.org; 'Oleksandr
>>>> Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau
>>>> Monné' <roger.pau@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Julien Grall' <julien@xen.org>; 'Stefano
>>>> Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien.grall@arm.com>
>>>> Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>>>>
>>>> On 20.10.2020 11:14, Paul Durrant wrote:
>>>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
>>>>>> Sent: 15 October 2020 17:44
>>>>>>
>>>>>> --- a/xen/include/asm-x86/hvm/ioreq.h
>>>>>> +++ b/xen/include/asm-x86/hvm/ioreq.h
>>>>>> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>>>>>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>>>>>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>>>>>
>>>>>> +#define ioreq_complete_mmio handle_mmio
>>>>>> +
>>>>> A #define? Really? Can we not have a static inline?
>>>> I guess this would require further shuffling: handle_mmio() is
>>>> an inline function in hvm/emulate.h, and hvm/ioreq.h has no
>>>> need to include the former (and imo it also shouldn't have).
>>>>
>>> I see. I think we need an x86 ioreq.c anyway, to deal with the legacy use of magic pages, so it could be dealt with there instead.
>> I am afraid I don't entirely understand the required changes. Could you
>> please clarify where the "inline(?)" ioreq_complete_mmio() should
>> live? I included hvm/emulate.h here not for the "handle_mmio()" reason
>> only, but for "struct hvm_emulate_ctxt" also (see arch_io_completion()).
> I'm sorry, but in the context of this patch there's no use of any
> struct hvm_emulate_ctxt instance. I'm not going to wade through 23
> patches to find what you mean.

Sorry for not being precise here. I meant arch_io_completion() added at [1]


[1]
https://patchwork.kernel.org/project/xen-devel/patch/1602780274-29141-2-git-send-email-olekstysh@gmail.com/

--
Regards,

Oleksandr Tyshchenko
Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio() [ In reply to ]
On 11.11.2020 09:09, Oleksandr wrote:
>
> On 11.11.20 09:27, Jan Beulich wrote:
>
> Hi Jan
>
>> On 10.11.2020 20:44, Oleksandr wrote:
>>> On 20.10.20 13:38, Paul Durrant wrote:
>>>
>>> Hi Jan, Paul
>>>
>>> Sorry for the late response.
>>>
>>>>> -----Original Message-----
>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>> Sent: 20 October 2020 11:05
>>>>> To: paul@xen.org
>>>>> Cc: 'Oleksandr Tyshchenko' <olekstysh@gmail.com>; xen-devel@lists.xenproject.org; 'Oleksandr
>>>>> Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau
>>>>> Monné' <roger.pau@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Julien Grall' <julien@xen.org>; 'Stefano
>>>>> Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien.grall@arm.com>
>>>>> Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>>>>>
>>>>> On 20.10.2020 11:14, Paul Durrant wrote:
>>>>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
>>>>>>> Sent: 15 October 2020 17:44
>>>>>>>
>>>>>>> --- a/xen/include/asm-x86/hvm/ioreq.h
>>>>>>> +++ b/xen/include/asm-x86/hvm/ioreq.h
>>>>>>> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>>>>>>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>>>>>>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>>>>>>
>>>>>>> +#define ioreq_complete_mmio handle_mmio
>>>>>>> +
>>>>>> A #define? Really? Can we not have a static inline?
>>>>> I guess this would require further shuffling: handle_mmio() is
>>>>> an inline function in hvm/emulate.h, and hvm/ioreq.h has no
>>>>> need to include the former (and imo it also shouldn't have).
>>>>>
>>>> I see. I think we need an x86 ioreq.c anyway, to deal with the legacy use of magic pages, so it could be dealt with there instead.
>>> I am afraid I don't entirely understand the required changes. Could you
>>> please clarify where the "inline(?)" ioreq_complete_mmio() should
>>> live? I included hvm/emulate.h here not for the "handle_mmio()" reason
>>> only, but for "struct hvm_emulate_ctxt" also (see arch_io_completion()).
>> I'm sorry, but in the context of this patch there's no use of any
>> struct hvm_emulate_ctxt instance. I'm not going to wade through 23
>> patches to find what you mean.
>
> Sorry for not being precise here. I meant arch_io_completion() added at [1]

At least some of the inlines you add there are way too large to be
inline functions, imo. But consensus appears to be now to retain a
per-arch ioreq.c anyway.

Jan

> [1]
> https://patchwork.kernel.org/project/xen-devel/patch/1602780274-29141-2-git-send-email-olekstysh@gmail.com/
>