[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: Mon, 23 Sep 2019 09:00:55 +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=0lVOoA9er69sbHdGQBICX6R4TdDfGQyHLlccK/9OQWk=; b=O/oKXWDIQlUu29o8cAGHOm10wDgHcbBIknNFAH61ollz6wT3ioN1Fe/ERy9Ve1ztJstkuAKVVvlS+UQI5ocZ+0cWMWG5fEG2T4b10++8CRajc51HsgCwfqVJiUwsTop1ebqrW28kUkSS8k+W8PqaVGg+nvpU3aqVhMlCEBND2fbEIhPPrRPsaLQgRfDrKeqwJ9T1+VSCIkkV/aBIvOkaAvQ3oIPD/1Gh+alFgA1ImwJoRd7iAdaBuW6dpl0GBiNZ46Hg7p0wWM7RMooRWuQuNIC59RAASDyDSkqGj3gjA81qqCvllKdxB5wh6H/UYYVueWppNfH7O7EwBagGdsAAlQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=k+lhY5VftcfnKVRTk2mdA/W8K+jSfoMoZas+xSm43Vc/Rey7bOVz7qhKcwOXFTv4H7NdmwFQ/dM6QI/nDVkmHbXrAO3NbblXi5yzpPyeQaTwW19TBLh5Htc/8LPuax81aHOaQKikokhI+zPHgIOEOC/VDtdklU8H/zFIp6d5TLI8HpTNumz/wd9W4m0GnSVjOEG/dIo+G26B//bFM4z530wzG3WPv+sJYARQTJCEWlMzmsQPahUsQAH/Z3Pn3hlNED9AUGmel/eRePU5TjcJ1t5bQlZBg6TLpUTcqS5twpw29MojjdUtGPPasxektSk5fxfplj6Wy5gBj/IPg8bCEQ==
  • 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: Mon, 23 Sep 2019 09:01:12 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVb61ItuWfOmtXxUSrWqDa6jVDyqc0nmqAgAA8aoD//9OTgIAETPmA
  • Thread-topic: [PATCH v12] x86/emulate: Send vm_event from emulate


On 20.09.2019 18:20, Jan Beulich wrote:
> On 20.09.2019 16:59, Alexandru Stefan ISAILA wrote:
>>
>>
>> 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.

This will be changed for:
"A new return type was added, HVMTRANS_need_retry, in order to have all 
the places that consume HVMTRANS* return X86EMUL_RETRY."

>>
>>>
>>>> @@ -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.
> 
> The result should be consistent (i.e. between the case here
> and the rep_movs / rep_stos cases below). Overall I think it
> would be cleanest if in all three cases an ASSERT_UNREACHABLE()
> fell through to a "return X86EMUL_RETRY;".
> 

Ok, just to make sure this is what is needed and limit the patch 
versions, rep_movs / rep_stos should have a switch like this:

         switch ( rc )
         {
         case HVMTRANS_okay:
             return X86EMUL_OKAY;
         case HVMTRANS_need_retry:
             ASSERT_UNREACHABLE();
             /* fall through */
         case HVMTRANS_gfn_paged_out:
         case HVMTRANS_gfn_shared:
             return X86EMUL_RETRY;
         }

Then hvmemul_map_linear_addr() should have:

         case HVMTRANS_need_retry:
             ASSERT_UNREACHABLE();
             /* fall through */
         case HVMTRANS_gfn_shared:
         case HVMTRANS_gfn_paged_out:
             err = ERR_PTR(~X86EMUL_RETRY);


>>>> @@ -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?
> 
> It shouldn't be the default case that gains this assertion,
> but the HVMTRANS_need_retry one that is to be added.
> 
>>> 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?
> 
> If you go the ASSERT_UNREACHABLE() route, then the comment(s)
> should be code comments next to these assertions. They'd be
> there to avoid people having to dig out the reason for why
> they're there, to make it easy to decide whether it is safe to
> drop them once some new producer of HVMTRANS_need_retry would
> appear.
> 

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