On 7/15/21 11:56 AM, Sean Christopherson wrote:

quoted

quoted

quoted

+       /* Retry if another processor is modifying the RMP entry. */
+       do {
+               /* Binutils version 2.36 supports the PSMASH mnemonic. */
+               asm volatile(".byte 0xF3, 0x0F, 0x01, 0xFF"
+                             : "=a"(ret)
+                             : "a"(spa)
+                             : "memory", "cc");
+       } while (ret == FAIL_INUSE);

Should there be some retry limit here for safety? Or do we know that
we'll never be stuck in this loop? Ditto for the loop in rmpupdate.

It's probably fine to just leave this.  While you could *theoretically*
lose this race forever, it's unlikely to happen in practice.  If it
does, you'll get an easy-to-understand softlockup backtrace which should
point here pretty quickly.

But should failure here even be tolerated?  The TDX cases spin on flows that are
_not_ due to (direct) contenion, e.g. a pending interrupt while flushing the
cache or lack of randomness when generating a key.  In this case, there are two
CPUs racing to modify the RMP entry, which implies that the final state of the
RMP entry is not deterministic.

I was envisioning that two different CPUs could try to smash two
*different* 4k physical pages, but collide since they share
a 2M page.

But, in patch 33, this is called via:

+		write_lock(&kvm->mmu_lock);
+
+		switch (op) {
+		case SNP_PAGE_STATE_SHARED:
+			rc = snp_make_page_shared(vcpu, gpa, pfn, level);

...

Which should make collisions impossible.  Did I miss another call-site?