From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] add support for XCHG instruction accessing APIC Date: Wed, 05 Apr 2006 10:26:03 -0400 Message-ID: <4433D37B.2060102@virtualiron.com> References: <4432E412.1010200@virtualiron.com> <93eed1ff132a9eaf49f74dd3c8d57f5b@cl.cam.ac.uk> <4433CDB4.6080408@virtualiron.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------030109010000030103090009" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------030109010000030103090009 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 --------------030109010000030103090009 Content-Type: text/x-patch; name="xchg.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="xchg.patch" # # Add support for XCHG instruction accessing APIC # Signed-off-by: Boris Ostrovsky 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 --------------030109010000030103090009 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------030109010000030103090009--