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

On Mon, 31 Mar 2025, Mark Kettenis wrote:
> > 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.

Thanks, fix committed (the original diff was already committed).

> 
> > +	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?

Yes, exactly. We may want a better API for this, but that can wait until 
SEV-ES/SEV-SNP support is committed.


> >  	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
> > 
> > 
>