| Message ID | 20250325174109.267974-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/emul: Emulate %cr8 accesses | expand |
On 25.03.2025 18:41, Andrew Cooper wrote: > Petr reports: > > (XEN) MMIO emulation failed (1): d12v1 64bit @ 0010:fffff8057ba7dfbf -> 45 0f 20 c2 ... > > during introspection. > > This is MOV %cr8, which is wired up for hvm_mov_{to,from}_cr(); the VMExit > fastpaths, but not for the full emulation slowpaths. > > Xen's handling of %cr8 turns out to be quite wrong. At a minimum, we need > storage for %cr8 separate to APIC_TPR, and to alter intercepts based on > whether the vLAPIC is enabled or not. But that's more work than there is time > for in the short term, so make a stopgap fix. > > Extend hvmemul_{read,write}_cr() with %cr8 cases. Unlike hvm_mov_to_cr(), > hardware hasn't filtered out invalid values (#GP checks are ahead of > intercepts), so introduce X86_CR8_VALID_MASK. > > Reported-by: Petr Beneš <w1benny@gmail.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -2288,6 +2288,10 @@ static int cf_check hvmemul_read_cr( > val = curr->arch.hvm.guest_cr[reg]; > break; > > + case 8: > + val = (vlapic_get_reg(vcpu_vlapic(curr), APIC_TASKPRI) & 0xf0) >> 4; No new #define then to use MASK_EXTR() here and ... > @@ -2333,6 +2337,17 @@ static int cf_check hvmemul_write_cr( > rc = hvm_set_cr4(val, true); > break; > > + case 8: > + if ( val & ~X86_CR8_VALID_MASK ) > + { > + rc = X86EMUL_EXCEPTION; > + break; > + } > + > + vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, val << 4); ... MASK_INSR() here? Jan
On 26/03/2025 11:19 am, Jan Beulich wrote: > On 25.03.2025 18:41, Andrew Cooper wrote: >> Petr reports: >> >> (XEN) MMIO emulation failed (1): d12v1 64bit @ 0010:fffff8057ba7dfbf -> 45 0f 20 c2 ... >> >> during introspection. >> >> This is MOV %cr8, which is wired up for hvm_mov_{to,from}_cr(); the VMExit >> fastpaths, but not for the full emulation slowpaths. >> >> Xen's handling of %cr8 turns out to be quite wrong. At a minimum, we need >> storage for %cr8 separate to APIC_TPR, and to alter intercepts based on >> whether the vLAPIC is enabled or not. But that's more work than there is time >> for in the short term, so make a stopgap fix. >> >> Extend hvmemul_{read,write}_cr() with %cr8 cases. Unlike hvm_mov_to_cr(), >> hardware hasn't filtered out invalid values (#GP checks are ahead of >> intercepts), so introduce X86_CR8_VALID_MASK. >> >> Reported-by: Petr Beneš <w1benny@gmail.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -2288,6 +2288,10 @@ static int cf_check hvmemul_read_cr( >> val = curr->arch.hvm.guest_cr[reg]; >> break; >> >> + case 8: >> + val = (vlapic_get_reg(vcpu_vlapic(curr), APIC_TASKPRI) & 0xf0) >> 4; > No new #define then to use MASK_EXTR() here and ... > >> @@ -2333,6 +2337,17 @@ static int cf_check hvmemul_write_cr( >> rc = hvm_set_cr4(val, true); >> break; >> >> + case 8: >> + if ( val & ~X86_CR8_VALID_MASK ) >> + { >> + rc = X86EMUL_EXCEPTION; >> + break; >> + } >> + >> + vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, val << 4); > ... MASK_INSR() here? No. The logic wont survive fixing cr8 I don't think. AFAICT, what we need is plain storage, and vlapic_{get,set}_tpr() accessors which account for hw-disable, and then call back into hvm_set_reg() to adjust intercepts. ~Andrew
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 172ed55bd5e7..61aa356d5736 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -2288,6 +2288,10 @@ static int cf_check hvmemul_read_cr( val = curr->arch.hvm.guest_cr[reg]; break; + case 8: + val = (vlapic_get_reg(vcpu_vlapic(curr), APIC_TASKPRI) & 0xf0) >> 4; + break; + default: return X86EMUL_UNHANDLEABLE; } @@ -2333,6 +2337,17 @@ static int cf_check hvmemul_write_cr( rc = hvm_set_cr4(val, true); break; + case 8: + if ( val & ~X86_CR8_VALID_MASK ) + { + rc = X86EMUL_EXCEPTION; + break; + } + + vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, val << 4); + rc = X86EMUL_OKAY; + break; + default: rc = X86EMUL_UNHANDLEABLE; break; diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h index 61b0cea8f37c..23579c471f4a 100644 --- a/xen/arch/x86/include/asm/x86-defns.h +++ b/xen/arch/x86/include/asm/x86-defns.h @@ -76,6 +76,8 @@ #define X86_CR4_CET 0x00800000 /* Control-flow Enforcement Technology */ #define X86_CR4_PKS 0x01000000 /* Protection Key Supervisor */ +#define X86_CR8_VALID_MASK 0xf + /* * XSTATE component flags in XCR0 | MSR_XSS */
Petr reports: (XEN) MMIO emulation failed (1): d12v1 64bit @ 0010:fffff8057ba7dfbf -> 45 0f 20 c2 ... during introspection. This is MOV %cr8, which is wired up for hvm_mov_{to,from}_cr(); the VMExit fastpaths, but not for the full emulation slowpaths. Xen's handling of %cr8 turns out to be quite wrong. At a minimum, we need storage for %cr8 separate to APIC_TPR, and to alter intercepts based on whether the vLAPIC is enabled or not. But that's more work than there is time for in the short term, so make a stopgap fix. Extend hvmemul_{read,write}_cr() with %cr8 cases. Unlike hvm_mov_to_cr(), hardware hasn't filtered out invalid values (#GP checks are ahead of intercepts), so introduce X86_CR8_VALID_MASK. Reported-by: Petr Beneš <w1benny@gmail.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Petr Beneš <w1benny@gmail.com> v2: * Check for reserved bits In CPUs, CR8 has has 4 bits of storage, and can explicitly get out of sync of APIC_TPR if the the LAPIC is hardware-disabled in MSR_APIC_BASE. Writes to APIC_TPR don't get reflected back into CR8 (according to AMD at least). Both Intel and AMD accelerate TPR differently; SVM in the base architecture, and VT-x as the ~first optional extension. --- xen/arch/x86/hvm/emulate.c | 15 +++++++++++++++ xen/arch/x86/include/asm/x86-defns.h | 2 ++ 2 files changed, 17 insertions(+)