Re: [PATCH v14 111/113] RFC: KVM: x86, TDX: Add check for setting CPUID

[Huang, Kai]

On Sun, 2023-05-28 at 21:20 -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> 
> Add a hook to setting CPUID for additional consistency check of
> KVM_SET_CPUID2.
> 
> Because intel TDX or AMD SNP has restriction on the value of cpuid.  Some
> value can't be changed after boot.  Some can be only set at the VM
> creation time and can't be changed later.  Check if the new values are
> consistent with the old values.

You might want to use some grammar tool to check against the changelog.

Also, personally I think it's better to briefly mention why we choose this
design but not another (e.g., why we just don't make KVM remember all CPUIDs in
TDH.MNG.INIT and simply ignores/rejects further KVM_SET_CPUID2).  It would be
helpful for the reviewer, or some git blamer in the future.

And why this patch is placed at the very bottom of this series? This logic
should belong to TD creation part which should be at a relative earlier position
in this series.

[...]


> @@ -32,6 +32,13 @@ struct kvm_tdx {
>  	atomic_t tdh_mem_track;
>  
>  	u64 tsc_offset;
> +
> +	/*
> +	 * For KVM_SET_CPUID to check consistency. Remember the one passed to
> +	 * TDH.MNG_INIT
> +	 */
> +	int cpuid_nent;
> +	struct kvm_cpuid_entry2 *cpuid;

Since these CPUIDs may only be part of the vcpu's CPUIDs, you may want a more
specific name, for instance, consistent_cpuid?

Also if you want this be common to AMD, then perhaps the comment should be
improved too to be more generic.