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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>
  • Date: Fri, 20 Sep 2019 14:59:08 +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=h2W8jOy23Gm90mxwGAMp8Y34qhHyzmDmbqet3Ebe1Qs=; b=Nzs65mLjGU0hkQBPD9djurYhGC/Vmk9lV/PoiPB2s7uOGdSNaQt+Egw9YMI7g0/GxTz3tkW8M15NZlx0wYrrC6pRraDJu5j8rVQ9mzYRzdo1b1LXoS/M9SGR7ZYubD/qvBykdHy8pwHBfxKsG7fiF+CUjf6blLorHnnEs5I/FHclnHLj18lrq6L2iZCs/e/Ovr3GedjBJtZr8IPjgNnkw5GTo3HSp8XnM8lBxh69TJWgfeO9ZjaxwKf2amAmaKuX7xFh9ICyuTswnHo+kEb2VdrHQBbnx/UC65IrkJv4BcyFcGMtoxI1+UxkrSIKpup8o0nMJLfH6QSfLtId2fqlHg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Y/mmTDNMuOb1f5N+pU9a2QhT0jjahm0U/DF6APgaUvu0i4C71QtL7hCWQuN8ive/0FIk0D83sU1tEZWbH58NqtygiQEIRtAZrdY9J8CpJTe33oBR7yEBYmWBvslWZsYcygUO09CBWEiE8QVPUB1J8Ljr0BN11Vbv8USZuK4N5vEqvskni273/IyHPjYcV5HHHSvxKPrZgmBKgRMAdWSGjxsmv05Ed9Zgq5eO766o5fzlZxcRhKpFkhKtbYj4TASNu/TcQf7l7cGuhflTnzT/83SgjUt523oxwmpqh3qPSaqhlhSPEb8QbQ96RmRH5k6evgR3kpv5Nca9UV1QPBuaIA==
  • 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 14:59:24 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVb61ItuWfOmtXxUSrWqDa6jVDyqc0nmqAgAAKH4A=
  • Thread-topic: [PATCH v12] x86/emulate: Send vm_event from emulate


On 20.09.2019 17:22, Jan Beulich wrote:
> On 20.09.2019 14:16, Alexandru Stefan ISAILA wrote:
>> In order to have __hvm_copy() issue ~X86EMUL_RETRY a new return type,
>> HVMTRANS_need_retry, was added and all the places that consume HVMTRANS*
>> and needed adjustment where changed accordingly.
> 
> This is wrong and hence confusing: __hvm_copy() would never return
> ~X86EMUL_RETRY. In fact I think you've confused yourself enough to
> make a questionable (possibly resulting) change:

The idea was to get X86EMUL_RETRY down the line from __hvm_copy().
I will adjust this.

> 
>> @@ -582,7 +583,7 @@ static void *hvmemul_map_linear_addr(
>>           ASSERT(mfn_x(*mfn) == 0);
>>   
>>           res = hvm_translate_get_page(curr, addr, true, pfec,
>> -                                     &pfinfo, &page, NULL, &p2mt);
>> +                                     &pfinfo, &page, &gfn, &p2mt);
> 
> This function ...
> 
>>           switch ( res )
>>           {
>> @@ -601,6 +602,7 @@ static void *hvmemul_map_linear_addr(
>>   
>>           case HVMTRANS_gfn_paged_out:
>>           case HVMTRANS_gfn_shared:
>> +        case HVMTRANS_need_retry:
> 
> ... can't return this value, so you should omit this addition,
> letting the new return value go through "default:".

It is very clear that HVMTRANS_need_retry will not be returned form that 
function. At least for now. I thought you wanted to have every possible 
case covered in the switch. I can remove that case, there is not problem 
here because, like I've said, it will never enter that case.

But as you said later work with HVMTRANS_need_retry will result in 
returning X86EMUL_RETRY. Any way it's your call if I should remove it or 
not.

> 
>> @@ -1852,6 +1864,8 @@ static int hvmemul_rep_movs(
>>   
>>       xfree(buf);
>>   
>> +    ASSERT(rc != HVMTRANS_need_retry);
>> +
>>       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_need_retry);
>> +
>>           switch ( rc )
>>           {
>>           case HVMTRANS_gfn_paged_out:
> 
> Looking at this again, I think it would better be an addition to
> the switch() (using ASSERT_UNREACHABLE()). Generally this is
> true for the rep_movs case as well, but that one would first
> need converting to switch(), which I agree is beyond the scope

I agree that this is beyond the scope of this patch but it's not that 
big of a change and it can be done.

But isn't having a default ASSERT_UNREACHABLE(); in both switch cases 
change the behavior of the function?

> of this change. In both cases a brief comment would seem
> worthwhile adding, clarifying that the new return value can
> result from hvm_copy_*_guest_linear() only. This might become
> relevant in particular if, down the road, we invent more cases
> where HVMTRANS_need_retry is produced.

Is this comment aimed for the commit message or another place?

> 
> Then again maybe switching rep_movs to switch() would still be
> a good thing to do here: Don't you agree that from an abstract
> pov in both cases above X86EMUL_RETRY should be produced, if at
> a future point physical accesses could also produce
> HVMTRANS_need_retry? With this retaining the assertions is
> certainly an option, but I think the fallback return value for
> this case should still be X86EMUL_RETRY.
> 
_______________________________________________
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®.