Re: [05/17][PATCH] kvm/ia64 : Add head files for kvm/ia64

Re: [05/17][PATCH] kvm/ia64 : Add head files for kvm/ia64

[Jes Sorensen]

Hi Xiantao,

More comments.

Zhang, Xiantao wrote:
>>From 696b9eea9f5001a7b7a07c0e58514aa10306b91a Mon Sep 17 00:00:00 2001
> From: Xiantao Zhang <xiantao.zhang@intel.com>
> 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@intel.com>

> +/*
> + * 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

RE: [05/17][PATCH] kvm/ia64 : Add head files for kvm/ia64

[Zhang, Xiantao]

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@intel.com>
>> 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@intel.com>
> 
>> +/*
>> + * 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

Re: [05/17][PATCH] kvm/ia64 : Add head files for kvm/ia64

[Carsten Otte]

> +/**********************************************************************
> ****
> +  VCPU control register access routines
> +
> ************************************************************************
> **/
> +static inline u64 vcpu_get_itir(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, itir));
> +}
> +
> +static inline void vcpu_set_itir(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, itir) = val;
> +}
> +
> +static inline u64 vcpu_get_ifa(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, ifa));
> +}
> +
> +static inline void vcpu_set_ifa(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, ifa) = val;
> +}
> +
> +static inline u64 vcpu_get_iva(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, iva));
> +}
> +
> +static inline u64 vcpu_get_pta(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, pta));
> +}
> +
> +static inline u64 vcpu_get_lid(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, lid));
> +}
> +
> +static inline u64 vcpu_get_tpr(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, tpr));
> +}
> +
> +static inline u64 vcpu_get_eoi(VCPU *vcpu)
> +{
> +	return (0UL);		/*reads of eoi always return 0 */
> +}
> +
> +static inline u64 vcpu_get_irr0(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, irr[0]));
> +}
> +
> +static inline u64 vcpu_get_irr1(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, irr[1]));
> +}
> +
> +static inline u64 vcpu_get_irr2(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, irr[2]));
> +}
> +
> +static inline u64 vcpu_get_irr3(VCPU *vcpu)
> +{
> +	return ((u64)VCPU(vcpu, irr[3]));
> +}
> +
> +static inline void vcpu_set_dcr(VCPU *vcpu, u64 val)
> +{
> +	ia64_set_dcr(val);
> +}
> +
> +static inline void vcpu_set_isr(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, isr) = val;
> +}
> +
> +static inline void vcpu_set_lid(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, lid) = val;
> +}
> +
> +static inline void vcpu_set_ipsr(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, ipsr) = val;
> +}
> +
> +static inline void vcpu_set_iip(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, iip) = val;
> +}
> +
> +static inline void vcpu_set_ifs(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, ifs) = val;
> +}
> +
> +static inline void vcpu_set_iipa(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, iipa) = val;
> +}
> +
> +static inline void vcpu_set_iha(VCPU *vcpu, u64 val)
> +{
> +	VCPU(vcpu, iha) = val;
> +}
> +
> +
> +static inline u64 vcpu_get_rr(VCPU *vcpu, u64 reg)
> +{
> +	return vcpu->arch.vrr[reg>>61];
> +}
Looks to me like most of them can be replaced by a few macros using 
macro_##.

> +static inline int highest_bits(int *dat)
> +{
> +	u32  bits, bitnum;
> +	int i;
> +
> +	/* loop for all 256 bits */
> +	for (i = 7; i >= 0 ; i --) {
> +		bits = dat[i];
> +		if (bits) {
> +			bitnum = fls(bits);
> +			return i * 32 + bitnum - 1;
> +		}
> +	}
> +	return NULL_VECTOR;
> +}
duplicate to asm/bitops.h find_first_bit().