Re: [Xen-devel] [PATCH v4 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.07.15 at 23:25, <ravi.sahita@xxxxxxxxx> wrote:
>> From: Sahita, Ravi
>>Sent: Saturday, July 11, 2015 1:01 PM
>>>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>Sent: Friday, July 10, 2015 2:31 AM
>>>>>> On 10.07.15 at 02:52, <edmund.h.white@xxxxxxxxx> wrote:
>>>> @@ -3825,10 +3826,7 @@ x86_emulate(
>>>>                                     ctxt)) )
>>>>                  goto done;
>>>>              break;
>>>> -        }
>>>> -
>>>> -        if ( modrm == 0xf9 ) /* rdtscp */
>>>> -        {
>>>> +        case 0xf9: /* rdtscp */ {
>>>>              uint64_t tsc_aux;
>>>>              fail_if(ops->read_msr == NULL);
>>>>              if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt))
>>>> !=
>>>> 0 ) @@ -3836,7 +3834,19 @@ x86_emulate(
>>>>              _regs.ecx = (uint32_t)tsc_aux;
>>>>              goto rdtsc;
>>>>          }
>>>> +        case 0xd4: /* vmfunc */
>>>> +            generate_exception_if(lock_prefix | rep_prefix() |
>>>> + (vex.pfx ==
>>>vex_66),
>>>> +                                  EXC_UD, -1);
>>>> +            fail_if(ops->vmfunc == NULL);
>>>> +            if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
>>>> +                goto done;
>>>> +            break;
>>>> +        default:
>>>> +            goto continue_grp7;
>>>> +        }
>>>> +        break;
>>>>
>>>> + continue_grp7:
>>>
>>>Already when first looking at this I disliked this label. Looking at it
>>>again, I'd really like to see it gone: RDTSCP handling already ends in
>>>a goto. Since the only VMFUNC currently implemented doesn't modify any
>>>register state either, its handling could end in an unconditional "goto done"
>>too for now.
>>>And INVLPG, not modifying any register state, could follow suit.
>>>
>>
>>Sure - no issues with that.
>>
> 
> On second thoughts, I cannot really use a goto done for these 2 cases since 
> that will skip the single-step tracing check that's performed in the existing 
> flow.

Good point.

> So I can add a new label entrypoint before the tracing check, or goto 
> writeback (with the dst.type switch there being a wasted check), or I can 
> keep the flow as is - which would you prefer?

I think "goto writeback" for an insn not having any register state to
write back may end up being confusing to future readers. I.e. such
use would need to at least be annotated with a brief comment.
Whether to go that route or add a new label no_writeback or
insn_done or some such (again accompanied by a brief comment)
I'd leave up to you.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel