Mailing List Archive: [PATCH 1 of 2 V3] x86/IST: Create set

[PATCH 1 of 2 V3] x86/IST: Create set_ist() helper function

Dec 6, 2012, 1:42 PM

Post #1 of 3 (189 views)

... to save using open-coded bitwise operations, and update all IST
manipulation sites to use the function.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--

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 );
+ idt->a = ( idt->a & ~(7UL << 32) ) | ( (ist & 7UL) << 32 );
+}

#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

Re: [PATCH 1 of 2 V3] x86/IST: Create set_ist() helper function [ In reply to ]

JBeulich at suse

Dec 7, 2012, 3:45 AM

Post #2 of 3 (183 views)

Permalink

>>> On 06.12.12 at 22:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> ... to save using open-coded bitwise operations, and update all IST
> manipulation sites to use the function.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --
>
> 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.

> + idt->a = ( idt->a & ~(7UL << 32) ) | ( (ist & 7UL) << 32 );

And with the check above, the right most & is pretty pointless.

Jan

> +}
>
> #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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1 of 2 V3] x86/IST: Create set_ist() helper function [ In reply to ]

andrew.cooper3 at citrix

Dec 7, 2012, 1:01 PM

Post #3 of 3 (185 views)

Permalink

On 07/12/2012 11:45, Jan Beulich wrote:
>>>> On 06.12.12 at 22:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> ... to save using open-coded bitwise operations, and update all IST
>> manipulation sites to use the function.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> --
>>
>> 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(<entry>, 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
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel