From: Jan Beulich <jbeulich@suse.com>
To: Paul Durrant <Paul.Durrant@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
RogerPau Monne <roger.pau@citrix.com>, Wei Liu <wl@xen.org>,
Andrew Cooper <andrew.cooper3@citrix.com>
Subject: [Xen-devel] Ping: [PATCH 1/6] x86emul: generalize wbinvd() hook
Date: Mon, 2 Sep 2019 13:10:29 +0200 [thread overview]
Message-ID: <b37d16e7-ede4-98ce-c0f1-8d0d9aa2ec35@suse.com> (raw)
In-Reply-To: <3f30c73d-94a7-f9ca-5914-0400f1f98cc3@suse.com>
On 01.07.2019 13:55, 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Paul,
any chance I could get your ack (or otherwise) here? I thought I did
answer the one question you had raised to your satisfaction.
Thanks, Jan
> ---
> 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.
> 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).
>
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -382,10 +382,13 @@ static int fuzz_invlpg(
> return maybe_fail(ctxt, "invlpg", false);
> }
>
> -static int fuzz_wbinvd(
> +static int fuzz_cache_op(
> + enum x86emul_cache_op op,
> + enum x86_segment seg,
> + unsigned long offset,
> struct x86_emulate_ctxt *ctxt)
> {
> - return maybe_fail(ctxt, "wbinvd", true);
> + return maybe_fail(ctxt, "cache-management", true);
> }
>
> static int fuzz_write_io(
> @@ -620,7 +623,7 @@ static const struct x86_emulate_ops all_
> SET(read_xcr),
> SET(read_msr),
> SET(write_msr),
> - SET(wbinvd),
> + SET(cache_op),
> SET(invlpg),
> .get_fpu = emul_test_get_fpu,
> .put_fpu = emul_test_put_fpu,
> @@ -729,7 +732,7 @@ enum {
> HOOK_read_xcr,
> HOOK_read_msr,
> HOOK_write_msr,
> - HOOK_wbinvd,
> + HOOK_cache_op,
> HOOK_cpuid,
> HOOK_inject_hw_exception,
> HOOK_inject_sw_interrupt,
> @@ -773,7 +776,7 @@ static void disable_hooks(struct x86_emu
> MAYBE_DISABLE_HOOK(read_xcr);
> MAYBE_DISABLE_HOOK(read_msr);
> MAYBE_DISABLE_HOOK(write_msr);
> - MAYBE_DISABLE_HOOK(wbinvd);
> + MAYBE_DISABLE_HOOK(cache_op);
> MAYBE_DISABLE_HOOK(cpuid);
> MAYBE_DISABLE_HOOK(get_fpu);
> MAYBE_DISABLE_HOOK(invlpg);
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -19,7 +19,9 @@ $(call as-option-add,CFLAGS,CC,"crc32 %e
> $(call as-option-add,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_AS_EPT)
> $(call as-option-add,CFLAGS,CC,"rdrand %eax",-DHAVE_AS_RDRAND)
> $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
> +$(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
> $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
> +$(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
> $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
> -U__OBJECT_LABEL__ -DHAVE_AS_QUOTED_SYM \
> '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -25,6 +25,7 @@
> #include <asm/hvm/trace.h>
> #include <asm/hvm/support.h>
> #include <asm/hvm/svm/svm.h>
> +#include <asm/iocap.h>
> #include <asm/vm_event.h>
>
> static void hvmtrace_io_assist(const ioreq_t *p)
> @@ -555,16 +556,12 @@ static void *hvmemul_map_linear_addr(
> mfn_t *mfn = &hvmemul_ctxt->mfn[0];
>
> /*
> - * The caller has no legitimate reason for trying a zero-byte write, but
> - * all other code here is written to work if the check below was dropped.
> - *
> - * The maximum write size depends on the number of adjacent mfns[] which
> + * The maximum access size depends on the number of adjacent mfns[] which
> * can be vmap()'d, accouting for possible misalignment within the region.
> * The higher level emulation callers are responsible for ensuring that
> - * mfns[] is large enough for the requested write size.
> + * mfns[] is large enough for the requested access size.
> */
> - if ( bytes == 0 ||
> - nr_frames > ARRAY_SIZE(hvmemul_ctxt->mfn) )
> + if ( nr_frames > ARRAY_SIZE(hvmemul_ctxt->mfn) )
> {
> ASSERT_UNREACHABLE();
> goto unhandleable;
> @@ -669,8 +666,6 @@ static void hvmemul_unmap_linear_addr(
> unsigned int i;
> mfn_t *mfn = &hvmemul_ctxt->mfn[0];
>
> - ASSERT(bytes > 0);
> -
> if ( nr_frames == 1 )
> unmap_domain_page(mapping);
> else
> @@ -1473,7 +1468,10 @@ static int hvmemul_write_msr_discard(
> return X86EMUL_OKAY;
> }
>
> -static int hvmemul_wbinvd_discard(
> +static int hvmemul_cache_op_discard(
> + enum x86emul_cache_op op,
> + enum x86_segment seg,
> + unsigned long offset,
> struct x86_emulate_ctxt *ctxt)
> {
> return X86EMUL_OKAY;
> @@ -2149,10 +2147,65 @@ static int hvmemul_write_msr(
> return rc;
> }
>
> -static int hvmemul_wbinvd(
> +static int hvmemul_cache_op(
> + enum x86emul_cache_op op,
> + enum x86_segment seg,
> + unsigned long offset,
> struct x86_emulate_ctxt *ctxt)
> {
> - alternative_vcall(hvm_funcs.wbinvd_intercept);
> + struct hvm_emulate_ctxt *hvmemul_ctxt =
> + container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> + unsigned long addr, reps = 1;
> + uint32_t pfec = PFEC_page_present;
> + int rc;
> + void *mapping;
> +
> + if ( !cache_flush_permitted(current->domain) )
> + return X86EMUL_OKAY;
> +
> + switch ( op )
> + {
> + case x86emul_clflush:
> + case x86emul_clflushopt:
> + case x86emul_clwb:
> + ASSERT(!is_x86_system_segment(seg));
> +
> + rc = hvmemul_virtual_to_linear(seg, offset, 0, &reps,
> + hvm_access_read, hvmemul_ctxt, &addr);
> + if ( rc != X86EMUL_OKAY )
> + break;
> +
> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
> + pfec |= PFEC_user_mode;
> +
> + mapping = hvmemul_map_linear_addr(addr, 0, pfec, hvmemul_ctxt,
> + current->arch.hvm.data_cache);
> + if ( mapping == ERR_PTR(~X86EMUL_EXCEPTION) )
> + return X86EMUL_EXCEPTION;
> + if ( IS_ERR_OR_NULL(mapping) )
> + break;
> +
> + if ( cpu_has_clflush )
> + {
> + if ( op == x86emul_clwb && cpu_has_clwb )
> + clwb(mapping);
> + else if ( op == x86emul_clflushopt && cpu_has_clflushopt )
> + clflushopt(mapping);
> + else
> + clflush(mapping);
> +
> + hvmemul_unmap_linear_addr(mapping, addr, 0, hvmemul_ctxt);
> + break;
> + }
> +
> + hvmemul_unmap_linear_addr(mapping, addr, 0, hvmemul_ctxt);
> + /* fall through */
> + case x86emul_invd:
> + case x86emul_wbinvd:
> + alternative_vcall(hvm_funcs.wbinvd_intercept);
> + break;
> + }
> +
> return X86EMUL_OKAY;
> }
>
> @@ -2353,7 +2406,7 @@ static const struct x86_emulate_ops hvm_
> .write_xcr = hvmemul_write_xcr,
> .read_msr = hvmemul_read_msr,
> .write_msr = hvmemul_write_msr,
> - .wbinvd = hvmemul_wbinvd,
> + .cache_op = hvmemul_cache_op,
> .cpuid = x86emul_cpuid,
> .get_fpu = hvmemul_get_fpu,
> .put_fpu = hvmemul_put_fpu,
> @@ -2380,7 +2433,7 @@ static const struct x86_emulate_ops hvm_
> .write_xcr = hvmemul_write_xcr,
> .read_msr = hvmemul_read_msr,
> .write_msr = hvmemul_write_msr_discard,
> - .wbinvd = hvmemul_wbinvd_discard,
> + .cache_op = hvmemul_cache_op_discard,
> .cpuid = x86emul_cpuid,
> .get_fpu = hvmemul_get_fpu,
> .put_fpu = hvmemul_put_fpu,
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -1118,9 +1118,11 @@ static int write_msr(unsigned int reg, u
> return X86EMUL_UNHANDLEABLE;
> }
>
> -/* Name it differently to avoid clashing with wbinvd() */
> -static int _wbinvd(struct x86_emulate_ctxt *ctxt)
> +static int cache_op(enum x86emul_cache_op op, enum x86_segment seg,
> + unsigned long offset, struct x86_emulate_ctxt *ctxt)
> {
> + ASSERT(op == x86emul_wbinvd);
> +
> /* Ignore the instruction if unprivileged. */
> if ( !cache_flush_permitted(current->domain) )
> /*
> @@ -1238,7 +1240,7 @@ static const struct x86_emulate_ops priv
> .read_msr = read_msr,
> .write_msr = write_msr,
> .cpuid = x86emul_cpuid,
> - .wbinvd = _wbinvd,
> + .cache_op = cache_op,
> };
>
> int pv_emulate_privileged_op(struct cpu_user_regs *regs)
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -5933,8 +5933,11 @@ x86_emulate(
> case X86EMUL_OPC(0x0f, 0x08): /* invd */
> case X86EMUL_OPC(0x0f, 0x09): /* wbinvd */
> generate_exception_if(!mode_ring0(), EXC_GP, 0);
> - fail_if(ops->wbinvd == NULL);
> - if ( (rc = ops->wbinvd(ctxt)) != 0 )
> + fail_if(!ops->cache_op);
> + if ( (rc = ops->cache_op(b == 0x09 ? x86emul_wbinvd
> + : x86emul_invd,
> + x86_seg_none, 0,
> + ctxt)) != X86EMUL_OKAY )
> goto done;
> break;
>
> @@ -7801,8 +7804,9 @@ x86_emulate(
> /* else clwb */
> fail_if(!vex.pfx);
> vcpu_must_have(clwb);
> - fail_if(!ops->wbinvd);
> - if ( (rc = ops->wbinvd(ctxt)) != X86EMUL_OKAY )
> + fail_if(!ops->cache_op);
> + if ( (rc = ops->cache_op(x86emul_clwb, ea.mem.seg, ea.mem.off,
> + ctxt)) != X86EMUL_OKAY )
> goto done;
> break;
> case 7:
> @@ -7818,8 +7822,11 @@ x86_emulate(
> vcpu_must_have(clflush);
> else
> vcpu_must_have(clflushopt);
> - fail_if(ops->wbinvd == NULL);
> - if ( (rc = ops->wbinvd(ctxt)) != 0 )
> + fail_if(!ops->cache_op);
> + if ( (rc = ops->cache_op(vex.pfx ? x86emul_clflushopt
> + : x86emul_clflush,
> + ea.mem.seg, ea.mem.off,
> + ctxt)) != X86EMUL_OKAY )
> goto done;
> break;
> default:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -176,6 +176,14 @@ enum x86_emulate_fpu_type {
> X86EMUL_FPU_none
> };
>
> +enum x86emul_cache_op {
> + x86emul_clflush,
> + x86emul_clflushopt,
> + x86emul_clwb,
> + x86emul_invd,
> + x86emul_wbinvd,
> +};
> +
> struct x86_emulate_state;
>
> /*
> @@ -452,8 +460,15 @@ struct x86_emulate_ops
> uint64_t val,
> struct x86_emulate_ctxt *ctxt);
>
> - /* wbinvd: Write-back and invalidate cache contents. */
> - int (*wbinvd)(
> + /*
> + * cache_op: Write-back and/or invalidate cache contents.
> + *
> + * @seg:@offset applicable only to some of enum x86emul_cache_op.
> + */
> + int (*cache_op)(
> + enum x86emul_cache_op op,
> + enum x86_segment seg,
> + unsigned long offset,
> struct x86_emulate_ctxt *ctxt);
>
> /* cpuid: Emulate CPUID via given set of EAX-EDX inputs/outputs. */
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -102,6 +102,8 @@
> #define cpu_has_rdseed boot_cpu_has(X86_FEATURE_RDSEED)
> #define cpu_has_smap boot_cpu_has(X86_FEATURE_SMAP)
> #define cpu_has_avx512_ifma boot_cpu_has(X86_FEATURE_AVX512_IFMA)
> +#define cpu_has_clflushopt boot_cpu_has(X86_FEATURE_CLFLUSHOPT)
> +#define cpu_has_clwb boot_cpu_has(X86_FEATURE_CLWB)
> #define cpu_has_avx512er boot_cpu_has(X86_FEATURE_AVX512ER)
> #define cpu_has_avx512cd boot_cpu_has(X86_FEATURE_AVX512CD)
> #define cpu_has_sha boot_cpu_has(X86_FEATURE_SHA)
> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -21,6 +21,23 @@ static inline void clflush(const void *p
> asm volatile ( "clflush %0" :: "m" (*(const char *)p) );
> }
>
> +static inline void clflushopt(const void *p)
> +{
> + asm volatile ( "data16 clflush %0" :: "m" (*(const char *)p) );
> +}
> +
> +static inline void clwb(const void *p)
> +{
> +#if defined(HAVE_AS_CLWB)
> + asm volatile ( "clwb %0" :: "m" (*(const char *)p) );
> +#elif defined(HAVE_AS_XSAVEOPT)
> + asm volatile ( "data16 xsaveopt %0" :: "m" (*(const char *)p) );
> +#else
> + asm volatile ( ".byte 0x66, 0x0f, 0xae, 0x32"
> + :: "d" (p), "m" (*(const char *)p) );
> +#endif
> +}
> +
> #define xchg(ptr,v) \
> ((__typeof__(*(ptr)))__xchg((unsigned long)(v),(ptr),sizeof(*(ptr))))
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-09-02 11:10 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-01 11:47 [Xen-devel] [PATCH 0/6] x86emul: further work Jan Beulich
2019-07-01 11:56 ` [Xen-devel] [PATCH 1/6] x86emul: generalize wbinvd() hook Jan Beulich
2019-07-02 10:22 ` Paul Durrant
2019-07-02 10:50 ` Jan Beulich
2019-07-02 10:53 ` Paul Durrant
2019-08-27 10:44 ` Andrew Cooper
2019-08-27 12:51 ` Andrew Cooper
2019-08-27 18:47 ` Andrew Cooper
2019-09-02 11:10 ` Jan Beulich [this message]
2019-09-02 12:04 ` [Xen-devel] Ping: " Paul Durrant
2019-07-01 11:56 ` [Xen-devel] [PATCH 2/6] x86emul: support WBNOINVD Jan Beulich
2019-07-02 10:38 ` Paul Durrant
2019-08-27 14:47 ` Andrew Cooper
2019-08-27 15:08 ` Jan Beulich
2019-08-27 16:45 ` Andrew Cooper
2019-08-28 11:42 ` Andrew Cooper
2019-07-01 11:56 ` [Xen-devel] [PATCH 3/6] x86emul: generalize invlpg() hook Jan Beulich
2019-07-02 10:47 ` Paul Durrant
2019-08-27 14:55 ` Andrew Cooper
2019-07-01 11:57 ` [Xen-devel] [PATCH 4/6] x86: move INVPCID_TYPE_* to x86-defns.h Jan Beulich
2019-07-02 10:49 ` Paul Durrant
2019-08-27 14:57 ` Andrew Cooper
2019-07-01 11:57 ` [Xen-devel] [PATCH 5/6] x86emul: support INVPCID Jan Beulich
2019-07-02 12:54 ` Paul Durrant
2019-08-27 15:31 ` Andrew Cooper
2019-08-27 15:53 ` Jan Beulich
2019-08-27 17:27 ` Andrew Cooper
2019-08-28 7:14 ` Jan Beulich
2019-08-28 11:33 ` Andrew Cooper
2019-07-01 11:58 ` [Xen-devel] [PATCH 6/6] x86emul: support MOVDIR{I,64B} insns Jan Beulich
2019-08-27 16:04 ` Andrew Cooper
2019-08-28 6:26 ` Jan Beulich
2019-08-28 11:51 ` Andrew Cooper
2019-08-28 12:19 ` Jan Beulich
2019-08-15 14:24 ` [Xen-devel] [PATCH 0/6] x86emul: further work Andrew Cooper
2019-08-27 8:37 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b37d16e7-ede4-98ce-c0f1-8d0d9aa2ec35@suse.com \
--to=jbeulich@suse.com \
--cc=Paul.Durrant@citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).