KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	kvm@vger.kernel.org, ak@linux.intel.com,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Tony Luck <tony.luck@intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	David Rientjes <rientjes@google.com>,
	Sean Christopherson <seanjc@google.com>
Subject: Re: [RFC Part1 PATCH 09/13] x86/kernel: add support to validate memory in early enc attribute change
Date: Thu, 8 Apr 2021 13:40:49 +0200
Message-ID: <20210408114049.GI10192@zn.tnic> (raw)
In-Reply-To: <20210324164424.28124-10-brijesh.singh@amd.com>

On Wed, Mar 24, 2021 at 11:44:20AM -0500, Brijesh Singh wrote:
> @@ -63,6 +63,10 @@ struct __packed snp_page_state_change {
>  #define GHCB_REGISTER_GPA_RESP	0x013UL
>  #define		GHCB_REGISTER_GPA_RESP_VAL(val)		((val) >> 12)
>  
> +/* Macro to convert the x86 page level to the RMP level and vice versa */
> +#define X86_RMP_PG_LEVEL(level)	(((level) == PG_LEVEL_4K) ? RMP_PG_SIZE_4K : RMP_PG_SIZE_2M)
> +#define RMP_X86_PG_LEVEL(level)	(((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M)

Please add those with the patch which uses them for the first time.

Also, it seems to me the names should be

X86_TO_RMP_PG_LEVEL
RMP_TO_X86_PG_LEVEL

...

> @@ -56,3 +56,108 @@ void sev_snp_register_ghcb(unsigned long paddr)
>  	/* Restore the GHCB MSR value */
>  	sev_es_wr_ghcb_msr(old);
>  }
> +
> +static void sev_snp_issue_pvalidate(unsigned long vaddr, unsigned int npages, bool validate)

pvalidate_pages() I guess.

> +{
> +	unsigned long eflags, vaddr_end, vaddr_next;
> +	int rc;
> +
> +	vaddr = vaddr & PAGE_MASK;
> +	vaddr_end = vaddr + (npages << PAGE_SHIFT);
> +
> +	for (; vaddr < vaddr_end; vaddr = vaddr_next) {

Yuck, that vaddr_next gets initialized at the end of the loop. How about
using a while loop here instead?

	while (vaddr < vaddr_end) {

		...

		vaddr += PAGE_SIZE;
	}

then you don't need vaddr_next at all. Ditto for all the other loops in
this patch which iterate over pages.

> +		rc = __pvalidate(vaddr, RMP_PG_SIZE_4K, validate, &eflags);

So this function gets only 4K pages to pvalidate?

> +

^ Superfluous newline.

> +		if (rc) {
> +			pr_err("Failed to validate address 0x%lx ret %d\n", vaddr, rc);

You can combine the pr_err and dump_stack() below into a WARN() here:

		WARN(rc, ...);

> +			goto e_fail;
> +		}
> +
> +		/* Check for the double validation condition */
> +		if (eflags & X86_EFLAGS_CF) {
> +			pr_err("Double %salidation detected (address 0x%lx)\n",
> +					validate ? "v" : "inv", vaddr);
> +			goto e_fail;
> +		}

As before - this should be communicated by a special retval from
__pvalidate().

> +
> +		vaddr_next = vaddr + PAGE_SIZE;
> +	}
> +
> +	return;
> +
> +e_fail:
> +	/* Dump stack for the debugging purpose */
> +	dump_stack();
> +
> +	/* Ask to terminate the guest */
> +	sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);

Another termination reason to #define.

> +}
> +
> +static void __init early_snp_set_page_state(unsigned long paddr, unsigned int npages, int op)
> +{
> +	unsigned long paddr_end, paddr_next;
> +	u64 old, val;
> +
> +	paddr = paddr & PAGE_MASK;
> +	paddr_end = paddr + (npages << PAGE_SHIFT);
> +
> +	/* save the old GHCB MSR */
> +	old = sev_es_rd_ghcb_msr();
> +
> +	for (; paddr < paddr_end; paddr = paddr_next) {
> +
> +		/*
> +		 * Use the MSR protocol VMGEXIT to request the page state change. We use the MSR
> +		 * protocol VMGEXIT because in early boot we may not have the full GHCB setup
> +		 * yet.
> +		 */
> +		sev_es_wr_ghcb_msr(GHCB_SNP_PAGE_STATE_REQ_GFN(paddr >> PAGE_SHIFT, op));
> +		VMGEXIT();

Yeah, I know we don't always strictly adhere to 80 columns but there's
no real need not to fit that in 80 cols here so please shorten names and
comments. Ditto for the rest.

> +
> +		val = sev_es_rd_ghcb_msr();
> +
> +		/* Read the response, if the page state change failed then terminate the guest. */
> +		if (GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PAGE_STATE_CHANGE_RESP)
> +			sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);

if (...)
	goto fail;

and add that fail label at the end so that all the error handling path
is out of the way.

> +
> +		if (GHCB_SNP_PAGE_STATE_RESP_VAL(val) != 0) {

s/!= 0//

> +			pr_err("Failed to change page state to '%s' paddr 0x%lx error 0x%llx\n",
> +					op == SNP_PAGE_STATE_PRIVATE ? "private" : "shared",
> +					paddr, GHCB_SNP_PAGE_STATE_RESP_VAL(val));
> +
> +			/* Dump stack for the debugging purpose */
> +			dump_stack();

WARN as above.

> @@ -49,6 +50,27 @@ bool sev_enabled __section(".data");
>  /* Buffer used for early in-place encryption by BSP, no locking needed */
>  static char sme_early_buffer[PAGE_SIZE] __initdata __aligned(PAGE_SIZE);
>  
> +/*
> + * When SNP is active, this routine changes the page state from private to shared before
> + * copying the data from the source to destination and restore after the copy. This is required
> + * because the source address is mapped as decrypted by the caller of the routine.
> + */
> +static inline void __init snp_aware_memcpy(void *dst, void *src, size_t sz,
> +					   unsigned long paddr, bool dec)

snp_memcpy() simply.

> +{
> +	unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +
> +	/* If the paddr need to accessed decrypted, make the page shared before memcpy. */

*needs*

> +	if (sev_snp_active() && dec)

Flip that test so that you don't have it twice in the code:

	if (!sev_snp_active()) {
		memcpy(dst, src, sz);
	} else {
		...
	}


-- 
Regards/Gruss,
    Boris.

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

  reply index

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 16:44 [RFC Part1 PATCH 00/13] Add AMD Secure Nested Paging (SEV-SNP) Guest Support Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 01/13] x86/cpufeatures: Add SEV-SNP CPU feature Brijesh Singh
2021-03-25 10:54   ` Borislav Petkov
2021-03-25 14:50     ` Brijesh Singh
2021-03-25 16:29       ` Borislav Petkov
2021-03-24 16:44 ` [RFC Part1 PATCH 02/13] x86/mm: add sev_snp_active() helper Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 03/13] x86: add a helper routine for the PVALIDATE instruction Brijesh Singh
2021-03-26 14:30   ` Borislav Petkov
2021-03-26 15:42     ` Brijesh Singh
2021-03-26 18:22       ` Brijesh Singh
2021-03-26 19:12         ` Borislav Petkov
2021-03-26 20:04           ` Brijesh Singh
2021-03-26 19:22       ` Borislav Petkov
2021-03-26 20:01         ` Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 04/13] x86/sev-snp: define page state change VMGEXIT structure Brijesh Singh
2021-04-01 10:32   ` Borislav Petkov
2021-04-01 14:11     ` Brijesh Singh
2021-04-02 15:44       ` Borislav Petkov
2021-03-24 16:44 ` [RFC Part1 PATCH 05/13] X86/sev-es: move few helper functions in common file Brijesh Singh
2021-04-02 19:27   ` Borislav Petkov
2021-04-02 21:33     ` Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 06/13] x86/compressed: rescinds and validate the memory used for the GHCB Brijesh Singh
2021-04-06 10:33   ` Borislav Petkov
2021-04-06 15:47     ` Brijesh Singh
2021-04-06 19:42       ` Tom Lendacky
2021-04-07 11:25         ` Borislav Petkov
2021-04-07 19:45           ` Borislav Petkov
2021-04-08 13:57             ` Tom Lendacky
2021-04-07 11:16       ` Borislav Petkov
2021-04-07 13:35         ` Brijesh Singh
2021-04-07 14:21           ` Tom Lendacky
2021-04-07 17:15             ` Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 07/13] x86/compressed: register GHCB memory when SNP is active Brijesh Singh
2021-04-07 11:59   ` Borislav Petkov
2021-04-07 17:34     ` Brijesh Singh
2021-04-07 17:54       ` Tom Lendacky
2021-04-08  8:17       ` Borislav Petkov
2021-03-24 16:44 ` [RFC Part1 PATCH 08/13] x86/sev-es: register GHCB memory when SEV-SNP " Brijesh Singh
2021-04-08  8:38   ` Borislav Petkov
2021-03-24 16:44 ` [RFC Part1 PATCH 09/13] x86/kernel: add support to validate memory in early enc attribute change Brijesh Singh
2021-04-08 11:40   ` Borislav Petkov [this message]
2021-04-08 12:25     ` Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 10/13] X86: kernel: make the bss.decrypted section shared in RMP table Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 11/13] x86/kernel: validate rom memory before accessing when SEV-SNP is active Brijesh Singh
2021-04-09 16:53   ` Borislav Petkov
2021-04-09 17:40     ` Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 12/13] x86/sev-es: make GHCB get and put helper accessible outside Brijesh Singh
2021-03-24 16:44 ` [RFC Part1 PATCH 13/13] x86/kernel: add support to validate memory when changing C-bit Brijesh Singh
2021-04-12 11:49   ` Borislav Petkov
2021-04-12 12:55     ` Brijesh Singh
2021-04-12 13:05       ` Borislav Petkov
2021-04-12 14:31         ` Brijesh Singh

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=20210408114049.GI10192@zn.tnic \
    --to=bp@alien8.de \
    --cc=ak@linux.intel.com \
    --cc=brijesh.singh@amd.com \
    --cc=dave.hansen@intel.com \
    --cc=hpa@zytor.com \
    --cc=jroedel@suse.de \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=tglx@linutronix.de \
    --cc=thomas.lendacky@amd.com \
    --cc=tony.luck@intel.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

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git