From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: pvclock: Map shared on SEV
To: Stefan Fritsch <sf@openbsd.org>
Cc: tech@openbsd.org, hshoexer@genua.de, bluhm@openbsd.org
Date: Mon, 31 Mar 2025 16:47:25 +0200

> Date: Mon, 31 Mar 2025 15:13:27 +0200 (CEST)
> From: Stefan Fritsch <sf@openbsd.org>
> 
> Hi,
> 
> if SEV is enabled, we need to map the pvclock page as unencrypted / shared 
> with the hypervisor. Otherwise, the pvclock_attach() code may hang during 
> boot due to garbage in the page.
> 
> Found and fix tested by bluhm@
> 
> ok?
> 
> Cheers,
> Stefan
> 
> 
> diff --git a/sys/dev/pv/pvclock.c b/sys/dev/pv/pvclock.c
> index 994fc4a337c..f04d0e87c6b 100644
> --- a/sys/dev/pv/pvclock.c
> +++ b/sys/dev/pv/pvclock.c
> @@ -31,6 +31,10 @@
>  #include <dev/pv/pvvar.h>
>  #include <dev/pv/pvreg.h>
>  
> +#ifndef PMAP_NOCRYPT
> +#define	PMAP_NOCRYPT	0
> +#endif
> +
>  uint pvclock_lastcount;
>  
>  struct pvclock_softc {
> @@ -123,17 +127,19 @@ pvclock_attach(struct device *parent, struct device *self, void *aux)
>  	paddr_t			 	 pa;
>  	uint32_t			 version;
>  	uint8_t				 flags;
> +	struct vm_page			*page;
>  
> -	if ((sc->sc_time = km_alloc(PAGE_SIZE,
> -	    &kv_any, &kp_zero, &kd_nowait)) == NULL) {
> -		printf(": time page allocation failed\n");
> -		return;
> -	}
> -	if (!pmap_extract(pmap_kernel(), (vaddr_t)sc->sc_time, &pa)) {
> -		printf(": time page PA extraction failed\n");
> -		km_free(sc->sc_time, PAGE_SIZE, &kv_any, &kp_zero);
> -		return;
> -	}
> +	page = uvm_pagealloc(NULL, 0, NULL, 0);
> +	if (page == NULL)
> +		goto err;
> +	sc->sc_time = km_alloc(PAGE_SIZE, &kv_any, &kp_none, &kd_nowait);
> +	if (sc->sc_time == NULL)
> +		goto err;
> +
> +	pa = page->phys_addr;

You should use VM_PAGE_TO_PHYS here.

> +	pmap_kenter_pa((vaddr_t)sc->sc_time, pa | PMAP_NOCRYPT,
> +		PROT_READ | PROT_WRITE);
> +	memset(sc->sc_time, 0, PAGE_SIZE);

Is that because using UVM_PGA_ZERO in uvm_pagealloc(9) would allocate
a page that contain encrypted zeroes and therefore still contains
garbage when viewed from the hypervisor?

>  	wrmsr(KVM_MSR_SYSTEM_TIME, pa | PVCLOCK_SYSTEM_TIME_ENABLE);
>  	sc->sc_paddr = pa;
> @@ -163,6 +169,11 @@ pvclock_attach(struct device *parent, struct device *self, void *aux)
>  	tc_init(sc->sc_tc);
>  
>  	printf("\n");
> +	return;
> +err:
> +	if (page)
> +		uvm_pagefree(page);
> +	printf(": time page allocation failed\n");
>  }
>  
>  int
> 
>