Re: [PATCH-for-4.9 v1 2/8] dm_op: convert HVMOP_*ioreq_server*

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 25 November 2016 09:28
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-
> devel@lists.xenproject.org; Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Subject: RE: [Xen-devel] [PATCH-for-4.9 v1 2/8] dm_op: convert
> HVMOP_*ioreq_server*
> 
> >>> On 25.11.16 at 10:01, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 24 November 2016 17:02
> >> >>> On 18.11.16 at 18:13, <paul.durrant@citrix.com> wrote:
> >> > --- a/xen/arch/x86/hvm/dm.c
> >> > +++ b/xen/arch/x86/hvm/dm.c
> >> > @@ -102,6 +102,61 @@ long do_dm_op(domid_t domid,
> >> >
> >> >      switch ( op.op )
> >> >      {
> >> > +    case DMOP_create_ioreq_server:
> >> > +    {
> >> > +        struct domain *curr_d = current->domain;
> >> > +        struct xen_dm_op_create_ioreq_server *data =
> >> > +            &op.u.create_ioreq_server;
> >> > +
> >> > +        rc = hvm_create_ioreq_server(d, curr_d->domain_id, 0,
> >> > +                                     data->handle_bufioreq, &data->id);
> >> > +        break;
> >> > +    }
> >> > +    case DMOP_get_ioreq_server_info:
> >>
> >> Blank lines between non-fall-through case statements please.
> >
> > Even where there are braces?
> 
> Yes please, because the closing brace alone is no indication of
> whether there is fall-through involved here.
> 
> >> > --- a/xen/include/public/hvm/hvm_op.h
> >> > +++ b/xen/include/public/hvm/hvm_op.h
> >> > @@ -26,6 +26,7 @@
> >> >  #include "../xen.h"
> >> >  #include "../trace.h"
> >> >  #include "../event_channel.h"
> >> > +#include "dm_op.h"
> >>
> >> I'd really wish we could avoid that additional dependency, and I seem
> >> to have got away without in my hvmctl series.
> >
> > I can do that but it means I need to typedef ioservid_t in both headers,
> > which I thought was less preferable.
> 
> Hmm, are there any uses of that type left in this header after you
> actually removed everything that doesn't need to be here anymore?
> 

Ah true. I was still thinking that I needed to keep the old HVMOPs for compatibility but of course I don't so, yes, I can get rid of the inclusion.

  Paul

> >> > +struct xen_dm_op_ioreq_server_range {
> >> > +    /* IN - server id */
> >> > +    ioservid_t id;
> >> > +    uint16_t __pad;
> >> > +    /* IN - type of range */
> >> > +    uint32_t type;
> >>
> >> Any reason for making this 32 bits wide, instead of 16 (and leaving
> >> 32 for future use)?
> >
> > Not really. I could probably shrink it to 8.
> 
> I wouldn't go that far, as then you'd need two padding fields.
> 
> >> > +struct xen_dm_op_set_ioreq_server_state {
> >> > +    /* IN - server id */
> >> > +    ioservid_t id;
> >> > +    uint16_t __pad;
> >> > +    /* IN - enabled? */
> >> > +    uint8_t enabled;
> >> > +};
> >>
> >> Why 16 bits of padding ahead of an 8-bit field, the more that
> >> ioservid_t is also just 16 bits?
> >>
> >
> > That's a mistake. I'll drop it.
> 
> s/drop/change/ I suppose, as you'll need to add tail padding?
> 
> Jan


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