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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 02 July 2019 11:50
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>
> Subject: Re: [PATCH 1/6] x86emul: generalize wbinvd() hook
> 
> On 02.07.2019 12:22, Paul Durrant wrote:
> >> From: Jan Beulich <JBeulich@xxxxxxxx>
> >> Sent: 01 July 2019 12:56
> >>
> >> @@ -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;
> 
> This return ...
> 
> >> +        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?
> 
> ... is what I'm after: We want exception behavior to be correct.
> 

Ok, fair enough. Just wasn't obvious to me from the commit comment.

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