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: 24 November 2016 17:02
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@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 18.11.16 at 18:13, <paul.durrant@citrix.com> wrote:
> > NOTE: The definitions of HVM_IOREQSRV_BUFIOREQ_*,
> HVMOP_IO_RANGE_* and
> >       HVMOP_PCI_SBDF have to persist for new interface versions as
> >       they are already in use by callers of the libxc interface.
> 
> Looking back at my original hvmctl patch, I agree with the first, but
> where did you find uses of the latter two outside of libxc (IOW what
> did I overlook back then)? The libxc interfaces, after all, are meant
> to abstract those away.
> 

You are correct. For some reason I thought the encoding was exposed at the libxc level but it isn't so I can keep the definition inside the #ifdef.

> > --- 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?

> > +    {
> > +        struct xen_dm_op_get_ioreq_server_info *data =
> > +            &op.u.get_ioreq_server_info;
> > +
> > +        rc = hvm_get_ioreq_server_info(d, data->id,
> > +                                       &data->ioreq_pfn,
> > +                                       &data->bufioreq_pfn,
> > +                                       &data->bufioreq_port);
> 
> Before the call you should check the __pad field to be zero
> (presumably also elsewhere).
> 

Yes, I'll go through and check.

> > +    case DMOP_destroy_ioreq_server:
> > +    {
> > +        struct xen_dm_op_destroy_ioreq_server *data =
> > +            &op.u.destroy_ioreq_server;
> > +
> > +        rc = hvm_destroy_ioreq_server(d, data->id);
> > +        break;
> 
> When there are multiple uses of "data" I can see the point of
> using it to help readability, but here I'm unconvinced.

Without it the call to hvm_destroy_ioreq_server() looks unwieldy because the union field specifier makes the line longer than 80 chars. It looked neater this way.

> 
> > --- 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.

> > +/*
> > + * DMOP_create_ioreq_server: Instantiate a new IOREQ Server for a
> secondary
> > + *                           emulator servicing domain <domid>.
> > + *
> > + * The <id> handed back is unique for <domid>. If <handle_bufioreq> is
> zero
> > + * the buffered ioreq ring will not be allocated and hence all emulation
> > + * requestes to this server will be synchronous.
> > + */
> > +#define DMOP_create_ioreq_server 1
> 
> Missing XEN_ prefix.
> 

Yep.

> > +struct xen_dm_op_create_ioreq_server {
> > +    /* IN - should server handle buffered ioreqs */
> > +    uint8_t handle_bufioreq;
> > +#define DMOP_BUFIOREQ_OFF    0
> > +#define DMOP_BUFIOREQ_LEGACY 1
> 
> Again (and of course more below).
> 
> > +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.

> > +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.

> >  struct xen_dm_op {
> >      uint32_t op;
> > +    union {
> 
> Even if no current structure needs it, I think we should have a 32-bit
> padding field ahead of the union right away, to cover (current or
> future) uint64_aligned_t uses inside the union members.
> 
> > @@ -242,6 +243,8 @@ struct xen_hvm_inject_msi {
> >  typedef struct xen_hvm_inject_msi xen_hvm_inject_msi_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_msi_t);
> >
> > +#if __XEN_INTERFACE_VERSION__ < 0x00040900
> > +
> >  /*
> >   * IOREQ Servers
> >   *
> 
> This lives inside a __XEN__ / __XEN_TOOLS__ only region, so does
> not need to be guarded (or the contents preserved).
> 

Ok.

> > @@ -383,6 +370,27 @@ struct xen_hvm_set_ioreq_server_state {
> >  typedef struct xen_hvm_set_ioreq_server_state
> xen_hvm_set_ioreq_server_state_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_ioreq_server_state_t);
> >
> > +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040900 */
> > +
> > +/*
> > + * Definitions relating to HVMOP/DMOP_create_ioreq_server.
> > + */
> > +
> > +#define HVM_IOREQSRV_BUFIOREQ_OFF    DMOP_BUFIOREQ_OFF
> > +#define HVM_IOREQSRV_BUFIOREQ_LEGACY DMOP_BUFIOREQ_LEGACY
> > +#define HVM_IOREQSRV_BUFIOREQ_ATOMIC
> DMOP_BUFIOREQ_ATOMIC
> > +
> > +/*
> > + * Definitions relating to HVMOP/DMOP_map_io_range_to_ioreq_server
> and
> > + * HVMOP/DMOP_unmap_io_range_from_ioreq_server
> > + */
> > +
> > +#define HVMOP_IO_RANGE_PORT   DMOP_IO_RANGE_PORT
> > +#define HVMOP_IO_RANGE_MEMORY DMOP_IO_RANGE_MEMORY
> > +#define HVMOP_IO_RANGE_PCI    DMOP_IO_RANGE_PCI
> > +
> > +#define HVMOP_PCI_SBDF        DMOP_PCI_SBDF
> 
> Instead these additions (or, as said above, any parts thereof
> which really need retaining) should then go into an interface
> version guarded block, as we don't want new code to use them.
> 

Ok. Like with the SBDF definition, I mistakenly thought the range definitions were being used outside of libxc. Since they were tools-only, I should be able to drop them.

> Jan

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