Re: [PATCH v7 07/10] x86/virt/tdx: Trim away tail null CMRs

["Huang, Kai" <kai.huang@intel.com>]

On Mon, 2024-11-11 at 18:32 +0200, Nikolay Borisov wrote:
> 
> On 11.11.24 г. 12:39 ч., Kai Huang wrote:
> > TDX architecturally supports up to 32 CMRs.  The global metadata field
> > "NUM_CMRS" reports the number of CMR entries that can be read by the
> > kernel.  However, that field may just report the maximum number of CMRs
> > albeit the actual number of CMRs is smaller, in which case there are
> > tail null CMRs (size is 0).
> 
> nit: Is it safe to assume that null CMRs are going to be sequential and 
> always at the end? Nothing in the TDX module spec suggests this. I.e 
> can't we have :
> 
> 
> 1. Valid CMR region
> 2. ZERO CMR
> 3. Valid CMR
> 
> Sure, it might be a dummy and pointless but nothing prevents such CMR 
> records. In any case I think the mentioning of "tail" is a bit too much 
> detail and adds to unnecessary mental overload. Simply say you trim 
> empty CMR's and that such regions will be sequential (if that's the 
> case) and be done with it.
> 
> Because having "tail null cmr" can be interpreted as also having  there 
> might be "non-tail null CMR", which doesn't seem to be the case?

It's described in the comment in the code:

+	 * Note the CMRs are generated by the BIOS, but the MCHECK
+	 * verifies CMRs before enabling TDX on hardware.  Skip other
+	 * sanity checks (e.g., verify CMR is 4KB aligned) but trust
+	 * MCHECK to work properly.
+	 *
+	 * The spec doesn't say whether it's legal to have null CMRs
+	 * in the middle of valid CMRs.  For now assume no sane BIOS
+	 * would do that (even MCHECK allows).

I don't see why a sane BIOS would need to do that, and we have never seen such
case in reality.  IMO we don't need to be too skeptical now.  If we see this can
indeed happen in the future, we can always come up with a patch to fix.
 
[...]


> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> 

Thanks!