From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1 of 2 V3] x86/IST: Create set_ist() helper function Date: Fri, 7 Dec 2012 21:01:39 +0000 Message-ID: <50C25933.1050407@citrix.com> References: <43f86afe90be582e1579.1354830149@andrewcoop.uk.xensource.com> <50C1E4F802000078000AEED9@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50C1E4F802000078000AEED9@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 07/12/2012 11:45, Jan Beulich wrote: >>>> On 06.12.12 at 22:42, Andrew Cooper wrote: >> ... to save using open-coded bitwise operations, and update all IST >> manipulation sites to use the function. >> >> Signed-off-by: Andrew Cooper >> >> -- >> >> I am not overly happy with the name set_ist(), and certainly not tied to >> it. However, I am unable to think of a better name. set_idt_ist() is >> wrong, as is set_irq_ist(), while set_idt_entry_ist() just seems to >> cludgy. The comment and parameter types do explicitly state what is >> expected t be passed, but suggestions welcome for a better name. >> >> diff -r bc624b00d6d6 -r 43f86afe90be xen/arch/x86/hvm/svm/svm.c >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -869,9 +869,9 @@ static void svm_ctxt_switch_from(struct >> svm_vmload(per_cpu(root_vmcb, cpu)); >> >> /* Resume use of ISTs now that the host TR is reinstated. */ >> - idt_tables[cpu][TRAP_double_fault].a |= IST_DF << 32; >> - idt_tables[cpu][TRAP_nmi].a |= IST_NMI << 32; >> - idt_tables[cpu][TRAP_machine_check].a |= IST_MCE << 32; >> + set_ist(&idt_tables[cpu][TRAP_double_fault], IST_DF); >> + set_ist(&idt_tables[cpu][TRAP_nmi], IST_NMI); >> + set_ist(&idt_tables[cpu][TRAP_machine_check], IST_MCE); >> } >> >> static void svm_ctxt_switch_to(struct vcpu *v) >> @@ -893,9 +893,9 @@ static void svm_ctxt_switch_to(struct vc >> * Cannot use ISTs for NMI/#MC/#DF while we are running with the guest TR. >> * But this doesn't matter: the IST is only req'd to handle SYSCALL/SYSRET. >> */ >> - idt_tables[cpu][TRAP_double_fault].a &= ~(7UL << 32); >> - idt_tables[cpu][TRAP_nmi].a &= ~(7UL << 32); >> - idt_tables[cpu][TRAP_machine_check].a &= ~(7UL << 32); >> + set_ist(&idt_tables[cpu][TRAP_double_fault], IST_NONE); >> + set_ist(&idt_tables[cpu][TRAP_nmi], IST_NONE); >> + set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE); >> >> svm_restore_dr(v); >> >> diff -r bc624b00d6d6 -r 43f86afe90be xen/arch/x86/x86_64/traps.c >> --- a/xen/arch/x86/x86_64/traps.c >> +++ b/xen/arch/x86/x86_64/traps.c >> @@ -370,9 +370,9 @@ void __devinit subarch_percpu_traps_init >> { >> /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */ >> set_intr_gate(TRAP_double_fault, &double_fault); >> - idt_table[TRAP_double_fault].a |= IST_DF << 32; >> - idt_table[TRAP_nmi].a |= IST_NMI << 32; >> - idt_table[TRAP_machine_check].a |= IST_MCE << 32; >> + set_ist(&idt_table[TRAP_double_fault], IST_DF); >> + set_ist(&idt_table[TRAP_nmi], IST_NMI); >> + set_ist(&idt_table[TRAP_machine_check], IST_MCE); >> >> /* >> * The 32-on-64 hypercall entry vector is only accessible from ring >> 1. >> diff -r bc624b00d6d6 -r 43f86afe90be xen/include/asm-x86/processor.h >> --- a/xen/include/asm-x86/processor.h >> +++ b/xen/include/asm-x86/processor.h >> @@ -425,10 +425,20 @@ struct tss_struct { >> u8 __cacheline_filler[24]; >> } __cacheline_aligned __attribute__((packed)); >> >> -#define IST_DF 1UL >> -#define IST_NMI 2UL >> -#define IST_MCE 3UL >> -#define IST_MAX 3UL >> +#define IST_NONE 0UL >> +#define IST_DF 1UL >> +#define IST_NMI 2UL >> +#define IST_MCE 3UL >> +#define IST_MAX 3UL >> + >> +/* Set the interrupt stack table used by a particular interrupt >> + * descriptor table entry. */ >> +static always_inline void set_ist(idt_entry_t * idt, unsigned long ist) >> +{ >> + /* ist is a 3 bit field, 32 bits into the idt entry. */ >> + ASSERT( ist < 8 ); > This ought to check against IST_MAX. Ok > >> + idt->a = ( idt->a & ~(7UL << 32) ) | ( (ist & 7UL) << 32 ); > And with the check above, the right most & is pretty pointless. > > Jan I was trying to protect against overflowing into the rest of the bits. I would hope that all actual instantiations use the IST_ macros, and the compiler will optimise it away, but in the case that someone calls set_ist(, 100), the compiler will do the right thing. Thinking about it though, anyone calling set_ist() with a value greater than 7 probably has larger bugs in their code. I will remove it. ~Andrew > >> +} >> >> #define IDT_ENTRIES 256 >> extern idt_entry_t idt_table[]; >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > >