On Wed, Jul 11, 2007 at 03:52:15PM +0200, Dietmar Hahn wrote:
> Hi
My comments...
> diff -r 87b0b6a08dbd -r 44ccb8aa58cc xen/arch/ia64/xen/domain.c
> --- a/xen/arch/ia64/xen/domain.c Mon Jul 9 09:22:58 2007 -0600
> +++ b/xen/arch/ia64/xen/domain.c Wed Jul 11 15:37:09 2007 +0200
> @@ -262,6 +262,9 @@ void context_switch(struct vcpu *prev, s
> load_region_regs(current);
> ia64_set_pta(vcpu_pta(current));
> vcpu_load_kernel_regs(current);
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> + vcpu_load_pkr_regs(current);
> +#endif
You should put the test from vpu_load_pkr_regs here. This will make
the conditionnal load obvious.
>
> switch (vector) {
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> + case 6:
> + vector = IA64_INST_KEY_MISS_VECTOR;
> + break;
> + case 7:
> + vector = IA64_DATA_KEY_MISS_VECTOR;
> + break;
> +#endif
No need of the ifdef here.
> .phys_add_size = 44,
> .key_size = 16,
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> + .max_pkr = NPKRS,
> +#else
> .max_pkr = 15,
> +#endif
No need of the ifdef. You should renames NPKRS to XEN_IA64_NPKRS to make
it obvious it is xen/ia64 macro only.
> // PAGE_SIZE!)
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> +u64 translate_domain_pte(u64 pteval, u64 address, u64 itir__, u64* logps,
> + u64* key, struct p2m_entry* entry)
> +#else
> u64 translate_domain_pte(u64 pteval, u64 address, u64 itir__, u64* logps,
> struct p2m_entry* entry)
> +#endif
Arghh... Replace u64 *logps by u64 *itir. There is no need to split logps and
key from itir. The ifdef also makes the code difficult to read.
> + * cache.
> + */
> +static inline void
> +ia64_itc_PKR (__u64 target_mask, __u64 vmaddr, __u64 pte,
> + __u64 log_page_size, __u64 key)
If log_page_size and key are merged into itir, no need to define this
function.
> @@ -284,8 +332,10 @@ IA64FAULT vcpu_reset_psr_sm(VCPU * vcpu,
> // just handle psr.up and psr.pp for now
> if (imm24 & ~(IA64_PSR_BE | IA64_PSR_PP | IA64_PSR_UP | IA64_PSR_SP |
> IA64_PSR_I | IA64_PSR_IC | IA64_PSR_DT |
> - IA64_PSR_DFL | IA64_PSR_DFH))
> + IA64_PSR_DFL | IA64_PSR_DFH | IA64_PSR_PK))
> + {
> return IA64_ILLOP_FAULT;
> + }
Style: no newline before {.
> @@ -340,9 +401,12 @@ IA64FAULT vcpu_set_psr_sm(VCPU * vcpu, u
> // just handle psr.sp,pp and psr.i,ic (and user mask) for now
> mask =
> IA64_PSR_PP | IA64_PSR_SP | IA64_PSR_I | IA64_PSR_IC | IA64_PSR_UM |
> - IA64_PSR_DT | IA64_PSR_DFL | IA64_PSR_DFH | IA64_PSR_BE;
> + IA64_PSR_DT | IA64_PSR_DFL | IA64_PSR_DFH | IA64_PSR_BE |
> + IA64_PSR_PK;
> if (imm24 & ~mask)
> + {
> return IA64_ILLOP_FAULT;
> + }
Style.
> IA64FAULT vcpu_get_pkr(VCPU * vcpu, u64 reg, u64 * pval)
> {
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> + if (reg > NPKRS) /* index to large */
> + return IA64_RSVDREG_FAULT;
> + *pval = (u64) ia64_get_pkr(reg);
> + return IA64_NO_FAULT;
> +#endif
I think there is a leak here: a domain can read pkrs from other domain.
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> +void
> +vcpu_itc_no_srlz(VCPU * vcpu, u64 IorD, u64 vaddr, u64 pte,
> + u64 mp_pte, u64 logps, u64 key, struct p2m_entry *entry)
> +#else
> void
> vcpu_itc_no_srlz(VCPU * vcpu, u64 IorD, u64 vaddr, u64 pte,
> u64 mp_pte, u64 logps, struct p2m_entry *entry)
> +#endif
Merge key and logps into itir.
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> +void vhpt_insert (unsigned long vadr, unsigned long pte, unsigned long itir)
> +#else
> void vhpt_insert (unsigned long vadr, unsigned long pte, unsigned long logps)
> +#endif
Keep only the itir declaration.
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> +void vhpt_multiple_insert(unsigned long vaddr, unsigned long pte,
> + unsigned long logps, unsigned long key)
> +#else
> void vhpt_multiple_insert(unsigned long vaddr, unsigned long pte, unsigned
> long logps)
> +#endif
Merge.
> diff -r 87b0b6a08dbd -r 44ccb8aa58cc xen/include/asm-ia64/xenkregs.h
> --- a/xen/include/asm-ia64/xenkregs.h Mon Jul 9 09:22:58 2007 -0600
> +++ b/xen/include/asm-ia64/xenkregs.h Wed Jul 11 15:37:09 2007 +0200
> @@ -47,4 +47,22 @@
> #define IA64_ITIR_PS_KEY(_ps, _key) (((_ps) << IA64_ITIR_PS) | \
> (((_key) << IA64_ITIR_KEY)))
>
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> +
> +/* Define Protection Key Register (PKR) */
> +#define IA64_PKR_V 0
> +#define IA64_PKR_WD 1
> +#define IA64_PKR_RD 2
> +#define IA64_PKR_XD 3
> +#define IA64_PKR_MBZ0 4
> +#define IA64_PKR_KEY 8
> +#define IA64_PKR_KEY_LEN 24
> +#define IA64_PKR_MBZ1 32
> +
> +#define IA64_PKR_VALID (1 << IA64_PKR_V)
> +#define IA64_PKR_KEY_MASK (((__IA64_UL(1)<<IA64_PKR_KEY_LEN)-1) \
> + <<IA64_PKR_KEY)
> +#endif /* defined(CONFIG_XEN_IA64_USE_PKR) */
> +
> +
No need of the ifdef.
> +#if defined(CONFIG_XEN_IA64_USE_PKR)
> +typedef union {
> + u64 val;
> + struct {
> + u64 v : 1;
> + u64 wd : 1;
> + u64 rd : 1;
> + u64 xd : 1;
> + u64 reserved1 : 4;
> + u64 key : 24;
> + u64 reserved2 : 32;
> + };
> +} ia64_pkr_t;
> +#endif /* defined(CONFIG_XEN_IA64_USE_PKR) */
No need of the ifdef.
> --- a/xen/include/public/arch-ia64.h Mon Jul 9 09:22:58 2007 -0600
> +++ b/xen/include/public/arch-ia64.h Wed Jul 11 15:37:09 2007 +0200
> @@ -236,7 +236,9 @@ struct mapped_regs {
> int banknum; // 0 or 1, which virtual register bank is active
> unsigned long rrs[8]; // region registers
> unsigned long krs[8]; // kernel registers
> +#if !defined(CONFIG_XEN_IA64_USE_PKR)
> unsigned long pkrs[8]; // protection key registers
> +#endif
> unsigned long tmp[8]; // temp registers (e.g. for hyperprivops)
> };
Cf Isaku's comment.
To sum up: nothing fundamental, I'd just prefer a more integrated patch
before merging. I'd propose to get rid of CONFIG_XEN_IA64_USE_PKR. CONFIG_
macros do not really make the code more readable.
Thank you for working on pkr!
Tristan.
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel
|
Home