[v9,07/14] KVM: Don't allocate dirty bitmap if dirty ring is enabled
diff mbox series

Message ID 20200523225659.1027044-8-peterx@redhat.com
State New
Headers show
Series
  • KVM: Dirty ring interface
Related show

Commit Message

Peter Xu May 23, 2020, 10:56 p.m. UTC
Because kvm dirty rings and kvm dirty log is used in an exclusive way,
Let's avoid creating the dirty_bitmap when kvm dirty ring is enabled.
At the meantime, since the dirty_bitmap will be conditionally created
now, we can't use it as a sign of "whether this memory slot enabled
dirty tracking".  Change users like that to check against the kvm
memory slot flags.

Note that there still can be chances where the kvm memory slot got its
dirty_bitmap allocated, _if_ the memory slots are created before
enabling of the dirty rings and at the same time with the dirty
tracking capability enabled, they'll still with the dirty_bitmap.
However it should not hurt much (e.g., the bitmaps will always be
freed if they are there), and the real users normally won't trigger
this because dirty bit tracking flag should in most cases only be
applied to kvm slots only before migration starts, that should be far
latter than kvm initializes (VM starts).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/mmu/mmu.c   | 4 ++--
 include/linux/kvm_host.h | 5 +++++
 virt/kvm/kvm_main.c      | 4 ++--
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

kernel test robot May 26, 2020, 3:05 p.m. UTC | #1
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on vhost/linux-next]
[also build test WARNING on linus/master v5.7-rc7]
[cannot apply to kvm/linux-next tip/auto-latest linux/master next-20200522]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Peter-Xu/KVM-Dirty-ring-interface/20200524-070926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> arch/x86/kvm/mmu/mmu.c:1280:3: warning: Returning an integer in a function with pointer return type is not portable. [CastIntegerToAddressAtReturn]
     return false;
     ^
   arch/x86/kvm/mmu/mmu.c:3725:9: warning: Redundant initialization for 'root'. The initialized value is overwritten before it is read. [redundantInitialization]
      root = __pa(sp->spt);
           ^
   arch/x86/kvm/mmu/mmu.c:3715:15: note: root is initialized
      hpa_t root = vcpu->arch.mmu->pae_root[i];
                 ^
   arch/x86/kvm/mmu/mmu.c:3725:9: note: root is overwritten
      root = __pa(sp->spt);
           ^
   arch/x86/kvm/mmu/mmu.c:3769:8: warning: Redundant initialization for 'root'. The initialized value is overwritten before it is read. [redundantInitialization]
     root = __pa(sp->spt);
          ^
   arch/x86/kvm/mmu/mmu.c:3758:14: note: root is initialized
     hpa_t root = vcpu->arch.mmu->root_hpa;
                ^
   arch/x86/kvm/mmu/mmu.c:3769:8: note: root is overwritten
     root = __pa(sp->spt);
          ^
   arch/x86/kvm/mmu/mmu.c:4670:15: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
    const u8 x = BYTE_MASK(ACC_EXEC_MASK);
                 ^
   arch/x86/kvm/mmu/mmu.c:4671:15: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
    const u8 w = BYTE_MASK(ACC_WRITE_MASK);
                 ^
   arch/x86/kvm/mmu/mmu.c:4672:15: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
    const u8 u = BYTE_MASK(ACC_USER_MASK);

# https://github.com/0day-ci/linux/commit/2c23bd2b96e30ae3814e3e56f01a6234131cb531
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 2c23bd2b96e30ae3814e3e56f01a6234131cb531
vim +1280 arch/x86/kvm/mmu/mmu.c

b8e8c8303ff28c arch/x86/kvm/mmu.c     Paolo Bonzini   2019-11-04  1269  
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1270  static struct kvm_memory_slot *
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1271  gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu, gfn_t gfn,
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1272  			    bool no_dirty_log)
05da45583de9b3 arch/x86/kvm/mmu.c     Marcelo Tosatti 2008-02-23  1273  {
05da45583de9b3 arch/x86/kvm/mmu.c     Marcelo Tosatti 2008-02-23  1274  	struct kvm_memory_slot *slot;
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1275  
54bf36aac52031 arch/x86/kvm/mmu.c     Paolo Bonzini   2015-04-08  1276  	slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
91b0d268a59dd9 arch/x86/kvm/mmu/mmu.c Paolo Bonzini   2020-01-21  1277  	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
91b0d268a59dd9 arch/x86/kvm/mmu/mmu.c Paolo Bonzini   2020-01-21  1278  		return NULL;
2c23bd2b96e30a arch/x86/kvm/mmu/mmu.c Peter Xu        2020-05-23  1279  	if (no_dirty_log && kvm_slot_dirty_track_enabled(slot))
2c23bd2b96e30a arch/x86/kvm/mmu/mmu.c Peter Xu        2020-05-23 @1280  		return false;
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1281  
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1282  	return slot;
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1283  }
5d163b1c9d6e55 arch/x86/kvm/mmu.c     Xiao Guangrong  2011-03-09  1284  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Peter Xu May 26, 2020, 3:21 p.m. UTC | #2
On Tue, May 26, 2020 at 11:05:47PM +0800, kbuild test robot wrote:
> >> arch/x86/kvm/mmu/mmu.c:1280:3: warning: Returning an integer in a function with pointer return type is not portable. [CastIntegerToAddressAtReturn]
>      return false;
>      ^

