|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 2/5] ioreq-server: create basic ioreq server abstraction.
On 30/01/14 14:19, Paul Durrant wrote:
> Collect together data structures concerning device emulation together into
> a new struct hvm_ioreq_server.
>
> Code that deals with the shared and buffered ioreq pages is extracted from
> functions such as hvm_domain_initialise, hvm_vcpu_initialise and do_hvm_op
> and consolidated into a set of hvm_ioreq_server_XXX functions.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> xen/arch/x86/hvm/hvm.c | 318
> ++++++++++++++++++++++++++------------
> xen/include/asm-x86/hvm/domain.h | 9 +-
> xen/include/asm-x86/hvm/vcpu.h | 2 +-
> 3 files changed, 229 insertions(+), 100 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 71a44db..a0eaadb 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -345,16 +345,16 @@ void hvm_migrate_pirqs(struct vcpu *v)
> spin_unlock(&d->event_lock);
> }
>
> -static ioreq_t *get_ioreq(struct vcpu *v)
> +static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, int id)
> {
> - struct domain *d = v->domain;
> - shared_iopage_t *p = d->arch.hvm_domain.ioreq.va;
> - ASSERT((v == current) || spin_is_locked(&d->arch.hvm_domain.ioreq.lock));
> - return p ? &p->vcpu_ioreq[v->vcpu_id] : NULL;
> + shared_iopage_t *p = s->ioreq.va;
> + ASSERT(p != NULL);
> + return &p->vcpu_ioreq[id];
> }
>
> void hvm_do_resume(struct vcpu *v)
> {
> + struct hvm_ioreq_server *s;
> ioreq_t *p;
>
> check_wakeup_from_wait();
> @@ -362,10 +362,14 @@ void hvm_do_resume(struct vcpu *v)
> if ( is_hvm_vcpu(v) )
> pt_restore_timer(v);
>
> - /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
> - if ( !(p = get_ioreq(v)) )
> + s = v->arch.hvm_vcpu.ioreq_server;
This assignment can be part of the declaration of 's' (and likewise in
most later examples).
> + v->arch.hvm_vcpu.ioreq_server = NULL;
> +
> + if ( !s )
> goto check_inject_trap;
>
> + /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
> + p = get_ioreq(s, v->vcpu_id);
> while ( p->state != STATE_IOREQ_NONE )
> {
> switch ( p->state )
> @@ -375,7 +379,7 @@ void hvm_do_resume(struct vcpu *v)
> break;
> case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> IORESP_READY
> */
> case STATE_IOREQ_INPROCESS:
> - wait_on_xen_event_channel(v->arch.hvm_vcpu.xen_port,
> + wait_on_xen_event_channel(p->vp_eport,
> (p->state != STATE_IOREQ_READY) &&
> (p->state != STATE_IOREQ_INPROCESS));
> break;
> @@ -398,7 +402,6 @@ void hvm_do_resume(struct vcpu *v)
> static void hvm_init_ioreq_page(
> struct domain *d, struct hvm_ioreq_page *iorp)
> {
> - memset(iorp, 0, sizeof(*iorp));
Is it worth keeping this function? the two back to back
domain_pause()'s from the callers are redundant.
> spin_lock_init(&iorp->lock);
> domain_pause(d);
> }
> @@ -541,6 +544,167 @@ static int handle_pvh_io(
> return X86EMUL_OKAY;
> }
>
> +static int hvm_init_ioreq_server(struct domain *d)
> +{
> + struct hvm_ioreq_server *s;
> + int i;
> +
> + s = xzalloc(struct hvm_ioreq_server);
> + if ( !s )
> + return -ENOMEM;
> +
> + s->domain = d;
> +
> + for ( i = 0; i < MAX_HVM_VCPUS; i++ )
> + s->ioreq_evtchn[i] = -1;
> + s->buf_ioreq_evtchn = -1;
> +
> + hvm_init_ioreq_page(d, &s->ioreq);
> + hvm_init_ioreq_page(d, &s->buf_ioreq);
> +
> + d->arch.hvm_domain.ioreq_server = s;
> + return 0;
> +}
> +
> +static void hvm_deinit_ioreq_server(struct domain *d)
> +{
> + struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> +
> + hvm_destroy_ioreq_page(d, &s->ioreq);
> + hvm_destroy_ioreq_page(d, &s->buf_ioreq);
> +
> + xfree(s);
> +}
> +
> +static void hvm_update_ioreq_server_evtchn(struct hvm_ioreq_server *s)
> +{
> + struct domain *d = s->domain;
> +
> + if ( s->ioreq.va != NULL )
> + {
> + shared_iopage_t *p = s->ioreq.va;
> + struct vcpu *v;
> +
> + for_each_vcpu ( d, v )
> + p->vcpu_ioreq[v->vcpu_id].vp_eport = s->ioreq_evtchn[v->vcpu_id];
> + }
> +}
> +
> +static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s, struct vcpu
> *v)
> +{
> + int rc;
> +
> + /* Create ioreq event channel. */
> + rc = alloc_unbound_xen_event_channel(v, s->domid, NULL);
> + if ( rc < 0 )
> + goto done;
> +
> + /* Register ioreq event channel. */
> + s->ioreq_evtchn[v->vcpu_id] = rc;
> +
> + if ( v->vcpu_id == 0 )
> + {
> + /* Create bufioreq event channel. */
> + rc = alloc_unbound_xen_event_channel(v, s->domid, NULL);
> + if ( rc < 0 )
> + goto done;
skipping hvm_update_ioreq_server_evtchn() even in the case of a
successful ioreq event channel?
> +
> + s->buf_ioreq_evtchn = rc;
> + }
> +
> + hvm_update_ioreq_server_evtchn(s);
> + rc = 0;
> +
> +done:
> + return rc;
> +}
> +
> +static void hvm_ioreq_server_remove_vcpu(struct hvm_ioreq_server *s, struct
> vcpu *v)
> +{
> + if ( v->vcpu_id == 0 )
> + {
> + if ( s->buf_ioreq_evtchn >= 0 )
> + {
> + free_xen_event_channel(v, s->buf_ioreq_evtchn);
> + s->buf_ioreq_evtchn = -1;
> + }
> + }
> +
> + if ( s->ioreq_evtchn[v->vcpu_id] >= 0 )
> + {
> + free_xen_event_channel(v, s->ioreq_evtchn[v->vcpu_id]);
> + s->ioreq_evtchn[v->vcpu_id] = -1;
> + }
> +}
> +
> +static int hvm_replace_event_channel(struct vcpu *v, domid_t remote_domid,
> + int *p_port)
> +{
> + int old_port, new_port;
> +
> + new_port = alloc_unbound_xen_event_channel(v, remote_domid, NULL);
> + if ( new_port < 0 )
> + return new_port;
> +
> + /* xchg() ensures that only we call free_xen_event_channel(). */
> + old_port = xchg(p_port, new_port);
> + free_xen_event_channel(v, old_port);
> + return 0;
> +}
> +
> +static int hvm_set_ioreq_server_domid(struct hvm_ioreq_server *s, domid_t
> domid)
> +{
> + struct domain *d = s->domain;
> + struct vcpu *v;
> + int rc = 0;
> +
> + domain_pause(d);
> +
> + if ( d->vcpu[0] )
> + {
> + rc = hvm_replace_event_channel(d->vcpu[0], domid,
> &s->buf_ioreq_evtchn);
> + if ( rc < 0 )
> + goto done;
> + }
> +
> + for_each_vcpu ( d, v )
> + {
> + rc = hvm_replace_event_channel(v, domid,
> &s->ioreq_evtchn[v->vcpu_id]);
> + if ( rc < 0 )
> + goto done;
> + }
> +
> + hvm_update_ioreq_server_evtchn(s);
> +
> + s->domid = domid;
> +
> +done:
> + domain_unpause(d);
> +
> + return rc;
> +}
> +
> +static int hvm_set_ioreq_server_pfn(struct hvm_ioreq_server *s, unsigned
> long pfn)
> +{
> + struct domain *d = s->domain;
> + int rc;
> +
> + rc = hvm_set_ioreq_page(d, &s->ioreq, pfn);
> + if ( rc < 0 )
> + return rc;
> +
> + hvm_update_ioreq_server_evtchn(s);
> +
> + return 0;
> +}
> +
> +static int hvm_set_ioreq_server_buf_pfn(struct hvm_ioreq_server *s, unsigned
> long pfn)
> +{
> + struct domain *d = s->domain;
> +
> + return hvm_set_ioreq_page(d, &s->buf_ioreq, pfn);
Double space.
> +}
> +
> int hvm_domain_initialise(struct domain *d)
> {
> int rc;
> @@ -608,17 +772,20 @@ int hvm_domain_initialise(struct domain *d)
>
> rtc_init(d);
>
> - hvm_init_ioreq_page(d, &d->arch.hvm_domain.ioreq);
> - hvm_init_ioreq_page(d, &d->arch.hvm_domain.buf_ioreq);
> + rc = hvm_init_ioreq_server(d);
> + if ( rc != 0 )
> + goto fail2;
>
> register_portio_handler(d, 0xe9, 1, hvm_print_line);
>
> rc = hvm_funcs.domain_initialise(d);
> if ( rc != 0 )
> - goto fail2;
> + goto fail3;
>
> return 0;
>
> + fail3:
> + hvm_deinit_ioreq_server(d);
> fail2:
> rtc_deinit(d);
> stdvga_deinit(d);
> @@ -642,8 +809,7 @@ void hvm_domain_relinquish_resources(struct domain *d)
> if ( hvm_funcs.nhvm_domain_relinquish_resources )
> hvm_funcs.nhvm_domain_relinquish_resources(d);
>
> - hvm_destroy_ioreq_page(d, &d->arch.hvm_domain.ioreq);
> - hvm_destroy_ioreq_page(d, &d->arch.hvm_domain.buf_ioreq);
> + hvm_deinit_ioreq_server(d);
>
> msixtbl_pt_cleanup(d);
>
> @@ -1155,7 +1321,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
> {
> int rc;
> struct domain *d = v->domain;
> - domid_t dm_domid;
> + struct hvm_ioreq_server *s;
>
> hvm_asid_flush_vcpu(v);
>
> @@ -1198,30 +1364,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
> && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown:
> nestedhvm_vcpu_destroy */
> goto fail5;
>
> - dm_domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> + s = d->arch.hvm_domain.ioreq_server;
>
> - /* Create ioreq event channel. */
> - rc = alloc_unbound_xen_event_channel(v, dm_domid, NULL); /* teardown:
> none */
> + rc = hvm_ioreq_server_add_vcpu(s, v);
> if ( rc < 0 )
> goto fail6;
>
> - /* Register ioreq event channel. */
> - v->arch.hvm_vcpu.xen_port = rc;
> -
> - if ( v->vcpu_id == 0 )
> - {
> - /* Create bufioreq event channel. */
> - rc = alloc_unbound_xen_event_channel(v, dm_domid, NULL); /*
> teardown: none */
> - if ( rc < 0 )
> - goto fail6;
> - d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN] = rc;
> - }
> -
> - spin_lock(&d->arch.hvm_domain.ioreq.lock);
> - if ( d->arch.hvm_domain.ioreq.va != NULL )
> - get_ioreq(v)->vp_eport = v->arch.hvm_vcpu.xen_port;
> - spin_unlock(&d->arch.hvm_domain.ioreq.lock);
> -
> if ( v->vcpu_id == 0 )
> {
> /* NB. All these really belong in hvm_domain_initialise(). */
> @@ -1255,6 +1403,11 @@ int hvm_vcpu_initialise(struct vcpu *v)
>
> void hvm_vcpu_destroy(struct vcpu *v)
> {
> + struct domain *d = v->domain;
> + struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> +
> + hvm_ioreq_server_remove_vcpu(s, v);
> +
> nestedhvm_vcpu_destroy(v);
>
> free_compat_arg_xlat(v);
> @@ -1266,9 +1419,6 @@ void hvm_vcpu_destroy(struct vcpu *v)
> vlapic_destroy(v);
>
> hvm_funcs.vcpu_destroy(v);
> -
> - /* Event channel is already freed by evtchn_destroy(). */
> - /*free_xen_event_channel(v, v->arch.hvm_vcpu.xen_port);*/
> }
>
> void hvm_vcpu_down(struct vcpu *v)
> @@ -1298,8 +1448,10 @@ void hvm_vcpu_down(struct vcpu *v)
> int hvm_buffered_io_send(ioreq_t *p)
> {
> struct vcpu *v = current;
> - struct hvm_ioreq_page *iorp = &v->domain->arch.hvm_domain.buf_ioreq;
> - buffered_iopage_t *pg = iorp->va;
> + struct domain *d = v->domain;
> + struct hvm_ioreq_server *s;
> + struct hvm_ioreq_page *iorp;
> + buffered_iopage_t *pg;
> buf_ioreq_t bp;
> /* Timeoffset sends 64b data, but no address. Use two consecutive slots.
> */
> int qw = 0;
> @@ -1307,6 +1459,13 @@ int hvm_buffered_io_send(ioreq_t *p)
> /* Ensure buffered_iopage fits in a page */
> BUILD_BUG_ON(sizeof(buffered_iopage_t) > PAGE_SIZE);
>
> + s = d->arch.hvm_domain.ioreq_server;
> + if ( !s )
> + return 0;
> +
> + iorp = &s->buf_ioreq;
> + pg = iorp->va;
> +
> /*
> * Return 0 for the cases we can't deal with:
> * - 'addr' is only a 20-bit field, so we cannot address beyond 1MB
> @@ -1367,8 +1526,7 @@ int hvm_buffered_io_send(ioreq_t *p)
> wmb();
> pg->write_pointer += qw ? 2 : 1;
>
> - notify_via_xen_event_channel(v->domain,
> - v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
> + notify_via_xen_event_channel(d, s->buf_ioreq_evtchn);
> spin_unlock(&iorp->lock);
>
> return 1;
> @@ -1376,22 +1534,29 @@ int hvm_buffered_io_send(ioreq_t *p)
>
> bool_t hvm_send_assist_req(struct vcpu *v, ioreq_t *proto_p)
> {
> + struct domain *d = v->domain;
> + struct hvm_ioreq_server *s;
> ioreq_t *p;
>
> if ( unlikely(!vcpu_start_shutdown_deferral(v)) )
> return 0; /* implicitly bins the i/o operation */
>
> - if ( !(p = get_ioreq(v)) )
> + s = d->arch.hvm_domain.ioreq_server;
> + if ( !s )
> return 0;
>
> + p = get_ioreq(s, v->vcpu_id);
> +
> if ( unlikely(p->state != STATE_IOREQ_NONE) )
> {
> /* This indicates a bug in the device model. Crash the domain. */
> gdprintk(XENLOG_ERR, "Device model set bad IO state %d.\n",
> p->state);
> - domain_crash(v->domain);
> + domain_crash(d);
> return 0;
> }
>
> + v->arch.hvm_vcpu.ioreq_server = s;
> +
> p->dir = proto_p->dir;
> p->data_is_ptr = proto_p->data_is_ptr;
> p->type = proto_p->type;
> @@ -1401,14 +1566,14 @@ bool_t hvm_send_assist_req(struct vcpu *v, ioreq_t
> *proto_p)
> p->df = proto_p->df;
> p->data = proto_p->data;
>
> - prepare_wait_on_xen_event_channel(v->arch.hvm_vcpu.xen_port);
> + prepare_wait_on_xen_event_channel(p->vp_eport);
>
> /*
> * Following happens /after/ blocking and setting up ioreq contents.
> * prepare_wait_on_xen_event_channel() is an implicit barrier.
> */
> p->state = STATE_IOREQ_READY;
> - notify_via_xen_event_channel(v->domain, v->arch.hvm_vcpu.xen_port);
> + notify_via_xen_event_channel(d, p->vp_eport);
>
> return 1;
> }
> @@ -3995,21 +4160,6 @@ static int hvmop_flush_tlb_all(void)
> return 0;
> }
>
> -static int hvm_replace_event_channel(struct vcpu *v, domid_t remote_domid,
> - int *p_port)
> -{
> - int old_port, new_port;
> -
> - new_port = alloc_unbound_xen_event_channel(v, remote_domid, NULL);
> - if ( new_port < 0 )
> - return new_port;
> -
> - /* xchg() ensures that only we call free_xen_event_channel(). */
> - old_port = xchg(p_port, new_port);
> - free_xen_event_channel(v, old_port);
> - return 0;
> -}
> -
> long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>
> {
> @@ -4022,7 +4172,7 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> case HVMOP_get_param:
> {
> struct xen_hvm_param a;
> - struct hvm_ioreq_page *iorp;
> + struct hvm_ioreq_server *s;
> struct domain *d;
> struct vcpu *v;
>
> @@ -4048,6 +4198,8 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( rc )
> goto param_fail;
>
> + s = d->arch.hvm_domain.ioreq_server;
> +
This should be reduced in lexical scope, and I would have said that it
can just be 'inlined' into each of the 4 uses later.
> if ( op == HVMOP_set_param )
> {
> rc = 0;
> @@ -4055,19 +4207,10 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> switch ( a.index )
> {
> case HVM_PARAM_IOREQ_PFN:
> - iorp = &d->arch.hvm_domain.ioreq;
> - if ( (rc = hvm_set_ioreq_page(d, iorp, a.value)) != 0 )
> - break;
> - spin_lock(&iorp->lock);
> - if ( iorp->va != NULL )
> - /* Initialise evtchn port info if VCPUs already created.
> */
> - for_each_vcpu ( d, v )
> - get_ioreq(v)->vp_eport = v->arch.hvm_vcpu.xen_port;
> - spin_unlock(&iorp->lock);
> + rc = hvm_set_ioreq_server_pfn(s, a.value);
> break;
> case HVM_PARAM_BUFIOREQ_PFN:
> - iorp = &d->arch.hvm_domain.buf_ioreq;
> - rc = hvm_set_ioreq_page(d, iorp, a.value);
> + rc = hvm_set_ioreq_server_buf_pfn(s, a.value);
> break;
> case HVM_PARAM_CALLBACK_IRQ:
> hvm_set_callback_via(d, a.value);
> @@ -4122,31 +4265,7 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( a.value == DOMID_SELF )
> a.value = curr_d->domain_id;
>
> - rc = 0;
> - domain_pause(d); /* safe to change per-vcpu xen_port */
> - if ( d->vcpu[0] )
> - rc = hvm_replace_event_channel(d->vcpu[0], a.value,
> - (int
> *)&d->vcpu[0]->domain->arch.hvm_domain.params
> - [HVM_PARAM_BUFIOREQ_EVTCHN]);
> - if ( rc )
> - {
> - domain_unpause(d);
> - break;
> - }
> - iorp = &d->arch.hvm_domain.ioreq;
> - for_each_vcpu ( d, v )
> - {
> - rc = hvm_replace_event_channel(v, a.value,
> -
> &v->arch.hvm_vcpu.xen_port);
> - if ( rc )
> - break;
> -
> - spin_lock(&iorp->lock);
> - if ( iorp->va != NULL )
> - get_ioreq(v)->vp_eport = v->arch.hvm_vcpu.xen_port;
> - spin_unlock(&iorp->lock);
> - }
> - domain_unpause(d);
> + rc = hvm_set_ioreq_server_domid(s, a.value);
> break;
> case HVM_PARAM_ACPI_S_STATE:
> /* Not reflexive, as we must domain_pause(). */
> @@ -4241,6 +4360,9 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> switch ( a.index )
> {
> + case HVM_PARAM_BUFIOREQ_EVTCHN:
> + a.value = s->buf_ioreq_evtchn;
> + break;
> case HVM_PARAM_ACPI_S_STATE:
> a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
> break;
> diff --git a/xen/include/asm-x86/hvm/domain.h
> b/xen/include/asm-x86/hvm/domain.h
> index b1e3187..4c039f8 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -41,10 +41,17 @@ struct hvm_ioreq_page {
> void *va;
> };
>
> -struct hvm_domain {
> +struct hvm_ioreq_server {
> + struct domain *domain;
> + domid_t domid;
> struct hvm_ioreq_page ioreq;
> + int ioreq_evtchn[MAX_HVM_VCPUS];
> struct hvm_ioreq_page buf_ioreq;
> + int buf_ioreq_evtchn;
> +};
>
> +struct hvm_domain {
> + struct hvm_ioreq_server *ioreq_server;
> struct pl_time pl_time;
>
> struct hvm_io_handler *io_handler;
> diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
> index 122ab0d..4c9d7ee 100644
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -138,7 +138,7 @@ struct hvm_vcpu {
> spinlock_t tm_lock;
> struct list_head tm_list;
>
> - int xen_port;
> + struct hvm_ioreq_server *ioreq_server;
>
Why do both hvm_vcpu and hvm_domain need ioreq_server pointers? I cant
spot anything which actually uses the vcpu one.
~Andrew
> bool_t flag_dr_dirty;
> bool_t debug_state_latch;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |