Re: [PATCH] kvm tools: Add memory gap for larger RAM sizes

[Ingo Molnar <mingo@elte.hu>]

* Sasha Levin <levinsasha928@gmail.com> 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