Re: [RFC 6/6] capabilities: convert attach debugger into a capability

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/1/23 21:06, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> > Expresses the ability to attach a debugger as a capability that a domain can be
> > provisioned.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   xen/arch/x86/hvm/svm/svm.c      |  8 ++++----
> >   xen/arch/x86/hvm/vmx/realmode.c |  2 +-
> >   xen/arch/x86/hvm/vmx/vmcs.c     |  2 +-
> >   xen/arch/x86/hvm/vmx/vmx.c      | 10 +++++-----
> >   xen/arch/x86/traps.c            |  6 ++++--
> >   xen/common/domctl.c             |  6 ++++--
> >   xen/include/xen/sched.h         |  9 ++++++---
> >   7 files changed, 25 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index 27170213ae..9872804d39 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -999,7 +999,7 @@ static void noreturn cf_check svm_do_resume(void)
> >   {
> >       struct vcpu *v = current;
> >       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> > -    bool debug_state = (v->domain->debugger_attached ||
> > +    bool debug_state = (domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) ||
> >                           v->domain->arch.monitor.software_breakpoint_enabled ||
> >                           v->domain->arch.monitor.debug_exception_enabled);
> >       bool_t vcpu_guestmode = 0;
> > @@ -1335,7 +1335,7 @@ static void cf_check svm_inject_event(const struct 
> > x86_event *event)
> >           }
> >           /* fall through */
> >       case X86_EXC_BP:
> > -        if ( curr->domain->debugger_attached )
> > +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
> >           {
> >               /* Debug/Int3: Trap to debugger. */
> >               domain_pause_for_debugger();
> > @@ -2732,7 +2732,7 @@ void svm_vmexit_handler(void)
> >   
> >       case VMEXIT_ICEBP:
> >       case VMEXIT_EXCEPTION_DB:
> > -        if ( !v->domain->debugger_attached )
> > +        if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
> >           {
> >               unsigned int trap_type;
> >   
> > @@ -2769,7 +2769,7 @@ void svm_vmexit_handler(void)
> >           if ( insn_len == 0 )
> >                break;
> >   
> > -        if ( v->domain->debugger_attached )
> > +        if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
> >           {
> >               /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update 
> > RIP. */
> >               __update_guest_eip(regs, insn_len);
> > diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
> > index ff44ddcfa6..f761026a9d 100644
> > --- a/xen/arch/x86/hvm/vmx/realmode.c
> > +++ b/xen/arch/x86/hvm/vmx/realmode.c
> > @@ -121,7 +121,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt 
> > *hvmemul_ctxt)
> >   
> >       if ( rc == X86EMUL_EXCEPTION )
> >       {
> > -        if ( unlikely(curr->domain->debugger_attached) &&
> > +        if ( unlikely(domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH)) &&
> >                ((hvmemul_ctxt->ctxt.event.vector == X86_EXC_DB) ||
> >                 (hvmemul_ctxt->ctxt.event.vector == X86_EXC_BP)) )
> >           {
> > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> > index 13719cc923..9474869018 100644
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -1912,7 +1912,7 @@ void cf_check vmx_do_resume(void)
> >           hvm_asid_flush_vcpu(v);
> >       }
> >   
> > -    debug_state = v->domain->debugger_attached
> > +    debug_state = domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH)
> >                     || v->domain->arch.monitor.software_breakpoint_enabled
> >                     || v->domain->arch.monitor.singlestep_enabled;
> >   
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 7ec44018d4..5069e3cbf3 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2041,7 +2041,7 @@ static void cf_check vmx_inject_event(const struct 
> > x86_event *event)
> >               break;
> >           /* fall through */
> >       case X86_EXC_BP:
> > -        if ( curr->domain->debugger_attached )
> > +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
> >           {
> >               /* Debug/Int3: Trap to debugger. */
> >               domain_pause_for_debugger();
> > @@ -2121,7 +2121,7 @@ static void cf_check vmx_set_info_guest(struct vcpu *v)
> >        * immediately vmexit and hence make no progress.
> >        */
> >       __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
> > -    if ( v->domain->debugger_attached &&
> > +    if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) &&
> >            (v->arch.user_regs.eflags & X86_EFLAGS_TF) &&
> >            (intr_shadow & VMX_INTR_SHADOW_STI) )
> >       {
> > @@ -4283,7 +4283,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >                   }
> >               }
> >   
> > -            if ( !v->domain->debugger_attached )
> > +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
> >               {
> >                   unsigned long insn_len = 0;
> >                   int rc;
> > @@ -4307,7 +4307,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >               break;
> >           case X86_EXC_BP:
> >               HVMTRACE_1D(TRAP, vector);
> > -            if ( !v->domain->debugger_attached )
> > +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
> >               {
> >                   unsigned long insn_len;
> >                   int rc;
> > @@ -4647,7 +4647,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >                                 HVM_MONITOR_SINGLESTEP_BREAKPOINT,
> >                                 0, 0, 0);
> >   
> > -            if ( v->domain->debugger_attached )
> > +            if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
> >                   domain_pause_for_debugger();
> >           }
> >   
> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index 4229bda159..041ced35ea 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -1214,7 +1214,8 @@ void do_int3(struct cpu_user_regs *regs)
> >           return;
> >       }
> >   
> > -    if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
> > +    if ( guest_kernel_mode(curr, regs) &&
> > +         domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
> >       {
> >           curr->arch.gdbsx_vcpu_event = X86_EXC_BP;
> >           domain_pause_for_debugger();
> > @@ -1995,7 +1996,8 @@ void do_debug(struct cpu_user_regs *regs)
> >       v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
> >       v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
> >   
> > -    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
> > +    if ( guest_kernel_mode(v, regs) &&
> > +         domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
> >       {
> >           domain_pause_for_debugger();
> >           return;
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index 505e29c0dc..895ddf0600 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -99,7 +99,8 @@ void getdomaininfo(struct domain *d, struct 
> > xen_domctl_getdomaininfo *info)
> >           ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
> >           (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
> >           (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
> > -        (d->debugger_attached           ? XEN_DOMINF_debugged  : 0) |
> > +        (domain_has_cap(d, CAP_DEBUGGER_ATTACH) ?
> > +                                          XEN_DOMINF_debugged  : 0) |
> >           (is_xenstore_domain(d)          ? XEN_DOMINF_xs_domain : 0) |
> >           (is_hvm_domain(d)               ? XEN_DOMINF_hvm_guest : 0) |
> >           d->shutdown_code << XEN_DOMINF_shutdownshift;
> > @@ -643,7 +644,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > u_domctl)
> >           else
> >           {
> >               domain_pause(d);
> > -            d->debugger_attached = !!op->u.setdebugging.enable;
> > +            if ( !!op->u.setdebugging.enable )
> > +                domain_set_cap(d, CAP_DEBUGGER_ATTACH);
> >               domain_unpause(d); /* causes guest to latch new status */
> >           }
> >           break;
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index ebfe65cd73..47eadb5008 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -474,9 +474,8 @@ struct domain
> >       uint8_t          role;
> >   #define CAP_CONSOLE_IO         (1U<<0)
> >   #define CAP_DISABLE_CPU_FAULT  (1U<<1)
> > -    uint8_t          capabilities;
> > -    /* Is this guest being debugged by dom0? */
> > -    bool             debugger_attached;
> > +#define CAP_DEBUGGER_ATTACH    (1U<<2)
> > +    uint16_t         capabilities;
> No need to switch to uint16_t just yet?
I know space is tight in struct domain, wanted to reclaim the freed 
space into capabilities (or roles). One thing I was considering if 
enough space could be found is instead replace it all with a pointer to 
a new struct that held these values. It would allow using heap space and 
future growth of the structure. As of this patch, it is consuming 5 
bytes and would need to find an additional 3 bytes. Is there a 
willingness to entertain such an approach?
> >       /*
> >        * Set to true at the very end of domain creation, when the domain is
> >        * unpaused for the first time by the systemcontroller.
> > @@ -1166,6 +1165,10 @@ static always_inline bool domain_set_cap(
> >           if ( is_pv_domain(d) && is_control_domain(d) )
> >               d->capabilities |= cap;
> >           break;
> > +    case CAP_DEBUGGER_ATTACH:
> > +        if ( !is_control_domain(d) )
> > +            d->capabilities |= cap;
> I take it is not possible to attach it to dom0? I am not familiar with
> XEN_DOMCTL_setdebugging but the hypercall doesn't seem to have such
> restriction.
Good question, on first glance I can't see why I added this. Going to 
walk through it again and see if I can figure out why, otherwise I will 
drop it.
> > +        break;
> >       default:
> >           return false;
> >       }
> > --
> > 2.20.1