Re: [Xen-devel] [PATCH v1] x86/hvm: Generic instruction re-execution mechanism for execute faults

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

>> +    /* Now transform our RWX values in a XENMEM_access_* constant. */
>> +    if ( r == 0 && w == 0 && x == 0 )
>> +        new_access = XENMEM_access_n;
>> +    else if ( r == 0 && w == 0 && x == 1 )
>> +        new_access = XENMEM_access_x;
>> +    else if ( r == 0 && w == 1 && x == 0 )
>> +        new_access = XENMEM_access_w;
>> +    else if ( r == 0 && w == 1 && x == 1 )
>> +        new_access = XENMEM_access_wx;
>> +    else if ( r == 1 && w == 0 && x == 0 )
>> +        new_access = XENMEM_access_r;
>> +    else if ( r == 1 && w == 0 && x == 1 )
>> +        new_access = XENMEM_access_rx;
>> +    else if ( r == 1 && w == 1 && x == 0 )
>> +        new_access = XENMEM_access_rw;
>> +    else if ( r == 1 && w == 1 && x == 1 )
>> +        new_access = XENMEM_access_rwx;
>> +    else
>> +        new_access = required_access; /* Should never get here. */
> 
> There seems to be a lot of translation from xenmem_access_t to bool
> fields and then to xenmem_access_t again. Can't you just avoid the
> booleans?

The translation is done because the rights are cumulative and I think 
this is the clear way to do this.


>>       if ( vm_event_check_ring(d->vm_event_monitor) &&
>>            d->arch.monitor.inguest_pagefault_disabled &&
>> -         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>> +         npfec.kind != npfec_kind_with_gla &&
>> +         hvm_funcs.start_reexecute_instruction ) /* don't send a mem_event 
>> */
>>       {
>> -        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, 
>> X86_EVENT_NO_EC);
>> -
>> +        v->arch.vm_event->emulate_flags = 0;
>> +        hvm_funcs.start_reexecute_instruction(v, gpa, XENMEM_access_rw);
>>           return true;
>>       }
> 
> Don't you need to fallback to using hvm_emulate_one_vm_event if
> start_reexecute_instruction is not available?

Fallback with hvm_emulate_one_vm_event can result in loosing events.

>> +        for_each_vcpu(d, v)
>> +        {
>> +            if ( !v->arch.rexec_level )
>> +                continue;
>> +
>> +            for ( i = v->arch.rexec_level - 1; i >= 0; i-- )
> 
> Is there any reason this has to be done backwards?
> 
> If you do it from 0 to v->arch.rexec_level you could use an unsigned
> int as the index.

This is done backwards because of the corresponding code in 
vmx_stop_reexecute_instruction() but here it can be turned the other way 
if you insist on i to be unsigned.

>> +#define REEXECUTION_MAX_DEPTH 8
>> +    struct rexec_context_t {
>> +        unsigned long gpa;
>> +        xenmem_access_t old_access;
>> +        xenmem_access_t cur_access;
>> +        bool_t old_single_step;
> 
> bool please
> 
>> +    } rexec_context[REEXECUTION_MAX_DEPTH];
> 
> This is fairly big amount of data that's only used if vm events are
> enabled, could this be allocated on a per-guest basis?

Yes, this can be moved to d->arch.monitor in the next version.

> 
>> +
>> +    int rexec_level;
>> +
>> +    /*
>> +     *  Will be true when the vcpu is in VMX root,
>> +     * false when it is not.
>> +     */
>> +    bool_t in_host;
> 
> bool.
> 
>> +
>>       struct arch_vm_event *vm_event;
>>   
>>       struct vcpu_msrs *msrs;
>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
>> index 3d3250dff0..1f5d43a98d 100644
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -167,6 +167,8 @@ struct hvm_function_table {
>>   
>>       int  (*cpu_up)(void);
>>       void (*cpu_down)(void);
>> +    int  (*start_reexecute_instruction)(struct vcpu *v, unsigned long gpa,
>> +                                        xenmem_access_t required_access);
> 
> I would name this reexecute_instruction, I don't think the start_
> prefix adds any value to the handler.

Sure, I will drop the start on the next version

Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel