Re: [Xen-devel] [PATCH 1/5] x86/hvm: Rework HVM_HCALL_invalidate handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/02/17 16:49, Jan Beulich wrote:
>>>> On 13.02.17 at 14:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Sending an invalidation to the device model is an internal detail of
>> completing the hypercall; callers should not need to be responsible for it.
>> Drop HVM_HCALL_invalidate entirely and call send_invalidate_req() when
>> appropriate.
>>
>> This makes the function boolean in nature, although the existing
>> HVM_HCALL_{completed,preempted} to aid code clarity.
> "are being retained" missing somewhere here?

Yes.  I already noticed and fixed that up.  It now reads

"This makes the function boolean in nature, although the existing
HVM_HCALL_{completed,preempted} constants are kept to aid code clarity."

>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3874,7 +3874,7 @@ static const hypercall_table_t hvm_hypercall_table[] = 
>> {
>>  #undef HYPERCALL
>>  #undef COMPAT_CALL
>>  
>> -int hvm_do_hypercall(struct cpu_user_regs *regs)
>> +bool hvm_hypercall(struct cpu_user_regs *regs)
> I don't think bool is a particularly good choice when the callers can't
> sensibly use the result without comparing with HVM_HCALL_*

Ok.  I will keep it as int.

>
>> @@ -4011,9 +4011,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>          return HVM_HCALL_preempted;
>>  
>>      if ( unlikely(currd->arch.hvm_domain.qemu_mapcache_invalidate) &&
>> -         test_and_clear_bool(currd->arch.hvm_domain.
>> -                             qemu_mapcache_invalidate) )
>> -        return HVM_HCALL_invalidate;
>> +         
>> test_and_clear_bool(currd->arch.hvm_domain.qemu_mapcache_invalidate) )
>> +        send_invalidate_req();
> I wonder why things were done the old way in the first place. Did
> something else change, making that old model no longer required?
> I'm somewhat afraid we overlook something here, and hence an
> attempt to understand why this more immediate model wasn't
> used (and perhaps usable) back then might help... That aside, the
> patch looks fine.

Looks like it has been the same since it was first introduced in aeb2e1298b7

While that change does indicate it was replacing various I/O port hacks,
I can't see any reason why HVM_HCALL_invalidate was exposed outside of
hvm_do_hypercall().

~Andrew

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