On Thu, May 19, 2022 at 04:45:20PM +0200, Roger Pau Monné wrote:
> On Thu, May 19, 2022 at 12:10:24AM +0000, Andrew Cooper wrote:
> > On 17/05/2022 14:21, Roger Pau Monne wrote:
> > > @@ -1333,6 +1338,19 @@ static int construct_vmcs(struct vcpu *v)
> > > rc = vmx_add_msr(v, MSR_FLUSH_CMD, FLUSH_CMD_L1D,
> > > VMX_MSR_GUEST_LOADONLY);
> > >
> > > + if ( cpu_has_vmx_notify_vm_exiting && vm_notify_window >= 0 )
> > > + {
> > > + __vmwrite(NOTIFY_WINDOW, vm_notify_window);
> > > + /*
> > > + * Disable #AC and #DB interception: by using VM Notify Xen is
> > > + * guaranteed to get a VM exit even if the guest manages to lock
> > > the
> > > + * CPU.
> > > + */
> > > + v->arch.hvm.vmx.exception_bitmap &= ~((1U << TRAP_debug) |
> > > + (1U <<
> > > TRAP_alignment_check));
> > > + vmx_update_exception_bitmap(v);
> >
> > IIRC, it's not quite this easy. There are conditions, e.g. attaching
> > gdbsx, where #DB interception wants turning on/off dynamically, and the
> > logic got simplified to nothing following XSA-156, so will need
> > reintroducing.
> >
> > AMD Milan (Zen3) actually has NoNestedDataBp in CPUID.80000021.eax[0]
> > which allows us to not intercept #DB, so perhaps that might offer an
> > easier way of adjusting the interception logic. (Or maybe not. I can't
> > remember).
>
> OK, will look into it.
So after taking a look, I think we need to modify vmx_update_debug_state() so
it's:
void vmx_update_debug_state(struct vcpu *v)
{
unsigned int mask = 1u << TRAP_int3;
if ( v->arch.hvm.vmx.secondary_exec_control &
SECONDARY_EXEC_NOTIFY_VM_EXITING )
/*
* Only allow toggling TRAP_debug if notify VM exit is enabled, as
* unconditionally setting TRAP_debug is part of the XSA-156 fix.
*/
mask |= 1u << TRAP_debug;
if ( v->arch.hvm.debug_state_latch )
v->arch.hvm.vmx.exception_bitmap |= mask;
else
v->arch.hvm.vmx.exception_bitmap &= ~mask;
[...]
I'm however confused by the usage of cpu_has_monitor_trap_flag
previous to XSA-156, which was:
void vmx_update_debug_state(struct vcpu *v)
{
unsigned long mask;
mask = 1u << TRAP_int3;
if ( !cpu_has_monitor_trap_flag )
mask |= 1u << TRAP_debug;
Was it fine to not set TRAP_debug only if cpu_has_monitor_trap_flag
is supported by the CPU? (even if not currently set on
secondary_exec_control)?
Thanks, Roger.
