Re: [Xen-devel] [PATCH for-4.5 RFC v2] x86/HVM: Unconditionally crash guests on repeated vmentry failures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.11.14 at 11:26, <tim@xxxxxxx> wrote:
> At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
>> >>> On 26.11.14 at 18:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
>> > identify a failed vmentry and lets it fall into the default case of the
>> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
>> > hardware.
>> 
>> Right; I should have said "something along the lines of v1". An SVM
>> part is needed, but that should probably extend beyond what you
>> proposed in v2: There are a number of "goto exit_and_crash"
>> statements ahead of where you place your addition. I think they
>> all need to be treated similarly.
>> 
>> I therefore think we should revert the VMX part of 28b4baacd5
>> and make SVM behavior consistent with what results for VMX:
>> Crash the guest unconditionally on VMEXIT_INVALID, without
>> looking for recurring VM entry failures. See below/attached.
>> 
>> Jan
>> 
>> x86/HVM: prevent infinite VM entry retries
>> 
>> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
>> guest upon problems occurring in user mode") and gets SVM in line with
>> the resulting VMX behavior. This is because Andrew validly says
>> 
>> "A failed vmentry is overwhelmingly likely to be caused by corrupt
>>  VMC[SB] state.  As a result, injecting a fault and retrying the the
>>  vmentry is likely to fail in the same way."
>> 
>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Looking at the SVM side, AFAICT you're trying to filter out
> VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
> think it's fine just to always crash on nested-SVM failures: we know
> the guest wasn't in user mode because it successfully executed VMRUN.
> And looking at it, the other users of that label are for unexpected
> debugging exits, which can't be caused by the guest userspace either.
> 
> So how about this for the SVM side, reverting to crashing for
> everything except new, unsupported exit types?

Generally a good idea, but there are two paths to exit_and_crash
(for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
think would better crash only conditionally.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2675,16 +2675,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      default:
> -    exit_and_crash:
>          gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", 
> "
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason, 
>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        if ( vmcb_get_cpl(vmcb) )
> +        if ( vmcb_get_cpl(vmcb) ) {
>              hvm_inject_hw_exception(TRAP_invalid_op,
>                                      HVM_DELIVER_NO_ERROR_CODE);
> -        else
> -            domain_crash(v->domain);
> +            break;
> +        }
> +        /* else fall through */
> +    exit_and_crash:
> +        domain_crash(v->domain);
>          break;
>      }

Additionally this re-arrangement would make some domain_crash()
invocations "silent" (i.e. no other accompanying message), but that's
of course easy to fix.

And finally, if doing it that way we might better remove the
exit_and_crash label altogether and call domain_crash() directly
in the places we mean it to be called.

Jan


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