Hi Xiantao,
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.
> 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
> +#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.
> +#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.
> +#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?
> --- /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.
> +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 :-(
> +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.
> 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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
