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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.16 at 10:01, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 24 November 2016 17:02
>> >>> On 18.11.16 at 18:13, <paul.durrant@xxxxxxxxxx> 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?

>> > +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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel