Re: [PATCH RFC v9 04/51] KVM: x86: Determine shared/private faults using a configurable mask

[Huang, Kai]

> 
> So if we were to straight-forwardly implement that based on how TDX
> currently handles checking for the shared bit in GPA, paired with how
> SEV-SNP handles checking for private bit in fault flags, it would look
> something like:
> 
>   bool kvm_fault_is_private(kvm, gpa, err)
>   {
>     /* SEV-SNP handling */
>     if (kvm->arch.mmu_private_fault_mask)
>       return !!(err & arch.mmu_private_fault_mask);
> 
>     /* TDX handling */
>     if (kvm->arch.gfn_shared_mask)
>       return !!(gpa & arch.gfn_shared_mask);

The logic of the two are identical.  I think they need to be converged.

Either SEV-SNP should convert the error code private bit to the gfn_shared_mask,
or TDX's shared bit should be converted to some private error bit.

Perhaps converting SEV-SNP makes more sense because if I recall correctly SEV
guest also has a C-bit, correct?

Or, ...

> 
>     return false;
>   }
> 
>   kvm_mmu_do_page_fault(vcpu, gpa, err, ...)
>   {
>     struct kvm_page_fault fault = {
>       ...
>       .is_private = kvm_fault_is_private(vcpu->kvm, gpa, err)

... should we do something like:

	.is_private = static_call(kvm_x86_fault_is_private)(vcpu->kvm, gpa, 
							    err);

?

>     };
> 
>     ...
>   }
> 
> And then arch.mmu_private_fault_mask and arch.gfn_shared_mask would be
> set per-KVM-instance, just like they are now with current SNP and TDX
> patchsets, since stuff like KVM self-test wouldn't be setting those
> masks, so it makes sense to do it per-instance in that regard.
> 
> But that still gets a little awkward for the KVM self-test use-case where
> .is_private should sort of be ignored in favor of whatever the xarray
> reports via kvm_mem_is_private(). 
> 

I must have missed something.  Why does KVM self-test have impact to how does
KVM handles private fault? 

> In your Misc. series I believe you
> handled this by introducing a PFERR_HASATTR_MASK bit so we can determine
> whether existing value of fault->is_private should be
> ignored/overwritten or not.
> 
> So maybe kvm_fault_is_private() needs to return an integer value
> instead, like:
> 
>   enum {
>     KVM_FAULT_VMM_DEFINED,
>     KVM_FAULT_SHARED,
>     KVM_FAULT_PRIVATE,
>   }
> 
>   bool kvm_fault_is_private(kvm, gpa, err)
>   {
>     /* SEV-SNP handling */
>     if (kvm->arch.mmu_private_fault_mask)
>       (err & arch.mmu_private_fault_mask) ? KVM_FAULT_PRIVATE : KVM_FAULT_SHARED
> 
>     /* TDX handling */
>     if (kvm->arch.gfn_shared_mask)
>       (gpa & arch.gfn_shared_mask) ? KVM_FAULT_SHARED : KVM_FAULT_PRIVATE
> 
>     return KVM_FAULT_VMM_DEFINED;
>   }
> 
> And then down in __kvm_faultin_pfn() we do:
> 
>   if (fault->is_private == KVM_FAULT_VMM_DEFINED)
>     fault->is_private = kvm_mem_is_private(vcpu->kvm, fault->gfn);
>   else if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
>     return kvm_do_memory_fault_exit(vcpu, fault);
> 
>   if (fault->is_private)
>     return kvm_faultin_pfn_private(vcpu, fault);


What does KVM_FAULT_VMM_DEFINED mean, exactly?

Shouldn't the fault type come from _hardware_?