Re: [Xen-devel] [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.01.17 at 18:29, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/01/17 16:19, Jan Beulich wrote:
>>>>> On 03.01.17 at 16:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 03/01/17 13:10, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>>>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>>>  }
>>>>  
>>>> +static int hvmemul_validate(
>>>> +    const struct x86_emulate_state *state,
>>>> +    struct x86_emulate_ctxt *ctxt)
>>>> +{
>>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>> +
>>>> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, 
>>>> hvmemul_ctxt)
>>>> +                                  : X86EMUL_OKAY;
>>> There is nothing hvm-specific about any of the validation functions, and
>>> x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
>>> hvm-specific varients.
>>>
>>> Do you forsee any validation which would need to peek into hvmeml_ctxt? 
>>> I can't think of anything off the top of my head.
>>>
>>> If not, this would be cleaner and shorter to have an x86emul_validate_t
>>> based interface, always passing const struct x86_emulate_ctxt *ctxt.
>> I had thought about this, but it feels like a layering violation to
>> pass a pointer to a function taking x86_emulate_ctxt to functions
>> in the HVM emulation group. Even if it involves adding slightly more
>> code, I think it would better stay this way.
> 
> Given that one structure is embedded in the other, I am less concerned
> about this being a layering violation.
> 
> I was specifically thinking along the line of not needing hvm and sh
> stubs to call into x86_insn_is_mem_access(), as the hvm/sh nature isn't
> relevant to the operation.

Let me get a 3rd opinion then - Tim, if such filtering was added for
shadow mode code, would you rather see them go straight to an
x86_insn_is_*() function, or have a proper sh_*() layer in between?

Thanks, Jan


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