| Message ID | 20220715230016.3762909-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Memtype related cleanups | expand |
On Fri, 2022-07-15 at 23:00 +0000, Sean Christopherson wrote: > Return directly if kvm_arch_init() detects an error before doing any real > work, jumping through a label obfuscates what's happening and carries the > unnecessary risk of leaving 'r' uninitialized. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 12199c40f2bc..41aa3137665c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -9146,21 +9146,18 @@ int kvm_arch_init(void *opaque) > > if (kvm_x86_ops.hardware_enable) { > pr_err("kvm: already loaded vendor module '%s'\n", kvm_x86_ops.name); > - r = -EEXIST; > - goto out; > + return -EEXIST; > } > > if (!ops->cpu_has_kvm_support()) { > pr_err_ratelimited("kvm: no hardware support for '%s'\n", > ops->runtime_ops->name); > - r = -EOPNOTSUPP; > - goto out; > + return -EOPNOTSUPP; > } > if (ops->disabled_by_bios()) { > pr_err_ratelimited("kvm: support for '%s' disabled by bios\n", > ops->runtime_ops->name); > - r = -EOPNOTSUPP; > - goto out; > + return -EOPNOTSUPP; > } > > /* > @@ -9170,14 +9167,12 @@ int kvm_arch_init(void *opaque) > */ > if (!boot_cpu_has(X86_FEATURE_FPU) || !boot_cpu_has(X86_FEATURE_FXSR)) { > printk(KERN_ERR "kvm: inadequate fpu\n"); > - r = -EOPNOTSUPP; > - goto out; > + return -EOPNOTSUPP; > } > > if (IS_ENABLED(CONFIG_PREEMPT_RT) && !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { > pr_err("RT requires X86_FEATURE_CONSTANT_TSC\n"); > - r = -EOPNOTSUPP; > - goto out; > + return -EOPNOTSUPP; > } > > /* > @@ -9190,21 +9185,19 @@ int kvm_arch_init(void *opaque) > if (rdmsrl_safe(MSR_IA32_CR_PAT, &host_pat) || > (host_pat & GENMASK(2, 0)) != 6) { > pr_err("kvm: host PAT[0] is not WB\n"); > - r = -EIO; > - goto out; > + return -EIO; > } > > - r = -ENOMEM; > - > x86_emulator_cache = kvm_alloc_emulator_cache(); > if (!x86_emulator_cache) { > pr_err("kvm: failed to allocate cache for x86 emulator\n"); > - goto out; > + return -ENOMEM; > } > > user_return_msrs = alloc_percpu(struct kvm_user_return_msrs); > if (!user_return_msrs) { > printk(KERN_ERR "kvm: failed to allocate percpu kvm_user_return_msrs\n"); > + r = -ENOMEM; > goto out_free_x86_emulator_cache; > } > kvm_nr_uret_msrs = 0; > @@ -9235,7 +9228,6 @@ int kvm_arch_init(void *opaque) > free_percpu(user_return_msrs); > out_free_x86_emulator_cache: > kmem_cache_destroy(x86_emulator_cache); > -out: > return r; > } > I honestly don't see much value in this change, but I don't mind it either. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Mon, Jul 18, 2022, Maxim Levitsky wrote:
> I honestly don't see much value in this change, but I don't mind it either.
Yeah, this particular instance isn't a significant improvement, but I really dislike
the pattern (if the target is a raw return) and want to discourage its use in KVM.
For longer functions, having to scroll down to see that the target is nothing more
than a raw return is quite annoying. And for more complex usage, the pattern sometimes
leads to setting the return value well ahead of the "goto", which combined with the
scrolling is very unfriendly to readers.
E.g. prior to commit 71a4c30bf0d3 ("KVM: Refactor error handling for setting memory
region"), the memslot code input validation was written as so. The "r = 0" in the
"Nothing to change" path was especially gross.
r = -EINVAL;
as_id = mem->slot >> 16;
id = (u16)mem->slot;
/* General sanity checks */
if (mem->memory_size & (PAGE_SIZE - 1))
goto out;
if (mem->guest_phys_addr & (PAGE_SIZE - 1))
goto out;
/* We can read the guest memory with __xxx_user() later on. */
if ((id < KVM_USER_MEM_SLOTS) &&
((mem->userspace_addr & (PAGE_SIZE - 1)) ||
!access_ok((void __user *)(unsigned long)mem->userspace_addr,
mem->memory_size)))
goto out;
if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_MEM_SLOTS_NUM)
goto out;
if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
goto out;
slot = id_to_memslot(__kvm_memslots(kvm, as_id), id);
base_gfn = mem->guest_phys_addr >> PAGE_SHIFT;
npages = mem->memory_size >> PAGE_SHIFT;
if (npages > KVM_MEM_MAX_NR_PAGES)
goto out;
new = old = *slot;
new.id = id;
new.base_gfn = base_gfn;
new.npages = npages;
new.flags = mem->flags;
new.userspace_addr = mem->userspace_addr;
if (npages) {
if (!old.npages)
change = KVM_MR_CREATE;
else { /* Modify an existing slot. */
if ((new.userspace_addr != old.userspace_addr) ||
(npages != old.npages) ||
((new.flags ^ old.flags) & KVM_MEM_READONLY))
goto out;
if (base_gfn != old.base_gfn)
change = KVM_MR_MOVE;
else if (new.flags != old.flags)
change = KVM_MR_FLAGS_ONLY;
else { /* Nothing to change. */
r = 0;
goto out;
}
}
} else {
if (!old.npages)
goto out;
change = KVM_MR_DELETE;
new.base_gfn = 0;
new.flags = 0;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 12199c40f2bc..41aa3137665c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9146,21 +9146,18 @@ int kvm_arch_init(void *opaque) if (kvm_x86_ops.hardware_enable) { pr_err("kvm: already loaded vendor module '%s'\n", kvm_x86_ops.name); - r = -EEXIST; - goto out; + return -EEXIST; } if (!ops->cpu_has_kvm_support()) { pr_err_ratelimited("kvm: no hardware support for '%s'\n", ops->runtime_ops->name); - r = -EOPNOTSUPP; - goto out; + return -EOPNOTSUPP; } if (ops->disabled_by_bios()) { pr_err_ratelimited("kvm: support for '%s' disabled by bios\n", ops->runtime_ops->name); - r = -EOPNOTSUPP; - goto out; + return -EOPNOTSUPP; } /* @@ -9170,14 +9167,12 @@ int kvm_arch_init(void *opaque) */ if (!boot_cpu_has(X86_FEATURE_FPU) || !boot_cpu_has(X86_FEATURE_FXSR)) { printk(KERN_ERR "kvm: inadequate fpu\n"); - r = -EOPNOTSUPP; - goto out; + return -EOPNOTSUPP; } if (IS_ENABLED(CONFIG_PREEMPT_RT) && !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { pr_err("RT requires X86_FEATURE_CONSTANT_TSC\n"); - r = -EOPNOTSUPP; - goto out; + return -EOPNOTSUPP; } /* @@ -9190,21 +9185,19 @@ int kvm_arch_init(void *opaque) if (rdmsrl_safe(MSR_IA32_CR_PAT, &host_pat) || (host_pat & GENMASK(2, 0)) != 6) { pr_err("kvm: host PAT[0] is not WB\n"); - r = -EIO; - goto out; + return -EIO; } - r = -ENOMEM; - x86_emulator_cache = kvm_alloc_emulator_cache(); if (!x86_emulator_cache) { pr_err("kvm: failed to allocate cache for x86 emulator\n"); - goto out; + return -ENOMEM; } user_return_msrs = alloc_percpu(struct kvm_user_return_msrs); if (!user_return_msrs) { printk(KERN_ERR "kvm: failed to allocate percpu kvm_user_return_msrs\n"); + r = -ENOMEM; goto out_free_x86_emulator_cache; } kvm_nr_uret_msrs = 0; @@ -9235,7 +9228,6 @@ int kvm_arch_init(void *opaque) free_percpu(user_return_msrs); out_free_x86_emulator_cache: kmem_cache_destroy(x86_emulator_cache); -out: return r; }
Return directly if kvm_arch_init() detects an error before doing any real work, jumping through a label obfuscates what's happening and carries the unnecessary risk of leaving 'r' uninitialized. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)