Re: [Xen-devel] [PATCH RFC v3 2/2] x86/emulate: Send vm_event from emulate

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.02.19 at 13:53, <aisaila@xxxxxxxxxxxxxxx> wrote:
> This patch aims to have mem access vm events sent from the emulator.
> This is useful in the case of page-walks that have to emulate
> instructions in access denied pages.

I'm afraid that I can't make sense of this: How could "page-walks
have to emulate instructions"? Instructions can (and actually will)
cause page walks to occur. And page walks hitting access denied
pages may trigger emulation of the insn having initiated the walk.

> We use hvmemul_map_linear_addr() ro intercept r/w access and
> hvmemul_insn_fetch() to intercept exec access.
> 
> First we try to send a vm event and if the event is sent then emulation
> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
> emulation goes on as expected.

The meaning of this new emulator return value needs explanation.
I notice its #define is also not accompanied by any comment. And
any addition of a new emulator return code should come with a
discussion of how existing users are affected. I'm not going to
exclude that indeed no other adjustments are necessary, but that's
far from obvious. You may recall that it had taken several iterations
to get the addition of X86EMUL_UNIMPLEMENTED right throughout
the code base.

Overall I guess I'm simply not deeply enough into vm-event to
be able to judge whether / how all of this makes sense.

> @@ -530,6 +532,55 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
>      return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
>  }
>  
> +static bool hvmemul_send_vm_event(paddr_t gpa, unsigned long gla, gfn_t gfn,
> +                                  uint32_t pfec, struct hvm_emulate_ctxt 
> *ctxt)

Why both gpa and gfn?

> @@ -704,6 +756,23 @@ static void *hvmemul_map_linear_addr(
>  
>          if ( pfec & PFEC_write_access )
>          {
> +            unsigned long reps = 1;
> +            struct hvm_emulate_ctxt old;
> +            int rc = 0;
> +            paddr_t gpa;
> +
> +            old = *hvmemul_ctxt;
> +            rc = hvmemul_linear_to_phys(addr, &gpa, bytes, &reps,
> +                                        pfec, hvmemul_ctxt);
> +            if ( rc == X86EMUL_EXCEPTION )
> +                *hvmemul_ctxt = old;

Something like this - if it is _really_ needed - has to be accompanied
by a justification. I find it questionable though that you really should
need to save/restore the entire context structure.

But I also don't understand why you need to re-do the translation
here: Just above of this hunk you've altered the call to
hvm_translate_get_page() to give you the gfn. And if there was
a reason to re-do it for the sending of the event, then it should
be restricted to that cases, i.e. un-monitored VMs should not be
impacted.

> @@ -1224,7 +1293,35 @@ int hvmemul_insn_fetch(
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>      /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
>      uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
> +    paddr_t gpa;
> +    uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
> +    unsigned long addr, reps = 1;
> +    int rc;
> +    struct hvm_emulate_ctxt old;
> +
> +    rc = hvmemul_virtual_to_linear(seg, offset, bytes, &reps,
> +                                   hvm_access_insn_fetch, hvmemul_ctxt, 
> &addr);

The double translation is as problematic here, but what's worse:

> +    if ( rc == X86EMUL_EXCEPTION )
> +    {
> +        x86_emul_reset_event(ctxt);
> +        rc = X86EMUL_OKAY;
> +    }

You zap an error here, but ...

> +    if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
> +        pfec |= PFEC_user_mode;
> +
> +    old = *hvmemul_ctxt;
> +    rc = hvmemul_linear_to_phys(addr, &gpa, bytes, &reps,
> +                                pfec, hvmemul_ctxt);

... you happily use "addr" here anyway.

> +    if ( rc == X86EMUL_EXCEPTION )
> +    {
> +        *hvmemul_ctxt = old;
> +        rc = X86EMUL_OKAY;
> +    }
>  
> +    if ( gpa && hvmemul_send_vm_event(gpa, addr, gaddr_to_gfn(gpa),
> +                                      pfec, hvmemul_ctxt) )
> +        return X86EMUL_ACCESS_EXCEPTION;

Is there anything rendering gpa being zero an invalid / impossible
situation?

Jan


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