Re: [Xen-devel] [PATCH v2 2/4] x86/hvm: take a reference on ioreq server emulating domain

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 20 March 2018 16:37
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/4] x86/hvm: take a reference on ioreq server
> emulating domain
> 
> >>> On 16.03.18 at 17:58, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -305,6 +305,7 @@ static int hvm_ioreq_server_add_vcpu(struct
> hvm_ioreq_server *s,
> >                                       bool is_default, struct vcpu *v)
> >  {
> >      struct hvm_ioreq_vcpu *sv;
> > +    domid_t domid;
> >      int rc;
> >
> >      sv = xzalloc(struct hvm_ioreq_vcpu);
> > @@ -315,7 +316,9 @@ static int hvm_ioreq_server_add_vcpu(struct
> hvm_ioreq_server *s,
> >
> >      spin_lock(&s->lock);
> >
> > -    rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id, s-
> >domid,
> > +    domid = s->emulator->domain_id;
> > +
> > +    rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id,
> domid,
> >                                           NULL);
> >      if ( rc < 0 )
> >          goto fail2;
> > @@ -324,9 +327,9 @@ static int hvm_ioreq_server_add_vcpu(struct
> hvm_ioreq_server *s,
> >
> >      if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
> >      {
> > -        struct domain *d = s->domain;
> > +        struct domain *d = s->target;
> >
> > -        rc = alloc_unbound_xen_event_channel(v->domain, 0, s->domid,
> NULL);
> > +        rc = alloc_unbound_xen_event_channel(v->domain, 0, domid, NULL);
> 
> Especially for this second call site it would probably be more clear
> at the first glance which domain is meant if you didn't latch
> s->emulator->domain_id into a local variable.

Ok, I can get rid of that.

> 
> > @@ -602,12 +605,17 @@ static int hvm_ioreq_server_init(struct
> hvm_ioreq_server *s,
> >                                   struct domain *d, bool is_default,
> >                                   int bufioreq_handling, ioservid_t id)
> >  {
> > +    struct domain *currd = current->domain;
> >      struct vcpu *v;
> >      int rc;
> >
> >      s->id = id;
> > -    s->domain = d;
> > -    s->domid = current->domain->domain_id;
> > +    s->target = d;
> > +
> > +    if ( !get_domain(currd) )
> > +        return -EACCES;
> 
> Use get_knownalive_domain() here, which cannot fail?
> 

Oh, didn't know about that... sounds better.

> > @@ -651,6 +660,8 @@ static void hvm_ioreq_server_deinit(struct
> hvm_ioreq_server *s,
> >      hvm_ioreq_server_remove_all_vcpus(s);
> >      hvm_ioreq_server_unmap_pages(s, is_default);
> >      hvm_ioreq_server_free_rangesets(s, is_default);
> > +
> > +    put_domain(s->emulator);
> >  }
> 
> Could you clarify in the description that this runs in the context of
> XEN_DOMCTL_destroydomain and hence there's no risk of target
> and emulator preventing each other from being cleaned up?
> 

Ok, sure.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel