Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2b8f7201-24fe-0eba-2629-e35900f4d5d7@amd.com>
Date:   Mon, 5 Dec 2022 20:32:52 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v2] x86/sev: Add SEV-SNP guest feature negotiation support
Content-Language: en-US
To:     Borislav Petkov <bp@alien8.de>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org,
        mingo@redhat.com, tglx@linutronix.de, dave.hansen@linux.intel.com,
        seanjc@google.com, pbonzini@redhat.com, thomas.lendacky@amd.com,
        michael.roth@amd.com, stable@kernel.org
References: <20221201100423.7117-1-nikunj@amd.com> <Y44Amf9MJIIi4Ric@zn.tnic>
From:   "Nikunj A. Dadhania" <nikunj@amd.com>
In-Reply-To: <Y44Amf9MJIIi4Ric@zn.tnic>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?THROWStXcmhVVTN6SzVhOFJpNEQ0dkg4dmZTb0JOSlJabkJOTmhwUlFTQUd1?=
 =?utf-8?B?WXZqSThsNXFwa0ZvNUwyL0pPRVY2SWlhT3V5MmYrc1FVM0RRa3dsZmY3MzUr?=
 =?utf-8?B?ckZTQWJhQzZNZ3Y3NTBrRE5KTk9wTDVnTldNUHdsMnhrK2pKY1hwODdQMFdv?=
 =?utf-8?B?T214cG1tbW5ReExreHNESUZSanV5dkFvZjcxTVNsZkJ3aTlaSFpxQ0EzZm9V?=
 =?utf-8?B?MlBqbzZabFM0WElkclBlZjZtemNxSVhsTm9Oc2tacmY2OUJoZkZDS1FVVUdh?=
 =?utf-8?B?YTVtTm9UOE1PazlVZmVBRDFIaytTMWIwalV5TlVzd1FxZ2RKVGZvL1dseU5w?=
 =?utf-8?B?SE9USmZNNFZtU25lY3hmZzZiZTBxVlY0aEVrSGxCb0tFQ0MrK3JMaFFReVNl?=
 =?utf-8?B?eStFYURoOEZkMCtSRkkySUNYZGxIMGRXVlJla3ZiVjNKLzdyUENjSW5TT08y?=
 =?utf-8?B?cEVPOEZBQ05IZ0xNelJpSWFnRkNhWXdva3FRSGhCd3ppRklHZ0hKajV2bjM3?=
 =?utf-8?B?MzVhTDAyQ3hoNk5wRkZZRUNuZmRhbVlNc1ZYOW02bVVsWDdES2EyNWFXRmdh?=
 =?utf-8?B?dXRmeS81QW16RUFNeHo3R3F1SmZwZXJJZjgxQ3FCelVrV3l1OWNqejFMN2kz?=
 =?utf-8?B?ZGxBd0NVWDlBano1MnYvdjNTVzgxV25YWVZTMzNDeGt2eEUyaENIT2pxc3Yw?=
 =?utf-8?B?OHo3dnNpbExUQ00vbC91NUx5dEhFWkhsZXJLV21uSVp0RlZKM0VBS0ttL21h?=
 =?utf-8?B?WmNSTHFOSnFCcG5TSzJiRFlwMEVDeXprbWlQVkVCTnRxZTcxWXArcU5EdDhH?=
 =?utf-8?B?TnJ3dUF4Q3kyYXJLVy9UamxtZlF5Szk3b0U2RUxsUmd6aXE4RHhBdGszdnMy?=
 =?utf-8?B?bk10eDd3VkFDelVqUTRSdmoxemRDaHF6WnFuOVpCajhaN2FLemUrb2IxNjZH?=
 =?utf-8?B?aGliQ2xPWE04eUhMb1VSVXlTRFg0Y2RLN2VGcGt4UWlLS3NYQWQ3SGpsVDVZ?=
 =?utf-8?B?SGhFbE83bk90LzhubExJMGJGa2xVbzA4cytsSWVmRUJUYjc0RFF0RkMzdHNm?=
 =?utf-8?B?aEVJTDNsTlBvRVB4RDMxVXRjeDV0TVg0U1VRTWVYclI2NW1jT1REdkZySjhD?=
 =?utf-8?B?V2dGUHpvUUNnZGFsODdlenZGaDBubDRFbG1xOHFUMmNMcDA3eHpCM1BhNklJ?=
 =?utf-8?B?NE45bWtIdFdJdkJrZzhGTGFwSkYxeGJDb25GUWFiQjdTQ1d0Y3ZvSkFXY3Zz?=
 =?utf-8?B?WDloNEM5MGF1b2FwMmErOWxDdkVETndxRVV1NGFnckgxTDdyZG03dC9KQUJX?=
 =?utf-8?B?TUM4YVFKcERIMjNPdzBYSG91a0dvZ0J0NUx5eDVremF4K08xQnN4dktLbkNV?=
 =?utf-8?B?Sm5BZkZ4T1pXMm5ZWitHcHpXcFNOem5ENGZXRlhoUllSUGJMZ1gzTEtiZ3lX?=
 =?utf-8?B?WWhYLzB5YkxidXdlY0diN1REdnorSFl2aEtMS0phSUsxVS8zK2ZFN1loRDZW?=
 =?utf-8?B?NDdZMUVoTS9MYWREUHdxNFZBdG9Fc3kwNXBjdGtSMWdRK0I0VGNzYldvQnRX?=
 =?utf-8?B?MWg2aWJNakd3cmRwY00wWEU2anFUTEJEbEFqZnFZSWNQSE05UTZEd0gwamV1?=
 =?utf-8?B?dTdCVVhFK1B1V1YxTkRzYU1melJxbXBwb0I2T21JM3c2a003cUhtNE82bDN5?=
 =?utf-8?B?TjV0NU4wd2JkYWJPMlhEZWpRdkFXdGhhRFJVUlJmazJYbG1mc3NhVFRpUmY5?=
 =?utf-8?B?bzFpNmI0UldHbnFiS1RmSmZBSUY2d1NFRWl4WXhSLzNzUDVrSjc2TUVYOExq?=
 =?utf-8?B?RnVDVGRySmhtYXRqQkQ4b0FyampTN1pNL0F5a1FjK1VFbVA3L0NIN2g4bjk1?=
 =?utf-8?B?RHFPZHIzbjUybzl1ZFJnTkJxcUZkU1F4WFVxU3dUVW9iVUNlYmZCR1BDcFRX?=
 =?utf-8?B?eWhRODBsemE5UGtZd3VKMXpENnRuTXBZNkxRQ1loWEFCNmhHamc4eFo4YjNP?=
 =?utf-8?B?dWFhOW9qa3hjNk0zTXNsT1M5VC9CRS9LN096T2FlR0IxWEFOWmFobnE3TlU5?=
 =?utf-8?B?OGZpTExQcmZvUlEvem15UWttclNaYVh3TERaVkxGVHVPVWtRb3VWcXZaSHFx?=
 =?utf-8?Q?2nXzCEUkpacf3M9Vf0diLETJt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fd1da80c-8dca-4468-f65e-08dad6d1d103
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6309.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Dec 2022 15:03:07.0814
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: u47U9wfcSjIrUJFatcKzyRozLNyKdbM1yfQpfP0D3K3mDxg1b+OMOVyXWgSsWGGJw5lFIJQ8v79EDqcsg1uPuA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4326
X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 05/12/22 20:00, Borislav Petkov wrote:
> On Thu, Dec 01, 2022 at 03:34:23PM +0530, Nikunj A Dadhania wrote:
>> The hypervisor can enable various new features (SEV_FEATURES[1:63])
>> and start the SNP guest. Some of these features need guest side
>> implementation. If any of these features are enabled without guest
>> side implementation, the behavior of the SNP guest will be undefined.
>> The SNP guest boot may fail in a non-obvious way making it difficult
>> to debug.
>>
>> Instead of allowing the guest to continue and have it fail randomly
>> later, detect this early and fail gracefully.
>>
>> SEV_STATUS MSR indicates features which hypervisor has enabled. While
>> booting, SNP guests should ascertain that all the enabled features
>> have guest side implementation. In case any feature is not implemented
>> in the guest, the guest terminates booting with SNP feature
>> unsupported exit code.
>>
>> The below table lists the expected guest behavior with various
>> possible scenarios of guest/hypervisor SNP feature support.
>>
>> +---------------+---------------+---------------+---------------+
>> |Feature Enabled|  Guest needs  |   Guest has   |  Guest boot   |
>> |     by HV     |implementation |implementation |   behavior    |
>> +---------------+---------------+---------------+---------------+
>> |      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|
>> +---------------+---------------+---------------+---------------+
> 
> I like this table and I wouldn't want for it to go under in some commit
> message which is not that easy to retrieve so I'm thinking you should
> add it along with some blurb to
> 
>   Documentation/x86/amd-memory-encryption.rst
> 
> instead where it belongs.

