From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v12 7/8] Add IOREQ_TYPE_VMWARE_PORT Date: Fri, 03 Jul 2015 10:55:34 -0400 Message-ID: <5596A266.8020903@Gmail.com> References: <1435447665-5433-1-git-send-email-Don.Slutz@Gmail.com> <1435447665-5433-8-git-send-email-Don.Slutz@Gmail.com> <20150701204929.GE27014@l.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150701204929.GE27014@l.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: Tim Deegan , Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Eddie Dong , Ian Jackson , Don Slutz , xen-devel@lists.xen.org, George Dunlap , Aravind Gopalakrishnan , Jan Beulich , Andrew Cooper , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org 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 >> >> 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 To: 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 >>