A rebase accident for quite a few versions...  Fixed.
kernel test robot May 26, 2020, 7:21 p.m. UTC | #3
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on vhost/linux-next]
[also build test WARNING on linus/master v5.7-rc7]
[cannot apply to kvm/linux-next tip/auto-latest linux/master next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Peter-Xu/KVM-Dirty-ring-interface/20200524-070926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> arch/x86/kvm/mmu/mmu.c:1280:10: warning: expression which evaluates to zero treated as a null pointer constant of type 'struct kvm_memory_slot *' [-Wnon-literal-null-conversion]
return false;
^~~~~
1 warning generated.

vim +1280 arch/x86/kvm/mmu/mmu.c

  1269	
  1270	static struct kvm_memory_slot *
  1271	gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu, gfn_t gfn,
  1272				    bool no_dirty_log)
  1273	{
  1274		struct kvm_memory_slot *slot;
  1275	
  1276		slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
  1277		if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
  1278			return NULL;
  1279		if (no_dirty_log && kvm_slot_dirty_track_enabled(slot))
> 1280			return false;
  1281	
  1282		return slot;
  1283	}
  1284	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot May 27, 2020, 12:53 p.m. UTC | #4
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on vhost/linux-next]
[also build test WARNING on linus/master v5.7-rc7]
[cannot apply to kvm/linux-next tip/auto-latest linux/master next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Peter-Xu/KVM-Dirty-ring-interface/20200524-070926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: i386-randconfig-s002-20200527 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-240-gf0fe1cd9-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   arch/x86/kvm/mmu/mmu.c:693:18: sparse: sparse: cast to non-scalar
   arch/x86/kvm/mmu/mmu.c:712:18: sparse: sparse: cast to non-scalar
   arch/x86/kvm/mmu/mmu.c:731:18: sparse: sparse: cast to non-scalar
>> arch/x86/kvm/mmu/mmu.c:1280:24: sparse: sparse: Using plain integer as NULL pointer
   arch/x86/kvm/mmu/mmu.c:4687:57: sparse: sparse: cast truncates bits from constant value (ffffff33 becomes 33)
   arch/x86/kvm/mmu/mmu.c:4689:56: sparse: sparse: cast truncates bits from constant value (ffffff0f becomes f)
   arch/x86/kvm/mmu/mmu.c:4691:57: sparse: sparse: cast truncates bits from constant value (ffffff55 becomes 55)

vim +1280 arch/x86/kvm/mmu/mmu.c

  1269	
  1270	static struct kvm_memory_slot *
  1271	gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu, gfn_t gfn,
  1272				    bool no_dirty_log)
  1273	{
  1274		struct kvm_memory_slot *slot;
  1275	
  1276		slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
  1277		if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
  1278			return NULL;
  1279		if (no_dirty_log && kvm_slot_dirty_track_enabled(slot))
> 1280			return false;
  1281	
  1282		return slot;
  1283	}
  1284	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Patch
diff mbox series

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 94d84a383b80..ebc86f661db3 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1276,8 +1276,8 @@  gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu, gfn_t gfn,
 	slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
 	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
 		return NULL;
-	if (no_dirty_log && slot->dirty_bitmap)
-		return NULL;
+	if (no_dirty_log && kvm_slot_dirty_track_enabled(slot))
+		return false;
 
 	return slot;
 }
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a7eaef494f45..5081c6e2ae06 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -351,6 +351,11 @@  struct kvm_memory_slot {
 	u16 as_id;
 };
 
+static inline bool kvm_slot_dirty_track_enabled(struct kvm_memory_slot *slot)
+{
+	return slot->flags & KVM_MEM_LOG_DIRTY_PAGES;
+}
+
 static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot)
 {
 	return ALIGN(memslot->npages, BITS_PER_LONG) / 8;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9639cf8d8c9c..ae7ba67eab63 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1294,7 +1294,7 @@  int __kvm_set_memory_region(struct kvm *kvm,
 	/* Allocate/free page dirty bitmap as needed */
 	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES))
 		new.dirty_bitmap = NULL;
-	else if (!new.dirty_bitmap) {
+	else if (!new.dirty_bitmap && !kvm->dirty_ring_size) {
 		r = kvm_alloc_dirty_bitmap(&new);
 		if (r)
 			return r;
@@ -2581,7 +2581,7 @@  static void mark_page_dirty_in_slot(struct kvm *kvm,
 				    struct kvm_memory_slot *memslot,
 				    gfn_t gfn)
 {
-	if (memslot && memslot->dirty_bitmap) {
+	if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
 		u32 slot = (memslot->as_id << 16) | memslot->id;