Re: [PATCH 02/17] KVM: monolithic: x86: convert the kvm_x86_ops methods to external functions

2019-09-23 Thread Andrea Arcangeli
On Mon, Sep 23, 2019 at 12:19:30PM +0200, Paolo Bonzini wrote:
> On 20/09/19 23:24, Andrea Arcangeli wrote:
> > diff --git a/arch/x86/kvm/svm_ops.c b/arch/x86/kvm/svm_ops.c
> > new file mode 100644
> > index ..2aaabda92179
> > --- /dev/null
> > +++ b/arch/x86/kvm/svm_ops.c
> > @@ -0,0 +1,672 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + *  arch/x86/kvm/svm_ops.c
> > + *
> > + *  Copyright 2019 Red Hat, Inc.
> > + */
> > +
> > +int kvm_x86_ops_cpu_has_kvm_support(void)
> > +{
> > +   return has_svm();
> > +}
> 
> Can you just rename all the functions in vmx/ and svm.c, instead of
> adding forwarders?

I can do that, I thought this was cleaner as it still retained the
abstraction separated from the mixup of the rest of the vmx/svm code,
but it'll work the same by dropping the abstraction in kvm_ops.h and
just maintaining a common name between the svm.c and vmx.c files, gcc
already built it that way after all.


Re: [PATCH 02/17] KVM: monolithic: x86: convert the kvm_x86_ops methods to external functions

