From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT Date: Thu, 3 Dec 2015 19:23:27 -0500 Message-ID: <5660DCFF.5050301@Gmail.com> References: <1448747105-19966-1-git-send-email-Don.Slutz@Gmail.com> <1448747105-19966-8-git-send-email-Don.Slutz@Gmail.com> <9AAE0902D5BC7E449B7C8E4E778ABCD02F69F705@AMSPEX01CL01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD02F69F705@AMSPEX01CL01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant , "xen-devel@lists.xen.org" Cc: Kevin Tian , "Keir (Xen.org)" , Ian Campbell , Andrew Cooper , Eddie Dong , George Dunlap , Don Slutz , "Tim (Xen.org)" , Jan Beulich , Stefano Stabellini , Aravind Gopalakrishnan , Jun Nakajima , Ian Jackson , Wei Liu , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org On 12/01/15 06:28, Paul Durrant wrote: >> -----Original Message----- >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- >> bounces@lists.xen.org] On Behalf Of Don Slutz >> Sent: 28 November 2015 21:45 >> To: xen-devel@lists.xen.org >> Cc: Jun Nakajima; Wei Liu; Kevin Tian; Keir (Xen.org); Ian Campbell; George >> Dunlap; Andrew Cooper; Stefano Stabellini; Eddie Dong; Don Slutz; Don Slutz; >> Tim (Xen.org); Aravind Gopalakrishnan; Jan Beulich; Suravee Suthikulpanit; >> Boris Ostrovsky; Ian Jackson >> Subject: [Xen-devel] [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT >> >> From: Don Slutz >> ... >> >> /* 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)) || > > is_vmware already incorporated !is_mmio so there's a redundant > check in that expression. The extra test also makes it look > pretty ugly... probably better re-factored into an if > statement. > Ok, Will add a variable, that is set via an if statement. Thinking about: case STATE_IORESP_READY: + { + uint8_t calc_type = p.type; + + if ( is_vmware ) + calc_type = IOREQ_TYPE_VMWARE_PORT; + vio->io_req.state = STATE_IOREQ_NONE; 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 != calc_type) || >> (p.addr != addr) || >> (p.size != size) || >> (p.count != reps) || ... >> + >> + p.type = IOREQ_TYPE_VMWARE_PORT; >> + vio->io_req.type = IOREQ_TYPE_VMWARE_PORT; > > This could be done in a single statement. > Ok. p.type = vio->io_req.type = IOREQ_TYPE_VMWARE_PORT; or vio->io_req.type = p.type = IOREQ_TYPE_VMWARE_PORT; is clearer to you? >> + s = hvm_select_ioreq_server(curr->domain, &p); ... >> >> 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); > > Is every ioreq server going to have one of these? It doesn't look > like it, so should you not have validity check on the pfn? > Currently the default is that all ioreq servers get the mapping: + /* VMware port */ + if ( i == HVMOP_IO_RANGE_VMWARE_PORT && + s->domain->arch.hvm_domain.is_vmware_port_enabled ) + rc = rangeset_add_range(s->range[i], 1, 1); but you are right that a check on is_vmware_port_enabled should be added. Will do. -Don Slutz > Paul >