From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Durrant Subject: Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT Date: Fri, 4 Dec 2015 08:59:41 +0000 Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD02F6A6099@AMSPEX01CL01.citrite.net> 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> <5660DCFF.5050301@Gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5660DCFF.5050301@Gmail.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Don Slutz , "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 > -----Original Message----- > From: Don Slutz [mailto:don.slutz@gmail.com] > Sent: 04 December 2015 00:23 > To: Paul Durrant; 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; Tim > (Xen.org); Aravind Gopalakrishnan; Jan Beulich; Suravee Suthikulpanit; Boris > Ostrovsky; Ian Jackson > Subject: Re: [Xen-devel] [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT > > 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? I think that the former is probably better. > > >> + 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: > That's probably a bit wasteful. It should probably be selectable in the create HVM op. I don't know whether you'd even need it there in the default server since I guess the QEMU end of things post-dates the use of the HVM op (rather than the old param). > + /* 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. Cool. Thanks, Paul > > -Don Slutz > > > Paul > >