Re: [PATCH] x86/sev: Add SEV-SNP guest feature negotiation support

[Nikunj A. Dadhania]

On 17/11/22 18:23, Borislav Petkov wrote:
> On Thu, Nov 17, 2022 at 05:50:34PM +0530, Nikunj A. Dadhania wrote:
>> Purpose of this patch is older guests kernel that have SNP enabled
>> (5.19 onward), when a particular SNP feature is enabled by the
>> hypervisor that needs enlightened guest, older kernel wont be able to
>> support the feature. There is no mechanism that the hypervisor can
>> find out what feature is supported by the SNP guest before hand.
>>
>> For example PREVENT_HOST_IBS needs changes on hypervisor and no
>> changes in the guest kernel. In this any guest kernel having SNP
>> support should work.
>>
>> While for SECURE_TSC, hypervisor and guest kernel changes are
>> required. And older guest kernel will not work if hypervisor enables
>> Secure TSC. When secure tsc feature is enabled following define should
>> be changed:
> 
> This all is still veiled in mist to me. What are you trying to do here?
> 
> - Make sure older SNP guests boot on newer hypervisors?

Yes and No.

Yes: For feature flags that do not need an enlightened guest, older guests should 
     boot.

No: For feature flags that need an enlightened guest, older guests should detect 
    and fail booting on any hypervisor that sets this feature flag. 

> - Newer guests boot on older hypervisors?

Yes, as the older hypervisor won't enable the new SNP feature flag.

> So, first, pls explain in detail what the goal here is.

The hypervisor can enable various new features flags(in SEV_FEATURES[1:63]) 
and start the guest. The hypervisor does not know beforehand that the guest 
kernel supports the feature that is being enabled.

While booting, SNP guests can discover the hypervisor-enabled features by looking 
at SEV_STATUS MSR. At this point, the SNP guest needs to decide either to 
continue boot or terminate depending on the feature support in the guest kernel.
Otherwise, if we let the guest continue booting with an unsupported feature, 
the guest can fail in non-obvious manner. 

* Primary goal here is to detect such unsupported features beforehand and 
  deterministically terminate the guest.
* Secondary goal is to use this mechanism to future-proof the current guests 
  for newer reserved features flags that may be defined later.

Here is the support matrix per feature that can explain better:

HypervisorEnabled: Feature bit is enabled in SEV_FEATURES
Required: Feature requires enlightened guest
Available: The guest is enlightened for that particular feature.
Boot: The expected behavior of the guest, whether it should boot or fail.

+-------------------+----------+-------------+----------+
| HypervisorEnabled | Required | Available   |   Boot   |
+-------------------+----------+-------------+----------+
| Y                 |  Y       |  N          | N (Fail) |
| Y                 |  Y       |  Y          | Y        |
| Y                 |  N       |  N          | Y        |
| N                 | Y/N      | Y/N         | Y        |
+-------------------+----------+-------------+----------+

> I'm reading the above in multiple ways so you need to spell out first
> what you wanna do.
> 
> PREVENT_HOST_IBS doesn't need any enablement. So why is it in the mask?

PREVENT_HOST_IBS will be defined but shouldn't be part of the 
"Required" mask.

> SECURE_TSC needs enablement on both. Why aren't you checking only this
> one.

SECURE_TSC should be part of "Required" mask and once secure tsc 
support is up-streamed it should be added to "Available" mask.

> IOW, I would expect to check *only* for features which the guest needs
> for the hypervisor to support before it boots. But not check everything
> wholesale.

Guests need to check for features enabled by the hypervisor that is not 
supported as well, so that we can terminate the guest right there.

> IOW, I see it this way: guest boots, sees what the hypervisor has
> enabled as SEV_STATUS cannot be intercepted and acts accordingly.
>
> Now, the question how *old* guests should act here is a whole different
> story as it depends on whether this gets backported to old guests -
> which doesn't make them old anymore as the checking will happen - 

Old guests with backport will deny booting with unsupported features. 

> or to
> really old guests without the checking. There it doesn't matter.

It does matter for old guests which dont have backport, the behavior will 
be undefined.

Hence fix needs to be back ported till 5.19, where SNP guest support was added.
 
> And come to think of it, this whole deal is no different than having
> feature bits in CPUID and the kernel implementing them.
> 
> If the kernel finds a feature bit set in CPUID, it enables the
> corresponding code. If it doesn't know about it, then it doesn't do
> anything.
> 
> Pretty much the same here: if a SNP guest finds a feature flag in
> SEV_STATUS, then it enables the code corresponding to it. 

This is the good case, where the feature flag is enabled and the guest 
kernel has support for the feature. This should boot fine.

Second Case which we are handling in this patch: SNP guest finds the 
feature flag enabled, but it does not have code corresponding to that 
feature. The guest boot should *fail*.

> If it doesn't
> find it but it needs it due to enablement, then it stops booting.

Above is the third case: where the SNP guest does not see the feature flag 
enabled and the  guest should boot fine irrespective of whether it is enlightened 
or not.

Regards,
Nikunj