Re: [Xen-devel] [PATCH v8 1/3] ioreq-server: add support for multiple servers

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 20 May 2014 16:28
> To: Paul Durrant
> Cc: Ian Jackson; Stefano Stabellini; xen-devel@xxxxxxxxxxxxx
> Subject: RE: [PATCH v8 1/3] ioreq-server: add support for multiple servers
> 
> >>> On 20.05.14 at 17:06, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 20 May 2014 15:55
> >> To: Paul Durrant
> >> Cc: Ian Jackson; Stefano Stabellini; xen-devel@xxxxxxxxxxxxx
> >> Subject: Re: [PATCH v8 1/3] ioreq-server: add support for multiple servers
> >>
> >> >>> On 19.05.14 at 15:47, <paul.durrant@xxxxxxxxxx> wrote:
> >> > +static int hvm_access_cf8(
> >> > +    int dir, uint32_t port, uint32_t bytes, uint32_t *val)
> >> > +{
> >> > +    struct domain *d = current->domain;
> >> > +
> >> > +    if ( dir == IOREQ_WRITE &&
> >> > +         bytes == 4 )
> >>
> >> Don't you also need to check port == 0xcf8 here?
> >>
> >
> > I don't think I do because (for AFAICT port IO at least) Xen should ensure
> > the only possible 4 byte write that can come in here has to be to cf8. Any
> > access to cf9, cfa, or cfb would have to be less than 4 bytes long.
> 
> Ah, indeed (and the same for MMIO). In that case if you could make
> the if() a single line...
> 

Sure.

> >> > +static ioservid_t next_ioservid(struct domain *d)
> >> > +{
> >> > +    struct hvm_ioreq_server *s;
> >> > +    ioservid_t id;
> >> > +
> >> > +    ASSERT(spin_is_locked(&d->arch.hvm_domain.ioreq_server.lock));
> >> > +
> >> > +    id = d->arch.hvm_domain.ioreq_server.id;
> >> > +
> >> > + again:
> >> > +    id++;
> >> > +
> >> > +    /* Check for uniqueness */
> >> > +    list_for_each_entry ( s,
> >> > +                          &d->arch.hvm_domain.ioreq_server.list,
> >> > +                          list_entry )
> >> > +    {
> >> > +        if (id == s->id)
> >>
> >> The beloved coding style remark again.
> >>
> >
> > Unnecessary braces? Ok.
> 
> No - the braces are a matter of taste. There are missing blanks inside
> the parentheses.
> 

Yeah - I noticed that afterwards; I'll keep the braces then.

  Paul

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