Re: [PATCH v12 7/8] Add IOREQ_TYPE_VMWARE_PORT

[Don Slutz <don.slutz@gmail.com>]

On 07/01/15 16:49, Konrad Rzeszutek Wilk wrote:
> On Sat, Jun 27, 2015 at 07:27:44PM -0400, Don Slutz wrote:
>> From: Don Slutz <dslutz@verizon.com>
>>
>> This adds synchronization of the 6 vcpu registers (only 32bits of
>> them) that vmport.c needs between Xen and QEMU.
>>
...
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -169,33 +169,88 @@ static int hvmemul_do_io(
>>               vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>>           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 ( vmport_check_port(p.addr, p.size) )
> I would have made this !. Thought if Jan or Andrew said to make it that
> way - then please ignore me.

I have not checked, but I do not remember any direct statement either 
way.  Last was Jan's "looks better".

> That would mean you could also change the function to be 'is_port_vmport' or
> such.
>

Happy to make a change here if needed.  Note: This part of the patch 
conflicts with:


From: Paul Durrant <paul.durrant@citrix.com>
To: <xen-devel@lists.xenproject.org>
Date: Tue, 30 Jun 2015 14:05:43 +0100
Message-ID: <1435669558-5421-2-git-send-email-paul.durrant@citrix.com>
Subject: [Xen-devel] [PATCH v5 01/16] x86/hvm: make sure emulation is
	retried if domain is shutting down


As so is waiting till that is sorted out.

>>           {
>> -            hvm_complete_assist_req(&p);
>> -            rc = X86EMUL_OKAY;
>> -            vio->io_state = HVMIO_none;
...
>> +
>> +                vio->io_state = HVMIO_handle_pio_awaiting_completion;
>> +                rc = hvm_send_assist_req(s, &p);
>> +                if ( rc != X86EMUL_RETRY )
>> +                {
>> +                    vio->io_state = HVMIO_none;
>> +                    if ( rc == X86EMUL_OKAY )
>> +                        rc = X86EMUL_RETRY;
>> +                }
>> +            }
>>           }
>>           break;
>> -    }
>>       default:
>>           BUG();
>>       }
>>   
>>       if ( rc != X86EMUL_OKAY )
>> +    {
>> +        /*
>> +         * If rc is X86EMUL_RETRY and vio->io_state is
>> +         * HVMIO_handle_pio_awaiting_completion, then were are of
> were are? Perhaps 'were'?
>> +         * type IOREQ_TYPE_VMWARE_PORT, so completion in
>> +         * hvm_io_assist() with no re-emulation required
> .. is required.
>

Ok. Plan to change.


With commit 2df1aa01 (and the result of Paul Durrant's change) I was 
considering
just adding a "return X86EMUL_OKAY" as the else of the check on rc 
above, and therefore drop the need for this check and comment.


>> +         */
>> +        if ( rc == X86EMUL_RETRY &&
>> +             vio->io_state == HVMIO_handle_pio_awaiting_completion )
>> +            rc = X86EMUL_OKAY;
>>           return rc;
>> +    }
>>   
>>    finish_access:
>>       if ( dir == IOREQ_READ )
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index f8ec170..ce8cd09 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -394,6 +394,47 @@ static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
>>       return &p->vcpu_ioreq[v->vcpu_id];
>>   }
>>   
>> +static vmware_regs_t *get_vmport_regs_one(struct hvm_ioreq_server *s,
>> +                                          struct vcpu *v)
>> +{
>> +    struct hvm_ioreq_vcpu *sv;
>> +
>> +    list_for_each_entry ( sv,
>> +                          &s->ioreq_vcpu_list,
>> +                          list_entry )
> Could it be just made it one nice line?

I think so, so plan to change.

> .. snip..
>
>> @@ -2507,6 +2637,13 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>>               }
>>   
>>               break;
>> +        case IOREQ_TYPE_VMWARE_PORT:
>> +        case IOREQ_TYPE_TIMEOFFSET:
>> +            /* The 'special' range of [1,1] is checked for being enabled */
> Missing full stop.

