On Mon, Nov 04, 2024, Pratik R. Sampat wrote: > > > On 10/31/2024 11:27 AM, Sean Christopherson wrote: > > On Thu, Oct 31, 2024, Pratik R. Sampat wrote: > >> Hi Sean, > >> > >> On 10/30/2024 12:57 PM, Sean Christopherson wrote: > >>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > >>>> On 10/30/2024 8:46 AM, Sean Christopherson wrote: > >>>>> +/* Minimum firmware version required for the SEV-SNP support */ > >>>>> +#define SNP_FW_REQ_VER_MAJOR 1 > >>>>> +#define SNP_FW_REQ_VER_MINOR 51 > >>>>> > >>>>> Side topic, why are these hardcoded? And where did they come from? If they're > >>>>> arbitrary KVM selftests values, make that super duper clear. > >>>> > >>>> Well, it's not entirely arbitrary. This was the version that SNP GA'd > >>>> with first so that kind of became the minimum required version needed. > >>>> > >>>> I think the only place we've documented this is here - > >>>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > >>>> > >>>> Maybe, I can modify the comment above to say something like - > >>>> Minimum general availability release firmware required for SEV-SNP support. > >>> > >>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on > >>> earth is that not checked and enforced by the kernel? Relying on userspace to > >>> not crash the host (or worse) because of unsupported firmware is not a winning > >>> strategy. > >> > >> We do check against the firmware level 1.51 while setting things up > >> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail > >> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any > >> other corresponding SNP calls should fail cleanly without any adverse > >> effects to the host. > > > > And I'm saying, that's not good enough. If the platform doesn't support SNP, > > the KVM *must not* advertise support for SNP. > > > > Sure, fair to expect this. Currently, if the FW check fails, SNP is not > setup and there is nothing that indicates in the KVM capabilities (apart > from one dmesg error) that the support does not exist. > > One thing I could do (as an independent patch) is to introduce a CC API > that abstracts the FW version check made by the CCP module. Since sev > platform status can be gotten before INIT to extract the major and minor > version numbers, KVM can also call into this API and use that to decide > if the KVM capabilities for SNP must be set or not. Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support SNP? KVM shouldn't have to care about some arbitrary firmware API version, the whole point of a driver is so that KVM doesn't have to deal with such details. I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM itself shouldn't need to manually check the firmware version.
