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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.11.16 at 18:13, <paul.durrant@xxxxxxxxxx> 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.

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

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

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

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

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

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

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

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

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

Jan

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