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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 01 July 2019 12:56
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant 
> <Paul.Durrant@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: [PATCH 1/6] x86emul: generalize wbinvd() hook
> 
> 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@xxxxxxxx>
> ---
> 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).
>
[snip]
> --- 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);

Since the mapping is ditched here, why bother getting one at all in the 
!cpu_has_clflush case? Are you trying to flush out an error condition that was 
previously missed?

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