> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 25 November 2016 09:28
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Ian Jackson
> <ian.jack...@citrix.com>; Wei Liu <wei.l...@citrix.com>; xen-
> de...@lists.xenproject.org; Daniel De Graaf <dgde...@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.durr...@citrix.com> wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 24 November 2016 17:02
> >> >>> On 18.11.16 at 18:13, <paul.durr...@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
