From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EEBEA2F21 for ; Thu, 3 Mar 2022 17:33:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1646328828; x=1677864828; h=message-id:date:mime-version:to:cc:references:from: subject:in-reply-to:content-transfer-encoding; bh=/6BWrWtvMDgxG3GRtmu6+t9kH9b6E5eZZRPbC6t9AzU=; b=JYMrLkv7yKJlhiGS77Uea0+2fxGDbjwVhHefPfZTIVtDrzGhl3GQM1nZ e3yefVeALKeERJmIGHCTzIIKll7h5U7Q34YDfs21st4PGmEZrebUyeW50 osOf5Q2i55SULRrp89FSqBFzBaX+pWsmno4s0P2LeoMrrfPoYrgXt1PDi l7+nSh31R0cpjVEpoKBt2kVUQtir8WL7+DtIBRKnSZZMTHG0AtOiXaDZE JIkrydtquFJedlU/nDQHXGsK8NU5rLpaNmgAQQdgwG7J6Y+4j6n1v/uR6 hfHlVT0LcBGbvv2L6lBISbg3qAlJ8jbf9Dog3IZZ1Jtzf4qMhXOkPsGMF A==; X-IronPort-AV: E=McAfee;i="6200,9189,10275"; a="278429410" X-IronPort-AV: E=Sophos;i="5.90,151,1643702400"; d="scan'208";a="278429410" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Mar 2022 09:33:48 -0800 X-IronPort-AV: E=Sophos;i="5.90,151,1643702400"; d="scan'208";a="642197252" Received: from eabada-mobl2.amr.corp.intel.com (HELO [10.209.6.252]) ([10.209.6.252]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Mar 2022 09:33:45 -0800 Message-ID: Date: Thu, 3 Mar 2022 09:33:40 -0800 Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Content-Language: en-US To: Brijesh Singh , x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev, linux-mm@kvack.org Cc: Thomas Gleixner , Ingo Molnar , Joerg Roedel , Tom Lendacky , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Gonda , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , "Kirill A . Shutemov" , Andi Kleen , "Dr . David Alan Gilbert" , brijesh.ksingh@gmail.com, tony.luck@intel.com, marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com References: <20220224165625.2175020-1-brijesh.singh@amd.com> <20220224165625.2175020-43-brijesh.singh@amd.com> From: Dave Hansen Subject: Re: [PATCH v11 42/45] virt: Add SEV-SNP guest driver In-Reply-To: <20220224165625.2175020-43-brijesh.singh@amd.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit > +++ b/drivers/virt/coco/sevguest/Kconfig > @@ -0,0 +1,12 @@ > +config SEV_GUEST > + tristate "AMD SEV Guest driver" > + default m > + depends on AMD_MEM_ENCRYPT && CRYPTO_AEAD2 > + help > + SEV-SNP firmware provides the guest a mechanism to communicate with > + the PSP without risk from a malicious hypervisor who wishes to read, > + alter, drop or replay the messages sent. The driver provides > + userspace interface to communicate with the PSP to request the > + attestation report and more. > + > + If you choose 'M' here, this module will be called sevguest. ... > +/* Return a non-zero on success */ > +static u64 snp_get_msg_seqno(struct snp_guest_dev *snp_dev) > +{ > + u64 count = __snp_get_msg_seqno(snp_dev); > + > + /* > + * The message sequence counter for the SNP guest request is a 64-bit > + * value but the version 2 of GHCB specification defines a 32-bit storage > + * for it. If the counter exceeds the 32-bit value then return zero. > + * The caller should check the return value, but if the caller happens to > + * not check the value and use it, then the firmware treats zero as an > + * invalid number and will fail the message request. > + */ > + if (count >= UINT_MAX) { > + pr_err_ratelimited("request message sequence counter overflow\n"); > + return 0; > + } > + > + return count; > +} I didn't see a pr_fmt defined anywhere. But, for a "driver", should this be a dev_err()? ... > +static void free_shared_pages(void *buf, size_t sz) > +{ > + unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT; > + > + if (!buf) > + return; > + > + if (WARN_ONCE(set_memory_encrypted((unsigned long)buf, npages), > + "failed to restore encryption mask (leak it)\n")) > + return; > + > + __free_pages(virt_to_page(buf), get_order(sz)); > +} Nit: It's a bad practice to do important things inside a WARN_ON() _or_ and if(). This should be: int ret; ... ret = set_memory_encrypted((unsigned long)buf, npages)); if (ret) { WARN_ONCE(...); return; } BTW, this look like a generic allocator thingy. But it's only ever used to allocate a 'struct snp_guest_msg'. Why all the trouble to allocate and free one fixed-size structure? The changelog and comments don't shed any light.