From: Mike Larkin Subject: Re: SEV-ES guest: Fix GHCB trap frame synchronisation for 32-bit values To: tech@openbsd.org Date: Fri, 11 Jul 2025 09:31:49 -0700 On Tue, Jul 08, 2025 at 11:45:14AM +0200, Hans-Jörg Höxer wrote: > Hi, > > In 64-bit mode, when performing 32-bit operations with a GPR destination, > the 32-bit value gets zero extended to the full 64-bit destination size. > This means, unlike 8-bit or 16-bit values, the upper 32 bits of the > destination register are not retained for 32-bit values. > > Therefore, when syncing values back to the stack frame, use a "all 1" > mask for 32-bit values. > > Thus duplicate mask array into two, one for syncing out from trap frame > into GHCB and one for syncing back in from GHCB to the trap frame. In the > latter adjust mask for 32-bit values. > > This was pointed out by Sebastian Sturm (ssturm@genua.de). > > Take care, > Hans-Joerg > ok mlarkin > --------------------- > commit 7176ba3ed665f7e9ff2e12289331108bc5995f26 > Author: Hans-Joerg Hoexer > Date: Mon Jul 7 13:54:22 2025 +0200 > > SEV-ES guest: Fix GHCB trap frame synchronisation for 32-bit values > > In 64-bit mode, when performing 32-bit operations with a GPR > destination, the 32-bit value gets zero extended to the full 64-bit > destination size. This means, unlike 8-bit or 16-bit values, the > upper 32 bits of the destination register are not retained for > 32-bit values. > > Therefore, when syncing values back to the stack frame, use a "all > 1" mask for 32-bit values. > > Thus duplicate mask array into two, one for syncing out from trap > frame into GHCB and one for syncing back in from GHCB to the trap > frame. In the latter adjust mask for 32-bit values. > > This was pointed out by Sebastian Sturm (ssturm@genua.de). > > diff --git a/sys/arch/amd64/amd64/ghcb.c b/sys/arch/amd64/amd64/ghcb.c > index c2ce3bab498..904680b0400 100644 > --- a/sys/arch/amd64/amd64/ghcb.c > +++ b/sys/arch/amd64/amd64/ghcb.c > @@ -22,12 +22,27 @@ > #include > #include > > -/* Mask for adjusting GPR sizes. */ > -const uint64_t ghcb_sz_masks[] = { > +/* Masks for adjusting GPR sizes. */ > +const uint64_t ghcb_sz_masks_out[] = { > 0x00000000000000ffULL, 0x000000000000ffffULL, > 0x00000000ffffffffULL, 0xffffffffffffffffULL > }; > > +/* > + * In 64-bit mode, when performing 32-bit operations with a GPR > + * destination, the 32-bit value gets zero extended to the full > + * 64-bit destination size. This means, unlike 8-bit or 16-bit > + * values, the upper 32 bits of the destination register are not > + * retained for 32-bit values. > + * > + * Therefore, when syncing values back to the stack frame, use a > + * "all 1" mask for 32-bit values. > + */ > +const uint64_t ghcb_sz_masks_in[] = { > + 0x00000000000000ffULL, 0x000000000000ffffULL, > + 0xffffffffffffffffULL, 0xffffffffffffffffULL > +}; > + > vaddr_t ghcb_vaddr; > paddr_t ghcb_paddr; > > @@ -179,13 +194,13 @@ ghcb_sync_out(struct trapframe *frame, uint64_t exitcode, uint64_t exitinfo1, > sizeof(ghcb->valid_bitmap)); > > if (ghcb_valbm_isset(gsout->valid_bitmap, GHCB_RAX)) > - ghcb->v_rax = frame->tf_rax & ghcb_sz_masks[gsout->sz_a]; > + ghcb->v_rax = frame->tf_rax & ghcb_sz_masks_out[gsout->sz_a]; > if (ghcb_valbm_isset(gsout->valid_bitmap, GHCB_RBX)) > - ghcb->v_rbx = frame->tf_rbx & ghcb_sz_masks[gsout->sz_b]; > + ghcb->v_rbx = frame->tf_rbx & ghcb_sz_masks_out[gsout->sz_b]; > if (ghcb_valbm_isset(gsout->valid_bitmap, GHCB_RCX)) > - ghcb->v_rcx = frame->tf_rcx & ghcb_sz_masks[gsout->sz_c]; > + ghcb->v_rcx = frame->tf_rcx & ghcb_sz_masks_out[gsout->sz_c]; > if (ghcb_valbm_isset(gsout->valid_bitmap, GHCB_RDX)) > - ghcb->v_rdx = frame->tf_rdx & ghcb_sz_masks[gsout->sz_d]; > + ghcb->v_rdx = frame->tf_rdx & ghcb_sz_masks_out[gsout->sz_d]; > > if (ghcb_valbm_isset(gsout->valid_bitmap, GHCB_SW_EXITCODE)) > ghcb->v_sw_exitcode = exitcode; > @@ -206,20 +221,20 @@ ghcb_sync_in(struct trapframe *frame, struct ghcb_sa *ghcb, > struct ghcb_sync *gsin) > { > if (ghcb_valbm_isset(gsin->valid_bitmap, GHCB_RAX)) { > - frame->tf_rax &= ~ghcb_sz_masks[gsin->sz_a]; > - frame->tf_rax |= (ghcb->v_rax & ghcb_sz_masks[gsin->sz_a]); > + frame->tf_rax &= ~ghcb_sz_masks_in[gsin->sz_a]; > + frame->tf_rax |= (ghcb->v_rax & ghcb_sz_masks_in[gsin->sz_a]); > } > if (ghcb_valbm_isset(gsin->valid_bitmap, GHCB_RBX)) { > - frame->tf_rbx &= ~ghcb_sz_masks[gsin->sz_b]; > - frame->tf_rbx |= (ghcb->v_rbx & ghcb_sz_masks[gsin->sz_b]); > + frame->tf_rbx &= ~ghcb_sz_masks_in[gsin->sz_b]; > + frame->tf_rbx |= (ghcb->v_rbx & ghcb_sz_masks_in[gsin->sz_b]); > } > if (ghcb_valbm_isset(gsin->valid_bitmap, GHCB_RCX)) { > - frame->tf_rcx &= ~ghcb_sz_masks[gsin->sz_c]; > - frame->tf_rcx |= (ghcb->v_rcx & ghcb_sz_masks[gsin->sz_c]); > + frame->tf_rcx &= ~ghcb_sz_masks_in[gsin->sz_c]; > + frame->tf_rcx |= (ghcb->v_rcx & ghcb_sz_masks_in[gsin->sz_c]); > } > if (ghcb_valbm_isset(gsin->valid_bitmap, GHCB_RDX)) { > - frame->tf_rdx &= ~ghcb_sz_masks[gsin->sz_d]; > - frame->tf_rdx |= (ghcb->v_rdx & ghcb_sz_masks[gsin->sz_d]); > + frame->tf_rdx &= ~ghcb_sz_masks_in[gsin->sz_d]; > + frame->tf_rdx |= (ghcb->v_rdx & ghcb_sz_masks_in[gsin->sz_d]); > } > > ghcb_clear(ghcb);