Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

[Kim Phillips]

On 8/12/25 1:29 PM, Kalra, Ashish wrote:
> On 8/12/2025 11:45 AM, Kim Phillips wrote:
> > On 8/12/25 9:40 AM, Kalra, Ashish wrote:
> > > On 8/12/2025 7:06 AM, Kim Phillips wrote:
> > > >    arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
> > > >    1 file changed, 18 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > index 7ac0f0f25e68..57c6e4717e51 100644
> > > > --- a/arch/x86/kvm/svm/sev.c
> > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > @@ -2970,42 +2970,29 @@ static bool is_sev_snp_initialized(void)
> > > >
> > > >    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()) {
> > > > +       if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
> > > >                   pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
> > > >                   return false;
> > > >           }
> > > This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
> > > get a warning of an invalid ciphertext_hiding_asids module parameter.
> > >
> > > When this module parameter is optional why should the user get a warning about an invalid module parameter.
> > Ack, sorry, new diff below that fixes this.
> >
> > > Again, why do we want to do all these checks below if this module parameter has not been specified by
> > > the user ?
> > Not sure what you mean by 'below' here (assuming in the resulting code), but, in general, there are less checks with this diff than the original v7 code.
> >
> > > > -       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);
> > > A *combined* error message such as this:
> > > "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
> > >
> > > is going to be really confusing to the user.
> > >
> > > It is much simpler for user to understand if the error/warning is:
> > > "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
> > > OR
> > > "Module parameter ciphertext_hiding_asids XXX invalid"
> > I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100, they see:
> >
> >       kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 < minimum SEV ASID 100)
> >
> > which the user can easily unmistakably and quickly deduce that the problem is the latter - not the former - condition that has the problem.
> >
> > The original v7 code in that same case would emit:
> >
> > kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or equals minimum SEV ASID (100)
> >
> > ...to which the user would ask themselves "What's wrong with equalling the minimum SEV ASID (100)"?
> I disagree, the documentation mentions clearly that:
> For SEV-ES/SEV-SNP guests the maximum ASID available is MIN_SEV_ASID - 1.
>
> Which the above message conveys quite clearly.

The point of clear error messaging is to avoid the user having to 
(re-)read the documentation.

> > It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
> > OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
> >
> >       kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
> >
> > which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!

Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.

> And how can user input of 0x1, result in max_snp_asid == 99 ?

It doesn't, again, the 0x is the invalid part.

> This is the issue with combining the checks and emitting a combined error message:
>
> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
> !(0 < 99 < minimum SEV ASID 100)

It's not, it says it's *OR* that condition.

> The original message is much simpler to understand and correct too:
> Module parameter ciphertext_hiding_asids (-1) invalid

Which is wildly different from any possible derivation of 0x1.

> > But all this is nothing compared to the added simplicity resulting from making the change to the original v7 code.
> I disagree, combining checks and emitting a combined error message is going to be more confusing to the user as the above case of (ciphertext_hiding_asids=0x1) shows.

I don't, but nevertheless, it can still be differentiated and still be 
cleaner code than the original v7...

> Thanks,
> Ashish

Thanks,

Kim