From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Chang S. Bae" <chang.seok.bae@intel.com>,
        Mingwei Zhang <mizhang@google.com>,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, linux-kselftest@vger.kernel.org,
        Jim Mattson <jmattson@google.com>,
        Venkatesh Srinivas <venkateshs@google.com>,
        Aaron Lewis <aaronlewis@google.com>,
        Chao Gao <chao.gao@intel.com>
Subject: Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address
 of init_fpstate if fpstate contains the component
In-Reply-To: <e91b9172-8a2e-e299-a84f-1e9331c51cb7@intel.com>
References: <20230221163655.920289-1-mizhang@google.com>
 <20230221163655.920289-2-mizhang@google.com>
 <e91b9172-8a2e-e299-a84f-1e9331c51cb7@intel.com>
Date:   Wed, 22 Feb 2023 09:38:02 +0100
Message-ID: <87ilfum6xh.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Tue, Feb 21 2023 at 19:05, Chang S. Bae wrote:
> On 2/21/2023 8:36 AM, Mingwei Zhang wrote:
>> @@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
>>   			pkru.pkru = pkru_val;
>>   			membuf_write(&to, &pkru, sizeof(pkru));
>>   		} else {
>> -			copy_feature(header.xfeatures & BIT_ULL(i), &to,
>> -				     __raw_xsave_addr(xsave, i),
>> -				     __raw_xsave_addr(xinit, i),
>> -				     xstate_sizes[i]);
>> +			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
>> +				__raw_xsave_addr(xsave, i) :
>> +				__raw_xsave_addr(xinit, i);
>> +
>> +			membuf_write(&to, xsave_addr, xstate_sizes[i]);
>>   		}
>>   		/*
>>   		 * Keep track of the last copied state in the non-compacted
>
> So this hunk is under for_each_extended_xfeature(i, mask) -- it skips 
> the copy routine if mask[i] == 0; instead, it fills zeros.
>
> We have this [1]:
>
> 	if (fpu_state_size_dynamic())
> 		mask &= (header.xfeatures | xinit->header.xcomp_bv);
>
> If header.xfeatures[18] = 0 then mask[18] = 0 because 
> xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm 
> confused about the problem that you described here.

Read the suggested changelog I wrote in my reply to Mingwei.

TLDR:

        xsave.header.xfeatures[18] = 1
        xinit.header.xfeatures[18] = 0
    ->  mask[18] = 1
    ->  __raw_xsave_addr(xsave, 18)     <- Success
    ->  __raw_xsave_addr(xinit, 18)     <- WARN

Thanks,

        tglx