> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 24 November 2016 17:02
> To: Paul Durrant <[email protected]>
> Cc: Andrew Cooper <[email protected]>; Wei Liu
> <[email protected]>; Ian Jackson <[email protected]>; xen-
> [email protected]; Daniel De Graaf <[email protected]>
> Subject: Re: [Xen-devel] [PATCH-for-4.9 v1 2/8] dm_op: convert
> HVMOP_*ioreq_server*
>
> >>> On 18.11.16 at 18:13, <[email protected]> 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
[email protected]
https://lists.xen.org/xen-devel
