Received-SPF: pass (google.com: domain of linux-kernel+bounces-102724-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date: Wed, 13 Mar 2024 21:49:52 -0500
From: Michael Roth <michael.roth@amd.com>
To: Paolo Bonzini <pbonzini@redhat.com>
CC: <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>,
	<seanjc@google.com>, <aik@amd.com>, <pankaj.gupta@amd.com>
Subject: Re: [PATCH v3 10/15] KVM: x86: add fields to struct kvm_arch for
 CoCo features
Message-ID: <20240314024952.w6n6ol5hjzqayn2g@amd.com>
References: <20240226190344.787149-1-pbonzini@redhat.com>
 <20240226190344.787149-11-pbonzini@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240226190344.787149-11-pbonzini@redhat.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2024 02:50:25.6494
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 9a38f629-c0f5-4ef2-6422-08dc43d1803b
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	MWH0EPF000971E3.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7929

On Mon, Feb 26, 2024 at 02:03:39PM -0500, Paolo Bonzini wrote:
> Some VM types have characteristics in common; in fact, the only use
> of VM types right now is kvm_arch_has_private_mem and it assumes that
> _all_ nonzero VM types have private memory.
> 
> We will soon introduce a VM type for SEV and SEV-ES VMs, and at that
> point we will have two special characteristics of confidential VMs
> that depend on the VM type: not just if memory is private, but
> also whether guest state is protected.  For the latter we have
> kvm->arch.guest_state_protected, which is only set on a fully initialized
> VM.
> 
> For VM types with protected guest state, we can actually fix a problem in
> the SEV-ES implementation, where ioctls to set registers do not cause an
> error even if the VM has been initialized and the guest state encrypted.
> Make sure that when using VM types that will become an error.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20240209183743.22030-7-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  7 ++-
>  arch/x86/kvm/x86.c              | 95 +++++++++++++++++++++++++++------
>  2 files changed, 84 insertions(+), 18 deletions(-)
> 
> @@ -5552,9 +5561,13 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>  }
>  
>  
> -static void kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> -					  u8 *state, unsigned int size)
> +static int kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> +					 u8 *state, unsigned int size)
>  {
> +	if (vcpu->kvm->arch.has_protected_state &&
> +	    fpstate_is_confidential(&vcpu->arch.guest_fpu))
> +		return -EINVAL;
> +
>  	/*
>  	 * Only copy state for features that are enabled for the guest.  The
>  	 * state itself isn't problematic, but setting bits in the header for
> @@ -5571,22 +5584,27 @@ static void kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
>  			     XFEATURE_MASK_FPSSE;
>  
>  	if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
> -		return;
> +		return 0;
>  
>  	fpu_copy_guest_fpstate_to_uabi(&vcpu->arch.guest_fpu, state, size,
>  				       supported_xcr0, vcpu->arch.pkru);
> +	return 0;
>  }
>  
> -static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> -					 struct kvm_xsave *guest_xsave)
> +static int kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> +					struct kvm_xsave *guest_xsave)
>  {
> -	kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
> -				      sizeof(guest_xsave->region));
> +	return kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
> +					     sizeof(guest_xsave->region));
>  }
>  
>  static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>  					struct kvm_xsave *guest_xsave)
>  {
> +	if (vcpu->kvm->arch.has_protected_state &&
> +	    fpstate_is_confidential(&vcpu->arch.guest_fpu))
> +		return -EINVAL;
> +
>  	if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
>  		return 0;

I've been trying to get SNP running on top of these patches and hit and
issue with these due to fpstate_set_confidential() being done during
svm_vcpu_create(), so when QEMU tries to sync FPU state prior to calling
SNP_LAUNCH_FINISH it errors out. I think the same would happen with
SEV-ES as well.

Maybe fpstate_set_confidential() should be relocated to SEV_LAUNCH_FINISH
site as part of these patches?

Also, do you happen to have a pointer to the WIP QEMU patches? Happy to
help with posting/testing those since we'll need similar for
SEV_INIT2-based SNP patches.

Thanks,

Mike