Re: [Xen-devel] [PATCH v4 2/8] ioreq-server: centralize access to ioreq structures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.04.14 at 17:11, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -173,15 +160,6 @@ static int hvmemul_do_io(
>          return X86EMUL_UNHANDLEABLE;
>      }
>  
> -    if ( p->state != STATE_IOREQ_NONE )
> -    {
> -        gdprintk(XENLOG_WARNING, "WARNING: io already pending (%d)?\n",
> -                 p->state);
> -        if ( ram_page )
> -            put_page(ram_page);
> -        return X86EMUL_UNHANDLEABLE;
> -    }
> -

Shouldn't this be replaced with a call to hvm_io_pending() instead of
just getting deleted?

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -352,6 +352,26 @@ void hvm_migrate_pirqs(struct vcpu *v)
>      spin_unlock(&d->event_lock);
>  }
>  
> +static ioreq_t *get_ioreq(struct vcpu *v)
> +{
> +    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;
> +}
> +
> +bool_t hvm_io_pending(struct vcpu *v)
> +{
> +    ioreq_t *p = get_ioreq(v);
> +
> +    if ( !p )
> +        return 0;
> +
> +    return ( p->state != STATE_IOREQ_NONE );

The parentheses are pointless but a matter of taste (but then again
using them here makes little sense if you don't also use them around
the conditional expression in the function right above), but the blanks
inside them clearly don't belong there according to our coding style.

> @@ -1432,14 +1533,23 @@ bool_t hvm_send_assist_req(struct vcpu *v)
>          return 0;
>      }
>  
> -    prepare_wait_on_xen_event_channel(v->arch.hvm_vcpu.xen_port);
> +    p->dir = proto_p->dir;
> +    p->data_is_ptr = proto_p->data_is_ptr;
> +    p->type = proto_p->type;
> +    p->size = proto_p->size;
> +    p->addr = proto_p->addr;
> +    p->count = proto_p->count;
> +    p->df = proto_p->df;
> +    p->data = proto_p->data;

I realize that you do this piecemeal copying because of wanting to
leave alone ->state. If you didn't have the input pointer const, and
if no caller depended on that field having any specific contents (I'm
sure none of them cares), you could set the input structure's field
first to STATE_IOREQ_NONE and then copy the whole structure in
one go. And that all if the field's value is meaningful at all across the
call to prepare_wait_on_xen_event_channel(), as it gets set to
STATE_IOREQ_READY right afterwards.

Hmm, I see vp_eport also needs to be left untouched. I wonder
whether there shouldn't be a sub-structure with all the fields set
above, and with only that sub-structure getting passed around
after setting up on-stack.

Jan


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