Messages in this thread | | | Date | Wed, 8 May 2024 15:09:02 -0500 | From | Tom Lendacky <> | Subject | Re: [PATCH v4 05/15] x86/sev: Use kernel provided SVSM Calling Areas |
| |
On 5/8/24 14:58, Borislav Petkov wrote: > On Wed, May 08, 2024 at 02:13:17PM -0500, Tom Lendacky wrote: >> ok, maybe __perform_svsm_msr_protocol or such. > > We'll bikeshed it in the coming weeks.
:)
> >> There's quite a bit of interaction so I'll make sure to prefix everything. > > Ack. > >> The paravirt versions of local_irq_save and local_irq_restore can't be used >> as early as this routine is called. > > tglx says you should do native_local_irq_save()/.._restore() helpers > just like the arch_local_irq_save()/..._restore() ones but use only > native_ functions without the paravirt gunk. > > In a prepatch pls.
Will do.
> >>>> + struct svsm_call call = {}; >>>> + int ret; >>>> + u64 pa; >>>> + >>>> + /* >>>> + * Record the SVSM Calling Area address (CAA) if the guest is not >>>> + * running at VMPL0. The CA will be used to communicate with the >>>> + * SVSM to perform the SVSM services. >>>> + */ >>>> + setup_svsm_ca(cc_info); >>>> + >>>> + /* Nothing to do if not running under an SVSM. */ >>>> + if (!vmpl) >>>> + return; >>> >>> You set up stuff above and now you bail out? >> >> setup_svsm_ca() is what sets the vmpl variable. So nothing will have been >> setup if the VMPL is zero, in which case we don't continue on. > > You still assign > > /* > * The CA is identity mapped when this routine is called, both by the > * decompressor code and the early kernel code. > */ > boot_svsm_caa = (struct svsm_ca *)caa; > boot_svsm_caa_pa = caa; > > regardless of vmpl.
If we're not running at VMPL0 (based on the RMPADJUST check) and if the SVSM doesn't advertise a non-zero VMPL value, we will self-terminate. So those values are only set if we are not running at VMPL0 and the SVSM has provided a non-zero value to us.
I'm going to turn the function into a bool function so that the call becomes:
if (!svsm_setup_caa(cc_info)) return;
> > I think you should assign those only when vmpl != 0.
I do. I think you're missing the RMPADJUST check that causes the function to return early if we're running at VMPL0.
> > Otherwise the code is confusing. > >>> Judging by setup_svsm_ca() you don't really need that vmpl var but you >>> can check >>> >>> if (!boot_svsm_caa) >>> return; >>> >>> to determine whether a SVSM was detected. >> >> Yes, but the vmpl var will be used for attestation requests, sysfs, etc. > > I guess that comes later in the patchset... > >>> Huh, setup_svsm_ca() already assigned those... >> >> setup_svsm_ca() assigned the ones from the secrets page. The kernel now >> switches to using its own CA. > > Comment pls.
There's a block comment above it all, but maybe it isn't clear enough. I'll rework it.
Thanks, Tom
> > Thx. >
| |