From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH] kvm tools: Add memory gap for larger RAM sizes Date: Wed, 11 May 2011 13:13:58 +0200 Message-ID: <20110511111358.GC18521@elte.hu> References: <1305109881-11189-1-git-send-email-levinsasha928@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: penberg@kernel.org, asias.hejun@gmail.com, prasadjoshi124@gmail.com, avi@redhat.com, gorcunov@gmail.com, kvm@vger.kernel.org To: Sasha Levin Return-path: Received: from fallback.mail.elte.hu ([157.181.151.13]:37355 "EHLO fallback.mail.elte.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756128Ab1EKP4y (ORCPT ); Wed, 11 May 2011 11:56:54 -0400 Received: from mx2.mail.elte.hu ([157.181.151.9]) by fallback.mail.elte.hu with esmtp (Exim) id 1QK7Vu-0004Wi-Ce from for ; Wed, 11 May 2011 13:23:34 +0200 Content-Disposition: inline In-Reply-To: <1305109881-11189-1-git-send-email-levinsasha928@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: * Sasha Levin wrote: > } > > +void kvm__init_ram(struct kvm *self) > +{ Why is there no comment explaining what this function does and what the whole gap logic is about? The bug this problem caused was non-obvious, so any future developer reading this code will wonder what this is all about. > + if (self->ram_size < KVM_32BIT_GAP_START) { > + kvm_register_mem_slot(self, 0, 0, self->ram_size, self->ram_start); > + } else { > + kvm_register_mem_slot(self, 0, 0, KVM_32BIT_GAP_START, self->ram_start); > + kvm_register_mem_slot(self, 1, 0x100000000ULL, self->ram_size - KVM_32BIT_GAP_START, self->ram_start + 0x100000000ULL); > + } > +} Why not write it in a much more obvious and almost self-documenting way: /* First RAM range from zero to the PCI gap: */ phys_start = 0; phys_size = KVM_32BIT_GAP_START; host_mem = self->ram_start; kvm_register_mem_slot(self, 0, phys_start, phys_size, host_mem); /* Second RAM range from 4GB to the end of RAM: */ phys_start = 0x100000000ULL; phys_size = self->ram_size - phys_size; host_mem = self->ram_start + phys_start; kvm_register_mem_slot(self, 1, phys_start, phys_size, host_mem); ? Btw., could we please also stop using 'self' for function parameters? It's utterly meaningless as a name and makes grepping pretty hard. Use a consistent and meaningful convention please, such as: struct kvm_cpu *vcpu And obviously CPU related methods will always have a vcpu parameter around. Thanks, Ingo