Re: [kvm-unit-tests PATCH v2 03/10] x86: desc: Split IDT entry setup into a generic helper

[Sean Christopherson <seanjc@google.com>]

On Tue, Apr 12, 2022, Varad Gautam wrote:
> EFI bootstrapping code configures a call gate in a later commit to jump
> from 16-bit to 32-bit code.
> 
> Introduce a set_idt_entry_t() routine which can be used to fill both
> an interrupt descriptor and a call gate descriptor on x86.
> 
> Signed-off-by: Varad Gautam <varad.gautam@suse.com>
> ---
>  lib/x86/desc.c | 28 ++++++++++++++++++++++------
>  lib/x86/desc.h |  1 +
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 087e85c..049adeb 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -57,22 +57,38 @@ __attribute__((regparm(1)))
>  #endif
>  void do_handle_exception(struct ex_regs *regs);
>  
> -void set_idt_entry(int vec, void *addr, int dpl)
> +/*
> + * Fill an idt_entry_t, clearing e_sz bytes first.
> + *
> + * This can also be used to set up x86 call gates, since the gate
> + * descriptor layout is identical to idt_entry_t, except for the
> + * absence of .offset2 and .reserved fields. To do so, pass in e_sz
> + * according to the gate descriptor size.
> + */
> +void set_idt_entry_t(idt_entry_t *e, size_t e_sz, void *addr,
> +		u16 sel, u16 type, u16 dpl)

The usage in patch 7, "Transition APs from 16-bit to 32-bit mode" is really confusing
because it's calling an IDT helper to setup the GDT.  Also, the "_t" postfix usually
indicates a typedef, not a function

Rather than set_idt_entry_t, maybe set_oversized_desc_entry()?  That's not very good
either, but it's at least not outright confusing.  Definitely open to other suggestions...

>  {
> -	idt_entry_t *e = &boot_idt[vec];
> -	memset(e, 0, sizeof *e);
> +	memset(e, 0, e_sz);
>  	e->offset0 = (unsigned long)addr;
> -	e->selector = read_cs();
> +	e->selector = sel;
>  	e->ist = 0;
> -	e->type = 14;
> +	e->type = type;
>  	e->dpl = dpl;
>  	e->p = 1;
>  	e->offset1 = (unsigned long)addr >> 16;
>  #ifdef __x86_64__
> -	e->offset2 = (unsigned long)addr >> 32;
> +	if (e_sz == sizeof(*e)) {
> +		e->offset2 = (unsigned long)addr >> 32;
> +	}
>  #endif
>  }
>  
> +void set_idt_entry(int vec, void *addr, int dpl)
> +{
> +	idt_entry_t *e = &boot_idt[vec];
> +	set_idt_entry_t(e, sizeof *e, addr, read_cs(), 14, dpl);
> +}
> +
>  void set_idt_dpl(int vec, u16 dpl)
>  {
>  	idt_entry_t *e = &boot_idt[vec];
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index 3044409..ae0928f 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -217,6 +217,7 @@ unsigned exception_vector(void);
>  int write_cr4_checking(unsigned long val);
>  unsigned exception_error_code(void);
>  bool exception_rflags_rf(void);
> +void set_idt_entry_t(idt_entry_t *e, size_t e_sz, void *addr, u16 sel, u16 type, u16 dpl);
>  void set_idt_entry(int vec, void *addr, int dpl);
>  void set_idt_sel(int vec, u16 sel);
>  void set_idt_dpl(int vec, u16 dpl);
> -- 
> 2.32.0
>