Date: Tue, 5 May 2015 19:17:47 +0200
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, bsd@redhat.com,
        guangrong.xiao@linux.intel.com, Yang Zhang <yang.z.zhang@intel.com>,
        wanpeng.li@linux.intel.com
Subject: Re: [PATCH 12/13] KVM: x86: add KVM_MEM_X86_SMRAM memory slot flag
Message-ID: <20150505171747.GB17198@potion.brq.redhat.com>
References: <1430393772-27208-1-git-send-email-pbonzini@redhat.com>
 <1430393772-27208-13-git-send-email-pbonzini@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1430393772-27208-13-git-send-email-pbonzini@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2612
Lines: 58

2015-04-30 13:36+0200, Paolo Bonzini:
> This adds an arch-specific memslot flag that hides slots unless the
> VCPU is in system management mode.
> 
> Some care is needed in order to limit the overhead of x86_gfn_to_memslot
> when compared with gfn_to_memslot.  Thankfully, we have __gfn_to_memslot
> and search_memslots which are the same, so we can add some extra output
> to search_memslots.  The compiler will optimize it as dead code in
> __gfn_to_memslot, and will use it to thread jumps in x86_gfn_to_memslot.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> @@ -19,10 +19,23 @@
>  
>  #include <linux/module.h>
>  #include <linux/kvm_host.h>
> +#include "kvm_cache_regs.h"
>  
>  struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn)
>  {
> -	struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn);
> +	/* By using search_memslots directly the compiler can optimize away
> +	 * the "if (found)" check below.
> +         *
> +	 * It cannot do the same for gfn_to_memslot because it is not inlined,
> +	 * and it also cannot do the same for __gfn_to_memslot because the
> +	 * kernel is compiled with -fno-delete-null-pointer-checks.
> +	 */
> +	bool found;
> +	struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm);
> +	struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found);
> +
> +	if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu))
> +		return NULL;

Patch [10/13] made me sad and IIUIC, the line above is the only reason
for it ... what about renaming and changing kvm_* memory function to
vcpu_* and create 
  bool kvm_arch_vcpu_can_access_slot(vcpu, slot)
which could also be inline in arch/*/include/asm/kvm_host.h thanks to
the way we build.
We could be passing both kvm and vcpu in internal memslot operations and
not checking if vcpu is NULL.  This should allow all possible operations
with little code duplication and the compiler could also optimize the
case where vcpu is NULL.

Another option is adding something like "vcpu kvm_arch_fake_vcpu(kvm)"
for cases where the access doesn't have an associated vcpu, so it would
always succeed.  (Might not be generic enough.)

I prefer everything to copy-pasting code, so I'll try to come up with
more ideas if you don't like these :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/