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
|