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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 01.07.2019 16:13, Jan Beulich wrote:
>>>> On 04.06.19 at 13:49, <aisaila@xxxxxxxxxxxxxxx> 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()

I will use that

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

The pfec check is done because I encountered pfec 0 in tests. It could 
save some work if pfec == 0 when calling the function. Is there
a must in having this check removed? If so then it can be done. The 
send_event will be checked before calling the function as you said.

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

I will cut the default case and treat all of the enum members.

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

There should be no event if there is no access violation. So in this 
case the emulation is continued as expected.

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

We are interested only for the write access on this path. This also 
ensures that pfec is set.

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

I could move the flag to vcpu.arch.vm_event.send_event.

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

This is a good question. By looking at the flow I think I should use 
call the send_event here also. Assuming that having 
hvmemul_ctxt->insn_buf_bytes = 0 if the event was send is enough to have 
the emulation stop then I will give this a test.

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