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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Nov 19, 2018 at 01:30:09PM +0000, Alexandru Stefan ISAILA 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.

So the switch converts required_access using the following relation:

_r   -> r = 1 w = 0 x = 0
_w   -> r = 0 w = 1 x = 0
_x   -> r = 0 w = 0 x = 1
_rx  -> r = 1 w = 1 x = 0
_wx  -> r = 0 w = 1 x = 1
_rw  -> r = 1 w = 1 x = 0
_rwx -> r = 1 w = 1 x = 1

Then the if below performs the following transformation:

r = 0 w = 0 x = 0 -> _n
r = 1 w = 0 x = 0 -> _r
r = 0 w = 1 x = 0 -> _w
r = 0 w = 0 x = 1 -> _x
r = 1 w = 1 x = 0 -> _rw
r = 0 w = 1 x = 1 -> _wx
r = 1 w = 1 x = 0 -> _rw
r = 1 w = 1 x = 1 -> _rwx

I'm not sure I understand this chunk of code, because you end up
getting exactly the same type that you have as the input, and a type
not listed here is just silently passed through, so I don't see the
point in doing this transformation.

> 
> >>       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 by changing this here unconditionally you are removing this
functionality on AMD hardware, which it used to have before by making
use of hvm_emulate_one_vm_event.

I think this needs to at least be written in the commit message.

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

Yes, Jan has also suggested a way to make i unsigned while keeping the
loop backwards, but I don't see the point of performing the loop
backwards if there's no need.

Thanks, Roger.

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