From: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Jan Beulich" <JBeulich@suse.com>,
"Roger Pau Monné" <roger.pau@citrix.com>,
Xen-devel <xen-devel-bounces@lists.xenproject.org>
Subject: Re: [PATCH 2/3] x86/svm: Drop svmdebug.c
Date: Mon, 1 Dec 2025 16:33:33 +0100 [thread overview]
Message-ID: <DEMZMSXSLKT1.1MWK08XUVJTJ2@amd.com> (raw)
In-Reply-To: <20251128201937.1294742-3-andrew.cooper3@citrix.com>
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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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
next prev parent reply other threads:[~2025-12-01 15:34 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-28 20:19 [PATCH 0/3] x86/svm: Make various details private Andrew Cooper
2025-11-28 20:19 ` [PATCH 1/3] x86/svm: Make vmcb_struct private to svm/ Andrew Cooper
2025-12-01 15:23 ` Alejandro Vallejo
2025-12-08 9:05 ` Jan Beulich
2025-12-10 14:25 ` Andrew Cooper
2025-12-11 7:10 ` Jan Beulich
2025-11-28 20:19 ` [PATCH 2/3] x86/svm: Drop svmdebug.c Andrew Cooper
2025-12-01 15:33 ` Alejandro Vallejo [this message]
2025-12-08 9:06 ` Jan Beulich
2025-11-28 20:19 ` [PATCH 3/3] x86/svm: Drop svmdebug.h Andrew Cooper
2025-12-01 15:39 ` Alejandro Vallejo
2025-12-08 9:10 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DEMZMSXSLKT1.1MWK08XUVJTJ2@amd.com \
--to=alejandro.garciavallejo@amd.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=xen-devel-bounces@lists.xenproject.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.