Re: [Xen-devel] [PATCH v2] x86/amd: Avoid cpu_has_hypervisor evaluating true on native hardware

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2020 17:31, Roger Pau Monné wrote:
> On Tue, Feb 11, 2020 at 03:51:54PM +0000, Andrew Cooper wrote:
>> Currently when booting native on AMD hardware, cpuidmask_defaults._1cd gets
>> configured with the HYPERVISOR bit before native CPUID is scanned for feature
>> bits.
>>
>> This results in cpu_has_hypervisor becoming set as part of identify_cpu(), 
>> and
>> ends up appearing in the raw and host CPU policies.
>>
>> A combination of this bug, and c/s bb502a8ca59 "x86: check feature flags 
>> after
>> resume" which checks that feature bits don't go missing, results in broken S3
>> on AMD hardware.
>>
>> Alter amd_init_levelling() to exclude the HYPERVISOR bit from
>> cpumask_defaults, and update domain_cpu_policy_changed() to allow it to be
>> explicitly forwarded.
>>
>> This also fixes a bug on kexec, where the hypervisor bit is left enabled for
>> the new kernel to find.
>>
>> These changes highlight a further but - dom0 construction is asymetric with
>> domU construction, by not having any calls to domain_cpu_policy_changed().
>> Extend arch_domain_create() to always call domain_cpu_policy_changed().
>>
>> Reported-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>> CC: Claudia <claudia1@xxxxxxxxxxx>
>>
>> v2:
>>  * Rewrite the commit message.  No change to the patch content.
>>
>> Marek/Claudia: Do either of you want a Reported-by tag seeing as you found a
>> brand new way that this was broken?

I understand this is addressing only one half of their issue. Since
you said you don't find it surprising, do you have any idea why the
OSXSAVE bit is behaving differently on AMD and on Intel?

>> @@ -106,6 +106,13 @@ static void domain_cpu_policy_changed(struct domain *d)
>>                      ecx = 0;
>>                  edx = cpufeat_mask(X86_FEATURE_APIC);
>>  
>> +                /*
>> +                 * If the Hypervisor bit is set in the policy, we can also
>> +                 * forward it into real CPUID.
>> +                 */
>> +                if ( p->basic.hypervisor )
>> +                    ecx |= cpufeat_mask(X86_FEATURE_HYPERVISOR);
> 
> AFAICT dom0 will also get the hypervisor bit set by default, as that's
> part of both the HVM and the PV max policy?
> 
> If so:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>



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