Re: [Xen-devel] [PATCH v4 6/8] ioreq-server: remove p2m entries when server is enabled

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.04.14 at 17:11, <paul.durrant@xxxxxxxxxx> wrote:
> For secondary servers, add a hvm op to enable/disable the server. The
> server will not accept IO until it is enabled and the act of enabling
> the server removes its pages from the guest p2m, thus preventing the guest
> from directly mapping the pages and synthesizing ioreqs.

So why do these pages get put into the physmap in the first place?

> +int xc_hvm_set_ioreq_server_state(xc_interface *xch,
> +                                  domid_t domid,
> +                                  ioservid_t id,
> +                                  int enabled)
> +{
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_set_ioreq_server_state_t, arg);
> +    int rc;
> +
> +    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    hypercall.op     = __HYPERVISOR_hvm_op;
> +    hypercall.arg[0] = HVMOP_set_ioreq_server_state;
> +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> +    arg->domid = domid;
> +    arg->id = id;
> +    arg->enabled = enabled;

Truncating int to uint8_t.

> @@ -996,7 +1035,7 @@ static void hvm_destroy_ioreq_server(struct domain *d, 
> ioservid_t id)
>  
>          --d->arch.hvm_domain.ioreq_server_count;
>          list_del_init(&s->list_entry);
> -        
> +

Stray white space cleanup (should be done right in the patch adding
that code).

> +static int hvmop_set_ioreq_server_state(
> +    XEN_GUEST_HANDLE_PARAM(xen_hvm_set_ioreq_server_state_t) uop)
> +{
> +    xen_hvm_set_ioreq_server_state_t op;
> +    struct domain *d;
> +    int rc;
> +
> +    if ( copy_from_guest(&op, uop, 1) )
> +        return -EFAULT;
> +
> +    rc = rcu_lock_remote_domain_by_id(op.domid, &d);
> +    if ( rc != 0 )
> +        return rc;
> +
> +    rc = -EINVAL;
> +    if ( !is_hvm_domain(d) )
> +        goto out;
> +
> +    rc = hvm_set_ioreq_server_state(d, op.id, op.enabled);

So you're converting uint8_t to bool_t here, which presently seems
to do what you want. But I think you'd be better of using !! here.

Also, you're pretty consistently naming the field/variable "enabled"
rather than "enable", despite it being a transition you're invoking
rather than obtaining state.

Jan


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