Re: [PATCH v3 2/9] KVM: x86: Move init-only kvm_x86_ops to separate struct
Sean Christopherson writes:
> On Mon, Mar 23, 2020 at 01:10:40PM +0100, Vitaly Kuznetsov wrote:
>> Sean Christopherson writes:
>>
>> > +
>> > + .runtime_ops = &svm_x86_ops,
>> > +};
>>
>> Unrelated to your patch but I think we can make the naming of some of
>> these functions more consistend on SVM/VMX, in particular I'd suggest
>>
>> has_svm() -> cpu_has_svm_support()
>> is_disabled -> svm_disabled_by_bios()
>> ...
>> (see below for VMX)
>>
>> > +
>> > static int __init svm_init(void)
>> > {
>> > - return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
>> > + return kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
>> >__alignof__(struct vcpu_svm), THIS_MODULE);
>> > }
>> >
>> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> > index 07299a957d4a..ffcdcc86f5b7 100644
>> > --- a/arch/x86/kvm/vmx/vmx.c
>> > +++ b/arch/x86/kvm/vmx/vmx.c
>> > @@ -7842,11 +7842,8 @@ static bool vmx_check_apicv_inhibit_reasons(ulong
>> > bit)
>> > }
>> >
>> > static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
>> > - .cpu_has_kvm_support = cpu_has_kvm_support,
>> > - .disabled_by_bios = vmx_disabled_by_bios,
>> > - .hardware_setup = hardware_setup,
>> >.hardware_unsetup = hardware_unsetup,
>> > - .check_processor_compatibility = vmx_check_processor_compat,
>> > +
>> >.hardware_enable = hardware_enable,
>> >.hardware_disable = hardware_disable,
>> >.cpu_has_accelerated_tpr = report_flexpriority,
>> > @@ -7981,6 +7978,15 @@ static struct kvm_x86_ops vmx_x86_ops
>> > __ro_after_init = {
>> >.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
>> > };
>> >
>> > +static struct kvm_x86_init_ops vmx_init_ops __initdata = {
>> > + .cpu_has_kvm_support = cpu_has_kvm_support,
>> > + .disabled_by_bios = vmx_disabled_by_bios,
>> > + .check_processor_compatibility = vmx_check_processor_compat,
>> > + .hardware_setup = hardware_setup,
>>
>> cpu_has_kvm_support() -> cpu_has_vmx_support()
>> hardware_setup() -> vmx_hardware_setup()
>
> Preaching to the choir on this one. The VMX functions without prefixes in
> in particular annoy me to no end, e.g. hardware_setup(). Though the worst
> is probably ".vcpu_create = vmx_create_vcpu", if I had a nickel for every
> time I've tried to find vmx_vcpu_create()...
>
> What if we added a macro to auto-generate the common/required hooks? E.g.:
>
> static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
> MANDATORY_KVM_X86_OPS(vmx),
>
> .pmu_ops = &intel_pmu_ops,
>
> ...
> };
>
> That'd enforce consistent naming, and would provide a bit of documentation
> as to which hooks are optional, e.g. many of the nested hooks, and which
> must be defined for KVM to function.
Sounds cool! (not sure that with only two implementations people won't
call it 'over-engineered' but cool). My personal wish would just be that
function names in function implementations are not auto-generated so
e.g. a simple 'git grep vmx_hardware_setup' works but the way how we
fill vmx_x86_ops in can be macroed I guess.
--
Vitaly
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 2/9] KVM: x86: Move init-only kvm_x86_ops to separate struct
On 23/03/20 17:24, Vitaly Kuznetsov wrote: > Sounds cool! (not sure that with only two implementations people won't > call it 'over-engineered' but cool). Yes, something like #define KVM_X86_OP(name) .name = vmx_##name (svm_##name for svm.c) and then KVM_X86_OP(check_nested_events) etc. > My personal wish would just be that > function names in function implementations are not auto-generated so > e.g. a simple 'git grep vmx_hardware_setup' works Yes, absolutely; the function names would still be written by hand. Paolo > but the way how we > fill vmx_x86_ops in can be macroed I guess. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 2/9] KVM: x86: Move init-only kvm_x86_ops to separate struct
On Mon, Mar 23, 2020 at 05:24:56PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson writes:
>
> > On Mon, Mar 23, 2020 at 01:10:40PM +0100, Vitaly Kuznetsov wrote:
> >> Sean Christopherson writes:
> >>
> >> > +
> >> > +.runtime_ops = &svm_x86_ops,
> >> > +};
> >>
> >> Unrelated to your patch but I think we can make the naming of some of
> >> these functions more consistend on SVM/VMX, in particular I'd suggest
> >>
> >> has_svm() -> cpu_has_svm_support()
> >> is_disabled -> svm_disabled_by_bios()
> >> ...
> >> (see below for VMX)
> >>
> >> > +
> >> > static int __init svm_init(void)
> >> > {
> >> > -return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
> >> > +return kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
> >> > __alignof__(struct vcpu_svm), THIS_MODULE);
> >> > }
> >> >
> >> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >> > index 07299a957d4a..ffcdcc86f5b7 100644
> >> > --- a/arch/x86/kvm/vmx/vmx.c
> >> > +++ b/arch/x86/kvm/vmx/vmx.c
> >> > @@ -7842,11 +7842,8 @@ static bool vmx_check_apicv_inhibit_reasons(ulong
> >> > bit)
> >> > }
> >> >
> >> > static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
> >> > -.cpu_has_kvm_support = cpu_has_kvm_support,
> >> > -.disabled_by_bios = vmx_disabled_by_bios,
> >> > -.hardware_setup = hardware_setup,
> >> > .hardware_unsetup = hardware_unsetup,
> >> > -.check_processor_compatibility = vmx_check_processor_compat,
> >> > +
> >> > .hardware_enable = hardware_enable,
> >> > .hardware_disable = hardware_disable,
> >> > .cpu_has_accelerated_tpr = report_flexpriority,
> >> > @@ -7981,6 +7978,15 @@ static struct kvm_x86_ops vmx_x86_ops
> >> > __ro_after_init = {
> >> > .apic_init_signal_blocked = vmx_apic_init_signal_blocked,
> >> > };
> >> >
> >> > +static struct kvm_x86_init_ops vmx_init_ops __initdata = {
> >> > +.cpu_has_kvm_support = cpu_has_kvm_support,
> >> > +.disabled_by_bios = vmx_disabled_by_bios,
> >> > +.check_processor_compatibility = vmx_check_processor_compat,
> >> > +.hardware_setup = hardware_setup,
> >>
> >> cpu_has_kvm_support() -> cpu_has_vmx_support()
> >> hardware_setup() -> vmx_hardware_setup()
> >
> > Preaching to the choir on this one. The VMX functions without prefixes in
> > in particular annoy me to no end, e.g. hardware_setup(). Though the worst
> > is probably ".vcpu_create = vmx_create_vcpu", if I had a nickel for every
> > time I've tried to find vmx_vcpu_create()...
> >
> > What if we added a macro to auto-generate the common/required hooks? E.g.:
> >
> > static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
> > MANDATORY_KVM_X86_OPS(vmx),
> >
> > .pmu_ops = &intel_pmu_ops,
> >
> > ...
> > };
> >
> > That'd enforce consistent naming, and would provide a bit of documentation
> > as to which hooks are optional, e.g. many of the nested hooks, and which
> > must be defined for KVM to function.
>
> Sounds cool! (not sure that with only two implementations people won't
> call it 'over-engineered' but cool). My personal wish would just be that
> function names in function implementations are not auto-generated so
> e.g. a simple 'git grep vmx_hardware_setup' works but the way how we
> fill vmx_x86_ops in can be macroed I guess.
Ya, I was thinking of just the macro. Even that has downsides though, e.g.
chasing kvm_x86_ops.hardware_setup() to find VMX's hardware_setup() becomes
a bit kludgy. On the other hand, _if_ you know how the fill macro works,
getting to the implementation should be easier.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 2/9] KVM: x86: Move init-only kvm_x86_ops to separate struct
On Mon, Mar 23, 2020 at 01:10:40PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson writes:
>
> > +
> > + .runtime_ops = &svm_x86_ops,
> > +};
>
> Unrelated to your patch but I think we can make the naming of some of
> these functions more consistend on SVM/VMX, in particular I'd suggest
>
> has_svm() -> cpu_has_svm_support()
> is_disabled -> svm_disabled_by_bios()
> ...
> (see below for VMX)
>
> > +
> > static int __init svm_init(void)
> > {
> > - return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
> > + return kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
> > __alignof__(struct vcpu_svm), THIS_MODULE);
> > }
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 07299a957d4a..ffcdcc86f5b7 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -7842,11 +7842,8 @@ static bool vmx_check_apicv_inhibit_reasons(ulong
> > bit)
> > }
> >
> > static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
> > - .cpu_has_kvm_support = cpu_has_kvm_support,
> > - .disabled_by_bios = vmx_disabled_by_bios,
> > - .hardware_setup = hardware_setup,
> > .hardware_unsetup = hardware_unsetup,
> > - .check_processor_compatibility = vmx_check_processor_compat,
> > +
> > .hardware_enable = hardware_enable,
> > .hardware_disable = hardware_disable,
> > .cpu_has_accelerated_tpr = report_flexpriority,
> > @@ -7981,6 +7978,15 @@ static struct kvm_x86_ops vmx_x86_ops
> > __ro_after_init = {
> > .apic_init_signal_blocked = vmx_apic_init_signal_blocked,
> > };
> >
> > +static struct kvm_x86_init_ops vmx_init_ops __initdata = {
> > + .cpu_has_kvm_support = cpu_has_kvm_support,
> > + .disabled_by_bios = vmx_disabled_by_bios,
> > + .check_processor_compatibility = vmx_check_processor_compat,
> > + .hardware_setup = hardware_setup,
>
> cpu_has_kvm_support() -> cpu_has_vmx_support()
> hardware_setup() -> vmx_hardware_setup()
Preaching to the choir on this one. The VMX functions without prefixes in
in particular annoy me to no end, e.g. hardware_setup(). Though the worst
is probably ".vcpu_create = vmx_create_vcpu", if I had a nickel for every
time I've tried to find vmx_vcpu_create()...
What if we added a macro to auto-generate the common/required hooks? E.g.:
static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
MANDATORY_KVM_X86_OPS(vmx),
.pmu_ops = &intel_pmu_ops,
...
};
That'd enforce consistent naming, and would provide a bit of documentation
as to which hooks are optional, e.g. many of the nested hooks, and which
must be defined for KVM to function.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 2/9] KVM: x86: Move init-only kvm_x86_ops to separate struct
Sean Christopherson writes:
> Move the kvm_x86_ops functions that are used only within the scope of
> kvm_init() into a separate struct, kvm_x86_init_ops. In addition to
> identifying the init-only functions without restorting to code comments,
> this also sets the stage for waiting until after ->hardware_setup() to
> set kvm_x86_ops. Setting kvm_x86_ops after ->hardware_setup() is
> desirable as many of the hooks are not usable until ->hardware_setup()
> completes.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson
> ---
> arch/x86/include/asm/kvm_host.h | 13 +
> arch/x86/kvm/svm.c | 15 ++-
> arch/x86/kvm/vmx/vmx.c | 16 +++-
> arch/x86/kvm/x86.c | 10 ++
> 4 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9a183e9d4cb1..f4c5b49299ff 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1054,12 +1054,8 @@ static inline u16 kvm_lapic_irq_dest_mode(bool
> dest_mode_logical)
> }
>
> struct kvm_x86_ops {
> - int (*cpu_has_kvm_support)(void); /* __init */
> - int (*disabled_by_bios)(void); /* __init */
> int (*hardware_enable)(void);
> void (*hardware_disable)(void);
> - int (*check_processor_compatibility)(void);/* __init */
> - int (*hardware_setup)(void); /* __init */
> void (*hardware_unsetup)(void);/* __exit */
> bool (*cpu_has_accelerated_tpr)(void);
> bool (*has_emulated_msr)(int index);
> @@ -1260,6 +1256,15 @@ struct kvm_x86_ops {
> int (*enable_direct_tlbflush)(struct kvm_vcpu *vcpu);
> };
>
> +struct kvm_x86_init_ops {
> + int (*cpu_has_kvm_support)(void);
> + int (*disabled_by_bios)(void);
> + int (*check_processor_compatibility)(void);
> + int (*hardware_setup)(void);
> +
> + struct kvm_x86_ops *runtime_ops;
> +};
> +
> struct kvm_arch_async_pf {
> u32 token;
> gfn_t gfn;
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2125c6ae5951..33e67c3389c2 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7351,11 +7351,7 @@ static void svm_pre_update_apicv_exec_ctrl(struct kvm
> *kvm, bool activate)
> }
>
> static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
> - .cpu_has_kvm_support = has_svm,
> - .disabled_by_bios = is_disabled,
> - .hardware_setup = svm_hardware_setup,
> .hardware_unsetup = svm_hardware_teardown,
> - .check_processor_compatibility = svm_check_processor_compat,
> .hardware_enable = svm_hardware_enable,
> .hardware_disable = svm_hardware_disable,
> .cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr,
> @@ -7480,9 +7476,18 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init
> = {
> .check_nested_events = svm_check_nested_events,
> };
>
> +static struct kvm_x86_init_ops svm_init_ops __initdata = {
> + .cpu_has_kvm_support = has_svm,
> + .disabled_by_bios = is_disabled,
> + .hardware_setup = svm_hardware_setup,
> + .check_processor_compatibility = svm_check_processor_compat,
> +
> + .runtime_ops = &svm_x86_ops,
> +};
Unrelated to your patch but I think we can make the naming of some of
these functions more consistend on SVM/VMX, in particular I'd suggest
has_svm() -> cpu_has_svm_support()
is_disabled -> svm_disabled_by_bios()
...
(see below for VMX)
> +
> static int __init svm_init(void)
> {
> - return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
> + return kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
> __alignof__(struct vcpu_svm), THIS_MODULE);
> }
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 07299a957d4a..ffcdcc86f5b7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7842,11 +7842,8 @@ static bool vmx_check_apicv_inhibit_reasons(ulong bit)
> }
>
> static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
> - .cpu_has_kvm_support = cpu_has_kvm_support,
> - .disabled_by_bios = vmx_disabled_by_bios,
> - .hardware_setup = hardware_setup,
> .hardware_unsetup = hardware_unsetup,
> - .check_processor_compatibility = vmx_check_processor_compat,
> +
> .hardware_enable = hardware_enable,
> .hardware_disable = hardware_disable,
> .cpu_has_accelerated_tpr = report_flexpriority,
> @@ -7981,6 +7978,15 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init
> = {
> .apic_init_signal_blocked = vmx_apic_init_signal_blocked,
> };
>
> +static struct kvm_x86_init_ops vmx_init_ops __initdata = {
> + .cpu_has_kvm_support = cpu_has_kvm_support,
> + .disabled_by_bios = vmx_disabled_by_bios,
> + .check_processor_compatibility = vmx_check_processor_compat,
> + .hardware_setup = hardware_setup,
cpu_has_kvm_support() -> cpu_has_vmx_support()
hardware_setup() -
[PATCH v3 2/9] KVM: x86: Move init-only kvm_x86_ops to separate struct
Move the kvm_x86_ops functions that are used only within the scope of
kvm_init() into a separate struct, kvm_x86_init_ops. In addition to
identifying the init-only functions without restorting to code comments,
this also sets the stage for waiting until after ->hardware_setup() to
set kvm_x86_ops. Setting kvm_x86_ops after ->hardware_setup() is
desirable as many of the hooks are not usable until ->hardware_setup()
completes.
No functional change intended.
Signed-off-by: Sean Christopherson
---
arch/x86/include/asm/kvm_host.h | 13 +
arch/x86/kvm/svm.c | 15 ++-
arch/x86/kvm/vmx/vmx.c | 16 +++-
arch/x86/kvm/x86.c | 10 ++
4 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9a183e9d4cb1..f4c5b49299ff 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1054,12 +1054,8 @@ static inline u16 kvm_lapic_irq_dest_mode(bool
dest_mode_logical)
}
struct kvm_x86_ops {
- int (*cpu_has_kvm_support)(void); /* __init */
- int (*disabled_by_bios)(void); /* __init */
int (*hardware_enable)(void);
void (*hardware_disable)(void);
- int (*check_processor_compatibility)(void);/* __init */
- int (*hardware_setup)(void); /* __init */
void (*hardware_unsetup)(void);/* __exit */
bool (*cpu_has_accelerated_tpr)(void);
bool (*has_emulated_msr)(int index);
@@ -1260,6 +1256,15 @@ struct kvm_x86_ops {
int (*enable_direct_tlbflush)(struct kvm_vcpu *vcpu);
};
+struct kvm_x86_init_ops {
+ int (*cpu_has_kvm_support)(void);
+ int (*disabled_by_bios)(void);
+ int (*check_processor_compatibility)(void);
+ int (*hardware_setup)(void);
+
+ struct kvm_x86_ops *runtime_ops;
+};
+
struct kvm_arch_async_pf {
u32 token;
gfn_t gfn;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2125c6ae5951..33e67c3389c2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7351,11 +7351,7 @@ static void svm_pre_update_apicv_exec_ctrl(struct kvm
*kvm, bool activate)
}
static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
- .cpu_has_kvm_support = has_svm,
- .disabled_by_bios = is_disabled,
- .hardware_setup = svm_hardware_setup,
.hardware_unsetup = svm_hardware_teardown,
- .check_processor_compatibility = svm_check_processor_compat,
.hardware_enable = svm_hardware_enable,
.hardware_disable = svm_hardware_disable,
.cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr,
@@ -7480,9 +7476,18 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
.check_nested_events = svm_check_nested_events,
};
+static struct kvm_x86_init_ops svm_init_ops __initdata = {
+ .cpu_has_kvm_support = has_svm,
+ .disabled_by_bios = is_disabled,
+ .hardware_setup = svm_hardware_setup,
+ .check_processor_compatibility = svm_check_processor_compat,
+
+ .runtime_ops = &svm_x86_ops,
+};
+
static int __init svm_init(void)
{
- return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
+ return kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
__alignof__(struct vcpu_svm), THIS_MODULE);
}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 07299a957d4a..ffcdcc86f5b7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7842,11 +7842,8 @@ static bool vmx_check_apicv_inhibit_reasons(ulong bit)
}
static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
- .cpu_has_kvm_support = cpu_has_kvm_support,
- .disabled_by_bios = vmx_disabled_by_bios,
- .hardware_setup = hardware_setup,
.hardware_unsetup = hardware_unsetup,
- .check_processor_compatibility = vmx_check_processor_compat,
+
.hardware_enable = hardware_enable,
.hardware_disable = hardware_disable,
.cpu_has_accelerated_tpr = report_flexpriority,
@@ -7981,6 +7978,15 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
};
+static struct kvm_x86_init_ops vmx_init_ops __initdata = {
+ .cpu_has_kvm_support = cpu_has_kvm_support,
+ .disabled_by_bios = vmx_disabled_by_bios,
+ .check_processor_compatibility = vmx_check_processor_compat,
+ .hardware_setup = hardware_setup,
+
+ .runtime_ops = &vmx_x86_ops,
+};
+
static void vmx_cleanup_l1d_flush(void)
{
if (vmx_l1d_flush_pages) {
@@ -8065,7 +8071,7 @@ static int __init vmx_init(void)
}
#endif
- r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
+ r = kvm_init(&vmx_init_ops, sizeof(struct vcpu_vmx),
__alignof__(struct vcpu_vmx), THIS_MODULE);
if (r)
return r;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86
