| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 2 Feb 2021 17:07:32 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Joe Perches <joe@...ches.com>,
Stephen Zhang <stephenzhangzsd@...il.com>, seanjc@...gle.com,
vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
joro@...tes.org, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, x86@...nel.org, hpa@...or.com
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86/mmu: Add '__func__' in rmap_printk()
On 27/01/21 18:25, Joe Perches wrote:
> On Wed, 2021-01-27 at 11:54 +0100, Paolo Bonzini wrote:
>> On 27/01/21 03:08, Stephen Zhang wrote:
>>> Given the common pattern:
>>>
>>> rmap_printk("%s:"..., __func__,...)
>>>
>>> we could improve this by adding '__func__' in rmap_printk().
>
> Currently, the MMU_DEBUG control is not defined so this isn't used.
>
> Another improvement would be to remove the macro altogether and
> rename the uses to the more standard style using pr_debug.
>
> arch/x86/kvm/mmu/mmu_internal.h-#undef MMU_DEBUG
> arch/x86/kvm/mmu/mmu_internal.h-
> arch/x86/kvm/mmu/mmu_internal.h-#ifdef MMU_DEBUG
> arch/x86/kvm/mmu/mmu_internal.h-extern bool dbg;
> arch/x86/kvm/mmu/mmu_internal.h-
> arch/x86/kvm/mmu/mmu_internal.h-#define pgprintk(x...) do { if (dbg) printk(x); } while (0)
> arch/x86/kvm/mmu/mmu_internal.h:#define rmap_printk(x...) do { if (dbg) printk(x); } while (0)
> arch/x86/kvm/mmu/mmu_internal.h-#define MMU_WARN_ON(x) WARN_ON(x)
> arch/x86/kvm/mmu/mmu_internal.h-#else
> arch/x86/kvm/mmu/mmu_internal.h-#define pgprintk(x...) do { } while (0)
> arch/x86/kvm/mmu/mmu_internal.h:#define rmap_printk(x...) do { } while (0)
> arch/x86/kvm/mmu/mmu_internal.h-#define MMU_WARN_ON(x) do { } while (0)
> arch/x86/kvm/mmu/mmu_internal.h-#endif
> arch/x86/kvm/mmu/mmu_internal.h-
>
> Also this define hasn't been set in quite awhile as there are
> format/argument mismatches in the code that use gpa_t that need
> to be converted from %lx to %llx with a cast to (u64)
>
> So I think this would be better as:
Joe, would you mind sending this out as a full patch with Signed-off-by?
Thanks,
Paolo
> ---
> arch/x86/kvm/mmu/mmu.c | 47 ++++++++++++++++++-----------------------
> arch/x86/kvm/mmu/mmu_internal.h | 10 +--------
> arch/x86/kvm/mmu/paging_tmpl.h | 10 ++++-----
> arch/x86/kvm/mmu/spte.c | 4 ++--
> 4 files changed, 28 insertions(+), 43 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 6d16481aa29d..54987bd64647 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -105,11 +105,6 @@ enum {
> AUDIT_POST_SYNC
> };
>
> -#ifdef MMU_DEBUG
> -bool dbg = 0;
> -module_param(dbg, bool, 0644);
> -#endif
> -
> #define PTE_PREFETCH_NUM 8
>
> #define PT32_LEVEL_BITS 10
> @@ -844,17 +839,17 @@ static int pte_list_add(struct kvm_vcpu *vcpu, u64 *spte,
> int i, count = 0;
>
> if (!rmap_head->val) {
> - rmap_printk("pte_list_add: %p %llx 0->1\n", spte, *spte);
> + pr_debug("%p %llx 0->1\n", spte, *spte);
> rmap_head->val = (unsigned long)spte;
> } else if (!(rmap_head->val & 1)) {
> - rmap_printk("pte_list_add: %p %llx 1->many\n", spte, *spte);
> + pr_debug("%p %llx 1->many\n", spte, *spte);
> desc = mmu_alloc_pte_list_desc(vcpu);
> desc->sptes[0] = (u64 *)rmap_head->val;
> desc->sptes[1] = spte;
> rmap_head->val = (unsigned long)desc | 1;
> ++count;
> } else {
> - rmap_printk("pte_list_add: %p %llx many->many\n", spte, *spte);
> + pr_debug("%p %llx many->many\n", spte, *spte);
> desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
> while (desc->sptes[PTE_LIST_EXT-1]) {
> count += PTE_LIST_EXT;
> @@ -906,14 +901,14 @@ static void __pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head)
> pr_err("%s: %p 0->BUG\n", __func__, spte);
> BUG();
> } else if (!(rmap_head->val & 1)) {
> - rmap_printk("%s: %p 1->0\n", __func__, spte);
> + pr_debug("%p 1->0\n", spte);
> if ((u64 *)rmap_head->val != spte) {
> pr_err("%s: %p 1->BUG\n", __func__, spte);
> BUG();
> }
> rmap_head->val = 0;
> } else {
> - rmap_printk("%s: %p many->many\n", __func__, spte);
> + pr_debug("%p many->many\n", spte);
> desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
> prev_desc = NULL;
> while (desc) {
> @@ -1115,7 +1110,7 @@ static bool spte_write_protect(u64 *sptep, bool pt_protect)
> !(pt_protect && spte_can_locklessly_be_made_writable(spte)))
> return false;
>
> - rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
> + pr_debug("spte %p %llx\n", sptep, *sptep);
>
> if (pt_protect)
> spte &= ~SPTE_MMU_WRITEABLE;
> @@ -1142,7 +1137,7 @@ static bool spte_clear_dirty(u64 *sptep)
> {
> u64 spte = *sptep;
>
> - rmap_printk("rmap_clear_dirty: spte %p %llx\n", sptep, *sptep);
> + pr_debug("spte %p %llx\n", sptep, *sptep);
>
> MMU_WARN_ON(!spte_ad_enabled(spte));
> spte &= ~shadow_dirty_mask;
> @@ -1184,7 +1179,7 @@ static bool spte_set_dirty(u64 *sptep)
> {
> u64 spte = *sptep;
>
> - rmap_printk("rmap_set_dirty: spte %p %llx\n", sptep, *sptep);
> + pr_debug("spte %p %llx\n", sptep, *sptep);
>
> /*
> * Similar to the !kvm_x86_ops.slot_disable_log_dirty case,
> @@ -1331,7 +1326,7 @@ static bool kvm_zap_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head)
> bool flush = false;
>
> while ((sptep = rmap_get_first(rmap_head, &iter))) {
> - rmap_printk("%s: spte %p %llx.\n", __func__, sptep, *sptep);
> + pr_debug("spte %p %llx.\n", sptep, *sptep);
>
> pte_list_remove(rmap_head, sptep);
> flush = true;
> @@ -1363,8 +1358,8 @@ static int kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>
> restart:
> for_each_rmap_spte(rmap_head, &iter, sptep) {
> - rmap_printk("kvm_set_pte_rmapp: spte %p %llx gfn %llx (%d)\n",
> - sptep, *sptep, gfn, level);
> + pr_debug("spte %p %llx gfn %llx (%d)\n",
> + sptep, *sptep, gfn, level);
>
> need_flush = 1;
>
> @@ -1605,7 +1600,7 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
> return young;
> }
>
> -#ifdef MMU_DEBUG
> +#ifdef DEBUG
> static int is_empty_shadow_page(u64 *spt)
> {
> u64 *pos;
> @@ -2490,12 +2485,11 @@ int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
> LIST_HEAD(invalid_list);
> int r;
>
> - pgprintk("%s: looking for gfn %llx\n", __func__, gfn);
> + pr_debug("looking for gfn %llx\n", gfn);
> r = 0;
> spin_lock(&kvm->mmu_lock);
> for_each_gfn_indirect_valid_sp(kvm, sp, gfn) {
> - pgprintk("%s: gfn %llx role %x\n", __func__, gfn,
> - sp->role.word);
> + pr_debug("gfn %llx role %x\n", gfn, sp->role.word);
> r = 1;
> kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
> }
> @@ -2614,7 +2608,7 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> int ret = RET_PF_FIXED;
> bool flush = false;
>
> - pgprintk("%s: spte %llx write_fault %d gfn %llx\n", __func__,
> + pr_debug("spte %llx write_fault %d gfn %llx\n",
> *sptep, write_fault, gfn);
>
> if (is_shadow_present_pte(*sptep)) {
> @@ -2630,7 +2624,7 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> drop_parent_pte(child, sptep);
> flush = true;
> } else if (pfn != spte_to_pfn(*sptep)) {
> - pgprintk("hfn old %llx new %llx\n",
> + pr_debug("hfn old %llx new %llx\n",
> spte_to_pfn(*sptep), pfn);
> drop_spte(vcpu->kvm, sptep);
> flush = true;
> @@ -2662,7 +2656,7 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> return RET_PF_SPURIOUS;
> }
>
> - pgprintk("%s: setting spte %llx\n", __func__, *sptep);
> + pr_debug("setting spte %llx\n", *sptep);
> trace_kvm_mmu_set_spte(level, gfn, sptep);
> if (!was_rmapped && is_large_pte(*sptep))
> ++vcpu->kvm->stat.lpages;
> @@ -3747,7 +3741,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
> static int nonpaging_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa,
> u32 error_code, bool prefault)
> {
> - pgprintk("%s: gva %lx error %x\n", __func__, gpa, error_code);
> + pr_debug("gva %llx error %x\n", (u64)gpa, error_code);
>
> /* This path builds a PAE pagetable, we can map 2mb pages at maximum. */
> return direct_page_fault(vcpu, gpa & PAGE_MASK, error_code, prefault,
> @@ -4904,8 +4898,7 @@ static bool detect_write_misaligned(struct kvm_mmu_page *sp, gpa_t gpa,
> {
> unsigned offset, pte_size, misaligned;
>
> - pgprintk("misaligned: gpa %llx bytes %d role %x\n",
> - gpa, bytes, sp->role.word);
> + pr_debug("gpa %llx bytes %d role %x\n", gpa, bytes, sp->role.word);
>
> offset = offset_in_page(gpa);
> pte_size = sp->role.gpte_is_8_bytes ? 8 : 4;
> @@ -4990,7 +4983,7 @@ static void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
>
> remote_flush = local_flush = false;
>
> - pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);
> + pr_debug("gpa %llx bytes %d\n", gpa, bytes);
>
> /*
> * No need to care whether allocation memory is successful
> diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
> index bfc6389edc28..095bdef63a03 100644
> --- a/arch/x86/kvm/mmu/mmu_internal.h
> +++ b/arch/x86/kvm/mmu/mmu_internal.h
> @@ -6,17 +6,9 @@
> #include <linux/kvm_host.h>
> #include <asm/kvm_host.h>
>
> -#undef MMU_DEBUG
> -
> -#ifdef MMU_DEBUG
> -extern bool dbg;
> -
> -#define pgprintk(x...) do { if (dbg) printk(x); } while (0)
> -#define rmap_printk(x...) do { if (dbg) printk(x); } while (0)
> +#ifdef DEBUG
> #define MMU_WARN_ON(x) WARN_ON(x)
> #else
> -#define pgprintk(x...) do { } while (0)
> -#define rmap_printk(x...) do { } while (0)
> #define MMU_WARN_ON(x) do { } while (0)
> #endif
>
> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index 50e268eb8e1a..ecbb4e469c1e 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -462,8 +462,8 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
> goto retry_walk;
> }
>
> - pgprintk("%s: pte %llx pte_access %x pt_access %x\n",
> - __func__, (u64)pte, walker->pte_access, walker->pt_access);
> + pr_debug("pte %llx pte_access %x pt_access %x\n",
> + (u64)pte, walker->pte_access, walker->pt_access);
> return 1;
>
> error:
> @@ -536,7 +536,7 @@ FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
> if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
> return false;
>
> - pgprintk("%s: gpte %llx spte %p\n", __func__, (u64)gpte, spte);
> + pr_debug("gpte %llx spte %p\n", (u64)gpte, spte);
>
> gfn = gpte_to_gfn(gpte);
> pte_access = sp->role.access & FNAME(gpte_access)(gpte);
> @@ -794,7 +794,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gpa_t addr, u32 error_code,
> bool map_writable, is_self_change_mapping;
> int max_level;
>
> - pgprintk("%s: addr %lx err %x\n", __func__, addr, error_code);
> + pr_debug("addr %llx err %x\n", (u64)addr, error_code);
>
> /*
> * If PFEC.RSVD is set, this is a shadow page fault.
> @@ -811,7 +811,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gpa_t addr, u32 error_code,
> * The page is not mapped by the guest. Let the guest handle it.
> */
> if (!r) {
> - pgprintk("%s: guest page fault\n", __func__);
> + pr_debug("guest page fault\n");
> if (!prefault)
> kvm_inject_emulated_page_fault(vcpu, &walker.fault);
>
> diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
> index c51ad544f25b..23adcb93f6cc 100644
> --- a/arch/x86/kvm/mmu/spte.c
> +++ b/arch/x86/kvm/mmu/spte.c
> @@ -146,8 +146,8 @@ int make_spte(struct kvm_vcpu *vcpu, unsigned int pte_access, int level,
> goto out;
>
> if (mmu_need_write_protect(vcpu, gfn, can_unsync)) {
> - pgprintk("%s: found shadow page for %llx, marking ro\n",
> - __func__, gfn);
> + pr_debug("found shadow page for %llx, marking ro\n",
> + gfn);
> ret |= SET_SPTE_WRITE_PROTECTED_PT;
> pte_access &= ~ACC_WRITE_MASK;
> spte &= ~(PT_WRITABLE_MASK | SPTE_MMU_WRITEABLE);
>
Powered by blists - more mailing lists