Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.11.15 at 22:45, <don.slutz@gmail.com> wrote:
> @@ -133,7 +134,7 @@ static int hvmemul_do_io(
>          p = vio->io_req;
>  
>          /* Verify the emulation request has been correctly re-issued */
> -        if ( (p.type != is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO) ||
> +        if ( (p.type != (is_mmio ? IOREQ_TYPE_COPY : is_vmware ? IOREQ_TYPE_VMWARE_PORT : IOREQ_TYPE_PIO)) ||

Long line needs breaking up.

> @@ -167,26 +168,65 @@ static int hvmemul_do_io(
>          vio->io_req.state = STATE_IOREQ_NONE;
>          break;
>      case X86EMUL_UNHANDLEABLE:
> -    {
> -        struct hvm_ioreq_server *s =
> -            hvm_select_ioreq_server(curr->domain, &p);
> -
> -        /* If there is no suitable backing DM, just ignore accesses */
> -        if ( !s )
> +        if ( !is_vmware )
>          {
> -            rc = hvm_process_io_intercept(&null_handler, &p);
> -            vio->io_req.state = STATE_IOREQ_NONE;
> +            struct hvm_ioreq_server *s =
> +                hvm_select_ioreq_server(curr->domain, &p);
> +
> +            /* If there is no suitable backing DM, just ignore accesses */
> +            if ( !s )
> +            {
> +                rc = hvm_process_io_intercept(&null_handler, &p);
> +                vio->io_req.state = STATE_IOREQ_NONE;
> +            }
> +            else
> +            {
> +                rc = hvm_send_ioreq(s, &p, 0);
> +                if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down )
> +                    vio->io_req.state = STATE_IOREQ_NONE;
> +                else if ( data_is_addr )
> +                    rc = X86EMUL_OKAY;
> +            }
>          }
>          else
>          {
> -            rc = hvm_send_ioreq(s, &p, 0);
> -            if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down )
> +            struct hvm_ioreq_server *s;
> +            vmware_regs_t *vr;
> +
> +            BUILD_BUG_ON(sizeof(ioreq_t) < sizeof(vmware_regs_t));
> +
> +            p.type = IOREQ_TYPE_VMWARE_PORT;
> +            vio->io_req.type = IOREQ_TYPE_VMWARE_PORT;
> +            s = hvm_select_ioreq_server(curr->domain, &p);
> +            vr = get_vmport_regs_any(s, curr);
> +
> +            /*
> +             * If there is no suitable backing DM, just ignore accesses.  If
> +             * we do not have access to registers to pass to QEMU, just
> +             * ignore access.
> +             */
> +            if ( !s || !vr )
> +            {
> +                rc = hvm_process_io_intercept(&null_handler, &p);
>                  vio->io_req.state = STATE_IOREQ_NONE;
> -            else if ( data_is_addr )
> -                rc = X86EMUL_OKAY;
> +            }
> +            else
> +            {
> +                struct cpu_user_regs *regs = guest_cpu_user_regs();

const

> +                p.data = regs->rax;
> +                vr->ebx = regs->_ebx;
> +                vr->ecx = regs->_ecx;
> +                vr->edx = regs->_edx;
> +                vr->esi = regs->_esi;
> +                vr->edi = regs->_edi;
> +
> +                rc = hvm_send_ioreq(s, &p, 0);
> +                if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down )
> +                    vio->io_req.state = STATE_IOREQ_NONE;
> +            }
>          }
>          break;

Especially if there is no common code to be broken out at the
beginning or end of these new if/else branches, then please avoid
re-indenting the entire existing code block (perhaps by making your
new code a if ( unlikely(is_vmware) ) ... else if ( ... ) prefix), to aid
reviewing.

> @@ -536,6 +575,21 @@ void hvm_do_resume(struct vcpu *v)
>          handle_mmio();
>          break;
>      case HVMIO_pio_completion:
> +        if ( vio->io_req.type == IOREQ_TYPE_VMWARE_PORT ) {
> +            vmware_regs_t *vr = get_vmport_regs_any(NULL, v);
> +
> +            if ( vr )
> +            {
> +                struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +                /* 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;
> +            }

Just like in one of the other patches the comment need extending to
say _why_ you only change the 32-bit parts (for future readers to
not consider this a bug).

> @@ -618,22 +672,56 @@ static void hvm_free_ioreq_gmfn(struct domain *d, unsigned long gmfn)
>          set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>  }
>  
> -static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool_t buf)
> +typedef enum {
> +    IOREQ_PAGE_TYPE_IOREQ,
> +    IOREQ_PAGE_TYPE_BUFIOREQ,
> +    IOREQ_PAGE_TYPE_VMPORT,

Lower case please (and this being a local enum the prefix probably
could be shortened).

> +} ioreq_page_type_t;
> +
> +static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, ioreq_page_type_t buf)

The parameter should no longer be named "buf".

> @@ -849,19 +937,32 @@ static void hvm_ioreq_server_remove_all_vcpus(struct hvm_ioreq_server *s)
>  
>  static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
>                                        unsigned long ioreq_pfn,
> -                                      unsigned long bufioreq_pfn)
> +                                      unsigned long bufioreq_pfn,
> +                                      unsigned long vmport_ioreq_pfn)
>  {
>      int rc;
>  
> -    rc = hvm_map_ioreq_page(s, 0, ioreq_pfn);
> +    rc = hvm_map_ioreq_page(s, IOREQ_PAGE_TYPE_IOREQ, ioreq_pfn);
>      if ( rc )
>          return rc;
>  
>      if ( bufioreq_pfn != INVALID_GFN )
> -        rc = hvm_map_ioreq_page(s, 1, bufioreq_pfn);
> +        rc = hvm_map_ioreq_page(s, IOREQ_PAGE_TYPE_BUFIOREQ, bufioreq_pfn);
>  
>      if ( rc )
> -        hvm_unmap_ioreq_page(s, 0);
> +    {
> +        hvm_unmap_ioreq_page(s, IOREQ_PAGE_TYPE_IOREQ);
> +        return rc;
> +    }
> +
> +    rc = hvm_map_ioreq_page(s, IOREQ_PAGE_TYPE_VMPORT, vmport_ioreq_pfn);

I think Paul has already pointed out that this shouldn't be
unconditional. And that not just nor for every server, but also
because the caller doesn't seem to guarantee validity of the passed
in PFN (and its implicit initializer value of zero is certainly not a valid
one to use here).

> @@ -948,12 +1056,38 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
>      for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
>      {
>          char *name;
> +        char *type_name = NULL;
> +        unsigned int limit;
>  
> -        rc = asprintf(&name, "ioreq_server %d %s", s->id,
> -                      (i == HVMOP_IO_RANGE_PORT) ? "port" :
> -                      (i == HVMOP_IO_RANGE_MEMORY) ? "memory" :
> -                      (i == HVMOP_IO_RANGE_PCI) ? "pci" :
> -                      "");
> +        switch ( i )
> +        {
> +        case HVMOP_IO_RANGE_PORT:
> +            type_name = "port";
> +            limit = MAX_NR_IO_RANGES;
> +            break;
> +        case HVMOP_IO_RANGE_MEMORY:
> +            type_name = "memory";
> +            limit = MAX_NR_IO_RANGES;
> +            break;
> +        case HVMOP_IO_RANGE_PCI:
> +            type_name = "pci";
> +            limit = MAX_NR_IO_RANGES;
> +            break;
> +        case HVMOP_IO_RANGE_VMWARE_PORT:
> +            type_name = "VMware port";
> +            limit = 1;
> +            break;

Do you really need to set up a (dummy) range set for this?

> +        case HVMOP_IO_RANGE_TIMEOFFSET:
> +            type_name = "timeoffset";
> +            limit = 1;
> +            break;

This case didn't exist before, and is unrelated to your change.

> @@ -2558,9 +2701,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;

I'd prefer if this shortcut could stay.

> --- a/xen/arch/x86/hvm/vmware/vmport.c
> +++ b/xen/arch/x86/hvm/vmware/vmport.c
> @@ -137,6 +137,20 @@ void vmport_register(struct domain *d)
>      register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
>  }
>  
> +int vmport_check_port(unsigned int port, unsigned int bytes)

bool_t

> +{
> +    struct vcpu *curr = current;

You don't really need this local variable.

> +    struct domain *currd = curr->domain;
> +
> +    if ( port + bytes > BDOOR_PORT && port < BDOOR_PORT + 4 &&

This will match for e.g. port == BDOOR_PORT + 1 and bytes == 4,
which I don't think is correct.

> +         is_hvm_domain(currd) &&
> +         currd->arch.hvm_domain.is_vmware_port_enabled )
> +    {
> +        return 1;
> +    }
> +    return 0;
> +}

All of this could be a single return statement.

> @@ -66,11 +69,25 @@ struct ioreq {
>  };
>  typedef struct ioreq ioreq_t;
>  
> +struct vmware_regs {
> +    uint32_t esi;
> +    uint32_t edi;
> +    uint32_t ebx;
> +    uint32_t ecx;
> +    uint32_t edx;
> +};
> +typedef struct vmware_regs vmware_regs_t;

I doubt you need the typedef (considering the use below), and I
don't think having something prefixed vmware_ in the Xen public
headers is a good idea.

Also throughout the series I didn't find any code addition to
guarantee (perhaps at build time) that BDOOR_PORT doesn't
collide with any other use ports (statically assigned ones as well
as the range used for dynamic assignment to PCI devices).

Jan