Xen project Mailing List

Re: [PATCH 2/3] x86/svm: Drop svmdebug.c

To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>

From: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>

Date: Mon, 1 Dec 2025 16:33:33 +0100

Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=citrix.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)

Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=mgjZe9wQQsYzCqmXtBpweMGHnOj6z7vqE8VdzpVNkAA=; b=bxqJQfHZsXd7vk+O3GPDce4o0nexhSGsIG9jsiJP0W9vj2rZ0pReC/mU6+a6Lap9mWwXPIYdHvQ/pGYlCZcY8T08eNtgWVrAGU3jJ6QdeZ3ZwpHa6frAwpNQoJYwU8GZbB8vdx0Vdwja2v/DUaTT/enhXZK5qBS2A4G0naI4JdSb46ezwOsgDOhnaV+nBXf+rI5MyTDg7Uor+Perfs8CWAlVZ1916F24faaXsL7qSMJz4RNf903QFR8K898F/F5sXckdKOLWWEp46dppLdhgn3XEn96YxFjhYGWpoK37XnTvCNL8dPUAJvsRh8LkHXkPAYUkXrrte5m/7a5eB8m4ZQ==

Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=sGUb7wYJCupUaogX6ehOPMDYc3792cyI9wwgYljL1cLsK8CAaAB/QeBXaXR81DRbI6D0gbl9bHOm9bp1xUXH1m7KraRBP5x7gM2FTxaoWOnlOjwqCvjLvgzEW+4tgOmerDDfJ9aWVe7NNQDweQH2XifXEJU7cSIATDLt6o8P91LVr96Lvlk9CQAzKJWUb4IFRhK/T80PWdGhQGbNz+8H3aWbM2tIyVlCOPcrBgGqHJtikiXqbtZJJ5unnayxXBn1wjVTkYEMdoyn8Ac9vsS3Q44phKQxWaXWN7EKBbDb5xTALbBqLrfUh1mZ5iaNTiRD/AbSzzkf6xvoK91gHVSVpQ==

Cc: Jan Beulich <JBeulich@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx>

Delivery-date: Mon, 01 Dec 2025 15:34:17 +0000

List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hi, On Fri Nov 28, 2025 at 9:19 PM CET, Andrew Cooper wrote: > Everything here is really VMCB functionality, so merge it into vmcb.c. Move > the declarations from the global svmdebug.h to the logical vmcb.h. > > No functional change. Not functional, but there's a single instance of style adjustment on move... > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > --- > xen/arch/x86/hvm/svm/Makefile | 1 - > xen/arch/x86/hvm/svm/nestedsvm.c | 1 - > xen/arch/x86/hvm/svm/svmdebug.c | 181 -------------------- > xen/arch/x86/hvm/svm/vmcb.c | 159 +++++++++++++++++ > xen/arch/x86/hvm/svm/vmcb.h | 3 + > xen/arch/x86/include/asm/hvm/svm/svmdebug.h | 3 - > 6 files changed, 162 insertions(+), 186 deletions(-) > delete mode 100644 xen/arch/x86/hvm/svm/svmdebug.c > > diff --git a/xen/arch/x86/hvm/svm/Makefile b/xen/arch/x86/hvm/svm/Makefile > index 760d2954da83..8a072cafd572 100644 > --- a/xen/arch/x86/hvm/svm/Makefile > +++ b/xen/arch/x86/hvm/svm/Makefile > @@ -4,5 +4,4 @@ obj-bin-y += entry.o > obj-y += intr.o > obj-y += nestedsvm.o > obj-y += svm.o > -obj-y += svmdebug.o > obj-y += vmcb.o > diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c > b/xen/arch/x86/hvm/svm/nestedsvm.c > index 191466755148..63ed6c86b775 100644 > --- a/xen/arch/x86/hvm/svm/nestedsvm.c > +++ b/xen/arch/x86/hvm/svm/nestedsvm.c > @@ -9,7 +9,6 @@ > #include <asm/hvm/svm/svm.h> > #include <asm/hvm/svm/vmcb.h> > #include <asm/hvm/nestedhvm.h> > -#include <asm/hvm/svm/svmdebug.h> > #include <asm/paging.h> /* paging_mode_hap */ > #include <asm/event.h> /* for local_event_delivery_(en|dis)able */ > #include <asm/p2m.h> /* p2m_get_pagetable, p2m_get_nestedp2m */ > diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c > deleted file mode 100644 > index bdb9ea3583ee..000000000000 > --- a/xen/arch/x86/hvm/svm/svmdebug.c > +++ /dev/null > @@ -1,181 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0-only */ > -/* > - * svmdebug.c: debug functions > - * Copyright (c) 2011, Advanced Micro Devices, Inc. > - * > - */ > - > -#include <xen/sched.h> > -#include <asm/processor.h> > -#include <asm/msr-index.h> > -#include <asm/hvm/svm/svmdebug.h> > - > -#include "vmcb.h" > - > -static void svm_dump_sel(const char *name, const struct segment_register *s) > -{ > - printk("%s: %04x %04x %08x %016"PRIx64"\n", > - name, s->sel, s->attr, s->limit, s->base); > -} > - > -void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb) > -{ > - struct vcpu *curr = current; > - > - /* > - * If we are dumping the VMCB currently in context, some guest state may > - * still be cached in hardware. Retrieve it. > - */ > - if ( vmcb == curr->arch.hvm.svm.vmcb ) > - svm_sync_vmcb(curr, vmcb_in_sync); > - > - printk("Dumping guest's current state at %s...\n", from); > - printk("Size of VMCB = %zu, paddr = %"PRIpaddr", vaddr = %p\n", > - sizeof(struct vmcb_struct), virt_to_maddr(vmcb), vmcb); > - > - printk("cr_intercepts = %#x dr_intercepts = %#x " > - "exception_intercepts = %#x\n", > - vmcb_get_cr_intercepts(vmcb), vmcb_get_dr_intercepts(vmcb), > - vmcb_get_exception_intercepts(vmcb)); > - printk("general1_intercepts = %#x general2_intercepts = %#x\n", > - vmcb_get_general1_intercepts(vmcb), > vmcb_get_general2_intercepts(vmcb)); > - printk("iopm_base_pa = %#"PRIx64" msrpm_base_pa = %#"PRIx64" tsc_offset > = %#"PRIx64"\n", > - vmcb_get_iopm_base_pa(vmcb), vmcb_get_msrpm_base_pa(vmcb), > - vmcb_get_tsc_offset(vmcb)); > - printk("tlb_control = %#x vintr = %#"PRIx64" int_stat = %#"PRIx64"\n", > - vmcb->tlb_control, vmcb_get_vintr(vmcb).bytes, > - vmcb->int_stat.raw); > - printk("event_inj %016"PRIx64", valid? %d, ec? %d, type %u, vector > %#x\n", > - vmcb->event_inj.raw, vmcb->event_inj.v, > - vmcb->event_inj.ev, vmcb->event_inj.type, > - vmcb->event_inj.vector); > - printk("exitcode = %#"PRIx64" exit_int_info = %#"PRIx64"\n", > - vmcb->exitcode, vmcb->exit_int_info.raw); > - printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n", > - vmcb->exitinfo1, vmcb->exitinfo2); > - printk("asid = %#x np_ctrl = %#"PRIx64":%s%s%s\n", > - vmcb_get_asid(vmcb), vmcb_get_np_ctrl(vmcb), > - vmcb_get_np(vmcb) ? " NP" : "", > - vmcb_get_sev(vmcb) ? " SEV" : "", > - vmcb_get_sev_es(vmcb) ? " SEV_ES" : ""); > - printk("virtual vmload/vmsave = %d, virt_ext = %#"PRIx64"\n", > - vmcb->virt_ext.fields.vloadsave_enable, vmcb->virt_ext.bytes); > - printk("cpl = %d efer = %#"PRIx64" star = %#"PRIx64" lstar = > %#"PRIx64"\n", > - vmcb_get_cpl(vmcb), vmcb_get_efer(vmcb), vmcb->star, vmcb->lstar); > - printk("CR0 = 0x%016"PRIx64" CR2 = 0x%016"PRIx64"\n", > - vmcb_get_cr0(vmcb), vmcb_get_cr2(vmcb)); > - printk("CR3 = 0x%016"PRIx64" CR4 = 0x%016"PRIx64"\n", > - vmcb_get_cr3(vmcb), vmcb_get_cr4(vmcb)); > - printk("RSP = 0x%016"PRIx64" RIP = 0x%016"PRIx64"\n", > - vmcb->rsp, vmcb->rip); > - printk("RAX = 0x%016"PRIx64" RFLAGS=0x%016"PRIx64"\n", > - vmcb->rax, vmcb->rflags); > - printk("DR6 = 0x%016"PRIx64", DR7 = 0x%016"PRIx64"\n", > - vmcb_get_dr6(vmcb), vmcb_get_dr7(vmcb)); > - printk("CSTAR = 0x%016"PRIx64" SFMask = 0x%016"PRIx64"\n", > - vmcb->cstar, vmcb->sfmask); > - printk("KernGSBase = 0x%016"PRIx64" PAT = 0x%016"PRIx64"\n", > - vmcb->kerngsbase, vmcb_get_g_pat(vmcb)); > - printk("SSP = 0x%016"PRIx64" S_CET = 0x%016"PRIx64" ISST = > 0x%016"PRIx64"\n", > - vmcb->_ssp, vmcb->_msr_s_cet, vmcb->_msr_isst); > - printk("H_CR3 = 0x%016"PRIx64" CleanBits = %#x\n", > - vmcb_get_h_cr3(vmcb), vmcb->cleanbits.raw); > - > - /* print out all the selectors */ > - printk(" sel attr limit base\n"); > - svm_dump_sel(" CS", &vmcb->cs); > - svm_dump_sel(" DS", &vmcb->ds); > - svm_dump_sel(" SS", &vmcb->ss); > - svm_dump_sel(" ES", &vmcb->es); > - svm_dump_sel(" FS", &vmcb->fs); > - svm_dump_sel(" GS", &vmcb->gs); > - svm_dump_sel("GDTR", &vmcb->gdtr); > - svm_dump_sel("LDTR", &vmcb->ldtr); > - svm_dump_sel("IDTR", &vmcb->idtr); > - svm_dump_sel(" TR", &vmcb->tr); > -} > - > -bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb, > - const struct vcpu *v, bool verbose) > -{ > - bool ret = false; /* ok */ > - unsigned long cr0 = vmcb_get_cr0(vmcb); > - unsigned long cr3 = vmcb_get_cr3(vmcb); > - unsigned long cr4 = vmcb_get_cr4(vmcb); > - unsigned long valid; > - uint64_t efer = vmcb_get_efer(vmcb); > - > -#define PRINTF(fmt, args...) do { \ > - if ( !verbose ) return true; \ > - ret = true; \ > - printk(XENLOG_GUEST "%pv[%s]: " fmt, v, from, ## args); \ > -} while (0) > - > - if ( !(efer & EFER_SVME) ) > - PRINTF("EFER: SVME bit not set (%#"PRIx64")\n", efer); > - > - if ( !(cr0 & X86_CR0_CD) && (cr0 & X86_CR0_NW) ) > - PRINTF("CR0: CD bit is zero and NW bit set (%#"PRIx64")\n", cr0); > - > - if ( cr0 >> 32 ) > - PRINTF("CR0: bits [63:32] are not zero (%#"PRIx64")\n", cr0); > - > - if ( (cr0 & X86_CR0_PG) && > - ((cr3 & 7) || > - ((!(cr4 & X86_CR4_PAE) || (efer & EFER_LMA)) && (cr3 & 0xfe0)) || > - ((efer & EFER_LMA) && > - (cr3 >> v->domain->arch.cpuid->extd.maxphysaddr))) ) > - PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", cr3); > - > - valid = hvm_cr4_guest_valid_bits(v->domain); > - if ( cr4 & ~valid ) > - PRINTF("CR4: invalid value %#lx (valid %#lx, rejected %#lx)\n", > - cr4, valid, cr4 & ~valid); > - > - if ( vmcb_get_dr6(vmcb) >> 32 ) > - PRINTF("DR6: bits [63:32] are not zero (%#"PRIx64")\n", > - vmcb_get_dr6(vmcb)); > - > - if ( vmcb_get_dr7(vmcb) >> 32 ) > - PRINTF("DR7: bits [63:32] are not zero (%#"PRIx64")\n", > - vmcb_get_dr7(vmcb)); > - > - if ( efer & ~EFER_KNOWN_MASK ) > - PRINTF("EFER: unknown bits are not zero (%#"PRIx64")\n", efer); > - > - if ( hvm_efer_valid(v, efer, -1) ) > - PRINTF("EFER: %s (%"PRIx64")\n", hvm_efer_valid(v, efer, -1), efer); > - > - if ( (efer & EFER_LME) && (cr0 & X86_CR0_PG) ) > - { > - if ( !(cr4 & X86_CR4_PAE) ) > - PRINTF("EFER_LME and CR0.PG are both set and CR4.PAE is zero\n"); > - if ( !(cr0 & X86_CR0_PE) ) > - PRINTF("EFER_LME and CR0.PG are both set and CR0.PE is zero\n"); > - } > - > - if ( (efer & EFER_LME) && (cr0 & X86_CR0_PG) && (cr4 & X86_CR4_PAE) && > - vmcb->cs.l && vmcb->cs.db ) > - PRINTF("EFER_LME, CR0.PG, CR4.PAE, CS.L and CS.D are all > non-zero\n"); > - > - if ( !(vmcb_get_general2_intercepts(vmcb) & GENERAL2_INTERCEPT_VMRUN) ) > - PRINTF("GENERAL2_INTERCEPT: VMRUN intercept bit is clear > (%#"PRIx32")\n", > - vmcb_get_general2_intercepts(vmcb)); > - > - if ( vmcb->event_inj.resvd1 ) > - PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n", > - vmcb->event_inj.raw); > - > -#undef PRINTF > - return ret; > -} > - > -/* > - * Local variables: > - * mode: C > - * c-file-style: "BSD" > - * c-basic-offset: 4 > - * tab-width: 4 > - * indent-tabs-mode: nil > - * End: > - */ > diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c > index 44fa76bf0228..b1a79d515143 100644 > --- a/xen/arch/x86/hvm/svm/vmcb.c > +++ b/xen/arch/x86/hvm/svm/vmcb.c > @@ -228,6 +228,165 @@ void svm_destroy_vmcb(struct vcpu *v) > svm->vmcb = NULL; > } > > +static void svm_dump_sel(const char *name, const struct segment_register *s) > +{ > + printk("%s: %04x %04x %08x %016"PRIx64"\n", > + name, s->sel, s->attr, s->limit, s->base); > +} > + > +void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb) > +{ > + struct vcpu *curr = current; > + > + /* > + * If we are dumping the VMCB currently in context, some guest state may > + * still be cached in hardware. Retrieve it. > + */ > + if ( vmcb == curr->arch.hvm.svm.vmcb ) > + svm_sync_vmcb(curr, vmcb_in_sync); > + > + printk("Dumping guest's current state at %s...\n", from); > + printk("Size of VMCB = %zu, paddr = %"PRIpaddr", vaddr = %p\n", > + sizeof(struct vmcb_struct), virt_to_maddr(vmcb), vmcb); > + > + printk("cr_intercepts = %#x dr_intercepts = %#x " > + "exception_intercepts = %#x\n", > + vmcb_get_cr_intercepts(vmcb), vmcb_get_dr_intercepts(vmcb), > + vmcb_get_exception_intercepts(vmcb)); > + printk("general1_intercepts = %#x general2_intercepts = %#x\n", > + vmcb_get_general1_intercepts(vmcb), > vmcb_get_general2_intercepts(vmcb)); > + printk("iopm_base_pa = %#"PRIx64" msrpm_base_pa = %#"PRIx64" tsc_offset > = %#"PRIx64"\n", > + vmcb_get_iopm_base_pa(vmcb), vmcb_get_msrpm_base_pa(vmcb), > + vmcb_get_tsc_offset(vmcb)); > + printk("tlb_control = %#x vintr = %#"PRIx64" int_stat = %#"PRIx64"\n", > + vmcb->tlb_control, vmcb_get_vintr(vmcb).bytes, > + vmcb->int_stat.raw); > + printk("event_inj %016"PRIx64", valid? %d, ec? %d, type %u, vector > %#x\n", > + vmcb->event_inj.raw, vmcb->event_inj.v, > + vmcb->event_inj.ev, vmcb->event_inj.type, > + vmcb->event_inj.vector); > + printk("exitcode = %#"PRIx64" exit_int_info = %#"PRIx64"\n", > + vmcb->exitcode, vmcb->exit_int_info.raw); > + printk("exitinfo1 = %#"PRIx64" exitinfo2 = %#"PRIx64"\n", > + vmcb->exitinfo1, vmcb->exitinfo2); > + printk("asid = %#x np_ctrl = %#"PRIx64":%s%s%s\n", > + vmcb_get_asid(vmcb), vmcb_get_np_ctrl(vmcb), > + vmcb_get_np(vmcb) ? " NP" : "", > + vmcb_get_sev(vmcb) ? " SEV" : "", > + vmcb_get_sev_es(vmcb) ? " SEV_ES" : ""); > + printk("virtual vmload/vmsave = %d, virt_ext = %#"PRIx64"\n", > + vmcb->virt_ext.fields.vloadsave_enable, vmcb->virt_ext.bytes); > + printk("cpl = %d efer = %#"PRIx64" star = %#"PRIx64" lstar = > %#"PRIx64"\n", > + vmcb_get_cpl(vmcb), vmcb_get_efer(vmcb), vmcb->star, vmcb->lstar); > + printk("CR0 = 0x%016"PRIx64" CR2 = 0x%016"PRIx64"\n", > + vmcb_get_cr0(vmcb), vmcb_get_cr2(vmcb)); > + printk("CR3 = 0x%016"PRIx64" CR4 = 0x%016"PRIx64"\n", > + vmcb_get_cr3(vmcb), vmcb_get_cr4(vmcb)); > + printk("RSP = 0x%016"PRIx64" RIP = 0x%016"PRIx64"\n", > + vmcb->rsp, vmcb->rip); > + printk("RAX = 0x%016"PRIx64" RFLAGS=0x%016"PRIx64"\n", > + vmcb->rax, vmcb->rflags); > + printk("DR6 = 0x%016"PRIx64", DR7 = 0x%016"PRIx64"\n", > + vmcb_get_dr6(vmcb), vmcb_get_dr7(vmcb)); > + printk("CSTAR = 0x%016"PRIx64" SFMask = 0x%016"PRIx64"\n", > + vmcb->cstar, vmcb->sfmask); > + printk("KernGSBase = 0x%016"PRIx64" PAT = 0x%016"PRIx64"\n", > + vmcb->kerngsbase, vmcb_get_g_pat(vmcb)); > + printk("SSP = 0x%016"PRIx64" S_CET = 0x%016"PRIx64" ISST = > 0x%016"PRIx64"\n", > + vmcb->_ssp, vmcb->_msr_s_cet, vmcb->_msr_isst); > + printk("H_CR3 = 0x%016"PRIx64" CleanBits = %#x\n", > + vmcb_get_h_cr3(vmcb), vmcb->cleanbits.raw); > + > + /* print out all the selectors */ > + printk(" sel attr limit base\n"); > + svm_dump_sel(" CS", &vmcb->cs); > + svm_dump_sel(" DS", &vmcb->ds); > + svm_dump_sel(" SS", &vmcb->ss); > + svm_dump_sel(" ES", &vmcb->es); > + svm_dump_sel(" FS", &vmcb->fs); > + svm_dump_sel(" GS", &vmcb->gs); > + svm_dump_sel("GDTR", &vmcb->gdtr); > + svm_dump_sel("LDTR", &vmcb->ldtr); > + svm_dump_sel("IDTR", &vmcb->idtr); > + svm_dump_sel(" TR", &vmcb->tr); > +} > + > +bool svm_vmcb_isvalid( > + const char *from, const struct vmcb_struct *vmcb, const struct vcpu *v, > + bool verbose) ... here, which is probably unintentional. If intentional it's worth mentioning in the commit message and worth keeping in sync with the form in the prototype. I personally prefer the former style rather than this one, FWIW. Cheers, Alejandro

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.