Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Don Slutz <dslutz@verizon.com>]

On 02/06/15 02:35, Jan Beulich wrote:
>>>> On 05.02.15 at 20:02, <dslutz@verizon.com> wrote:
>> On 02/03/15 09:58, Jan Beulich wrote:
>>>>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
>> Ok, will be working on a much better commit message.  Do you want the
>> new commit message copied here (in the summary of the changes), or just
>> that fact that there is a new commit message?
> 
> Only mention it there.
> 

Ok

>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -218,8 +218,11 @@ static int hvmemul_do_io(
>>>>              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>>>>          break;
>>>>      case X86EMUL_UNHANDLEABLE:
>>>> +    {
>>>> +        struct hvm_ioreq_server *s = hvm_has_dm(curr->domain, &p);
>>>> +
>>>>          /* If there is no backing DM, just ignore accesses */
>>>> -        if ( !hvm_has_dm(curr->domain) )
>>>> +        if ( !s )
>>>>          {
>>>>              rc = X86EMUL_OKAY;
>>>>              vio->io_state = HVMIO_none;
>>>
>>> You alter the flow here, but leave the (now stale) comment
>>> untouched - !hvm_has_dm() no longer has the original meaning.
>>>
>>
>> I will say that the comment is not stale. Also this code flow change
>> is what I am trying to do in this patch.  Since I am talking about
>> Paul's code also, I might be off base.
>>
>> It is true that hvm_has_dm() has changed.  However the change
>> is from "there is no backing DM" to "there is no backing DM
>> that will process this request".  To me they mean the same thing.
>>
>> That is why I did not change the name and did not see a need
>> to change the comment.  However I can adjust the comment to be much longer.
>>
>> How does:
>>
>> /*
>>  * If there is no backing DM, or there is no backing DM to handle
>>  * this request, act just like a missing device.
>>  */
>>
>> look as a new comment?
> 
> Simply inserting "suitable" into the original comment would do imo.
> 

Will change to:

/* If there is no suitable backing DM, just ignore accesses */


>> Would it help to s/hvm_has_dm/hvm_has_dm_for_req/?
> 
> No, not really. But see also below for the name.
> 

ok

>> Here is the proposed new commit message:
> 
> Looks a lot better.
> 
>> The key part of skipping the retry is to do "rc = X86EMUL_OKAY"
>> which is what the error path on the call to hvm_has_dm() does in
>> hvmemul_do_io() (the only call on hvm_has_dm()).
>>
>> Since this case is no longer handled in hvm_send_assist_req(), move
>> the call to hvm_complete_assist_req() into hvm_has_dm().
>>
>> As part of this change, do the work of hvm_complete_assist_req() in
>> the PVH case.  Acting more like real hardware looks to be better.
>>
>> Since there is only one caller of hvm_complete_assist_req(), fold
>> that routine into the changed hvm_has_dm().
> 
> As said before, with the current name it's not reasonable for the
> function to also complete the request (the name strictly suggests
> a "read-only" operation). But as it looks this one has only a single
> caller too, so maybe apart from renaming an alternative might
> again be to fold it into that single caller?
> 

I think that folding routines between files is not the way I would
normally go.  So how about not folding hvm_complete_assist_req()
anywhere, just change it to return void.  Drop the routine
hvm_has_dm(), and have hvmemul_do_io() call hvm_select_ioreq_server()
and in the error path call hvm_complete_assist_req().  This does
leave only one caller of hvm_complete_assist_req(), but the code blocks
are not changing files.  Here is the diff in hvmemul_do_io() to match
these words:

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -218,21 +218,27 @@ static int hvmemul_do_io(
             vio->io_state = HVMIO_handle_mmio_awaiting_completion;
         break;
     case X86EMUL_UNHANDLEABLE:
-        /* If there is no backing DM, just ignore accesses */
-        if ( !hvm_has_dm(curr->domain) )
+    {
+        struct hvm_ioreq_server *s =
+            hvm_select_ioreq_server(curr->domain, &p);
+
+        /* If there is no suitable backing DM, just ignore accesses */
+        if ( !s )
         {
+            hvm_complete_assist_req(&p);
             rc = X86EMUL_OKAY;
             vio->io_state = HVMIO_none;
         }
         else


>> Adding "rc = X86EMUL_OKAY" in the failing case of
>> hvm_send_assist_req() would unfix what was done in commit
>> bac0999325056a3b3a92f7622df7ffbc5388b1c3 and commit
>> f20f3c8ece5c10fa7626f253d28f570a43b23208.  We are currently doing
>> the succeeding case of hvm_send_assist_req() and retying the I/O.
> 
> Maybe s/unfix/break/ ?
> 

Yes.

   -Don Slutz

> Jan
>