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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> --- 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_*

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

Jan


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