Re: [Xen-devel] [PATCH v3] xen: get rid of paravirt op adjust_exception_frame

[Andrew Cooper]

On 08/08/17 08:02, Juergen Gross wrote:
> On 07/08/17 22:56, Boris Ostrovsky wrote:
>>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>>> index 811e4ddb3f37..a3dcd83187ce 100644
>>> --- a/arch/x86/xen/enlighten_pv.c
>>> +++ b/arch/x86/xen/enlighten_pv.c
>>> @@ -579,6 +579,71 @@ static void xen_write_ldt_entry(struct desc_struct 
>>> *dt, int entrynum,
>>> preempt_enable();
>>>  }
>>>  
>>> +#ifdef CONFIG_X86_64
>>> +static struct {
>>> +   void (*orig)(void);
>>> +   void (*xen)(void);
>>> +   bool ist_okay;
>>> +   bool handle;
>>> +} trap_array[] = {
>>> +   { debug, xen_xendebug, true, true },
>>> +   { int3, xen_xenint3, true, true },
>>> +   { double_fault, xen_double_fault, true, false },
>> Is it really worth adding 'handle' member to the structure because of a
>> single special case? We don't expect to ever have another such vector.
> Hmm, maybe you are right. We don't expect to ever see a double_fault in
> a pv domain, so we could just drop that special case by handling it like
> the other IST traps.

(This is steeped in a lot of history.)  There is no path where Xen will
raise #DF with a PV guest.

As Linux sets the DPL of the #DF handler to 0, the `int $8` emulation
will inject #GP (even for kernel uses!) rather than follow the #DF path.

I do however want to see about making Xen's behaviour rather more
architectural, e.g. raising #NP rather than crashing the guest.  If we
do get to a position of properly using contributory exceptions, #DF will
behave as #MC and #NMI currently do, by either switching from userspace
to the kernel stack, or pushed normally onto the current stack.

In some copious free time, it might also be good to invent a PV ABI to
behave like IST (even if only for the failsafe callback) so Linux can
actually recover sufficiently from a stack overflow to print some
diagnostics.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen: get rid of paravirt op adjust_exception_frame

[Juergen Gross]

On 07/08/17 22:56, Boris Ostrovsky wrote:
> 
>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>> index 811e4ddb3f37..a3dcd83187ce 100644
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -579,6 +579,71 @@ static void xen_write_ldt_entry(struct desc_struct *dt, 
>> int entrynum,
>>  preempt_enable();
>>  }
>>  
>> +#ifdef CONFIG_X86_64
>> +static struct {
>> +void (*orig)(void);
>> +void (*xen)(void);
>> +bool ist_okay;
>> +bool handle;
>> +} trap_array[] = {
>> +{ debug, xen_xendebug, true, true },
>> +{ int3, xen_xenint3, true, true },
>> +{ double_fault, xen_double_fault, true, false },
> 
> Is it really worth adding 'handle' member to the structure because of a
> single special case? We don't expect to ever have another such vector.

Hmm, maybe you are right. We don't expect to ever see a double_fault in
a pv domain, so we could just drop that special case by handling it like
the other IST traps.

> (TBH, I think current implementation of cvt_gate_to_trap() is clearer,
> even if it is not as general as what is in this patch. I know that Andy
> disagrees).

I think being able to concentrate as much pv interface stuff as possible
to Xen specific sources is a win.

The less Xen modifications are needed in non-Xen sources the better.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen: get rid of paravirt op adjust_exception_frame

[Andy Lutomirski]

On Mon, Aug 7, 2017 at 1:56 PM, Boris Ostrovsky
 wrote:
>
>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>> index 811e4ddb3f37..a3dcd83187ce 100644
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -579,6 +579,71 @@ static void xen_write_ldt_entry(struct desc_struct *dt, 
>> int entrynum,
>>   preempt_enable();
>>  }
>>
>> +#ifdef CONFIG_X86_64
>> +static struct {
>> + void (*orig)(void);
>> + void (*xen)(void);
>> + bool ist_okay;
>> + bool handle;
>> +} trap_array[] = {
>> + { debug, xen_xendebug, true, true },
>> + { int3, xen_xenint3, true, true },
>> + { double_fault, xen_double_fault, true, false },
>
> Is it really worth adding 'handle' member to the structure because of a
> single special case? We don't expect to ever have another such vector.
>
> (TBH, I think current implementation of cvt_gate_to_trap() is clearer,
> even if it is not as general as what is in this patch. I know that Andy
> disagrees).

I have no real opinion either way.  I just think it's nicer to put it
in cvt_gate_to_trap() instead of the the traps.c setup code.

--Andy

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen: get rid of paravirt op adjust_exception_frame

[Boris Ostrovsky]

> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 811e4ddb3f37..a3dcd83187ce 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -579,6 +579,71 @@ static void xen_write_ldt_entry(struct desc_struct *dt, 
> int entrynum,
>   preempt_enable();
>  }
>  
> +#ifdef CONFIG_X86_64
> +static struct {
> + void (*orig)(void);
> + void (*xen)(void);
> + bool ist_okay;
> + bool handle;
> +} trap_array[] = {
> + { debug, xen_xendebug, true, true },
> + { int3, xen_xenint3, true, true },
> + { double_fault, xen_double_fault, true, false },

Is it really worth adding 'handle' member to the structure because of a
single special case? We don't expect to ever have another such vector.

(TBH, I think current implementation of cvt_gate_to_trap() is clearer,
even if it is not as general as what is in this patch. I know that Andy
disagrees).

-boris

> +#ifdef CONFIG_X86_MCE
> + { machine_check, xen_machine_check, true, true },
> +#endif
> + { nmi, xen_nmi, true, true },
> + { overflow, xen_overflow, false, true },
> +#ifdef CONFIG_IA32_EMULATION
> + { entry_INT80_compat, xen_entry_INT80_compat, false, true },
> +#endif
> + { page_fault, xen_page_fault, false, true },
> + { divide_error, xen_divide_error, false, true },
> + { bounds, xen_bounds, false, true },
> + { invalid_op, xen_invalid_op, false, true },
> + { device_not_available, xen_device_not_available, false, true },
> + { coprocessor_segment_overrun, xen_coprocessor_segment_overrun,
> +   false, true },
> + { invalid_TSS, xen_invalid_TSS, false, true },
> + { segment_not_present, xen_segment_not_present, false, true },
> + { stack_segment, xen_stack_segment, false, true },
> + { general_protection, xen_general_protection, false, true },
> + { spurious_interrupt_bug, xen_spurious_interrupt_bug, false, true },
> + { coprocessor_error, xen_coprocessor_error, false, true },
> + { alignment_check, xen_alignment_check, false, true },
> + { simd_coprocessor_error, xen_simd_coprocessor_error, false, true },
> +#ifdef CONFIG_TRACING
> + { trace_page_fault, xen_trace_page_fault, false, true },
> +#endif
> +};
> +
> +static bool get_trap_addr(unsigned long *addr, unsigned int ist)
> +{
> + unsigned int nr;
> + bool handle = true, ist_okay = false;
> +
> + /*
> +  * Replace trap handler addresses by Xen specific ones.
> +  * Check for known traps using IST and whitelist them.
> +  * The debugger ones are the only ones we care about.
> +  * Xen will handle faults like double_fault, * so we should never see
> +  * them.  Warn if there's an unexpected IST-using fault handler.
> +  */
> + for (nr = 0; nr < ARRAY_SIZE(trap_array); nr++)
> + if (*addr == (unsigned long)trap_array[nr].orig) {
> + *addr = (unsigned long)trap_array[nr].xen;
> + ist_okay = trap_array[nr].ist_okay;
> + handle = trap_array[nr].handle;
> + break;
> + }
> +
> + if (WARN_ON(ist != 0 && !ist_okay))
> + handle = false;
> +
> + return handle;
> +}
> +#endif
> +
>  static int cvt_gate_to_trap(int vector, const gate_desc *val,
>   struct trap_info *info)
>  {
> @@ -591,40 +656,8 @@ static int cvt_gate_to_trap(int vector, const gate_desc 
> *val,
>  
>   addr = gate_offset(*val);
>  #ifdef CONFIG_X86_64
> - /*
> -  * Look for known traps using IST, and substitute them
> -  * appropriately.  The debugger ones are the only ones we care
> -  * about.  Xen will handle faults like double_fault,
> -  * so we should never see them.  Warn if
> -  * there's an unexpected IST-using fault handler.
> -  */
> - if (addr == (unsigned long)debug)
> - addr = (unsigned long)xen_debug;
> - else if (addr == (unsigned long)int3)
> - addr = (unsigned long)xen_int3;
> - else if (addr == (unsigned long)stack_segment)
> - addr = (unsigned long)xen_stack_segment;
> - else if (addr == (unsigned long)double_fault) {
> - /* Don't need to handle these */
> + if (!get_trap_addr(, val->ist))
>   return 0;
> -#ifdef CONFIG_X86_MCE
> - } else if (addr == (unsigned long)machine_check) {
> - /*
> -  * when xen hypervisor inject vMCE to guest,
> -  * use native mce handler to handle it
> -  */
> - ;
> -#endif
> - } else if (addr == (unsigned long)nmi)
> - /*
> -  * Use the native version as well.
> -  */
> - ;
> - else {
> - /* Some other trap using IST? */
> - if (WARN_ON(val->ist != 0))
> - return 0;
> - }
>  #endif   /* CONFIG_X86_64 */
>   info->address = addr;
>