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