Re: [Xen-devel] [PATCH 1/6] x86emul: generalize wbinvd() hook

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Paul Durrant <Paul.Durrant@citrix.com>, Wei Liu <wl@xen.org>,
	RogerPau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 1/6] x86emul: generalize wbinvd() hook
Date: Tue, 27 Aug 2019 11:44:57 +0100	[thread overview]
Message-ID: <fc5f0b03-2e69-8fa9-289f-50e8abb57210@citrix.com> (raw)
In-Reply-To: <3f30c73d-94a7-f9ca-5914-0400f1f98cc3@suse.com>

On 01/07/2019 12:56, Jan Beulich wrote:
> The hook is already in use for other purposes, and emulating e.g.
> CLFLUSH by issuing WBINVD is, well, not very nice. Rename the hook and
> add parameters. Use lighter weight flushing insns when possible in
> hvmemul_cache_op().
>
> hvmemul_cache_op() treating x86emul_invd the same as x86emul_wbinvd is
> to retain original behavior, but I'm not sure this is what we want in
> the long run.

There is no use for INVD in a VM, as it is never running with the memory
controllers yet-to-be configured.  The only place it may be found is at
the reset vector for a firmware which doesn't start in a
virtualisation-aware way.

Given how big a hammer WBINVD is, I reckon we should just nop it out.

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v2: Use cache_op() as hook name. Convert macros to inline functions in
>      system.h. Re-base.
> ---
> I was unsure about PREFETCH* and CLDEMOTE - both are cache management
> insns too, but the emulator currently treats them as a NOP without
> invoking any hooks.

They are just hints.  Nothing architecturally may depend on them having
any effect.  CLDEMOTE in particular is for reducing cache coherency
overhead on a producer/consumer setup, but any potential optimisation is
dwarfed by the VMExit.

> I was also uncertain about the new cache_flush_permitted() instance -
> generally I think it wouldn't be too bad if we allowed line flushes in
> all cases, in which case the checks in the ->wbinvd_intercept() handlers
> would suffice (as they did until now).

This is a more general issue which we need to address.  To support
encrypted memory in VM's, we must guarantee that WC mappings which the
guest creates are really WC, which means we must not use IPAT or play
any "fall back to WB" games.

Furthermore, AMD's encrypt-in-place algorithm requires the guest to be
able to use WBINVD.

Fixing this properly will be a good thing, and will fix the fact that at
the moment, any time you give a PCI device to a guest, its blk/net perf
becomes glacial, due to having the grant table being uncached.

> +
> +        if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
> +            pfec |= PFEC_user_mode;

As a note, this is fine here, but this whole system of choosing pfec
needs to be adjusted when we add support for WRUSS, which is a CPL0
instruction that executed as a user mode write, for adjusting the user
shadow stack.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel