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

[Nikunj A. Dadhania]

On 13/01/23 17:23, Zhi Wang wrote:
> On Thu, 12 Jan 2023 14:11:39 +0530
> Nikunj A Dadhania <nikunj@xxxxxxx> wrote:
> 

>> diff --git a/Documentation/x86/amd-memory-encryption.rst
>> b/Documentation/x86/amd-memory-encryption.rst index
>> a1940ebe7be5..b3adc39d7735 100644 ---
>> a/Documentation/x86/amd-memory-encryption.rst +++
>> b/Documentation/x86/amd-memory-encryption.rst @@ -95,3 +95,39 @@ by
>> supplying mem_encrypt=on on the kernel command line.  However, if BIOS
>> does not enable SME, then Linux will not be able to activate memory
>> encryption, even if configured to do so by default or the mem_encrypt=on
>> command line parameter is specified. +
>> +Secure Nested Paging (SNP)
>> +==========================
>> +
>> +SEV-SNP introduces new features (SEV_FEATURES[1:63]) which can be
>> enabled +by the hypervisor for security enhancements. Some of these
>> features need +guest side implementation to function correctly. The
>> below table lists the +expected guest behavior with various possible
>> scenarios of guest/hypervisor +SNP feature support.
>> +

> "guest needs implementation" seems a little bit confusing. I suppose it 
> means the feature is mandatory for the guest. 

That is not correct. None of these features are mandatory for the guest.
The hypervisor can enable this feature without the knowledge of guest 
kernel support. So there should be a mechanism in the guest to detect this
and fail the boot if needed.

> If so, on the second row 
> guest can boot without it. Some explanation? 

In the first and second row, HV has not enabled the feature, so the 
guest should boot fine irrespective of "Guest needs implementation".

>> +|      No         |      No       |      No       |     Boot         |

>> +|      No         |      Yes      |      No       |     Boot         |


>> ++-----------------+---------------+---------------+------------------+
>> +| Feature Enabled | Guest needs   | Guest has     | Guest boot       |
>> +| by the HV       | implementation| implementation| behaviour        |
>> ++=================+===============+===============+==================+>> +|      No         |      No       |      No       |     Boot         |
>> +|                 |               |               |                  |
>> ++-----------------+---------------+---------------+------------------+
>> +|      No         |      Yes      |      No       |     Boot         |
>> +|                 |               |               |                  |
>> ++-----------------+---------------+---------------+------------------+
>> +|      No         |      Yes      |      Yes      |     Boot         |
>> +|                 |               |               |                  |
>> ++-----------------+---------------+---------------+------------------+
>> +|      Yes        |      No       |      No       | Boot with        |
>> +|                 |               |               | feature enabled  |
>> ++-----------------+---------------+---------------+------------------+
>> +|      Yes        |      Yes      |      No       | Graceful boot    |
>> +|                 |               |               | failure          |
>> ++-----------------+---------------+---------------+------------------+
>> +|      Yes        |      Yes      |      Yes      | Boot with        |
>> +|                 |               |               | feature enabled  |
>> ++-----------------+---------------+---------------+------------------+
>> +
>> +More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
>> +
>> +[1] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf
> 
> Probably update the link here as well.

Sure.

>> diff --git a/arch/x86/include/uapi/asm/svm.h
>> b/arch/x86/include/uapi/asm/svm.h index f69c168391aa..a04fe07eb9a8 100644
>> --- a/arch/x86/include/uapi/asm/svm.h
>> +++ b/arch/x86/include/uapi/asm/svm.h
>> @@ -116,6 +116,12 @@
>>  #define SVM_VMGEXIT_AP_CREATE			1
>>  #define SVM_VMGEXIT_AP_DESTROY			2
>>  #define SVM_VMGEXIT_HV_FEATURES			0x8000fffd
>> +#define SVM_VMGEXIT_TERM_REQUEST		0x8000fffe
>> +#define SVM_VMGEXIT_TERM_REASON(reason_set, reason_code)	\
>> +	/* SW_EXITINFO1[3:0] */					\
>> +	(((((u64)reason_set) &  0xf)) |				\
>                                ^
> One extra space before 0xf should be removed.

Sure.

Thanks for the review.

Nikunj