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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.11.18 at 14:30, <aisaila@xxxxxxxxxxxxxxx> wrote:
>> > +    /* 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.

But then at the very least don't use == 0 and == 1, but
simple boolean tests.

>>>       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.

But is not doing anything at going to result in even worse a
situation?

Jan



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