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 03:56:14PM +0000, Alexandru Stefan ISAILA wrote:
> 
> 
> On 19.11.2018 17:08, Roger Pau Monné wrote:
> > 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.
> 
> The first switch is for cur_access and it sets r,w,x accordingly,
> the second switch is required_access where r,w,x are appended
> and then in the last if().. part new_access is assigned according to the
> previous assignments of r,w,x.

I would move the code that converts xenmem_access_t into a separate
helper (as it's used in two different places), and use a bitmap
instead of 3 boolean variables, so you can do:

void convert_access(xenmem_access_t *access, unsigned int *attr)

And don't need to repeat the switch in two different places.

> > 
> >>
> >>>>        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 AMD I could add if (cpu_has_svm()) and call emulate_one_vm_event. 

I would just use hvm_emulate_one_vm_event if
hvm_funcs.start_reexecute_instruction is unset, or else an explanation
needs to be added to the commit message about why
hvm_emulate_one_vm_event is not suitable.

Also, after looking at the code I'm not sure I see why this needs to
be VMX specific, AFAICT it doesn't directly call any VMX functions?

Thanks, Roger.

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