Re: [Xen-devel] [PATCH v2 4/6] ioreq-server: on-demand creation of ioreq server

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.14 at 12:40, Paul Durrant <paul.durrant@xxxxxxxxxx> wrote:
> +static void hvm_init_ioreq_page(struct hvm_ioreq_server *s, int buf)
>  {
> +    struct hvm_ioreq_page *iorp;
> +
> +    iorp = ( buf ) ? &s->buf_ioreq : &s->ioreq;

Pointless parentheses and spaces (also further down).

Also "buf" appears to be boolean, so please reflect this by using
bool_t.

> @@ -557,38 +557,6 @@ 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);
> -}

Odd - you completely remove code here that an earlier patch of
this series added, and you put it back in altered form further down.
The original patch in this case would better add it in the final place
right away, so that the diff actually tells the reader what changes.

> +static int hvm_create_ioreq_server(struct domain *d, domid_t domid)
> +{
> +    struct hvm_ioreq_server *s;
> +    unsigned long pfn;
> +    struct vcpu *v;
> +    int i, rc;
> +
> +    rc = -EEXIST;
> +    if ( d->arch.hvm_domain.ioreq_server != NULL )
> +        goto fail_exist;

Please don't use goto-s without actual need.

> +
> +    gdprintk(XENLOG_INFO, "%s: %d\n", __func__, d->domain_id);

That's a debugging left-over I assume?

> +fail_add_vcpu:
> +    for_each_vcpu ( d, v )
> +        hvm_ioreq_server_remove_vcpu(s, v);
> +    domain_unpause(d);
> +    hvm_destroy_ioreq_page(s, 1);
> +fail_set_buf_ioreq:
> +    hvm_destroy_ioreq_page(s, 0);
> +fail_set_ioreq:
> +    xfree(s);
> +fail_alloc:
> +fail_exist:

Labels should be indented by one space.

> +static void hvm_destroy_ioreq_server(struct domain *d)
> +{
> +    struct hvm_ioreq_server *s;
> +    struct vcpu *v;
> +
> +    gdprintk(XENLOG_INFO, "%s: %d\n", __func__, d->domain_id);

Again a leftover?

> @@ -697,27 +790,6 @@ done:
>      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);
> -}

Again code an earlier patch added getting moved around?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel