Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Sep 2022 15:03:53 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Ashish Kalra <Ashish.Kalra@amd.com>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com,
        ardb@kernel.org, pbonzini@redhat.com, seanjc@google.com,
        vkuznets@redhat.com, jmattson@google.com, luto@kernel.org,
        dave.hansen@linux.intel.com, slp@redhat.com, pgonda@google.com,
        peterz@infradead.org, srinivas.pandruvada@linux.intel.com,
        rientjes@google.com, dovmurik@linux.ibm.com, tobin@ibm.com,
        michael.roth@amd.com, vbabka@suse.cz, kirill@shutemov.name,
        ak@linux.intel.com, tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, jarkko@kernel.org
Subject: Re: [PATCH Part2 v6 11/49] crypto:ccp: Define the SEV-SNP commands
Message-ID: <Yym6Ob2tPYeb0Kq1@zn.tnic>
References: <cover.1655761627.git.ashish.kalra@amd.com>
 <f5dec307d246096768afd770d16a26be25fa28b3.1655761627.git.ashish.kalra@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <f5dec307d246096768afd770d16a26be25fa28b3.1655761627.git.ashish.kalra@amd.com>
Precedence: bulk

On Mon, Jun 20, 2022 at 11:04:14PM +0000, Ashish Kalra wrote:
> +/**
> + * struct sev_data_snp_platform_status_buf - SNP_PLATFORM_STATUS command params
> + *
> + * @address: physical address where the status should be copied
> + */
> +struct sev_data_snp_platform_status_buf {
> +	u64 status_paddr;			/* In */
> +} __packed;
> +
> +/**
> + * struct sev_data_snp_download_firmware - SNP_DOWNLOAD_FIRMWARE command params
> + *
> + * @address: physical address of firmware image
> + * @len: len of the firmware image
> + */
> +struct sev_data_snp_download_firmware {
> +	u64 address;				/* In */
> +	u32 len;				/* In */
> +} __packed;
> +
> +/**
> + * struct sev_data_snp_gctx_create - SNP_GCTX_CREATE command params
> + *
> + * @gctx_paddr: system physical address of the page donated to firmware by
> + *		the hypervisor to contain the guest context.
> + */
> +struct sev_data_snp_gctx_create {
> +	u64 gctx_paddr;				/* In */
> +} __packed;

So some of those structs have the same layout. Let's unify them pls.
I.e., for

sev_data_send_finish, sev_data_send_cancel, sev_data_receive_finish

you do

struct sev_data_tx {
	u32 handle;				/* In */
} __packed;

For sev_data_snp_platform_status_buf, sev_data_snp_gctx_create,
sev_data_snp_decommission and all those others who are a single u64, you
use a single

struct sev_data_addr {
	u64 gctx_paddr;				/* In */
} __packed;

so that we don't have gazillion structs all of different names but a lot
of them identical in content.


...

> +/**
> + * struct sev_data_snp_launch_finish - SNP_LAUNCH_FINISH command params
> + *
> + * @gctx_addr: system pphysical address of guest context page
			 ^^^^^^^^^

physical

> + */
> +struct sev_data_snp_launch_finish {
> +	u64 gctx_paddr;
> +	u64 id_block_paddr;
> +	u64 id_auth_paddr;
> +	u8 id_block_en:1;
> +	u8 auth_key_en:1;
> +	u64 rsvd:62;
> +	u8 host_data[32];
> +} __packed;
> +
> +/**
> + * struct sev_data_snp_guest_status - SNP_GUEST_STATUS command params
> + *
> + * @gctx_paddr: system physical address of guest context page
> + * @address: system physical address of guest status page
> + */
> +struct sev_data_snp_guest_status {
> +	u64 gctx_paddr;
> +	u64 address;
> +} __packed;
> +
> +/**
> + * struct sev_data_snp_page_reclaim - SNP_PAGE_RECLAIM command params
> + *
> + * @paddr: system physical address of page to be claimed. The BIT0 indicate
> + *	the page size. 0h indicates 4 kB and 1h indicates 2 MB page.
> + */
> +struct sev_data_snp_page_reclaim {
> +	u64 paddr;
> +} __packed;
> +
> +/**
> + * struct sev_data_snp_page_unsmash - SNP_PAGE_UNMASH command params
> + *
> + * @paddr: system physical address of page to be unmashed. The BIT0 indicate

Is "BIT0" the 0th bit in the address? This needs to be spelled out
explicitly.

also, s/unmash/unsmash/gi

Also, FW SPEC says bits 11:0 are MBZ. So I'm guessing bit 0 is being
cleared before sending it to sw. I guess I'll see that later.

Thx.

-- 
Regards/Gruss,
    Boris.

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