Re: [Xen-devel] [PATCH v9 08/13] Add IOREQ_TYPE_VMWARE_PORT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.15 at 00:05, <dslutz@xxxxxxxxxxx> wrote:
> @@ -501,22 +542,50 @@ static void hvm_free_ioreq_gmfn(struct domain *d, 
> unsigned long gmfn)
>      clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>  }
>  
> -static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool_t buf)
> +static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, int buf)
>  {
> -    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    struct hvm_ioreq_page *iorp = NULL;
> +
> +    switch ( buf )
> +    {
> +    case 0:
> +        iorp = &s->ioreq;
> +        break;
> +    case 1:
> +        iorp = &s->bufioreq;
> +        break;
> +    case 2:
> +        iorp = &s->vmport_ioreq;
> +        break;
> +    }

These literals should become an enum.

> @@ -2429,9 +2552,6 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct 
> domain *d,
>      if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
>          return NULL;
>  
> -    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> -        return d->arch.hvm_domain.default_ioreq_server;

Shouldn't this rather be amended than deleted?

> @@ -2474,7 +2594,12 @@ struct hvm_ioreq_server 
> *hvm_select_ioreq_server(struct domain *d,
>          BUILD_BUG_ON(IOREQ_TYPE_PIO != HVMOP_IO_RANGE_PORT);
>          BUILD_BUG_ON(IOREQ_TYPE_COPY != HVMOP_IO_RANGE_MEMORY);
>          BUILD_BUG_ON(IOREQ_TYPE_PCI_CONFIG != HVMOP_IO_RANGE_PCI);
> +        BUILD_BUG_ON(IOREQ_TYPE_VMWARE_PORT != HVMOP_IO_RANGE_VMWARE_PORT);
> +        BUILD_BUG_ON(IOREQ_TYPE_TIMEOFFSET != HVMOP_IO_RANGE_TIMEOFFSET);
> +        BUILD_BUG_ON(IOREQ_TYPE_INVALIDATE != HVMOP_IO_RANGE_INVALIDATE);
>          r = s->range[type];
> +        if ( !r )
> +            continue;

Why, all of the sudden?

> @@ -2501,6 +2626,13 @@ struct hvm_ioreq_server 
> *hvm_select_ioreq_server(struct domain *d,
>              }
>  
>              break;
> +        case IOREQ_TYPE_VMWARE_PORT:
> +        case IOREQ_TYPE_TIMEOFFSET:
> +        case IOREQ_TYPE_INVALIDATE:
> +            if ( rangeset_contains_singleton(r, 1) )
> +                return s;

This literal 1 at least needs explanation (i.e. a comment).

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -192,6 +192,21 @@ void hvm_io_assist(ioreq_t *p)
>          (void)handle_mmio();
>          break;
>      case HVMIO_handle_pio_awaiting_completion:
> +        if ( p->type == IOREQ_TYPE_VMWARE_PORT )
> +        {
> +            struct cpu_user_regs *regs = guest_cpu_user_regs();
> +            vmware_regs_t *vr = get_vmport_regs_any(NULL, curr);
> +
> +            if ( vr )
> +            {
> +                /* Only change the 32bit part of the register */
> +                regs->_ebx = vr->ebx;
> +                regs->_ecx = vr->ecx;
> +                regs->_edx = vr->edx;
> +                regs->_esi = vr->esi;
> +                regs->_edi = vr->edi;
> +            }

If you already nicely scope restrict the variables you add, please
move "regs" into the inner if().

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -112,6 +112,8 @@ struct __packed segment_register {
>  #define X86EMUL_RETRY          3
>   /* (cmpxchg accessor): CMPXCHG failed. Maps to X86EMUL_RETRY in caller. */
>  #define X86EMUL_CMPXCHG_FAILED 3
> + /* Send part of registers also to DM. */
> +#define X86EMUL_VMPORT_SEND    4

Introducing a new value here seems rather fragile, as various code
paths you don't touch would need auditing that they do the right
thing upon this value being returned. Plus even conceptually this
doesn't belong here - the instruction emulator shouldn't be concerned
with things like VMware emulation.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -314,6 +314,9 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_ioreq_server_info_t);
>   *
>   * NOTE: unless an emulation request falls entirely within a range mapped
>   * by a secondary emulator, it will not be passed to that emulator.
> + *
> + * NOTE: The 'special' range of 1 is what is checked for outside
> + * of the three types of I/O.
>   */
>  #define HVMOP_map_io_range_to_ioreq_server 19
>  #define HVMOP_unmap_io_range_from_ioreq_server 20
> @@ -324,6 +327,9 @@ struct xen_hvm_io_range {
>  # define HVMOP_IO_RANGE_PORT   0 /* I/O port range */
>  # define HVMOP_IO_RANGE_MEMORY 1 /* MMIO range */
>  # define HVMOP_IO_RANGE_PCI    2 /* PCI segment/bus/dev/func range */
> +# define HVMOP_IO_RANGE_VMWARE_PORT 3 /* VMware port special range */
> +# define HVMOP_IO_RANGE_TIMEOFFSET 7 /* TIMEOFFSET special range */
> +# define HVMOP_IO_RANGE_INVALIDATE 8 /* INVALIDATE special range */

I can't seem to connect the comment you add above to the three
new #define-s.

> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -35,6 +35,7 @@
>  #define IOREQ_TYPE_PIO          0 /* pio */
>  #define IOREQ_TYPE_COPY         1 /* mmio ops */
>  #define IOREQ_TYPE_PCI_CONFIG   2
> +#define IOREQ_TYPE_VMWARE_PORT  3
>  #define IOREQ_TYPE_TIMEOFFSET   7
>  #define IOREQ_TYPE_INVALIDATE   8 /* mapcache */

Are you sure (re-)using a value in the middle is safe? Did you check
where the sparseness originates from?


> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -50,6 +50,8 @@
>  #define HVM_PARAM_PAE_ENABLED  4
>  
>  #define HVM_PARAM_IOREQ_PFN    5
> +/* Extra vmport PFN. */
> +#define HVM_PARAM_VMPORT_REGS_PFN 36
>  
>  #define HVM_PARAM_BUFIOREQ_PFN 6
>  #define HVM_PARAM_BUFIOREQ_EVTCHN 26

I don't think this really belongs here - there's no strong association
with HVM_PARAM_IOREQ_PFN afaict.

Jan

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