Re: [RFC v2-fix 1/1] x86/tdx: Handle in-kernel MMIO
From: Andi Kleen
Date: Tue May 18 2021 - 13:15:50 EST
* If we didn't annotate we would need to add an alternative to every
MMIO access in the kernel (even though 99.9% will never be used on
TDX) which would be a complete waste and incredible binary bloat
for nothing.
That sounds like something objective we can measure. Does this cost 1
byte of extra text per readl/writel? 10? 100?
Agreed. And IMO, it's worth converting the common case (macros) if the overhead
is acceptable, while leaving the #VE handling in place for non-standard code.
We have many millions of lines of MMIO using driver code in the kernel
99.99% of which never runs in TDX. I don't see any point in impacting
everything for this. That would be just against all good code change
hygiene practices, and also just be bloated.
But we also don't don't want to touch every driver, for similar reasons.
What I think would make sense is to convert something to a direct TDCALL
if we figure out the extra #VE is a real life performance problem. AFAIK
the only candidate that I have in mind for this is the virtio doorbell
write (and potentially later its VMBus equivalent). But we should really
only do that if some measurements show it's needed.
Why does this code exist at all? TDX and SEV-ES absolutely must share code for
handling MMIO reflection. It will require a fair amount of refactoring to move
the guts of vc_handle_mmio() to common code, but there is zero reason to maintain
two separate versions of the opcode cracking.
While that's true on the high level, all the low level details are
different. We looked at unifying at some point, but it would have been a
callback hell. I don't think unifying would make anything cleaner.
Besides the bulk of the decoding work is already unified in the common
x86 instruction decoder. The actual actions are different, and the code
fetching is also different, so on the rest there isn't that much to unify.
The existing SEV-ES #VC handlers appear to be missing page split checks, so that
needs to be fixed.
Only if anyone in the kernel actually relies on it?
-Andi