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 19:33:37 +0200

> Date: Mon, 31 Mar 2025 17:43:46 +0200 (CEST)
> From: Stefan Fritsch <sf@openbsd.org>
> 
> 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.

It might be worth considering adding kp_nocrypt and/or kp_nocrypt_zero
modes to km_alloc(9) if this pattern of allocating unecnrypted memory
occurs more often.

I briefly considered suggesting replacing that memset(9) call with
uvm_pagezero().  But the underlying pmap_zero_page() uses the direct
map and therefore would use encryption, so that isn't a good idea ;).

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