2019-09-23 Thread Paolo Bonzini
On 23/09/19 18:13, Sean Christopherson wrote:
> Alternatively, what if we use macros in the call sites, e.g. keep/require
> vmx_ and svm_ prefixes for all functions, renaming VMX and SVM code as
> needed?  E.g.:
> 
> 
>   #define X86_OP(name) kvm_x86_vendor##_##name
> 
>   int kvm_arch_init(void *opaque)
>   {
>   if (X86_OP(supported_by_cpu())) {

Please no, the extra parentheses would be a mess to review.

Paolo


Re: [PATCH 02/17] KVM: monolithic: x86: convert the kvm_x86_ops methods to external functions

2019-09-23 Thread Sean Christopherson
On Mon, Sep 23, 2019 at 12:19:30PM +0200, Paolo Bonzini wrote:
> On 20/09/19 23:24, Andrea Arcangeli wrote:
> > diff --git a/arch/x86/kvm/svm_ops.c b/arch/x86/kvm/svm_ops.c
> > new file mode 100644
> > index ..2aaabda92179
> > --- /dev/null
> > +++ b/arch/x86/kvm/svm_ops.c
> > @@ -0,0 +1,672 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + *  arch/x86/kvm/svm_ops.c
> > + *
> > + *  Copyright 2019 Red Hat, Inc.
> > + */
> > +
> > +int kvm_x86_ops_cpu_has_kvm_support(void)
> > +{
> > +   return has_svm();
> > +}
> 
> Can you just rename all the functions in vmx/ and svm.c, instead of
> adding forwarders?

Yeah, having kvm_x86_ be analogous to kvm_arch_ seems like the obvious
approach.  The necessary VMX and SVM renaming can be done in separate
preparatory patches, and the conversion from kvm_x86_ops to direct calls
would be fairly straightforward.

Alternatively, what if we use macros in the call sites, e.g. keep/require
vmx_ and svm_ prefixes for all functions, renaming VMX and SVM code as
needed?  E.g.:

  cpu_has_vmx_support -> vmx_supported_by_cpu 
  cpu_has_svm_support -> svm_supported_by_cpu

  int vmx_disabled_by_bios(void)
  int svm_disabled_by_bios(void)


  #define X86_OP(name) kvm_x86_vendor##_##name

  int kvm_arch_init(void *opaque)
  {
if (X86_OP(supported_by_cpu())) {
printk(KERN_ERR "kvm: no hardware support\n");
r = -EOPNOTSUPP;
goto out;
}
if (X86_OP(disabled_by_bios())) {
printk(KERN_ERR "kvm: disabled by bios\n");
r = -EOPNOTSUPP;
goto out;
}   
  }

Pros:
  - Smaller patches due to less renaming in VMX and SVM
  - Calls to vendor code are very obvious
  - Stack traces contain vmx vs. svm instead of kvm_x86

Cons:
  - Macros
  - Annoying development environment, e.g. editors tend to struggle with
macrofied funtion/variable names.


Re: [PATCH 02/17] KVM: monolithic: x86: convert the kvm_x86_ops methods to external functions

2019-09-23 Thread Paolo Bonzini
On 20/09/19 23:24, Andrea Arcangeli wrote:
> diff --git a/arch/x86/kvm/svm_ops.c b/arch/x86/kvm/svm_ops.c
> new file mode 100644
> index ..2aaabda92179
> --- /dev/null
> +++ b/arch/x86/kvm/svm_ops.c
> @@ -0,0 +1,672 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  arch/x86/kvm/svm_ops.c
> + *
> + *  Copyright 2019 Red Hat, Inc.
> + */
> +
> +int kvm_x86_ops_cpu_has_kvm_support(void)
> +{
> + return has_svm();
> +}

Can you just rename all the functions in vmx/ and svm.c, instead of
adding forwarders?

Thanks,

Paolo

> +int kvm_x86_ops_disabled_by_bios(void)
> +{
> + return is_disabled();
> +}
> +
> +int kvm_x86_ops_hardware_enable(void)
> +{
> + return svm_hardware_enable();
> +}
> +
> +void kvm_x86_ops_hardware_disable(void)
> +{
> + svm_hardware_disable();
> +}
> +
> +__init int kvm_x86_ops_check_processor_compatibility(void)
> +{
> + return svm_check_processor_compat();
> +}
> +
> +__init int kvm_x86_ops_hardware_setup(void)
> +{
> + return svm_hardware_setup();
> +}
> +
> +void kvm_x86_ops_hardware_unsetup(void)
> +{
> + svm_hardware_unsetup();
> +}
> +
> +bool kvm_x86_ops_cpu_has_accelerated_tpr(void)
> +{
> + return svm_cpu_has_accelerated_tpr();
> +}
> +
> +bool kvm_x86_ops_has_emulated_msr(int index)
> +{
> + return svm_has_emulated_msr(index);
> +}
> +
> +void kvm_x86_ops_cpuid_update(struct kvm_vcpu *vcpu)
> +{
> + svm_cpuid_update(vcpu);
> +}
> +
> +struct kvm *kvm_x86_ops_vm_alloc(void)
> +{
> + return svm_vm_alloc();
> +}
> +
> +void kvm_x86_ops_vm_free(struct kvm *kvm)
> +{
> + svm_vm_free(kvm);
> +}
> +
> +int kvm_x86_ops_vm_init(struct kvm *kvm)
> +{
> + return avic_vm_init(kvm);
> +}
> +
> +void kvm_x86_ops_vm_destroy(struct kvm *kvm)
> +{
> + svm_vm_destroy(kvm);
> +}
> +
> +struct kvm_vcpu *kvm_x86_ops_vcpu_create(struct kvm *kvm, unsigned id)
> +{
> + return svm_create_vcpu(kvm, id);
> +}
> +
> +void kvm_x86_ops_vcpu_free(struct kvm_vcpu *vcpu)
> +{
> + svm_free_vcpu(vcpu);
> +}
> +
> +void kvm_x86_ops_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> +{
> + svm_vcpu_reset(vcpu, init_event);
> +}
> +
> +void kvm_x86_ops_prepare_guest_switch(struct kvm_vcpu *vcpu)
> +{
> + svm_prepare_guest_switch(vcpu);
> +}
> +
> +void kvm_x86_ops_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> +{
> + svm_vcpu_load(vcpu, cpu);
> +}
> +
> +void kvm_x86_ops_vcpu_put(struct kvm_vcpu *vcpu)
> +{
> + svm_vcpu_put(vcpu);
> +}
> +
> +void kvm_x86_ops_update_bp_intercept(struct kvm_vcpu *vcpu)
> +{
> + update_bp_intercept(vcpu);
> +}
> +
> +int kvm_x86_ops_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> +{
> + return svm_get_msr(vcpu, msr);
> +}
> +
> +int kvm_x86_ops_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> +{
> + return svm_set_msr(vcpu, msr);
> +}
> +
> +u64 kvm_x86_ops_get_segment_base(struct kvm_vcpu *vcpu, int seg)
> +{
> + return svm_get_segment_base(vcpu, seg);
> +}
> +
> +void kvm_x86_ops_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var,
> +  int seg)
> +{
> + svm_get_segment(vcpu, var, seg);
> +}
> +
> +int kvm_x86_ops_get_cpl(struct kvm_vcpu *vcpu)
> +{
> + return svm_get_cpl(vcpu);
> +}
> +
> +void kvm_x86_ops_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var,
> +  int seg)
> +{
> + svm_set_segment(vcpu, var, seg);
> +}
> +
> +void kvm_x86_ops_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l)
> +{
> + kvm_get_cs_db_l_bits(vcpu, db, l);
> +}
> +
> +void kvm_x86_ops_decache_cr0_guest_bits(struct kvm_vcpu *vcpu)
> +{
> + svm_decache_cr0_guest_bits(vcpu);
> +}
> +
> +void kvm_x86_ops_decache_cr3(struct kvm_vcpu *vcpu)
> +{
> + svm_decache_cr3(vcpu);
> +}
> +
> +void kvm_x86_ops_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
> +{
> + svm_decache_cr4_guest_bits(vcpu);
> +}
> +
> +void kvm_x86_ops_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
> +{
> + svm_set_cr0(vcpu, cr0);
> +}
> +
> +void kvm_x86_ops_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> +{
> + svm_set_cr3(vcpu, cr3);
> +}
> +
> +int kvm_x86_ops_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> +{
> + return svm_set_cr4(vcpu, cr4);
> +}
> +
> +void kvm_x86_ops_set_efer(struct kvm_vcpu *vcpu, u64 efer)
> +{
> + svm_set_efer(vcpu, efer);
> +}
> +
> +void kvm_x86_ops_get_idt(struct kvm_vcpu *vcpu, struct desc_ptr *dt)
> +{
> + svm_get_idt(vcpu, dt);
> +}
> +
> +void kvm_x86_ops_set_idt(struct kvm_vcpu *vcpu, struct desc_ptr *dt)
> +{
> + svm_set_idt(vcpu, dt);
> +}
> +
> +void kvm_x86_ops_get_gdt(struct kvm_vcpu *vcpu, struct desc_ptr *dt)
> +{
> + svm_get_gdt(vcpu, dt);
> +}
> +
> +void kvm_x86_ops_set_gdt(struct kvm_vcpu *vcpu, struct desc_ptr *dt)
> +{
> + svm_set_gdt(vcpu, dt);
> +}
> +
> +u64 kvm_x86_ops_get_dr6(struct kvm_vcpu *vcpu)
> +{
> + return svm_get_dr6(vcpu);
> +}
> +
> +void kvm_x86_ops_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
> +{
> +