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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.15 at 20:02, <dslutz@xxxxxxxxxxx> wrote:
> On 02/03/15 09:58, Jan Beulich wrote:
>>>>> On 02.02.15 at 16:22, <dslutz@xxxxxxxxxxx> 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.

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

> Would it help to s/hvm_has_dm/hvm_has_dm_for_req/?

No, not really. But see also below for the name.

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

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

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel