Re: [PATCH v2 3/6] kvm: x86: Emulate split-lock access as a write

[Xiaoyao Li]

On 2/4/2020 4:54 AM, Sean Christopherson wrote:
> On Mon, Feb 03, 2020 at 11:16:05PM +0800, Xiaoyao Li wrote:
> > If split lock detect is enabled (warn/fatal), #AC handler calls die()
> > when split lock happens in kernel.
> >
> > A sane guest should never tigger emulation on a split-lock access, but
> > it cannot prevent malicous guest from doing this. So just emulating the
> > access as a write if it's a split-lock access to avoid malicous guest
> > polluting the kernel log.
> >
> > More detail analysis can be found:
> > https://lkml.kernel.org/r/20200131200134.GD18946@xxxxxxxxxxxxxxx
> >
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > ---
> >   arch/x86/kvm/x86.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2d3be7f3ad67..821b7404c0fd 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5847,6 +5847,13 @@ static int emulator_write_emulated(struct x86_emulate_ctxt *ctxt,
> >   	(cmpxchg64((u64 *)(ptr), *(u64 *)(old), *(u64 *)(new)) == *(u64 *)(old))
> >   #endif
> >   
> > +static inline bool across_cache_line_access(gpa_t gpa, unsigned int bytes)
>
> s/across/split so as not to introduce another name.
>
> > +{
> > +	unsigned int cache_line_size = cache_line_size();
> > +
> > +	return (gpa & (cache_line_size - 1)) + bytes > cache_line_size;
>
> I'd prefer to use the same logic as the page-split to avoid having to
> reason about the correctness of two different algorithms.
>
> > +}
> > +
> >   static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
> >   				     unsigned long addr,
> >   				     const void *old,
> > @@ -5873,6 +5880,10 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
> >   	if (((gpa + bytes - 1) & PAGE_MASK) != (gpa & PAGE_MASK))
> >   		goto emul_write;
> >   
> > +	if (get_split_lock_detect_state() != sld_off &&
> > +	    across_cache_line_access(gpa, bytes))
> > +		goto emul_write;
>
> As an alternative to the above, the page/line splits can be handled in a
> single check, e.g.
>
> 	page_line_mask = PAGE_MASK;
> 	if (is_split_lock_detect_enabled())
> 		page_line_mask = cache_line_size() - 1;
> 	if (((gpa + bytes - 1) & page_line_mask) != (gpa & page_line_mask))
> 		goto emul_write;

It's better, will use your suggestion.
Thanks!

> > +
> >   	if (kvm_vcpu_map(vcpu, gpa_to_gfn(gpa), &map))
> >   		goto emul_write;
> >   
> > --
> > 2.23.0