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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 09 April 2014 13:21
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v4 6/8] ioreq-server: remove p2m entries when server is
> enabled
> 
> >>> 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.
> 

Ok.

> 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.
> 

Yes, because I'm setting whether the server state is 'enabled' or not. The 
value of the boolean is the end state not the transition, so it's correct to 
use the adjective rather than the verb.

  Paul

> Jan


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