.../admin-guide/kernel-parameters.txt | 18 +++ arch/x86/kvm/svm/sev.c | 96 +++++++++++-- drivers/crypto/ccp/sev-dev.c | 127 ++++++++++++++++-- drivers/crypto/ccp/sev-dev.h | 6 +- include/linux/psp-sev.h | 44 +++++- include/uapi/linux/psp-sev.h | 10 +- 6 files changed, 274 insertions(+), 27 deletions(-)
From: Ashish Kalra <ashish.kalra@amd.com>
Ciphertext hiding prevents host accesses from reading the ciphertext
of SNP guest private memory. Instead of reading ciphertext, the host
will see constant default values (0xff).
The SEV ASID space is split into SEV and SEV-ES/SNP ASID ranges.
Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
into separate ASID ranges for SEV-ES and SEV-SNP guests.
Add new module parameter to the KVM module to enable ciphertext hiding
support and a user configurable system-wide maximum SNP ASID value. If
the module parameter value is "max" then the complete SEV-ES/SEV-SNP
space is allocated to SEV-SNP guests.
v7:
- Fix comments.
- Move the check for module parameter ciphertext_hiding_asids inside
check_and_enable_sev_snp_ciphertext_hiding(), this keeps all the logic
related to the parameter in a single function.
v6:
- Fix module parameter ciphertext_hiding_asids=0 case.
- Coalesce multiple cases of handling invalid module parameter
ciphertext_hiding_asids into a single branch/label.
- Fix commit logs.
- Fix Documentation.
v5:
- Add pre-patch to cache SEV platform status and use this cached
information to set api_major/api_minor/build.
- Since the SEV platform status and SNP platform status differ,
remove the state field from sev_device structure and instead track
SEV platform state from cached SEV platform status.
- If SNP is enabled then cached SNP platform status is used for
api_major/api_minor/build.
- Fix using sev_do_cmd() instead of __sev_do_cmd_locked().
- Fix commit logs.
- Fix kernel-parameters documentation.
- Modify KVM module parameter to enable CipherTextHiding to support
"max" option to allow complete SEV-ES+ ASID space to be allocated
to SEV-SNP guests.
- Do not enable ciphertext hiding if module parameter to specify
maximum SNP ASID is invalid.
v4:
- Fix buffer allocation for SNP_FEATURE_INFO command to correctly
handle page boundary check requirements.
- Return correct length for SNP_FEATURE_INFO command from
sev_cmd_buffer_len().
- Switch to using SNP platform status instead of SEV platform status if
SNP is enabled and cache SNP platform status and feature information.
Modify sev_get_api_version() accordingly.
- Fix commit logs.
- Expand the comments on why both the feature info and the platform
status fields have to be checked for CipherTextHiding feature
detection and enablement.
- Add new preperation patch for CipherTextHiding feature which
introduces new {min,max}_{sev_es,snp}_asid variables along with
existing {min,max}_sev_asid variable to simplify partitioning of the
SEV and SEV-ES+ ASID space.
- Switch to single KVM module parameter to enable CipherTextHiding
feature and the maximum SNP ASID usable for SNP guests when
CipherTextHiding feature is enabled.
v3:
- rebase to linux-next.
- rebase on top of support to move SEV-SNP initialization to
KVM module from CCP driver.
- Split CipherTextHiding support between CCP driver and KVM module
with KVM module calling into CCP driver to initialize SNP with
CipherTextHiding enabled and MAX ASID usable for SNP guest if
KVM is enabling CipherTextHiding feature.
- Move module parameters to enable CipherTextHiding feature and
MAX ASID usable for SNP guests from CCP driver to KVM module
which allows KVM to be responsible for enabling CipherTextHiding
feature if end-user requests it.
v2:
- Fix and add more description to commit logs.
- Rename sev_cache_snp_platform_status_and_discover_features() to
snp_get_platform_data().
- Add check in snp_get_platform_data to guard against being called
after SNP_INIT_EX.
- Fix comments for new structure field definitions being added.
- Fix naming for new structure being added.
- Add new vm-type parameter to sev_asid_new().
- Fix identation.
- Rename CCP module parameters psp_cth_enabled to cipher_text_hiding and
psp_max_snp_asid to max_snp_asid.
- Rename max_snp_asid to snp_max_snp_asid.
Ashish Kalra (7):
crypto: ccp - New bit-field definitions for SNP_PLATFORM_STATUS
command
crypto: ccp - Cache SEV platform status and platform state
crypto: ccp - Add support for SNP_FEATURE_INFO command
crypto: ccp - Introduce new API interface to indicate SEV-SNP
Ciphertext hiding feature
crypto: ccp - Add support to enable CipherTextHiding on SNP_INIT_EX
KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables
KVM: SEV: Add SEV-SNP CipherTextHiding support
.../admin-guide/kernel-parameters.txt | 18 +++
arch/x86/kvm/svm/sev.c | 96 +++++++++++--
drivers/crypto/ccp/sev-dev.c | 127 ++++++++++++++++--
drivers/crypto/ccp/sev-dev.h | 6 +-
include/linux/psp-sev.h | 44 +++++-
include/uapi/linux/psp-sev.h | 10 +-
6 files changed, 274 insertions(+), 27 deletions(-)
--
2.34.1
Hi Herbert, can you please merge patches 1-5. Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging for patches 6 & 7. Thanks, Ashish
On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote: > Hi Herbert, can you please merge patches 1-5. > > Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging > for patches 6 & 7. These patches will be at the base of the cryptodev tree for 6.17 so it could be pulled into another tree without any risks. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 8/16/2025 3:39 AM, Herbert Xu wrote: > On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote: >> Hi Herbert, can you please merge patches 1-5. >> >> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging >> for patches 6 & 7. > > These patches will be at the base of the cryptodev tree for 6.17 > so it could be pulled into another tree without any risks. > > Cheers, Thanks Herbert for pulling in patches 1-5. Paolo, can you please merge patches 6 and 7 into the KVM tree. Thanks, Ashish
On 8/18/25 2:16 PM, Kalra, Ashish wrote: > On 8/16/2025 3:39 AM, Herbert Xu wrote: >> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote: >>> Hi Herbert, can you please merge patches 1-5. >>> >>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging >>> for patches 6 & 7. >> These patches will be at the base of the cryptodev tree for 6.17 >> so it could be pulled into another tree without any risks. >> >> Cheers, > Thanks Herbert for pulling in patches 1-5. > > Paolo, can you please merge patches 6 and 7 into the KVM tree. Hi Ashish, I have pending comments on patch 7: https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/ If still not welcome, can you say why you think: 1. The ciphertext_hiding_asid_nr variable is necessary 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check redundant? 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed. Thanks, Kim
On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote:
> I have pending comments on patch 7:
If you're so hell-bent on doing your improvements on-top or aside of them, you
take his patch, add your stuff ontop or rewrite it, test it and then you send
it out and say why yours is better.
Then the maintainer decides.
There's no need to debate ad absurdum - you simply offer your idea and the
maintainer decides which one is better. As it has always been done.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Aug 19, 2025, Borislav Petkov wrote: > On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote: > > I have pending comments on patch 7: > > If you're so hell-bent on doing your improvements on-top or aside of them, you > take his patch, add your stuff ontop or rewrite it, test it and then you send > it out and say why yours is better. > > Then the maintainer decides. > > There's no need to debate ad absurdum - you simply offer your idea and the > maintainer decides which one is better. As it has always been done. Or, the maintainer says "huh!?" and goes with option C. Why take a string in the first place? Just use '-1' as "max/auto".
Hello Sean, On 8/19/2025 7:05 PM, Sean Christopherson wrote: > On Tue, Aug 19, 2025, Borislav Petkov wrote: >> On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote: >>> I have pending comments on patch 7: >> >> If you're so hell-bent on doing your improvements on-top or aside of them, you >> take his patch, add your stuff ontop or rewrite it, test it and then you send >> it out and say why yours is better. >> >> Then the maintainer decides. >> >> There's no need to debate ad absurdum - you simply offer your idea and the >> maintainer decides which one is better. As it has always been done. > > Or, the maintainer says "huh!?" and goes with option C. > > Why take a string in the first place? Just use '-1' as "max/auto". It's just that there was general feedback to use a string like "max". But as a maintainer if you are suggesting to use '-1' as "max/auto", i can do that. Thanks, Ashish
On Tue, Aug 19, 2025, Ashish Kalra wrote:
> Hello Sean,
>
> On 8/19/2025 7:05 PM, Sean Christopherson wrote:
> > On Tue, Aug 19, 2025, Borislav Petkov wrote:
> >> On Mon, Aug 18, 2025 at 02:38:38PM -0500, Kim Phillips wrote:
> >>> I have pending comments on patch 7:
> >>
> >> If you're so hell-bent on doing your improvements on-top or aside of them, you
> >> take his patch, add your stuff ontop or rewrite it, test it and then you send
> >> it out and say why yours is better.
> >>
> >> Then the maintainer decides.
> >>
> >> There's no need to debate ad absurdum - you simply offer your idea and the
> >> maintainer decides which one is better. As it has always been done.
> >
> > Or, the maintainer says "huh!?" and goes with option C.
> >
> > Why take a string in the first place? Just use '-1' as "max/auto".
>
> It's just that there was general feedback to use a string like "max".
From who? There's definitely value in providing/using human-friendly names for
things like this, but that needs to be weighed against the cost and complexity
in the kernel. And the cost+complexity is quite high:
> +static bool check_and_enable_sev_snp_ciphertext_hiding(void)
> +{
> + unsigned int ciphertext_hiding_asid_nr = 0;
> +
> + if (!ciphertext_hiding_asids[0])
> + return false;
> +
> + if (!sev_is_snp_ciphertext_hiding_supported()) {
> + pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
This will print a spurious message if the admin explicitly loading KVM with
ciphertext_hiding_asids=0.
> + return false;
> + }
> +
> + if (isdigit(ciphertext_hiding_asids[0])) {
> + if (kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr))
> + goto invalid_parameter;
> +
> + /* Do sanity check on user-defined ciphertext_hiding_asids */
> + if (ciphertext_hiding_asid_nr >= min_sev_asid) {
> + pr_warn("Module parameter ciphertext_hiding_asids (%u) exceeds or equals minimum SEV ASID (%u)\n",
> + ciphertext_hiding_asid_nr, min_sev_asid);
> + return false;
This is unfortunate and probably unexpected behavior, because if the admin
provided a super large value, odds are good they would make SEV-ES unusable than
disable ciphertext hiding.
> + }
> + } else if (!strcmp(ciphertext_hiding_asids, "max")) {
> + ciphertext_hiding_asid_nr = min_sev_asid - 1;
The actual resolved value isn't captured in the module param.
> + }
> +
> + if (ciphertext_hiding_asid_nr) {
> + max_snp_asid = ciphertext_hiding_asid_nr;
> + min_sev_es_asid = max_snp_asid + 1;
> + pr_info("SEV-SNP ciphertext hiding enabled\n");
> +
> + return true;
> + }
This will fallthrough on ciphertext_hiding_asids=0 as well and yell about '0'
being invalid.
> +
> +invalid_parameter:
> + pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
> + ciphertext_hiding_asids);
> + return false;
> +}
> But as a maintainer if you are suggesting to use '-1' as "max/auto", i can do
> that.
Looking at this again, I don't see any reason to special case -1. Just make the
param a uint and cap it at that maximum possible value. As above, disabling
ciphertext hiding if the number of request SNP ASIDs is higher than what hardware
supports is probably not want the admin wants.
Compile tested only.
--
From: Ashish Kalra <ashish.kalra@amd.com>
Date: Mon, 21 Jul 2025 14:14:34 +0000
Subject: [PATCH] KVM: SEV: Add SEV-SNP CipherTextHiding support
Ciphertext hiding prevents host accesses from reading the ciphertext of
SNP guest private memory. Instead of reading ciphertext, the host reads
will see constant default values (0xff).
The SEV ASID space is split into SEV and SEV-ES/SEV-SNP ASID ranges.
Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
into separate ASID ranges for SEV-ES and SEV-SNP guests.
Add a new off-by-default kvm-amd module parameter enable ciphertext
hiding and allow the admin to configure the SEV-ES and SEV-SNP ASID
ranges. Simply cap the maximum SEV-SNP ASID as appropriate, i.e. don't
reject loading KVM or disable ciphertest hiding for a too-big value, as
KVM's general approach for module params is to sanitize inputs based on
hardware/kernel support, not burn the world down. This also allows the
admin to use -1u to assign all SEV-ES/SNP ASIDs to SNP without needing
dedicated handling in KVM.
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
.../admin-guide/kernel-parameters.txt | 19 +++++++++++
arch/x86/kvm/svm/sev.c | 32 ++++++++++++++++++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 747a55abf494..f4735931661e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2957,6 +2957,25 @@
(enabled). Disable by KVM if hardware lacks support
for NPT.
+ kvm-amd.ciphertext_hiding_asids=
+ [KVM,AMD] Ciphertext hiding prevents disallowed accesses
+ to SNP private memory from reading ciphertext. Instead,
+ reads will see constant default values (0xff).
+
+ If ciphertext hiding is enabled, the joint SEV-ES+SEV-SNP
+ ASID space is paritioned separate SEV-ES and SEV-SNP ASID
+ ranges, with the SEV-SNP ASID range starting at 1. For
+ SEV-ES/SEV-SNP guests the maximum ASID is MIN_SEV_ASID-1,
+ where MIN_SEV_ASID value is discovered by CPUID
+ Fn8000_001F[EDX].
+
+ A non-zero value enables SEV-SNP ciphertext hiding and
+ adjusts the ASID ranges for SEV-ES and SEV-SNP guests.
+ KVM caps the number of SEV-SNP ASIDs at the maximum
+ possible value, e.g. specifying -1u will assign all
+ join SEV-ES+SEV-SNP ASIDs to SEV-SNP and make SEV-ES
+ unusable.
+
kvm-arm.mode=
[KVM,ARM,EARLY] Select one of KVM/arm64's modes of
operation.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index cd9ce100627e..52efd43c333a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -59,6 +59,9 @@ static bool sev_es_debug_swap_enabled = true;
module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
static u64 sev_supported_vmsa_features;
+static unsigned int nr_ciphertext_hiding_asids;
+module_param_named(ciphertext_hiding_asids, nr_ciphertext_hiding_asids, uint, 0444);
+
#define AP_RESET_HOLD_NONE 0
#define AP_RESET_HOLD_NAE_EVENT 1
#define AP_RESET_HOLD_MSR_PROTO 2
@@ -201,6 +204,9 @@ static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
/*
* The min ASID can end up larger than the max if basic SEV support is
* effectively disabled by disallowing use of ASIDs for SEV guests.
+ * Similarly for SEV-ES guests the min ASID can end up larger than the
+ * max when ciphertext hiding is enabled, effectively disabling SEV-ES
+ * support.
*/
if (min_asid > max_asid)
return -ENOTTY;
@@ -3064,10 +3070,32 @@ void __init sev_hardware_setup(void)
out:
if (sev_enabled) {
init_args.probe = true;
+
+ if (sev_is_snp_ciphertext_hiding_supported())
+ init_args.max_snp_asid = min(nr_ciphertext_hiding_asids,
+ min_sev_asid - 1);
+
if (sev_platform_init(&init_args))
sev_supported = sev_es_supported = sev_snp_supported = false;
else if (sev_snp_supported)
sev_snp_supported = is_sev_snp_initialized();
+
+ if (sev_snp_supported)
+ nr_ciphertext_hiding_asids = init_args.max_snp_asid;
+
+ /*
+ * If ciphertext hiding is enabled, the joint SEV-ES/SEV-SNP
+ * ASID range is partitioned into separate SEV-ES and SEV-SNP
+ * ASID ranges, with the SEV-SNP range being [1..max_snp_asid]
+ * and the SEV-ES range being (max_snp_asid..max_sev_es_asid].
+ * Note, SEV-ES may effectively be disabled if all ASIDs from
+ * the joint range are assigned to SEV-SNP.
+ */
+ if (nr_ciphertext_hiding_asids) {
+ max_snp_asid = nr_ciphertext_hiding_asids;
+ min_sev_es_asid = max_snp_asid + 1;
+ pr_info("SEV-SNP ciphertext hiding enabled\n");
+ }
}
if (boot_cpu_has(X86_FEATURE_SEV))
@@ -3078,7 +3106,9 @@ void __init sev_hardware_setup(void)
min_sev_asid, max_sev_asid);
if (boot_cpu_has(X86_FEATURE_SEV_ES))
pr_info("SEV-ES %s (ASIDs %u - %u)\n",
- str_enabled_disabled(sev_es_supported),
+ sev_es_supported ? min_sev_es_asid <= max_sev_es_asid ? "enabled" :
+ "unusable" :
+ "disabled",
min_sev_es_asid, max_sev_es_asid);
if (boot_cpu_has(X86_FEATURE_SEV_SNP))
pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
base-commit: fba22ac9ea05ee5e15318823333114104045be2d
--
On 8/18/2025 2:38 PM, Kim Phillips wrote:
> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>> Hi Herbert, can you please merge patches 1-5.
>>>>
>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>> for patches 6 & 7.
>>> These patches will be at the base of the cryptodev tree for 6.17
>>> so it could be pulled into another tree without any risks.
>>>
>>> Cheers,
>> Thanks Herbert for pulling in patches 1-5.
>>
>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
> Hi Ashish,
>
> I have pending comments on patch 7:
>
> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
>
> If still not welcome, can you say why you think:
>
> 1. The ciphertext_hiding_asid_nr variable is necessary
I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity
check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
am sure all sanity checks have been done.
Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
failures, i don't like that.
>
> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check redundant?
isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
calling kstrtoint() if the parameter is not a number.
>
> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.
This is for understandable code flow :
1). Check module parameter is set by user.
2). Check ciphertext_hiding_feature enabled.
3). Check if parameter is numeric.
Sanity checks on numeric parameter
If checks fail goto invalid_parameter
4). Check if parameter is the string "max".
5). Set max_snp_asid and min_sev_es_asid.
6). Fall-through to invalid parameter.
invalid_parameter:
This is overall a more understandable code flow.
Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.
Thanks,
Ashish
>
> Thanks,
>
> Kim
On 8/18/25 3:39 PM, Kalra, Ashish wrote:
> On 8/18/2025 2:38 PM, Kim Phillips wrote:
>> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>>> Hi Herbert, can you please merge patches 1-5.
>>>>>
>>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>>> for patches 6 & 7.
>>>> These patches will be at the base of the cryptodev tree for 6.17
>>>> so it could be pulled into another tree without any risks.
>>>>
>>>> Cheers,
>>> Thanks Herbert for pulling in patches 1-5.
>>>
>>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
>> Hi Ashish,
>>
>> I have pending comments on patch 7:
>>
>> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
>>
>> If still not welcome, can you say why you think:
>>
>> 1. The ciphertext_hiding_asid_nr variable is necessary
> I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity
> check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
> am sure all sanity checks have been done.
>
> Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
> failures, i don't like that.
Item (1):
The rollback is in a single place, and the extra variable's existence
can be avoided, or, at least have 'temp' somewhere in its name.
FWIW, any "Safe coding" practices should have been performed prior to
the submission of the patch, IMO.
>> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check redundant?
> isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
> calling kstrtoint() if the parameter is not a number.
Item (2):
This is module initialization code, it's better optimized for
readability than for performance. As a reader of the code, I'm
constantly wondering why the redundancy exists, and am sure it is made
objectively easier to read if the isdigit() check were removed.
>> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.
> This is for understandable code flow :
>
> 1). Check module parameter is set by user.
> 2). Check ciphertext_hiding_feature enabled.
> 3). Check if parameter is numeric.
> Sanity checks on numeric parameter
> If checks fail goto invalid_parameter
> 4). Check if parameter is the string "max".
> 5). Set max_snp_asid and min_sev_es_asid.
> 6). Fall-through to invalid parameter.
> invalid_parameter:
>
> This is overall a more understandable code flow.
Item (3):
That's not how your original v7 flows, but I do now see the non-obvious
fall-through from the else if (...'max'...). I believe I am not alone
in missing that, and that a comment would have helped. Also, the 'else'
isn't required
Flow readability-wise, comparing the two, after the two common if()s,
your original v7 goes:
{
...
if (isdigit() {
if (kstrtoint())
goto invalid_parameter
if (temporary variable >= min_sev_asid) {
pr_warn()
return false;
} else if (..."max"...) {
temporary variable = ...
/* non-obvious fallthrough to invalid_parameter iff
temporary_variable == 0 */
}
if (temporary variable) {
max_snp_asid = ...
min_sev_es_asid = ...
pr_info(..."enabled"...)
return true;
}
invalid_parameter:
pr_warn()
return false;
}
vs the result of my latest diff:
{
...
if (..."max"...) {
max_snp_asid =
min_sev_es_asid = ...
return true;
}
if (kstrtoint()) {
pr_warn()
return false
}
if (max_snp_asid < 1 || >= min_sev_asid) {
pr_warn()
max_snp_asid = /* rollback */
return false;
}
min_sev_es_asid = ...
return true;
}
So, just from an outright flow perspective, I believe my latest diff is
objectively easier to follow.
> Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.
With this statement, you self-contradict your rationale to keep your
version of the above to Item (2): "isdigit() is a MACRO compared to
kstrtoint() call, it is more optimal to do an inline check and avoid
calling kstrtoint() if the parameter is not a number". If not willing to
take my latest diff as-is, I really would like to see:
Item (1)'s variable get a temporary-sounding name,
item (2)'s the isdigit() check (and thus a whole indentation level)
removed, and
item (3)'s flow reconsidered given the (IMO objective) readability
enhancement.
Thanks,
Kim
On 8/18/2025 6:23 PM, Kim Phillips wrote:
> On 8/18/25 3:39 PM, Kalra, Ashish wrote:
>> On 8/18/2025 2:38 PM, Kim Phillips wrote:
>>> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>>>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>>>> Hi Herbert, can you please merge patches 1-5.
>>>>>>
>>>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>>>> for patches 6 & 7.
>>>>> These patches will be at the base of the cryptodev tree for 6.17
>>>>> so it could be pulled into another tree without any risks.
>>>>>
>>>>> Cheers,
>>>> Thanks Herbert for pulling in patches 1-5.
>>>>
>>>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
>>> Hi Ashish,
>>>
>>> I have pending comments on patch 7:
>>>
>>> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
>>>
>>> If still not welcome, can you say why you think:
>>>
>>> 1. The ciphertext_hiding_asid_nr variable is necessary
>> I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity
>> check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
>> am sure all sanity checks have been done.
>>
>> Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
>> failures, i don't like that.
>
> Item (1):
>
> The rollback is in a single place, and the extra variable's existence can be avoided, or, at least have 'temp' somewhere in its name.
>
> FWIW, any "Safe coding" practices should have been performed prior to the submission of the patch, IMO.
>
>>> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check redundant?
>> isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
>> calling kstrtoint() if the parameter is not a number.
>
> Item (2):
>
> This is module initialization code, it's better optimized for readability than for performance. As a reader of the code, I'm constantly wondering why the redundancy exists, and am sure it is made objectively easier to read if the isdigit() check were removed.
>
>>> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.
>> This is for understandable code flow :
>>
>> 1). Check module parameter is set by user.
>> 2). Check ciphertext_hiding_feature enabled.
>> 3). Check if parameter is numeric.
>> Sanity checks on numeric parameter
>> If checks fail goto invalid_parameter
>> 4). Check if parameter is the string "max".
>> 5). Set max_snp_asid and min_sev_es_asid.
>> 6). Fall-through to invalid parameter.
>> invalid_parameter:
>>
>> This is overall a more understandable code flow.
>
> Item (3):
>
> That's not how your original v7 flows, but I do now see the non-obvious fall-through from the else if (...'max'...). I believe I am not alone in missing that, and that a comment would have helped. Also, the 'else' isn't required
>
> Flow readability-wise, comparing the two, after the two common if()s, your original v7 goes:
>
> {
> ...
> if (isdigit() {
> if (kstrtoint())
> goto invalid_parameter
> if (temporary variable >= min_sev_asid) {
> pr_warn()
> return false;
> } else if (..."max"...) {
> temporary variable = ...
> /* non-obvious fallthrough to invalid_parameter iff temporary_variable == 0 */
> }
>
> if (temporary variable) {
> max_snp_asid = ...
> min_sev_es_asid = ...
> pr_info(..."enabled"...)
> return true;
> }
>
> invalid_parameter:
> pr_warn()
> return false;
> }
>
> vs the result of my latest diff:
>
> {
> ...
> if (..."max"...) {
> max_snp_asid =
> min_sev_es_asid = ...
> return true;
> }
>
> if (kstrtoint()) {
> pr_warn()
> return false
> }
>
> if (max_snp_asid < 1 || >= min_sev_asid) {
> pr_warn()
> max_snp_asid = /* rollback */
> return false;
> }
>
> min_sev_es_asid = ...
>
> return true;
> }
>
> So, just from an outright flow perspective, I believe my latest diff is objectively easier to follow.
>
All the above are your opinions and i don't agree with them and my patch is bug-free and simple and easy to understand and works perfectly, and i am not making any more changes to it.
Thanks,
Ashish
>> Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.
> With this statement, you self-contradict your rationale to keep your version of the above to Item (2): "isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid calling kstrtoint() if the parameter is not a number". If not willing to take my latest diff as-is, I really would like to see:
>
> Item (1)'s variable get a temporary-sounding name,
> item (2)'s the isdigit() check (and thus a whole indentation level) removed, and
> item (3)'s flow reconsidered given the (IMO objective) readability enhancement.
>
> Thanks,
>
> Kim
>
On Mon, Jul 21, 2025 at 02:12:15PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> Ciphertext hiding prevents host accesses from reading the ciphertext
> of SNP guest private memory. Instead of reading ciphertext, the host
> will see constant default values (0xff).
>
> The SEV ASID space is split into SEV and SEV-ES/SNP ASID ranges.
> Enabling ciphertext hiding further splits the SEV-ES/SEV-SNP ASID space
> into separate ASID ranges for SEV-ES and SEV-SNP guests.
>
> Add new module parameter to the KVM module to enable ciphertext hiding
> support and a user configurable system-wide maximum SNP ASID value. If
> the module parameter value is "max" then the complete SEV-ES/SEV-SNP
> space is allocated to SEV-SNP guests.
>
> v7:
> - Fix comments.
> - Move the check for module parameter ciphertext_hiding_asids inside
> check_and_enable_sev_snp_ciphertext_hiding(), this keeps all the logic
> related to the parameter in a single function.
>
> v6:
> - Fix module parameter ciphertext_hiding_asids=0 case.
> - Coalesce multiple cases of handling invalid module parameter
> ciphertext_hiding_asids into a single branch/label.
> - Fix commit logs.
> - Fix Documentation.
>
> v5:
> - Add pre-patch to cache SEV platform status and use this cached
> information to set api_major/api_minor/build.
> - Since the SEV platform status and SNP platform status differ,
> remove the state field from sev_device structure and instead track
> SEV platform state from cached SEV platform status.
> - If SNP is enabled then cached SNP platform status is used for
> api_major/api_minor/build.
> - Fix using sev_do_cmd() instead of __sev_do_cmd_locked().
> - Fix commit logs.
> - Fix kernel-parameters documentation.
> - Modify KVM module parameter to enable CipherTextHiding to support
> "max" option to allow complete SEV-ES+ ASID space to be allocated
> to SEV-SNP guests.
> - Do not enable ciphertext hiding if module parameter to specify
> maximum SNP ASID is invalid.
>
> v4:
> - Fix buffer allocation for SNP_FEATURE_INFO command to correctly
> handle page boundary check requirements.
> - Return correct length for SNP_FEATURE_INFO command from
> sev_cmd_buffer_len().
> - Switch to using SNP platform status instead of SEV platform status if
> SNP is enabled and cache SNP platform status and feature information.
> Modify sev_get_api_version() accordingly.
> - Fix commit logs.
> - Expand the comments on why both the feature info and the platform
> status fields have to be checked for CipherTextHiding feature
> detection and enablement.
> - Add new preperation patch for CipherTextHiding feature which
> introduces new {min,max}_{sev_es,snp}_asid variables along with
> existing {min,max}_sev_asid variable to simplify partitioning of the
> SEV and SEV-ES+ ASID space.
> - Switch to single KVM module parameter to enable CipherTextHiding
> feature and the maximum SNP ASID usable for SNP guests when
> CipherTextHiding feature is enabled.
>
> v3:
> - rebase to linux-next.
> - rebase on top of support to move SEV-SNP initialization to
> KVM module from CCP driver.
> - Split CipherTextHiding support between CCP driver and KVM module
> with KVM module calling into CCP driver to initialize SNP with
> CipherTextHiding enabled and MAX ASID usable for SNP guest if
> KVM is enabling CipherTextHiding feature.
> - Move module parameters to enable CipherTextHiding feature and
> MAX ASID usable for SNP guests from CCP driver to KVM module
> which allows KVM to be responsible for enabling CipherTextHiding
> feature if end-user requests it.
>
> v2:
> - Fix and add more description to commit logs.
> - Rename sev_cache_snp_platform_status_and_discover_features() to
> snp_get_platform_data().
> - Add check in snp_get_platform_data to guard against being called
> after SNP_INIT_EX.
> - Fix comments for new structure field definitions being added.
> - Fix naming for new structure being added.
> - Add new vm-type parameter to sev_asid_new().
> - Fix identation.
> - Rename CCP module parameters psp_cth_enabled to cipher_text_hiding and
> psp_max_snp_asid to max_snp_asid.
> - Rename max_snp_asid to snp_max_snp_asid.
>
> Ashish Kalra (7):
> crypto: ccp - New bit-field definitions for SNP_PLATFORM_STATUS
> command
> crypto: ccp - Cache SEV platform status and platform state
> crypto: ccp - Add support for SNP_FEATURE_INFO command
> crypto: ccp - Introduce new API interface to indicate SEV-SNP
> Ciphertext hiding feature
> crypto: ccp - Add support to enable CipherTextHiding on SNP_INIT_EX
> KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables
> KVM: SEV: Add SEV-SNP CipherTextHiding support
>
> .../admin-guide/kernel-parameters.txt | 18 +++
> arch/x86/kvm/svm/sev.c | 96 +++++++++++--
> drivers/crypto/ccp/sev-dev.c | 127 ++++++++++++++++--
> drivers/crypto/ccp/sev-dev.h | 6 +-
> include/linux/psp-sev.h | 44 +++++-
> include/uapi/linux/psp-sev.h | 10 +-
> 6 files changed, 274 insertions(+), 27 deletions(-)
>
> --
> 2.34.1
Patches 1-5 applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Sat, Aug 16, 2025, Herbert Xu wrote:
> On Mon, Jul 21, 2025 at 02:12:15PM +0000, Ashish Kalra wrote:
> > Ashish Kalra (7):
> > crypto: ccp - New bit-field definitions for SNP_PLATFORM_STATUS
> > command
> > crypto: ccp - Cache SEV platform status and platform state
> > crypto: ccp - Add support for SNP_FEATURE_INFO command
> > crypto: ccp - Introduce new API interface to indicate SEV-SNP
> > Ciphertext hiding feature
> > crypto: ccp - Add support to enable CipherTextHiding on SNP_INIT_EX
> > KVM: SEV: Introduce new min,max sev_es and sev_snp asid variables
> > KVM: SEV: Add SEV-SNP CipherTextHiding support
> >
> > .../admin-guide/kernel-parameters.txt | 18 +++
> > arch/x86/kvm/svm/sev.c | 96 +++++++++++--
> > drivers/crypto/ccp/sev-dev.c | 127 ++++++++++++++++--
> > drivers/crypto/ccp/sev-dev.h | 6 +-
> > include/linux/psp-sev.h | 44 +++++-
> > include/uapi/linux/psp-sev.h | 10 +-
> > 6 files changed, 274 insertions(+), 27 deletions(-)
> >
> > --
> > 2.34.1
>
> Patches 1-5 applied. Thanks.
Can you provide a tag for commit c9760b0fca6b ("crypto: ccp - Add support to
enable CipherTextHiding on SNP_INIT_EX")? I'd like to apply the KVM side of
things for 6.17, and would prefer not to merge or base the KVM patches on a bare
commit.
Thanks!
© 2016 - 2025 Red Hat, Inc.