Re: [Xen-devel] [PATCH v5 4/9] ioreq-server: create basic ioreq server abstraction.

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 06 May 2014 13:55
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v5 4/9] ioreq-server: create basic ioreq server
> abstraction.
> 
> >>> On 01.05.14 at 14:08, <paul.durrant@xxxxxxxxxx> wrote:
> > Collect together data structures concerning device emulation together into
> > a new struct hvm_ioreq_server.
> >
> > Code that deals with the shared and buffered ioreq pages is extracted from
> > functions such as hvm_domain_initialise, hvm_vcpu_initialise and
> do_hvm_op
> > and consolidated into a set of hvm_ioreq_server manipulation functions.
> The
> > lock in the hvm_ioreq_page served two different purposes and has been
> > replaced by separate locks in the hvm_ioreq_server structure.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> 
> >  bool_t hvm_io_pending(struct vcpu *v)
> >  {
> > -    ioreq_t *p = get_ioreq(v);
> > +    struct hvm_ioreq_server *s = v->domain-
> >arch.hvm_domain.ioreq_server;
> > +    ioreq_t *p;
> >
> > -    if ( !p )
> > +    if ( !s )
> >          return 0;
> >
> > +    p = get_ioreq(s, v);
> >      return p->state != STATE_IOREQ_NONE;
> 
> I don't think you need the variable "p" here anymore.
> 

I left it in because I generally dislike dereferencing function returns 
directly. That's probably just my sense of aesthetic though.

> > @@ -426,14 +431,6 @@ void hvm_do_resume(struct vcpu *v)
> >      }
> >  }
> >
> > -static void hvm_init_ioreq_page(
> > -    struct domain *d, struct hvm_ioreq_page *iorp)
> > -{
> > -    memset(iorp, 0, sizeof(*iorp));
> > -    spin_lock_init(&iorp->lock);
> > -    domain_pause(d);
> 
> So where is this ...

Nowhere. As I said in the checkin comment, the lock has gone and the 
domain_pause() and subsequent domain_unpause() were always unnecessary AFAICT. 
I think the intention was that the domain was not unpaused until both the IOREQ 
PFNs were set, but since the PFNs are set in the domain build code in the 
toolstack I can't see why this was needed.

> 
> > @@ -513,22 +507,15 @@ static int hvm_map_ioreq_page(
> >      if ( (rc = prepare_ring_for_helper(d, gmfn, &page, &va)) )
> >          return rc;
> >
> > -    spin_lock(&iorp->lock);
> > -
> >      if ( (iorp->va != NULL) || d->is_dying )
> >      {
> >          destroy_ring_for_helper(&va, page);
> > -        spin_unlock(&iorp->lock);
> >          return -EINVAL;
> >      }
> >
> >      iorp->va = va;
> >      iorp->page = page;
> >
> > -    spin_unlock(&iorp->lock);
> > -
> > -    domain_unpause(d);
> 
> ... and this going? Or is the pausing no longer needed for a non-
> obvious reason?

As I said above, I don't think it was actually ever needed.

> 
> > +static int hvm_set_ioreq_pfn(struct domain *d, bool_t buf,
> > +                             unsigned long pfn)
> > +{
> > +    struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> > +    int rc;
> > +
> > +    spin_lock(&s->lock);
> > +
> > +    rc = hvm_map_ioreq_page(s, buf, pfn);
> > +    if ( rc )
> > +        goto fail;
> > +
> > +    if (!buf) {
> 
> Coding style. I'm afraid this isn't the first time I have to make such a
> remark on this series. Please check style before submitting.
> 

Oops. Sorry. It would be nice if there were a checkpatch script. 

> > +        struct hvm_ioreq_vcpu *sv;
> > +
> > +        list_for_each_entry ( sv,
> > +                              &s->ioreq_vcpu_list,
> > +                              list_entry )
> > +            hvm_update_ioreq_evtchn(s, sv);
> > +    }
> > +
> > +    spin_unlock(&s->lock);
> > +    return 0;
> > +
> > + fail:
> > +    spin_unlock(&s->lock);
> > +    return rc;
> > +}
> 
> If the function isn't going to change significantly in subsequent
> patches, there's no real point here in having the "fail" label and
> separate error exit path.
> 

Ok.

> > +static int hvm_replace_event_channel(struct vcpu *v, domid_t
> remote_domid,
> > +                                     evtchn_port_t *p_port)
> > +{
> > +    evtchn_port_t old_port, new_port;
> > +
> > +    new_port = alloc_unbound_xen_event_channel(v, remote_domid,
> NULL);
> > +    if ( new_port < 0 )
> > +        return new_port;
> 
> I'm pretty sure I commented on this too in a previous version:
> evtchn_port_t is an unsigned type, hence checking it to be negative
> is pointless.

Yes, but as I'm pretty sure I responded, alloc_unbound_xen_event_channel() 
doesn't return an evtchn_port_t!

  Paul

> 
> Jan


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