Re: [PATCH V1 13/16] xen/ioreq: Make x86's invalidate qemu mapcache handling common

From: Jan Beulich <jbeulich@suse.com>
To: "Oleksandr Tyshchenko" <olekstysh@gmail.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Wei Liu <wl@xen.org>,
	Paul Durrant <paul@xen.org>, Julien Grall <julien.grall@arm.com>
Subject: Re: [PATCH V1 13/16] xen/ioreq: Make x86's invalidate qemu mapcache handling common
Date: Wed, 16 Sep 2020 10:50:01 +0200	[thread overview]
Message-ID: <83dfb207-c191-8dad-1474-ce57b6d51102@suse.com> (raw)
In-Reply-To: <1599769330-17656-14-git-send-email-olekstysh@gmail.com>

On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1490,6 +1490,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>      /* Ensure the hypercall trap instruction is re-executed. */
>      if ( current->hcall_preempted )
>          regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
> +
> +#ifdef CONFIG_IOREQ_SERVER
> +    if ( unlikely(current->domain->qemu_mapcache_invalidate) &&
> +         test_and_clear_bool(current->domain->qemu_mapcache_invalidate) )
> +        send_invalidate_req();
> +#endif
>  }

There's a lot of uses of "current" here now, and these don't look to
exactly be cheap on Arm either (they aren't on x86), so I wonder
whether this is the point where at least "current" wants latching
into a local variable here.

> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -18,8 +18,10 @@
>   *
>   * Copyright (c) 2017 Citrix Systems Ltd.
>   */
> +
>  #include <xen/lib.h>
>  #include <xen/hypercall.h>
> +#include <xen/ioreq.h>
>  #include <xen/nospec.h>

While I don't care much about the presence of absence of the blank
line between head comment and #include-s, I don't see why you add
one here.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1651,6 +1651,11 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +#ifdef CONFIG_IOREQ_SERVER
> +    if ( op == XENMEM_decrease_reservation )
> +        curr_d->qemu_mapcache_invalidate = true;
> +#endif

I don't see why you put this right into decrease_reservation(). This
isn't just to avoid the extra conditional, but first and foremost to
avoid bypassing the earlier return from the function (in the case of
preemption). In the context of this I wonder whether the ordering of
operations in hvm_hypercall() is actually correct.

I'm also unconvinced curr_d is the right domain in all cases here;
while this may be a pre-existing issue in principle, I'm afraid it
gets more pronounced by the logic getting moved to common code.
Roger - thoughts either way with, in particular, PVH Dom0 in mind?

> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -97,6 +97,8 @@ static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
>             (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
>  }
>  
> +void send_invalidate_req(void);

Perhaps rename to ioreq_send_invalidate(), ioreq_send_invalidate_req(),
or send_invalidate_ioreq() at this occasion?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -512,6 +512,8 @@ struct domain
>      /* Argo interdomain communication support */
>      struct argo_domain *argo;
>  #endif
> +
> +    bool_t qemu_mapcache_invalidate;

"bool" please.

Jan