Re: [Xen-devel] [PATCH v5] x86/emulate: Send vm_event from emulate

* Re: [Xen-devel] [PATCH v5] x86/emulate: Send vm_event from emulate
@ 2019-07-01 13:13 Jan Beulich
  2019-07-01 14:45 ` Alexandru Stefan ISAILA
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-07-01 13:13 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA
  Cc: kevin.tian, tamas, rcojocaru, Wei Liu, jun.nakajima,
	Andrew Cooper, Tim Deegan, George Dunlap, Paul Durrant,
	suravee.suthikulpanit, xen-devel, Boris Ostrovsky, brian.woods,
	Roger Pau Monne

>>> On 04.06.19 at 13:49, <aisaila@bitdefender.com> wrote:
> This patch aims to have mem access vm events sent from the emulator.
> This is useful where we want to only emulate a page walk without
> checking the EPT, but we still want to check the EPT when emulating
> the instruction that caused the page walk. In this case, the original
> EPT fault is caused by the walk trying to set the accessed or dirty
> bits, but executing the instruction itself might also cause an EPT
> fault if permitted to run, and this second fault should not be lost.

I'm afraid I still can't translate this into what exactly is wanted and
why. While typically we don't use examples to demonstrate that is
wanted in commit messages, I think this is a rather good candidate
for actually using such an approach. This may then ...

> We use hvmemul_map_linear_addr() to intercept r/w access and
> __hvm_copy() to intercept exec access.
> 
> First we try to send a vm event and if the event is sent then emulation
> returns X86EMUL_RETRY in order to stop emulation on instructions that
> use access protected pages. If the event is not sent then the
> emulation goes on as expected.

... also help understanding this part, which I continue to be confused
by, too.

> @@ -530,6 +532,57 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
>      return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
>  }
>  
> +bool hvm_emulate_send_vm_event(unsigned long gla, gfn_t gfn,
> +                               uint32_t pfec, bool send_event)
> +{
> +    xenmem_access_t access;
> +    vm_event_request_t req = {};
> +    paddr_t gpa = ((gfn_x(gfn) << PAGE_SHIFT) | (gla & ~PAGE_MASK));

gfn_to_gaddr()

> +    if ( !send_event || !pfec )
> +        return false;

I think I've said before that the !pfec part need an explanation (in
a comment). Without such an explanation I'm inclined to say it
should be deleted. If otoh this is simply mean to be a shortcut,
then you should really just check the two bits you actually care
about further down.

Similarly I think I've said before that I'm not happy for the common
case to now be to call into here just to bail back out (when VM
events are disabled on a guest). IOW I don't think you should call
into this function in the first place when "send_event" is false.

> +    if ( p2m_get_mem_access(current->domain, gfn, &access,
> +                            altp2m_vcpu_idx(current)) != 0 )
> +        return false;
> +
> +    switch ( access ) {

Style.

> +    case XENMEM_access_x:
> +    case XENMEM_access_rx:
> +        if ( pfec & PFEC_write_access )
> +            req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
> +        break;
> +
> +    case XENMEM_access_w:
> +    case XENMEM_access_rw:
> +        if ( pfec & PFEC_insn_fetch )
> +            req.u.mem_access.flags = MEM_ACCESS_X;
> +        break;
> +
> +    case XENMEM_access_r:
> +    case XENMEM_access_n:
> +        if ( pfec & PFEC_write_access )
> +            req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
> +        if ( pfec & PFEC_insn_fetch )
> +            req.u.mem_access.flags |= MEM_ACCESS_X;
> +        break;
> +
> +    default:
> +        return false;

I think it would be more future proof to not have a default case
here: When a new access enumerator gets introduced, most
compilers would tell the developer right away that this new
enumerator value needs actively handling here.

> +    }
> +
> +    if ( !req.u.mem_access.flags )
> +        return false; /* no violation */

How is the "false" here (I think this is the one the description talks
about) matching up with the various other ones in the function?

> @@ -615,6 +669,13 @@ static void *hvmemul_map_linear_addr(
>  
>          if ( pfec & PFEC_write_access )
>          {
> +            if ( hvm_emulate_send_vm_event(addr, gfn, pfec,
> +                                           hvmemul_ctxt->send_event) )
> +            {
> +                err = ERR_PTR(~X86EMUL_RETRY);
> +                goto out;
> +            }

How come this sits only on the write path?

> @@ -1115,7 +1176,8 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
>       * clean up any interim state.
>       */
>      if ( !hvmemul_find_mmio_cache(vio, addr, IOREQ_READ, false) )
> -        rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
> +        rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo,
> +                                        hvmemul_ctxt->send_event);

I'm not very happy to see this new parameter/argument addition.
Did you consider putting the flag of interest elsewhere (into a
structure hanging off of current, or into pagefault_info_t)?

Furthermore, if the parameter is really unavoidable, then please
separate the mechanics of introducing it from the actual change
you're after.

> @@ -2629,7 +2692,7 @@ void hvm_emulate_init_per_insn(
>               hvm_copy_from_guest_linear(hvmemul_ctxt->insn_buf, addr,
>                                          sizeof(hvmemul_ctxt->insn_buf),
>                                          pfec | PFEC_insn_fetch,
> -                                        NULL) == HVMTRANS_okay) ?
> +                                        NULL, false) == HVMTRANS_okay) ?

If you pass false here, what's the point of handling insn fetches
in the new function you add?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread