Re: [PATCH] add support for XCHG instruction accessing APIC

[Boris Ostrovsky <bostrovsky@virtualiron.com>]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

Keir Fraser wrote:

>>
>> My only argument in favor of using the lock would be for completeness 
>> of the emulation. You are
>> absolutely right in that for Linux there seems to be no need to hold 
>> the lock. My concern is that
>> other OSs  may treat this differently. And if we don't have sources, 
>> it may be somewhat difficult
>> to  figure out that the atomicity (or lack of it) was the cause of a 
>> problem.
>>
>> If, however, there is a strong feeling that we don't need the lock, I 
>> am happy to drop it.
>> I guess you are mostly unhappy about adding a new field to 
>> hvm_domain, not about performance
>> impact?
>
>
> Yes, also my second argument was that there is *no way* for two VCPUs 
> to conflict on a local APIC access, since LAPIC accesses are always to 
> the VCPU's own LAPIC. So there is no potential concurrency that needs 
> to be serialised, regardless of the guest OS.


OK, that's fair. Here is updated patch with lock removed.

I don't think I then understand why Linux is using atomic accesses to
local APICs. It's interesting though that 64-bit code doesn't do it ---
they use vanilla apic_write().

-boris

[-- Attachment #2: xchg.patch --]
[-- Type: text/x-patch, Size: 4256 bytes --]

#
# Add support for XCHG instruction accessing APIC
#

Signed-off-by: Boris Ostrovsky <bostrovsky@virtualiron.com>


diff -r d0d3fef37685 xen/Rules.mk
--- a/xen/Rules.mk	Mon Apr  3 14:34:20 2006 +0100
+++ b/xen/Rules.mk	Wed Apr  5 10:18:35 2006 -0400
@@ -31,6 +31,9 @@ HDRS    += $(wildcard $(BASEDIR)/include
 HDRS    += $(wildcard $(BASEDIR)/include/public/*.h)
 HDRS    += $(wildcard $(BASEDIR)/include/asm-$(TARGET_ARCH)/*.h)
 HDRS    += $(wildcard $(BASEDIR)/include/asm-$(TARGET_ARCH)/$(TARGET_SUBARCH)/*.h)
+HDRS    += $(wildcard $(BASEDIR)/include/asm-$(TARGET_ARCH)/hvm/*.h)
+HDRS    += $(wildcard $(BASEDIR)/include/asm-$(TARGET_ARCH)/hvm/svm/*.h)
+HDRS    += $(wildcard $(BASEDIR)/include/asm-$(TARGET_ARCH)/hvm/vmx/*.h)
 # Do not depend on auto-generated header files.
 HDRS    := $(subst $(BASEDIR)/include/asm-$(TARGET_ARCH)/asm-offsets.h,,$(HDRS))
 HDRS    := $(subst $(BASEDIR)/include/xen/banner.h,,$(HDRS))
diff -r d0d3fef37685 xen/arch/x86/hvm/intercept.c
--- a/xen/arch/x86/hvm/intercept.c	Mon Apr  3 14:34:20 2006 +0100
+++ b/xen/arch/x86/hvm/intercept.c	Wed Apr  5 10:18:35 2006 -0400
@@ -123,6 +123,16 @@ static inline void hvm_mmio_access(struc
         req->u.data = tmp1;
         break;
 
+    case IOREQ_TYPE_XCHG:
+        /* 
+         * Note that we don't need to be atomic here since VCPU is accessing
+         * its own local APIC.
+         */
+        tmp1 = read_handler(v, req->addr, req->size);
+        write_handler(v, req->addr, req->size, (unsigned long) req->u.data);
+        req->u.data = tmp1;
+        break;
+
     default:
         printk("error ioreq type for local APIC %x\n", req->type);
         domain_crash_synchronous();
@@ -143,7 +153,7 @@ int hvm_mmio_intercept(ioreq_t *p)
         if ( hvm_mmio_handlers[i]->check_handler(v, p->addr) ) {
             hvm_mmio_access(v, p,
                             hvm_mmio_handlers[i]->read_handler,
-	                    hvm_mmio_handlers[i]->write_handler);
+                            hvm_mmio_handlers[i]->write_handler);
             return 1;
         }
     }
diff -r d0d3fef37685 xen/arch/x86/hvm/platform.c
--- a/xen/arch/x86/hvm/platform.c	Mon Apr  3 14:34:20 2006 +0100
+++ b/xen/arch/x86/hvm/platform.c	Wed Apr  5 10:18:35 2006 -0400
@@ -439,6 +439,14 @@ static int hvm_decode(int realmode, unsi
         GET_OP_SIZE_FOR_BYTE(size_reg);
         return mem_reg(size_reg, opcode, instr, rex);
 
+    case 0x87:  /* xchg {r/m16|r/m32}, {m/r16|m/r32} */
+        instr->instr = INSTR_XCHG;
+        GET_OP_SIZE_FOR_NONEBYTE(instr->op_size);
+        if (((*(opcode+1)) & 0xc7) == 5)
+            return reg_mem(instr->op_size, opcode, instr, rex);
+        else
+            return mem_reg(instr->op_size, opcode, instr, rex);
+
     case 0x88: /* mov r8, m8 */
         instr->instr = INSTR_MOV;
         instr->op_size = BYTE;
@@ -936,6 +944,17 @@ void handle_mmio(unsigned long va, unsig
             break;
         }
 
+    case INSTR_XCHG:
+        mmio_opp->flags = mmio_inst.flags;
+        mmio_opp->instr = mmio_inst.instr;
+        mmio_opp->operand[0] = mmio_inst.operand[0]; /* source */
+        mmio_opp->operand[1] = mmio_inst.operand[1]; /* destination */
+
+        /* send the request and wait for the value */
+        send_mmio_req(IOREQ_TYPE_XCHG, gpa, 1,
+                      mmio_inst.op_size, 0, IOREQ_WRITE, 0);
+        break;
+
     default:
         printf("Unhandled MMIO instruction\n");
         domain_crash_synchronous();
diff -r d0d3fef37685 xen/include/asm-x86/hvm/io.h
--- a/xen/include/asm-x86/hvm/io.h	Mon Apr  3 14:34:20 2006 +0100
+++ b/xen/include/asm-x86/hvm/io.h	Wed Apr  5 10:18:35 2006 -0400
@@ -66,6 +66,7 @@
 #define INSTR_STOS  10
 #define INSTR_TEST  11
 #define INSTR_BT    12
+#define INSTR_XCHG  13
 
 struct instruction {
     __s8    instr;        /* instruction type */
diff -r d0d3fef37685 xen/include/public/hvm/ioreq.h
--- a/xen/include/public/hvm/ioreq.h	Mon Apr  3 14:34:20 2006 +0100
+++ b/xen/include/public/hvm/ioreq.h	Wed Apr  5 10:18:35 2006 -0400
@@ -34,6 +34,7 @@
 #define IOREQ_TYPE_AND          2
 #define IOREQ_TYPE_OR           3
 #define IOREQ_TYPE_XOR          4
+#define IOREQ_TYPE_XCHG         5
 
 /*
  * VMExit dispatcher should cooperate with instruction decoder to

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel