Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Wen Congyang <wency@cn.fujitsu.com>]

On 09/10/2014 11:06 PM, Aravind Gopalakrishnan wrote:
> On 9/5/2014 4:11 AM, Wen Congyang wrote:
>> In colo mode, secondary vm is running, so VM_ENTRY_INTR_INFO may
>> valid before restoring vmcs. If there is no pending event after
>> restoring vm, we should clear it.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>
>> Also clear pending software exceptions.
>> Copy the fix to SVM as well.
>>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>>
>> vmx part:
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>
>> svm part:
>> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   xen/arch/x86/hvm/svm/svm.c | 15 ++++++++-------
>>   xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++-------------
>>   2 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index b5188e6..053e511 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -321,16 +321,17 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>>           vmcb_set_h_cr3(vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
>>       }
>>   -    if ( c->pending_valid )
>> +    if ( c->pending_valid &&
>> +         hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
>>       {
>>           gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
>>                    c->pending_event, c->error_code);
>> -
>> -        if ( hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
>> -        {
>> -            vmcb->eventinj.bytes = c->pending_event;
>> -            vmcb->eventinj.fields.errorcode = c->error_code;
>> -        }
>> +        vmcb->eventinj.bytes = c->pending_event;
>> +        vmcb->eventinj.fields.errorcode = c->error_code;
>> +    }
>> +    else
>> +    {
>> +        vmcb->eventinj.bytes = 0;
>>       }
>>         vmcb->cleanbits.bytes = 0;
>>
> 
> Hi,
> You mention that this 'fix' is just copied for svm. But you have not seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm equivalent is "vmcb->eventinj.bytes is valid").
> My concern is that we should test colo mode for svm first, since, if the problem is never really seen on svm, then fix may not be _necessary_

Agree.

> 
> At this point, my problems are with test setups. I can help testing scenarios, but as Wen had mentioned, 'colo testing' might be the way to test.
> So, if I can get some pointers to how I can reproduce the issue at hand, then it would be very helpful.

Currently, only COLO can trigger this problem, and COLO is not ready now. So I think this bugfix is not very important now.
If the COLO is ready, and I will give you the way to trigger this problem.

> 
> (Tim had mentioned we could try to simulate it by running a guest that takes lot of faults and save-restore another guest over it. However,
> I am not having much luck following this route. I got a hvm guest to continuously take SW exceptions on all vcpus and tried to save-restore.
> I can't see vmcb->eventinj.bytes containing any valid info.)

I don't understand this way..
If we can trigger this problem by this way, we can verify this patch now.

Thanks
Wen Congyang

> 
> Thanks,
> -Aravind.
> .
>