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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

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

I will refactor the code here so as not to have the 
hvmemul_linear_to_phys() here but rather in the send_event function.

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

In this case hvmemul_virtual_to_linear() can call
x86_emul_hw_exception() and then signals the caller to inject the 
exception. I don;t want to inject a non-user segment here and so I leave 
the ctxt as it was before.

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

The address here is ok to be used or maybe a if ( rc != X86EMUL_OKAY ) 
check can be put after getting the address.

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

In tests I came across gpa == 0 so that is why the check was there I 
will have to test is this can be solved with the X86EMUL_OKAY check from 
above.

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