From mboxrd@z Thu Jan  1 00:00:00 1970
From: Don Slutz <dslutz@verizon.com>
Subject: Re: [PATCH v9 09/13] Add xentrace to vmware_port
Date: Mon, 23 Feb 2015 14:13:20 -0500
Message-ID: <54EB7BD0.6010409@terremark.com>
References: <1424127915-27004-1-git-send-email-dslutz@verizon.com>
	<1424127915-27004-10-git-send-email-dslutz@verizon.com>
	<54EB69EE0200007800062BDA@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <54EB69EE0200007800062BDA@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, Don Slutz <dslutz@verizon.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>, Tim Deegan <tim@xen.org>, Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xen.org, Eddie Dong <eddie.dong@intel.com>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>
List-Id: xen-devel@lists.xenproject.org

On 02/23/15 11:57, Jan Beulich wrote:
>>>> On 17.02.15 at 00:05, <dslutz@verizon.com> wrote:
>> @@ -55,8 +56,9 @@ int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>>          uint64_t value;
>>          struct vcpu *curr = current;
>>          struct domain *d = curr->domain;
>> +        uint16_t cmd = regs->_ecx;
>>  
>> -        switch ( regs->_ecx & 0xffff )
>> +        switch ( cmd )
> 
> This surely doesn't belong here.
> 

Ah, this version was missing the diff:

@@ -116,11 +118,20 @@ static int vmport_ioport(int dir, uint32_t port,
uint32_t bytes, uint32_t *val)
             /* Let backing DM handle */
             return X86EMUL_VMPORT_SEND;
         }
+        HVMTRACE_ND7(VMPORT_HANDLED, 0, 0/*cycles*/, 7,
+                     cmd, new_eax, regs->_ebx, regs->_ecx,
+                     regs->_edx, regs->_esi, regs->_edi);
         if ( dir == IOREQ_READ )
             *val = new_eax;
     }
-    else if ( dir == IOREQ_READ )
-        *val = ~0u;
+    else
+    {
+        HVMTRACE_ND7(VMPORT_IGNORED, 0, 0/*cycles*/, 7,
+                     port, regs->_eax, regs->_ebx, regs->_ecx,
+                     regs->_edx, regs->_esi, regs->_edi);
+        if ( dir == IOREQ_READ )
+            *val = ~0u;
+    }

     return X86EMUL_OKAY;
 }

So, should cmd be in this patch or patch #5 (xen: Add vmware_port
support) where you said:

>> +    uint16_t cmd = regs->rcx;
>
> As you already have most other variables needed only inside the if()
> below declared in that scope, please be consistent with this one.
> Albeit the value of this variable is questionable anyway - it's being
> used exactly once.

?

   -Don Slutz

> Jan
>