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 <hshoexer@genua.de>
> 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 <machine/frame.h>
>  #include <machine/ghcb.h>
>
> -/* 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);