Re: [PATCH v1 1/1] target/i386: Mask xstate_bv based on the cpu enabled features

[Leonardo Brás]

Hello Igor,

On Tue, 2022-02-01 at 09:29 +0100, Igor Mammedov wrote:
> On Mon, 31 Jan 2022 12:53:31 +0000
> David Edmondson <david.edmondson@oracle.com> wrote:
> 
> > On Saturday, 2022-01-29 at 06:46:45 -03, Leonardo Bras wrote:
> > 
> > > The following steps describe a migration bug:
> > > 1 - Bring up a VM with -cpu EPYC on a host with EPYC-Milan cpu
> > > 2 - Migrate to a host with EPYC-Naples cpu
> > > 
> > > The guest kernel crashes shortly after the migration.
> > > 
> > > The crash happens due to a fault caused by XRSTOR:
> > > A set bit in XSTATE_BV is not set in XCR0.
> > > The faulting bit is FEATURE_PKRU (enabled in Milan, but not in
> > > Naples)  
> > 
> > I'm trying to understand how this happens.
> > 
> > If we boot on EPYC-Milan with "-cpu EPYC", the PKRU feature should
> > not
> > be exposed to the VM (it is not available in the EPYC CPU).
> > 
> > Given this, how would bit 0x200 (representing PKRU) end up set in
> > xstate_bv?
> > 
> > > To avoid this kind of bug:
> > > In kvm_get_xsave, mask-out from xstate_bv any bits that are not
> > > set in
> > > current vcpu's features.
> 
> In addition to above:
> 
> it's not good idea to silently mask something out.
> If we can't ensure the same feature-set for a CPU model
> and can't verify it by asking QEMU on source and
> target host, the next best thing would be to explicitly
> fail migration (i.e. adding check to.post_load hook or
> doing some other migration magic, CCing David)

Maybe there is something to do with the host kernel (kvm) doing some
strange stuff.

IIRC qemu ended up getting some masked version for using on migration,
since it was not failing as expected.

I will try to investigate further.
Please let me know if you have any information on that.

Best regards,
Leo

> 
> > > 
> > > This keeps cpu->env->xstate_bv with feature bits compatible with
> > > any
> > > host machine capable of running the vcpu model.
> > > 
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > ---
> > >  target/i386/xsave_helper.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/target/i386/xsave_helper.c
> > > b/target/i386/xsave_helper.c
> > > index ac61a96344..0628226234 100644
> > > --- a/target/i386/xsave_helper.c
> > > +++ b/target/i386/xsave_helper.c
> > > @@ -167,7 +167,7 @@ void x86_cpu_xrstor_all_areas(X86CPU *cpu,
> > > const void *buf, uint32_t buflen)
> > >          env->xmm_regs[i].ZMM_Q(1) = ldq_p(xmm + 8);
> > >      }
> > > 
> > > -    env->xstate_bv = header->xstate_bv;
> > > +    env->xstate_bv = header->xstate_bv & env-
> > > >features[FEAT_XSAVE_COMP_LO];
> > > 
> > >      e = &x86_ext_save_areas[XSTATE_YMM_BIT];
> > >      if (e->size && e->offset) {  
> > 
> > dme.
> 
>