Sure will do.

> 
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index c93930d5ccbd..571eb2576475 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c
>> @@ -270,6 +270,50 @@ static void enforce_vmpl0(void)
>>  		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
>>  }
>>  
>> +/*
>> + * SNP_FEATURES_NEED_GUEST_IMPLEMENTATION is the mask of SNP features that
>> + * will need guest side implementation for proper functioning of the guest.
>> + * If any of these features are enabled without guest side implementation,
> 
> "... are enabled in the hypervisor ... "
> 

Sure

>> + * the behavior of the guest will be undefined. The guest may fail in
>> + * non-obvious way making it difficult to debug.
>> + *
>> + * SNP reserved feature bits may or may not need guest side implementation.
> 
> Yah, get rid of that PPR formulation. If you see the verb "may" always
> run away. :-)
>

True, had to add "may" as the I don't know what the bit will be used for.
 
>> + * As the behavior of reserved feature bits are unknown, to be on the safer
>> + * side add them to the NEED_GUEST_IMPLEMENTATION mask.
> 
> Yah, that makes sense - you want to protect those for future use. Ack.
> 
>> + */
>> +#define SNP_FEATURES_NEED_GUEST_IMPLEMENTATION (MSR_AMD64_SNP_VTOM |			\
> 
> SNP_FEATURES_REQUIRED
> 
> Simpler and shorter.

