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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/02/2020 16:59, Jan Beulich wrote:
> 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?

It isn't behaving differently between Intel and AMD, I don't think.

The diagnostics are asymmetric - they ever printed when a feature
disappears, not for a feature appearing.

OSXSAVE is clear until fairly late on boot (therefore misses being
cached), but is restored as part of mmu_cr4_features (before the feature
check).

The only reason anything gets printed in the first place is because the
HYPERVISOR bit disappeared.

Overall, OSXSAVE (and in principle OSPKE if we start supporting it in PV
guests) are benign.  We could filter them out of the diagnostics but
don't currently have a suitable featuremask, and I'm not sure adding one
is worth it.

>
>>> @@ -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>

Thanks.

~Andrew

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