diff mbox series

[V2,04/23] xen/ioreq: Provide alias for the handle_mmio()

Message ID 1602780274-29141-5-git-send-email-olekstysh@gmail.com (mailing list archive)
State New, archived
Headers show
Series IOREQ feature (+ virtio-mmio) on Arm | expand

Commit Message

Oleksandr Tyshchenko Oct. 15, 2020, 4:44 p.m. UTC 
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(-)

Comments

Paul Durrant Oct. 20, 2020, 9:14 a.m. UTC | #1 
> -----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
>
Jan Beulich Oct. 20, 2020, 10:05 a.m. UTC | #2 
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
Paul Durrant Oct. 20, 2020, 10:38 a.m. UTC | #3 
> -----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
Oleksandr Tyshchenko Nov. 10, 2020, 7:44 p.m. UTC | #4 
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?
Jan Beulich Nov. 11, 2020, 7:27 a.m. UTC | #5 
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
Oleksandr Tyshchenko Nov. 11, 2020, 8:09 a.m. UTC | #6 
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/
Jan Beulich Nov. 11, 2020, 8:16 a.m. UTC | #7 
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/
>
diff mbox series

Patch

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