Yes, much better.

> 
>> +						MSR_AMD64_SNP_REFLECT_VC |		\
>> +						MSR_AMD64_SNP_RESTRICTED_INJ |		\
>> +						MSR_AMD64_SNP_ALT_INJ |			\
>> +						MSR_AMD64_SNP_DEBUG_SWAP |		\
>> +						MSR_AMD64_SNP_VMPL_SSS |		\
>> +						MSR_AMD64_SNP_SECURE_TSC |		\
>> +						MSR_AMD64_SNP_VMGEXIT_PARAM |		\
>> +						MSR_AMD64_SNP_VMSA_REG_PROTECTION |	\
>> +						MSR_AMD64_SNP_RESERVED_BIT13 |		\
>> +						MSR_AMD64_SNP_RESERVED_BIT15 |		\
>> +						MSR_AMD64_SNP_RESERVED_MASK)
>> +
>> +/*
>> + * SNP_FEATURES_HAS_GUEST_IMPLEMENTATION is the mask of SNP features that are
>> + * implemented by the guest kernel. As and when a new feature is implemented
>> + * in the guest kernel, a corresponding bit should be added to the mask.
> 
> And there's no way we won't notice that we've forgotten to do so because
> it'll terminate with the proper error code.
> 
>> + */
>> +#define SNP_FEATURES_HAS_GUEST_IMPLEMENTATION (0)
> 
> SNP_FEATURES_PRESENT
> 
> And so I've done a couple of changes ontop, here's a diff as it explains
> a lot better what I mean.
> 
> Have a look and let me know if something's wrong.

Looks good. Do you want me to send v3 with these changes ?

Regards
Nikunj

