[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>
  • Date: Fri, 20 Sep 2019 08:06:18 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=bitdefender.com; dmarc=pass action=none header.from=bitdefender.com; dkim=pass header.d=bitdefender.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=OI32upjA3SXcTBtGErFX3fbMc5G6GTRK20uo/oue4Yc=; b=WkrlhJiWNyBuBmcFisFRrnEAMspU835B4LJJjB+sklmDwqcC8nyTGGMv/koC2Ktcj6zM4bWgPTxsCS3o+lbru3v8bEAJGB/FuKFL3Xa+JyL5wUyP3cDaiu2Dm+P8kXOh9gAWRpfnkaX8/ieEb4IAmiGbxQS1ZDciuxn/M9dM56Hc7iTE786wyku4Vz98xnvqHoN+4EU0lFJIlzDD5AGvCQPOEBi3+FZBrFfBxEH9d/cx8B/HruXgHIfSFLGMaf7Kmh9mOlq2C9QHgNUB0Ud3LGvJl3TsyfeavKWHvTFW5p4pB2vL3ps6LoAuF8mfd66dAgkzj+9apHdWraJY6uVNig==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FzRYkg4AnIrHy69O0tmt4UDE9NIvlQcqTwVBQDYXSHQDMPIVnIsSn3Z9HlDMk891HXxh8ygIS9qegJyO0L0qo3nCbBXeTbmvwCL8bZ7e0cks7iaTybS44vHv+wn6FE30OSZ/NYe7xiJ8C6Wqkk/WkswIIQnUkCCbn5ZI6I36CZbo/MZm5BvKetvlc4GYxCuxEyBwIasnyQ7O6j9ikAtnkJnCb17Pxjtr/NdyKaNeaRr1ySroMywn4v2l0pdRn6Mi8kN+6Vwfnh7PbFAMfO/n/ul3Kqky++4MfPxiWqn5peY9P9U3ISGskknduQrYcvyaRGl/5zJypUONDlXhlz4AxQ==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=aisaila@xxxxxxxxxxxxxxx;
  • Cc: Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>, "tamas@xxxxxxxxxxxxx" <tamas@xxxxxxxxxxxxx>, "wl@xxxxxxx" <wl@xxxxxxx>, Razvan COJOCARU <rcojocaru@xxxxxxxxxxxxxxx>, "george.dunlap@xxxxxxxxxxxxx" <george.dunlap@xxxxxxxxxxxxx>, "andrew.cooper3@xxxxxxxxxx" <andrew.cooper3@xxxxxxxxxx>, "paul.durrant@xxxxxxxxxx" <paul.durrant@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "roger.pau@xxxxxxxxxx" <roger.pau@xxxxxxxxxx>
  • Delivery-date: Fri, 20 Sep 2019 08:06:39 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVbuqvObi2c4eeME6qC/Xll4ji7KczByuAgAEvi4A=
  • Thread-topic: [PATCH v11] x86/emulate: Send vm_event from emulate


On 19.09.2019 16:59, Jan Beulich wrote:
> On 19.09.2019 15:03, Alexandru Stefan ISAILA wrote:
>> @@ -601,6 +602,7 @@ static void *hvmemul_map_linear_addr(
>>   
>>           case HVMTRANS_gfn_paged_out:
>>           case HVMTRANS_gfn_shared:
>> +        case HVMTRANS_bad_gfn_access:
>>               err = ERR_PTR(~X86EMUL_RETRY);
>>               goto out;
> 
> This looks pretty suspicious now - why would (without knowing all
> the background) "bad access" translate into "retry". While you did
> post the suggested name before, it's nevertheless pretty clear now
> that it needs changing. Perhaps HVMTRANS_need_retry or some such?

It's fine by me, I will change the name to HVMTRANS_need_retry.

> 
>> @@ -1852,6 +1864,8 @@ static int hvmemul_rep_movs(
>>   
>>       xfree(buf);
>>   
>> +    ASSERT(rc != HVMTRANS_bad_gfn_access);
>> +
>>       if ( rc == HVMTRANS_gfn_paged_out )
>>           return X86EMUL_RETRY;
>>       if ( rc == HVMTRANS_gfn_shared )
>> @@ -1964,6 +1978,8 @@ static int hvmemul_rep_stos(
>>           if ( buf != p_data )
>>               xfree(buf);
>>   
>> +        ASSERT(rc != HVMTRANS_bad_gfn_access);
>> +
>>           switch ( rc )
>>           {
>>           case HVMTRANS_gfn_paged_out:
> 
> These are changes to places that were pointed out before do consume
> HVMTRANS_* return values. Did you go through and check nothing else
> needs adjustment? You don't say anything in this regard in the
> description. For example, if shadow's hvm_read() would get to see
> the new value, it would fall out of its switch() into a BUG().
> 

Yes, you are right, the only thing that saves shadow from not raising a 
BUG is the send_event flag. For safety reasons I will have a complete 
check of all the places that can fail from adding the new return value.

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3236,6 +3236,19 @@ static enum hvm_translation_result __hvm_copy(
>>               return HVMTRANS_bad_gfn_to_mfn;
>>           }
>>   
>> +        /*
>> +         * In case a vm event was sent return paged_out so the emulation 
>> will
>> +         * stop with no side effect
>> +         */
>> +        if ( (flags & HVMCOPY_linear) &&
>> +             unlikely(v->arch.vm_event) &&
>> +             v->arch.vm_event->send_event &&
>> +             hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) )
> 
> In such a sequence of checks with _some_ part using unlikely() I
> think it would be better to have the unlikely() one first (unless
> it's a relatively expensive check, which isn't the case here), to
> have as little as possible unnecessary computations / branches in
> the common (fast path) case.

I will change the order in the next version.

> 
> Furthermore while you now restrict the check to linear address
> based accesses, other than the description says (or at least
> implies) you do not restrict it to read and exec accesses. It's
> not clear to me whether that's intentional, yet it affects which
> hvm_copy_*_linear() callers need auditing.

The pfec var is checked in hvm_monitor_check_p2m(). If you think it is 
necessary I can add one more check here for (pfec & (PFEC_insn_fetch | 
PFEC_write_access)).

> 
> Finally, what about ->arch.vm_event->send_event remaining set
> after hvm_emulate_one_vm_event(), because hvm_monitor_check_p2m()
> (the only place where the flag would get cleared) was never hit
> in the process?
Thanks for pointing this out, indeed it's a problem here. A solution can 
be to move send_event = false; after hvm_emulate_one_vm_event() is 
finished. And state in the comment that the user is in charge of 
enabling and disabling the flag.
Or just have it in both places.

> And what about an instruction accessing two (or
> more) distinct addresses? The flag would be clear after the first
> one was checked afaict.

There is no problem here because emulation will stop after the first bad 
access so there is no need to continue.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.