| Message ID | 20190821145903.45934-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ioreq: add support for internal servers | expand |
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 21 August 2019 15:59 > To: xen-devel@lists.xenproject.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org> > Subject: [PATCH 4/7] ioreq: allow registering internal ioreq server handler > > Provide a routine to register the handler for an internal ioreq > server. Note the handler can only be set once. I'd prefer hvm_set_ioreq_handler() and some sort of guard to prevent enabling of an internal server with no handler (probably in the previous patch) would be prudent, I think. Also, why the set-once semantic? Paul > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/hvm/ioreq.c | 32 ++++++++++++++++++++++++++++++++ > xen/include/asm-x86/hvm/ioreq.h | 3 +++ > 2 files changed, 35 insertions(+) > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > index 3fb6fe9585..d8fea191aa 100644 > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -486,6 +486,38 @@ static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf) > return rc; > } > > +int hvm_add_ioreq_handler(struct domain *d, ioservid_t id, > + int (*handler)(struct vcpu *v, ioreq_t *)) > +{ > + struct hvm_ioreq_server *s; > + int rc = 0; > + > + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock); > + s = get_ioreq_server(d, id); > + if ( !s ) > + { > + rc = -ENOENT; > + goto out; > + } > + if ( !s->internal ) > + { > + rc = -EINVAL; > + goto out; > + } > + if ( s->handler != NULL ) > + { > + rc = -EBUSY; > + goto out; > + } > + > + s->handler = handler; > + > + out: > + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); > + > + return rc; > +} > + > static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s, > struct hvm_ioreq_vcpu *sv) > { > diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h > index e8119b26a6..2131c944d4 100644 > --- a/xen/include/asm-x86/hvm/ioreq.h > +++ b/xen/include/asm-x86/hvm/ioreq.h > @@ -55,6 +55,9 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered); > > void hvm_ioreq_init(struct domain *d); > > +int hvm_add_ioreq_handler(struct domain *d, ioservid_t id, > + int (*handler)(struct vcpu *v, ioreq_t *)); > + > #endif /* __ASM_X86_HVM_IOREQ_H__ */ > > /* > -- > 2.22.0
On Wed, Aug 21, 2019 at 06:35:15PM +0200, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: 21 August 2019 15:59 > > To: xen-devel@lists.xenproject.org > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > > <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org> > > Subject: [PATCH 4/7] ioreq: allow registering internal ioreq server handler > > > > Provide a routine to register the handler for an internal ioreq > > server. Note the handler can only be set once. > > I'd prefer hvm_set_ioreq_handler() and some sort of guard to prevent enabling of an internal server with no handler (probably in the previous patch) would be prudent, I think. Right, I will add it. > Also, why the set-once semantic? Well, I didn't have the need to change the handler of internal ioreq servers (vPCI) so I've coded it that way. If you think it's better to allow run time changes of the handler that's fine, I just didn't have the need for it given the current use-case and I thought it would be safer. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 22 August 2019 08:44 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH 4/7] ioreq: allow registering internal ioreq server handler > > On Wed, Aug 21, 2019 at 06:35:15PM +0200, Paul Durrant wrote: > > > -----Original Message----- > > > From: Roger Pau Monne <roger.pau@citrix.com> > > > Sent: 21 August 2019 15:59 > > > To: xen-devel@lists.xenproject.org > > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > > > <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org> > > > Subject: [PATCH 4/7] ioreq: allow registering internal ioreq server handler > > > > > > Provide a routine to register the handler for an internal ioreq > > > server. Note the handler can only be set once. > > > > I'd prefer hvm_set_ioreq_handler() and some sort of guard to prevent enabling of an internal server > with no handler (probably in the previous patch) would be prudent, I think. > > Right, I will add it. > > > Also, why the set-once semantic? > > Well, I didn't have the need to change the handler of internal ioreq > servers (vPCI) so I've coded it that way. If you think it's better to > allow run time changes of the handler that's fine, I just didn't have > the need for it given the current use-case and I thought it would be > safer. > I think a more relaxed semantic of only being able to change the handler when the ioreq server is disabled would be fine. Also, I wonder whether you ought to allow handler registration to set some opaque caller context too, rather than assuming that the vcpu is the appropriate context to pass to all handlers? Paul
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 3fb6fe9585..d8fea191aa 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -486,6 +486,38 @@ static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf) return rc; } +int hvm_add_ioreq_handler(struct domain *d, ioservid_t id, + int (*handler)(struct vcpu *v, ioreq_t *)) +{ + struct hvm_ioreq_server *s; + int rc = 0; + + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock); + s = get_ioreq_server(d, id); + if ( !s ) + { + rc = -ENOENT; + goto out; + } + if ( !s->internal ) + { + rc = -EINVAL; + goto out; + } + if ( s->handler != NULL ) + { + rc = -EBUSY; + goto out; + } + + s->handler = handler; + + out: + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); + + return rc; +} + static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s, struct hvm_ioreq_vcpu *sv) { diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h index e8119b26a6..2131c944d4 100644 --- a/xen/include/asm-x86/hvm/ioreq.h +++ b/xen/include/asm-x86/hvm/ioreq.h @@ -55,6 +55,9 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered); void hvm_ioreq_init(struct domain *d); +int hvm_add_ioreq_handler(struct domain *d, ioservid_t id, + int (*handler)(struct vcpu *v, ioreq_t *)); + #endif /* __ASM_X86_HVM_IOREQ_H__ */ /*
Provide a routine to register the handler for an internal ioreq server. Note the handler can only be set once. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/ioreq.c | 32 ++++++++++++++++++++++++++++++++ xen/include/asm-x86/hvm/ioreq.h | 3 +++ 2 files changed, 35 insertions(+)