> 
> Thx.
> 
> ---
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 10272835dfe9..f023d37e2c41 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -271,48 +271,35 @@ static void enforce_vmpl0(void)
>  }
>  
>  /*
> - * SNP_FEATURES_NEED_GUEST_IMPLEMENTATION is the mask of SNP features that
> - * will need guest side implementation for proper functioning of the guest.
> - * If any of these features are enabled without guest side implementation,
> - * the behavior of the guest will be undefined. The guest may fail in
> - * non-obvious way making it difficult to debug.
> - *
> - * SNP reserved feature bits may or may not need guest side implementation.
> - * As the behavior of reserved feature bits are unknown, to be on the safer
> - * side add them to the NEED_GUEST_IMPLEMENTATION mask.
> - */
> -#define SNP_FEATURES_NEED_GUEST_IMPLEMENTATION (MSR_AMD64_SNP_VTOM |			\
> -						MSR_AMD64_SNP_REFLECT_VC |		\
> -						MSR_AMD64_SNP_RESTRICTED_INJ |		\
> -						MSR_AMD64_SNP_ALT_INJ |			\
> -						MSR_AMD64_SNP_DEBUG_SWAP |		\
> -						MSR_AMD64_SNP_VMPL_SSS |		\
> -						MSR_AMD64_SNP_SECURE_TSC |		\
> -						MSR_AMD64_SNP_VMGEXIT_PARAM |		\
> -						MSR_AMD64_SNP_VMSA_REG_PROTECTION |	\
> -						MSR_AMD64_SNP_RESERVED_BIT13 |		\
> -						MSR_AMD64_SNP_RESERVED_BIT15 |		\
> -						MSR_AMD64_SNP_RESERVED_MASK)
>  
> -/*
> - * SNP_FEATURES_HAS_GUEST_IMPLEMENTATION is the mask of SNP features that are
> - * implemented by the guest kernel. As and when a new feature is implemented
> - * in the guest kernel, a corresponding bit should be added to the mask.
> + * SNP_FEATURES_REQUIRED is the mask of SNP features that will need
> + * guest side implementation for proper functioning of the guest. If any
> + * of these features are enabled in the hypervisor but are lacking guest
> + * side implementation, the behavior of the guest will be undefined. The
> + * guest could fail in non-obvious way making it difficult to debug.
> + *
> + * As the behavior of reserved feature bits is unknown to be on the
> + * safe side add them to the required features mask.
>   */
> -#define SNP_FEATURES_HAS_GUEST_IMPLEMENTATION (0)
> +#define SNP_FEATURES_REQUIRED		(MSR_AMD64_SNP_VTOM |			\
> +					MSR_AMD64_SNP_REFLECT_VC |		\
> +					MSR_AMD64_SNP_RESTRICTED_INJ |		\
> +					MSR_AMD64_SNP_ALT_INJ |			\
> +					MSR_AMD64_SNP_DEBUG_SWAP |		\
> +					MSR_AMD64_SNP_VMPL_SSS |		\
> +					MSR_AMD64_SNP_SECURE_TSC |		\
> +					MSR_AMD64_SNP_VMGEXIT_PARAM |		\
> +					MSR_AMD64_SNP_VMSA_REG_PROTECTION |	\
> +					MSR_AMD64_SNP_RESERVED_BIT13 |		\
> +					MSR_AMD64_SNP_RESERVED_BIT15 |		\
> +					MSR_AMD64_SNP_RESERVED_MASK)
>  
>  /*
> - * The hypervisor can enable various features flags (in SEV_FEATURES[1:63]) and
> - * start the SNP guest. Certain SNP features need guest side implementation.
> - * Check if the SNP guest has implementation for those features.
> + * SNP_FEATURES_PRESENT is the mask of SNP features that are implemented
> + * by the guest kernel. As and when a new feature is implemented in the
> + * guest kernel, a corresponding bit should be added to the mask.
>   */
> -static bool snp_guest_has_features_implemented(void)
> -{
> -	u64 guest_features_not_implemented = SNP_FEATURES_NEED_GUEST_IMPLEMENTATION &
> -		~SNP_FEATURES_HAS_GUEST_IMPLEMENTATION;
> -
> -	return !(sev_status & guest_features_not_implemented);
> -}
> +#define SNP_FEATURES_PRESENT (0)
>  
>  void sev_enable(struct boot_params *bp)
>  {
> @@ -383,7 +370,7 @@ void sev_enable(struct boot_params *bp)
>  		 * Terminate the boot if hypervisor has enabled any feature
>  		 * missing guest side implementation.
>  		 */
> -		if (!snp_guest_has_features_implemented())
> +		if (sev_status & SNP_FEATURES_REQUIRED & ~SNP_FEATURES_PRESENT)
>  			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED);
>  
>  		enforce_vmpl0();
>