Will add.

>> +            if ( rangeset_contains_singleton(r, 1) )
>> +                return s;
>> +
>> +            break;
>>           }
>>       }
>>   
>> @@ -2669,6 +2806,7 @@ void hvm_complete_assist_req(ioreq_t *p)
>>       case IOREQ_TYPE_PCI_CONFIG:
>>           ASSERT_UNREACHABLE();
>>           break;
>> +    case IOREQ_TYPE_VMWARE_PORT:
>>       case IOREQ_TYPE_COPY:
>>       case IOREQ_TYPE_PIO:
>>           if ( p->dir == IOREQ_READ )
>> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
>> index c0964ec..c1379bf 100644
>> --- a/xen/arch/x86/hvm/io.c
>> +++ b/xen/arch/x86/hvm/io.c
>> @@ -192,6 +192,22 @@ void hvm_io_assist(ioreq_t *p)
>>           (void)handle_mmio();
>>           break;
>>       case HVMIO_handle_pio_awaiting_completion:
>> +        if ( p->type == IOREQ_TYPE_VMWARE_PORT )
>> +        {
>> +            vmware_regs_t *vr = get_vmport_regs_any(NULL, curr);
>> +
>> +            if ( vr )
>> +            {
>> +                struct cpu_user_regs *regs = guest_cpu_user_regs();
>> +
>> +                /* Only change the 32bit part of the register */
> Missing full stop.

Will add.

>
>> +                regs->_ebx = vr->ebx;
>> +                regs->_ecx = vr->ecx;
>> +                regs->_edx = vr->edx;
>> +                regs->_esi = vr->esi;
>> +                regs->_edi = vr->edi;
>> +            }
>> +        }
>>           if ( vio->io_size == 4 ) /* Needs zero extension. */
>>               guest_cpu_user_regs()->rax = (uint32_t)p->data;
>>           else
>> diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
>> index f24d8e3..5e14402 100644
>> --- a/xen/arch/x86/hvm/vmware/vmport.c
>> +++ b/xen/arch/x86/hvm/vmware/vmport.c
>> @@ -137,6 +137,19 @@ 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)
>> +{
>> +    struct vcpu *curr = current;
>> +    struct domain *currd = curr->domain;
>> +
>> +    if ( port + bytes > BDOOR_PORT && port < BDOOR_PORT + 4 &&
>> +         is_hvm_domain(currd) &&
>> +         currd->arch.hvm_domain.is_vmware_port_enabled ) {
> The '{' should be on its own line.

Will fix.

>> +        return 0;
>> +    }
>> +    return 1;
>> +}
>> +
...
>>   
>> +struct shared_vmport_iopage {
>> +    struct vmware_regs vcpu_vmport_regs[1];
>> +};
>> +typedef struct shared_vmport_iopage shared_vmport_iopage_t;
>> +
>>   struct buf_ioreq {
>>       uint8_t  type;   /* I/O type                    */
>>       uint8_t  pad:1;
>> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
>> index 7c73089..130eba9 100644
>> --- 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. */
> s/Extra/optional/ ?

I do not think "optional" is good to use here.  This PFN is required for 
QEMU (or any other ioreq server) to handle vmport access.  If you have a 
better name then "extra", please let me know.

    -Don Slutz

>> +#define HVM_PARAM_VMPORT_REGS_PFN 35
>>   
>>   #define HVM_PARAM_BUFIOREQ_PFN 6
>>   #define HVM_PARAM_BUFIOREQ_EVTCHN 26
>> @@ -187,6 +189,6 @@
>>   /* Location of the VM Generation ID in guest physical address space. */
>>   #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>>   
>> -#define HVM_NR_PARAMS          35
>> +#define HVM_NR_PARAMS          36
>>   
>>   #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
>> -- 
>> 1.8.3.1
>>