x86/tdx: Port I/O emulation fixes
Subsystems (4)
Kiryl Shutsemau (Meta)
maintainerv2 cover x86/tdx: Port I/O emulation fixes
This series addresses two technical inaccuracies in the TDX guest port I/O emulation code reported by Borys Tsyrulnikov.
The first patch fixes an off-by-one error in the GENMASK() macro usage where the mask was being calculated as one bit too wide (e.g. 9 bits for an 8-bit operation).
The second patch ensures that 32-bit port I/O operations (INL) correctly zero-extend the result to the full 64-bit RAX register, as required by the x86 architecture. Currently, the emulation preserves the upper 32 bits of RAX during such operations.
Both issues were introduced in the initial implementation of the runtime hypercalls for port I/O.
v1: https://lore.kernel.org/all/[email protected]/
Changes in v2:
- Rephrase the size check in handle_in() as "if (size == 4)" for readability (Kuppuswamy)
- Add Link: to the bug report on both patches (Kuppuswamy)
- Collect Reviewed-by tags (Kai Huang, Kuppuswamy Sathyanarayanan)
- Rebase onto v7.1-rc1
Kiryl Shutsemau (Meta) (2):
x86/tdx: Fix off-by-one in port I/O handling
x86/tdx: Fix zero-extension for 32-bit port I/O
arch/x86/coco/tdx/tdx.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
Signature · 1 line
2.51.2
Kiryl Shutsemau (Meta)
maintainerPATCH v2 1/2 x86/tdx: Fix off-by-one in port I/O handling
handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:
u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
GENMASK(h, l) includes bit h. For size=1 (INB), this produces GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8 bits). The mask is one bit too wide for all I/O sizes.
Fix the mask calculation.
--- arch/x86/coco/tdx/tdx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/coco/tdx/tdx.c +2 -2
@@ -693,7 +693,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port) .r13 = PORT_READ, .r14 = port, }; -u64 mask =GENMASK (BITS_PER_BYTE * size,0 ); +u64 mask =GENMASK (BITS_PER_BYTE * size -1 ,0 );bool success;/*
@@ -713,7 +713,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)static bool handle_out (struct pt_regs *regs,int size,int port) { -u64 mask =GENMASK (BITS_PER_BYTE * size,0 ); +u64 mask =GENMASK (BITS_PER_BYTE * size -1 ,0 );/* * Emulate the I/O write via hypercall. More info about ABI can be found
Signature · 1 line
2.51.2
Kiryl Shutsemau (Meta)
maintainerPATCH v2 2/2 x86/tdx: Fix zero-extension for 32-bit port I/O
In reply to Kiryl Shutsemau (Meta)According to x86 architecture rules, 32-bit operations zero-extend the result to 64 bits. The current implementation of handle_in() only masks the lower 32 bits, which preserves the upper 32 bits of RAX when a 32-bit port IN instruction is emulated.
Update handle_in() to zero out the entire RAX register when the I/O size is 4 bytes to ensure correct zero-extension. For smaller sizes (1 or 2 bytes), continue to preserve the unaffected upper bits.
--- arch/x86/coco/tdx/tdx.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
arch/x86/coco/tdx/tdx.c +11 -2
@@ -703,8 +703,17 @@ static bool handle_in(struct pt_regs *regs, int size, int port)*/ success = !__tdx_hypercall (&args); -/* Update part of the register affected by the emulated instruction */ - regs->ax &= ~mask; +/* +* Update part of the register affected by the emulated instruction. +* +* 32-bit operands generate a 32-bit result, zero-extended to a 64-bit +* result. +*/ +if (size ==4 ) + regs->ax =0 ; +else + regs->ax &= ~mask; +if (success) regs->ax |= args.r11 & mask;
Signature · 1 line
2.51.2
Re: [PATCH v2 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
arch/x86/coco/tdx/tdx.c static bool handle_in(struct pt_regs *regs, int size, int port) · line 715
Quoted 4 lines · Kiryl Shutsemau (Meta)
arch/x86/coco/tdx/tdx.c +4
-0+if (size ==4 ) + regs->ax =0 ; +else + regs->ax &= ~mask;
I haven't thought about this _that_ much, but this feels wrong. Why is is 4 so special cased?
Also, what _are_ the limits on the registers that 'in' can be used on?
RAX - n/a, no 64-bit I/O
EAX - size=4
AX - size=2 AH - n/a no encoding for inb AL - size=1
I'd find this much easier to grasp if there was a nice table of what the registers, sizes, and masks ended up being usable. As usual, x86 is "fun" here.
H. Peter Anvin
reviewerRe: [PATCH v2 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
Quoted 13 lines · Dave Hansen
arch/x86/coco/tdx/tdx.c static bool handle_in(struct pt_regs *regs, int size, int port) · line 715
Quoted 4 lines · Kiryl Shutsemau (Meta)
arch/x86/coco/tdx/tdx.c +4
-0+if (size ==4 ) + regs->ax =0 ; +else + regs->ax &= ~mask;I haven't thought about this _that_ much, but this feels wrong. Why is is 4 so special cased?
Also, what _are_ the limits on the registers that 'in' can be used on?
RAX - n/a, no 64-bit I/O
EAX - size=4
Quoted 7 lines
AX - size=2 AH - n/a no encoding for inb AL - size=1I'd find this much easier to grasp if there was a nice table of what the registers, sizes, and masks ended up being usable. As usual, x86 is "fun" here.
Because zero extension only applies to dwords.
x86-64 has three subregisters per GPR:
Bits 7-0
Bits 15-8
Bits 63-16
Sean Christopherson
reviewerRe: [PATCH v2 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
Quoted 18 lines · H. Peter Anvin
Quoted 17 lines · Dave Hansen
arch/x86/coco/tdx/tdx.c static bool handle_in(struct pt_regs *regs, int size, int port) · line 715
Quoted 4 lines · Kiryl Shutsemau (Meta)
arch/x86/coco/tdx/tdx.c +4
-0+if (size ==4 ) + regs->ax =0 ; +else + regs->ax &= ~mask;I haven't thought about this _that_ much, but this feels wrong. Why is is 4 so special cased?
Also, what _are_ the limits on the registers that 'in' can be used on?
RAX - n/a, no 64-bit I/O
EAX - size=4AX - size=2 AH - n/a no encoding for inb AL - size=1
Quoted 7 lines
Quoted 3 lines
I'd find this much easier to grasp if there was a nice table of what the registers, sizes, and masks ended up being usable. As usual, x86 is "fun" here.
Because zero extension only applies to dwords.
x86-64 has three subregisters per GPR:
Aren't there four? The fourth being 31:0, which is the one that is zero-extended and so "clobbers" 63:32.
Quoted 3 lines
Bits 7-0
Bits 15-8
Bits 63-16
I assume you mean 15:0? 63:16 isn't addressable. And these are the ones that aren't zero-extended, i.e. don't "clobber" other bits.
Kiryl Shutsemau
maintainerRe: [PATCH v2 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
In reply to Dave HansenQuoted 13 lines · Dave Hansen
arch/x86/coco/tdx/tdx.c static bool handle_in(struct pt_regs *regs, int size, int port) · line 715
Quoted 4 lines · Kiryl Shutsemau (Meta)
arch/x86/coco/tdx/tdx.c +4
-0+if (size ==4 ) + regs->ax =0 ; +else + regs->ax &= ~mask;I haven't thought about this _that_ much, but this feels wrong. Why is is 4 so special cased?
Also, what _are_ the limits on the registers that 'in' can be used on?
RAX - n/a, no 64-bit I/O
EAX - size=4
Quoted 7 lines
AX - size=2 AH - n/a no encoding for inb AL - size=1I'd find this much easier to grasp if there was a nice table of what the registers, sizes, and masks ended up being usable. As usual, x86 is "fun" here.
How about this for the comment:
/*
* IN writes the result into a sub-register of RAX. Only the
* 32-bit form zero-extends; the smaller forms leave the upper
* bits untouched:
*
* insn dest size bits written bits preserved
* inb AL 1 RAX[ 7: 0] RAX[63: 8]
* inw AX 2 RAX[15: 0] RAX[63:16]
* inl EAX 4 RAX[63: 0] (none, zero-extended)
*
* 'mask' only covers the low 'size' bytes, which is exactly
* the range affected for size 1 and 2. For size 4 the write
* also clears RAX[63:32], so widen the clear-mask. */
Signature · 1 line
Kiryl Shutsemau / Kirill A. Shutemov
Kiryl Shutsemau
maintainerQuoted 3 lines · Kiryl Shutsemau (Meta)
Kiryl Shutsemau (Meta) (2):
x86/tdx: Fix off-by-one in port I/O handling
x86/tdx: Fix zero-extension for 32-bit port I/O
Dave, could get them applied?
Signature · 1 line
Kiryl Shutsemau / Kirill A. Shutemov
Quoted 5 lines · Kiryl Shutsemau
Quoted 3 lines · Kiryl Shutsemau (Meta)
Kiryl Shutsemau (Meta) (2):
x86/tdx: Fix off-by-one in port I/O handling
x86/tdx: Fix zero-extension for 32-bit port I/ODave, could get them applied?
I'll look on Monday. Thanks for the reminder.