Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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 <dslutz@verizon.com>
> >>
> ...
> >>
> >>          /* 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
> >