kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Nikunj A Dadhania <nikunj@amd.com>
Cc: linux-kernel@vger.kernel.org, thomas.lendacky@amd.com,
	x86@kernel.org, kvm@vger.kernel.org, mingo@redhat.com,
	tglx@linutronix.de, dave.hansen@linux.intel.com,
	dionnaglaze@google.com, pgonda@google.com, seanjc@google.com,
	pbonzini@redhat.com
Subject: Re: [PATCH v7 03/16] virt: sev-guest: Add SNP guest request structure
Date: Thu, 25 Jan 2024 12:59:52 +0100	[thread overview]
Message-ID: <20240125115952.GXZbJNOGfxfuiC5WRT@fat_crate.local> (raw)
In-Reply-To: <20231220151358.2147066-4-nikunj@amd.com>

On Wed, Dec 20, 2023 at 08:43:45PM +0530, Nikunj A Dadhania wrote:
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio)
> +int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input,
> +			    struct snp_guest_request_ioctl *rio)
>  {
>  	struct ghcb_state state;
>  	struct es_em_ctxt ctxt;
>  	unsigned long flags;
>  	struct ghcb *ghcb;
> +	u64 exit_code;

Silly local vars. Just use req->exit_code everywhere instead.

>  	int ret;
>  
>  	rio->exitinfo2 = SEV_RET_NO_FW_CALL;
> +	if (!req)
> +		return -EINVAL;

Such tests are done under the variable which is assigned, not randomly.

Also, what's the point in testing req? Will that ever be NULL? What are
you actually protecting against here?

> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 469e10d9bf35..5cafbd1c42cb 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -27,8 +27,7 @@
>  
>  #include <asm/svm.h>
>  #include <asm/sev.h>
> -
> -#include "sev-guest.h"
> +#include <asm/sev-guest.h>
>  
>  #define DEVICE_NAME	"sev-guest"
>  
> @@ -169,7 +168,7 @@ static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
>  	return ctx;
>  }
>  
> -static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, void *payload, u32 sz)
> +static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_req *guest_req)

So we call the request everywhere "req". But you've called it
"guest_req" here because...

>  {
>  	struct snp_guest_msg *resp = &snp_dev->secret_response;
>  	struct snp_guest_msg *req = &snp_dev->secret_request;

... there already is a "req" variable which is not a guest request thing
but a guest message. So why don't you call it "req_msg" instead and the
"resp" "resp_msg" so that it is clear what is what?

And then you can call the actual request var "req" and then the code
becomes more readable...

...

>  static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
>  {
>  	struct snp_report_req *req = &snp_dev->req.report;
> +	struct snp_guest_req guest_req = {0};

You have the same issue here.

If we aim at calling the local vars in every function the same, the code
becomes automatically much more readable.

And so on...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

  reply	other threads:[~2024-01-25 12:00 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-20 15:13 [PATCH v7 00/16] Add Secure TSC support for SNP guests Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 01/16] virt: sev-guest: Use AES GCM crypto library Nikunj A Dadhania
2024-01-25 10:36   ` Borislav Petkov
2024-01-27  3:54     ` Nikunj A. Dadhania
2023-12-20 15:13 ` [PATCH v7 02/16] virt: sev-guest: Replace dev_dbg with pr_debug Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 03/16] virt: sev-guest: Add SNP guest request structure Nikunj A Dadhania
2024-01-25 11:59   ` Borislav Petkov [this message]
2024-01-27  4:01     ` Nikunj A. Dadhania
2024-01-31 13:58     ` Nikunj A. Dadhania
2024-02-01 10:29       ` Borislav Petkov
2024-02-01 11:10         ` Nikunj A. Dadhania
2024-02-01 14:07           ` Borislav Petkov
2024-02-02  3:50             ` Nikunj A. Dadhania
2024-02-02 16:14               ` Borislav Petkov
2024-02-05  9:23                 ` Nikunj A. Dadhania
2024-02-06 10:04                   ` Borislav Petkov
2024-01-26 21:16   ` Tom Lendacky
2024-01-27  4:05     ` Nikunj A. Dadhania
2023-12-20 15:13 ` [PATCH v7 04/16] virt: sev-guest: Add vmpck_id to snp_guest_dev struct Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 05/16] x86/sev: Cache the secrets page address Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 06/16] virt: sev-guest: Move SNP Guest command mutex Nikunj A Dadhania
2024-01-26 22:11   ` Tom Lendacky
2024-01-27  4:06     ` Nikunj A. Dadhania
2023-12-20 15:13 ` [PATCH v7 07/16] x86/sev: Move and reorganize sev guest request api Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 08/16] x86/mm: Add generic guest initialization hook Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 09/16] x86/cpufeatures: Add synthetic Secure TSC bit Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 10/16] x86/sev: Add Secure TSC support for SNP guests Nikunj A Dadhania
2024-02-01 15:46   ` Paolo Bonzini
2024-02-01 15:48     ` Paolo Bonzini
2023-12-20 15:13 ` [PATCH v7 11/16] x86/sev: Change TSC MSR behavior for Secure TSC enabled guests Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 12/16] x86/sev: Prevent RDTSC/RDTSCP interception " Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 13/16] x86/kvmclock: Skip kvmclock when Secure TSC is available Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 14/16] x86/sev: Mark Secure TSC as reliable Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 15/16] x86/cpu/amd: Do not print FW_BUG for Secure TSC Nikunj A Dadhania
2023-12-20 15:13 ` [PATCH v7 16/16] x86/sev: Enable Secure TSC for SNP guests Nikunj A Dadhania
2024-01-25  6:08 ` [PATCH v7 00/16] Add Secure TSC support " Nikunj A. Dadhania
2024-01-26  1:00   ` Dionna Amalie Glaze
2024-01-27  4:10     ` Nikunj A. Dadhania

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240125115952.GXZbJNOGfxfuiC5WRT@fat_crate.local \
    --to=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=dionnaglaze@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=nikunj@amd.com \
    --cc=pbonzini@redhat.com \
    --cc=pgonda@google.com \
    --cc=seanjc@google.com \
    --cc=tglx@linutronix.de \
    --cc=thomas.lendacky@amd.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).