Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz <dslutz@verizon.com>]

On 02/02/15 03:36, Jan Beulich wrote:
>>>> On 30.01.15 at 20:19, <dslutz@verizon.com> wrote:
>> Soon after sending this, I came up with a 2nd way for fix.
>> Change hvm_has_dm() to use hvm_select_ioreq_server().  Then
>> the correct answer will be found, and so will not retry.
>>
>> To avoid 2 calls to hvm_select_ioreq_server(), it makes
>> snes to me to return s.  Also adding a call to hvm_complete_assist_req()
>> would help.  So based on all this is is a possible v2:
> 
> Looks reasonable (awaiting Paul's opinion though) except for
> mechanical aspects:
> 
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>>      return 1;
>>  }
>>
>> -bool_t hvm_has_dm(struct domain *d)
>> +static bool_t hvm_complete_assist_req(ioreq_t *p)
>>  {
...
>> +}
>> +
>> +void *hvm_has_dm(struct domain *d, ioreq_t *p)
> 
> I can't see why this needs to return void * instead of a properly typed
> pointer.
> 

Using "struct hvm_ioreq_server *s" did not build:

In file included from /home/don/xen-master/xen/include/asm/hvm/irq.h:26:0,
                 from /home/don/xen-master/xen/include/asm/hvm/vpt.h:32,
                 from /home/don/xen-master/xen/include/asm/hvm/vlapic.h:27,
                 from /home/don/xen-master/xen/include/asm/hvm/vcpu.h:25,
                 from /home/don/xen-master/xen/include/asm/domain.h:7,
                 from /home/don/xen-master/xen/include/xen/domain.h:6,
                 from /home/don/xen-master/xen/include/xen/sched.h:10,
                 from x86_64/asm-offsets.c:10:
/home/don/xen-master/xen/include/asm/hvm/hvm.h:231:35: error: 'struct
hvm_ioreq_server' declared inside parameter list [-Werror]
/home/don/xen-master/xen/include/asm/hvm/hvm.h:231:35: error: its scope
is only this definition or declaration, which is probably not what you
want [-Werror]
cc1: all warnings being treated as errors

So I went with void * to check the that the code would pass my unit
testing.


No clue why I did not say:

+struct hvm_ioreq_server;
+bool_t hvm_send_assist_req(struct hvm_ioreq_server *s, ioreq_t *p);

Unless I hear otherwise, this is the way I will go.



>> @@ -2571,44 +2607,15 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>>      return 0;
>>  }
>>
>> -static bool_t hvm_complete_assist_req(ioreq_t *p)
>> -{
...
>> -    return 0; /* implicitly bins the i/o operation */
>> -}
> 
> Moving hvm_has_dm() down here would make the patch smaller and
> hence quite a bit easier to review (as one doesn't need to manually
> check the differences between the much larger added/removed
> pieces of code.
> 

Will do.

   -Don Slutz


> Jan
>