Jes Sorensen wrote: > Hi Xiantao, Hi, Jes I fixed the coding style issues. Thanks! > More comments. > > Zhang, Xiantao wrote: >>> From 696b9eea9f5001a7b7a07c0e58514aa10306b91a Mon Sep 17 00:00:00 >>> 2001 >> From: Xiantao Zhang <xiantao.zhang@xxxxxxxxx> >> Date: Fri, 28 Mar 2008 09:51:36 +0800 >> Subject: [PATCH] KVM:IA64 : Add head files for kvm/ia64 >> >> ia64_regs: some defintions for special registers >> which aren't defined in asm-ia64/ia64regs. > > Please put missing definitions of registers into asm-ia64/ia64regs.h > if they are official definitions from the spec. Moved! >> kvm_minstate.h : Marcos about Min save routines. >> lapic.h: apic structure definition. >> vcpu.h : routions related to vcpu virtualization. >> vti.h : Some macros or routines for VT support on Itanium. >> Signed-off-by: Xiantao Zhang <xiantao.zhang@xxxxxxxxx> > >> +/* >> + * Flushrs instruction stream. >> + */ >> +#define ia64_flushrs() asm volatile ("flushrs;;":::"memory") + >> +#define ia64_loadrs() asm volatile ("loadrs;;":::"memory") > > Please put these into include/asm-ia64/gcc_intrin.h OK. >> +#define ia64_get_rsc() >> \ >> +({ >> \ >> + unsigned long val; >> \ >> + asm volatile ("mov %0=ar.rsc;;" : "=r"(val) :: "memory"); \ >> + val; >> \ >> +}) >> + >> +#define ia64_set_rsc(val) \ >> + asm volatile ("mov ar.rsc=%0;;" :: "r"(val) : "memory") > > Please update the ia64_get/set_reg macros to handle the RSC register > and use those macros. Moved. >> +#define ia64_get_bspstore() >> \ >> +({ >> \ >> + unsigned long val; >> \ >> + asm volatile ("mov %0=ar.bspstore;;" : "=r"(val) :: "memory"); \ >> + val; >> \ >> +}) > > Ditto for for AR.BSPSTORE > >> +#define ia64_get_rnat() >> \ >> +({ >> \ >> + unsigned long val; >> \ >> + asm volatile ("mov %0=ar.rnat;" : "=r"(val) :: "memory"); \ >> + val; >> \ >> +}) > > Ditto for AR.RNAT > >> +static inline unsigned long ia64_get_itc(void) >> +{ >> + unsigned long result; >> + result = ia64_getreg(_IA64_REG_AR_ITC); >> + return result; >> +} > > This exists in include/asm-ia64/delay.h > >> +static inline void ia64_set_dcr(unsigned long dcr) +{ >> + ia64_setreg(_IA64_REG_CR_DCR, dcr); >> +} > > Please just call ia64_setreg() in your code rather than defining a > wrapper for it. Sure. >> +#define ia64_ttag(addr) >> \ >> +({ >> \ >> + __u64 ia64_intri_res; >> \ >> + asm volatile ("ttag %0=%1" : "=r"(ia64_intri_res) : "r" (addr)); \ >> + ia64_intri_res; >> \ >> +}) > > Please add to include/asm-ia64/gcc_intrin.h instead. > >> diff --git a/arch/ia64/kvm/lapic.h b/arch/ia64/kvm/lapic.h >> new file mode 100644 >> index 0000000..152cbdc >> --- /dev/null >> +++ b/arch/ia64/kvm/lapic.h >> @@ -0,0 +1,27 @@ >> +#ifndef __KVM_IA64_LAPIC_H >> +#define __KVM_IA64_LAPIC_H >> + >> +#include "iodev.h" > > I don't understand why iodev.h is included here? It is inherited from x86 side, and forget to remove it. Seems redundant. >> --- /dev/null >> +++ b/arch/ia64/kvm/vcpu.h > > The formatting of this file is dodgy, please try and make it comply > with the Linux standards in Documentation/CodingStyle > >> +#define _vmm_raw_spin_lock(x) >> \ > [snip] >> + >> +#define _vmm_raw_spin_unlock(x) \ > > Could you explain the reasoning behind these two macros? Whenever I > see open coded spin lock modifications like these, I have to admit I > get a bit worried. In the architecture of kvm/ia64, gvmm and host are in the two different worlds, and gvmm can't call host's interface. In migration case, we need to take a lock to sync the status of dirty memory. In order to make it work, this spin_lock is defined and used. >> +typedef struct kvm_vcpu VCPU; >> +typedef struct kvm_pt_regs REGS; >> +typedef enum { DATA_REF, NA_REF, INST_REF, RSE_REF } vhpt_ref_t; >> +typedef enum { INSTRUCTION, DATA, REGISTER } miss_type; > > ARGH! Please see previous mail about typedefs! I suspect this is code > inherited from Xen ? Xen has a lot of really nasty and pointless > typedefs like these :-( Removed. >> +static inline void vcpu_set_dbr(VCPU *vcpu, u64 reg, u64 val) +{ >> + /* TODO: need to virtualize */ >> + __ia64_set_dbr(reg, val); >> +} >> + >> +static inline void vcpu_set_ibr(VCPU *vcpu, u64 reg, u64 val) +{ >> + /* TODO: need to virtualize */ >> + ia64_set_ibr(reg, val); >> +} >> + >> +static inline u64 vcpu_get_dbr(VCPU *vcpu, u64 reg) +{ >> + /* TODO: need to virtualize */ >> + return ((u64)__ia64_get_dbr(reg)); >> +} >> + >> +static inline u64 vcpu_get_ibr(VCPU *vcpu, u64 reg) +{ >> + /* TODO: need to virtualize */ >> + return ((u64)ia64_get_ibr(reg)); >> +} > > More wrapper macros that really should just use ia64_get/set_reg() > directly in the code. Removed, and used the one without wrapper. > >> diff --git a/arch/ia64/kvm/vti.h b/arch/ia64/kvm/vti.h >> new file mode 100644 >> index 0000000..591ab22 > [ship] >> +/* -*- Mode:C; c-basic-offset:4; tab-width:4; indent-tabs-mode:nil >> -*- */ > > Evil formatting again! > > Cheers, > Jes -- To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |