* Sasha Levin <levinsasha928@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
