Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT

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

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?

>> +            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
>