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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 21.11.2018 11:56, Roger Pau Monné wrote:
> 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.

This is a good thing and by this I will remove the new_access assignment 
as well.

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

Yes, that is what I was about to add on v2. I will add a note in the 
commit msg as well.


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

It is vmx specific because svm does not have single step. We talked 
about in the past about this and it turned out that it was to much 
trouble to make a custom single step.

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