2019-11-04 23:01:27

by Andrea Arcangeli

[permalink] [raw]
Subject: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

kvm_x86_set_hv_timer and kvm_x86_cancel_hv_timer needs to be defined
to succeed the 32bit kernel build, but they can't be called.

Signed-off-by: Andrea Arcangeli <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bd17ad61f7e3..1a58ae38c8f2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7195,6 +7195,17 @@ void kvm_x86_cancel_hv_timer(struct kvm_vcpu *vcpu)
{
to_vmx(vcpu)->hv_deadline_tsc = -1;
}
+#else
+int kvm_x86_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
+ bool *expired)
+{
+ BUG();
+}
+
+void kvm_x86_cancel_hv_timer(struct kvm_vcpu *vcpu)
+{
+ BUG();
+}
#endif

void kvm_x86_sched_in(struct kvm_vcpu *vcpu, int cpu)


2019-11-05 10:05:31

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 04/11/19 23:59, Andrea Arcangeli wrote:
> kvm_x86_set_hv_timer and kvm_x86_cancel_hv_timer needs to be defined
> to succeed the 32bit kernel build, but they can't be called.
>
> Signed-off-by: Andrea Arcangeli <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index bd17ad61f7e3..1a58ae38c8f2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7195,6 +7195,17 @@ void kvm_x86_cancel_hv_timer(struct kvm_vcpu *vcpu)
> {
> to_vmx(vcpu)->hv_deadline_tsc = -1;
> }
> +#else
> +int kvm_x86_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
> + bool *expired)
> +{
> + BUG();
> +}
> +
> +void kvm_x86_cancel_hv_timer(struct kvm_vcpu *vcpu)
> +{
> + BUG();
> +}
> #endif
>
> void kvm_x86_sched_in(struct kvm_vcpu *vcpu, int cpu)
>

I'll check for how long this has been broken. It may be the proof that
we can actually drop 32-bit KVM support.

Paolo

2019-11-05 10:40:54

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 05/11/19 11:04, Paolo Bonzini wrote:
> On 04/11/19 23:59, Andrea Arcangeli wrote:
>> kvm_x86_set_hv_timer and kvm_x86_cancel_hv_timer needs to be defined
>> to succeed the 32bit kernel build, but they can't be called.
>>
>> Signed-off-by: Andrea Arcangeli <[email protected]>
>> ---
>> arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index bd17ad61f7e3..1a58ae38c8f2 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7195,6 +7195,17 @@ void kvm_x86_cancel_hv_timer(struct kvm_vcpu *vcpu)
>> {
>> to_vmx(vcpu)->hv_deadline_tsc = -1;
>> }
>> +#else
>> +int kvm_x86_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
>> + bool *expired)
>> +{
>> + BUG();
>> +}
>> +
>> +void kvm_x86_cancel_hv_timer(struct kvm_vcpu *vcpu)
>> +{
>> + BUG();
>> +}
>> #endif
>>
>> void kvm_x86_sched_in(struct kvm_vcpu *vcpu, int cpu)
>>
>
> I'll check for how long this has been broken. It may be the proof that
> we can actually drop 32-bit KVM support.

Ah no, I was confused because this series is not bisectable (in addition
to doing two things at a same time, namely the monolithic kvm.ko and the
retpoline eliminations).

I have picked up the patches that are independent of the monolithic
kvm.ko work or can be considered bugfixes.

For the rest, please do this before posting again:

- ensure that everything is bisectable

- look into how to remove the modpost warnings. A simple (though
somewhat ugly) way is to keep a kvm.ko module that includes common
virt/kvm/ code as well as, for x86 only, page_track.o. A few functions,
such as kvm_mmu_gfn_disallow_lpage and kvm_mmu_gfn_allow_lpage, would
have to be moved into mmu.h, but that's not a big deal.

- provide at least some examples of replacing the NULL kvm_x86_ops
checks with error codes in the function (or just early "return"s). I
can help with the others, but remember that for the patch to be merged,
kvm_x86_ops must be removed completely.

Thanks,

Paolo

2019-11-05 13:55:41

by Andrea Arcangeli

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On Tue, Nov 05, 2019 at 11:37:47AM +0100, Paolo Bonzini wrote:
> For the rest, please do this before posting again:
>
> - ensure that everything is bisectable

x86-64 is already bisectable.

All other archs bisectable I didn't check them all anyway.

Even 4/13 is suboptimal and needs to be re-done later in more optimal
way. I prefer all logic changes to happen at later steps so one can at
least bisect to something that functionally works like before. And
4/13 also would need to be merged in the huge patch if one wants to
guarantee bisectability on all CPUs, but it'll just be hidden there in
the huge patch.

Obviously I can squash both 3/13 and 4/13 into 2/13 but I don't feel
like doing the right thing by squashing them just to increase
bisectability.

> - look into how to remove the modpost warnings. A simple (though
> somewhat ugly) way is to keep a kvm.ko module that includes common
> virt/kvm/ code as well as, for x86 only, page_track.o. A few functions,
> such as kvm_mmu_gfn_disallow_lpage and kvm_mmu_gfn_allow_lpage, would
> have to be moved into mmu.h, but that's not a big deal.

I think we should:

1) whitelist to shut off the warnings on demand

2) verify that if two modules are registering the same export symbol
the second one fails to load and the module code is robust about
that, this hopefully should already be the case

Provided verification of 2), the whitelist is more efficient than
losing 4k of ram in all KVM hypervisors out there.

> - provide at least some examples of replacing the NULL kvm_x86_ops
> checks with error codes in the function (or just early "return"s). I
> can help with the others, but remember that for the patch to be merged,
> kvm_x86_ops must be removed completely.

Even if kvm_x86_ops wouldn't be guaranteed to go away, this would
already provide all the performance benefit to the KVM users, so I
wouldn't see a reason not to apply it even if kvm_x86_ops cannot go
away. Said that it will go away and there's no concern about it. It's
just that the patchset seems large enough already and it rejects
heavily already at every port. I simply stopped at the first self
contained step that provides all performance benefits.

If I go ahead and remove kvm_x86_ops how do I know it won't reject
heavily the next day I rebase and I've to redo it all from scratch? If
you explain me how you're going to guarantee that I won't have to do
that work more than once I'd be happy to go ahead.

Thanks,
Andrea

2019-11-05 14:14:24

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 05/11/19 14:54, Andrea Arcangeli wrote:
> x86-64 is already bisectable.
>
> All other archs bisectable I didn't check them all anyway.
>
> Even 4/13 is suboptimal and needs to be re-done later in more optimal
> way. I prefer all logic changes to happen at later steps so one can at
> least bisect to something that functionally works like before. And
> 4/13 also would need to be merged in the huge patch if one wants to
> guarantee bisectability on all CPUs, but it'll just be hidden there in
> the huge patch.
>
> Obviously I can squash both 3/13 and 4/13 into 2/13 but I don't feel
> like doing the right thing by squashing them just to increase
> bisectability.

You can reorder patches so that kvm_x86_ops assignments never happen.
That way, 4/13 for example would be moved to the very beginning.

>> - look into how to remove the modpost warnings. A simple (though
>> somewhat ugly) way is to keep a kvm.ko module that includes common
>> virt/kvm/ code as well as, for x86 only, page_track.o. A few functions,
>> such as kvm_mmu_gfn_disallow_lpage and kvm_mmu_gfn_allow_lpage, would
>> have to be moved into mmu.h, but that's not a big deal.
>
> I think we should:
>
> 1) whitelist to shut off the warnings on demand

Do you mean adding a whitelist to modpost? That would work, though I am
not sure if the module maintainer (Jessica Yu) would accept that.

> 2) verify that if two modules are registering the same export symbol
> the second one fails to load and the module code is robust about
> that, this hopefully should already be the case
>
> Provided verification of 2), the whitelist is more efficient than
> losing 4k of ram in all KVM hypervisors out there.

I agree.

>> - provide at least some examples of replacing the NULL kvm_x86_ops
>> checks with error codes in the function (or just early "return"s). I
>> can help with the others, but remember that for the patch to be merged,
>> kvm_x86_ops must be removed completely.
>
> Even if kvm_x86_ops wouldn't be guaranteed to go away, this would
> already provide all the performance benefit to the KVM users, so I
> wouldn't see a reason not to apply it even if kvm_x86_ops cannot go
> away.

The answer is maintainability. My suggestion is that we start looking
into removing all assignments and tests of kvm_x86_ops, one step at a
time. Until this is done, unfortunately we won't be able to reap the
performance benefit. But the advantage is that this can be done in many
separate submissions; it doesn't have to be one huge patch.

Once this is done, removing kvm_x86_ops is trivial in the end. It's
okay if the intermediate step has minimal performance regressions, we
know what it will look like. I have to order patches with maintenance
first and performance second, if possible.

By the way, we are already planning to make some module parameters
per-VM instead of global, so this refactoring would also help that effort.

> Said that it will go away and there's no concern about it. It's
> just that the patchset seems large enough already and it rejects
> heavily already at every port. I simply stopped at the first self
> contained step that provides all performance benefits.

That is good enough to prove the feasibility of the idea, so I agree
that was a good plan.

Paolo

> If I go ahead and remove kvm_x86_ops how do I know it won't reject
> heavily the next day I rebase and I've to redo it all from scratch? If
> you explain me how you're going to guarantee that I won't have to do
> that work more than once I'd be happy to go ahead.

2019-11-05 15:01:23

by Andrea Arcangeli

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On Tue, Nov 05, 2019 at 03:09:37PM +0100, Paolo Bonzini wrote:
> You can reorder patches so that kvm_x86_ops assignments never happen.
> That way, 4/13 for example would be moved to the very beginning.

Ok 3/13 and 4/14 can both go before 2/13, I reordered them fine, the
end result at least is the same and the intermediate result should
improve. Hopefully this is the best solution for those two outliers.

Once this is committed I expect Sean to take over 4/14 with a more
optimal version to get rid of that branch like he proposed initially.

> >> - look into how to remove the modpost warnings. A simple (though
> >> somewhat ugly) way is to keep a kvm.ko module that includes common
> >> virt/kvm/ code as well as, for x86 only, page_track.o. A few functions,
> >> such as kvm_mmu_gfn_disallow_lpage and kvm_mmu_gfn_allow_lpage, would
> >> have to be moved into mmu.h, but that's not a big deal.
> >
> > I think we should:
> >
> > 1) whitelist to shut off the warnings on demand
>
> Do you mean adding a whitelist to modpost? That would work, though I am
> not sure if the module maintainer (Jessica Yu) would accept that.

Yes that's exactly what I meant.

> > 2) verify that if two modules are registering the same export symbol
> > the second one fails to load and the module code is robust about
> > that, this hopefully should already be the case
> >
> > Provided verification of 2), the whitelist is more efficient than
> > losing 4k of ram in all KVM hypervisors out there.
>
> I agree.

Ok, so I enlarged the CC list accordingly to check how the whitelist
can be done in modpost.

> >> - provide at least some examples of replacing the NULL kvm_x86_ops
> >> checks with error codes in the function (or just early "return"s). I
> >> can help with the others, but remember that for the patch to be merged,
> >> kvm_x86_ops must be removed completely.
> >
> > Even if kvm_x86_ops wouldn't be guaranteed to go away, this would
> > already provide all the performance benefit to the KVM users, so I
> > wouldn't see a reason not to apply it even if kvm_x86_ops cannot go
> > away.
>
> The answer is maintainability. My suggestion is that we start looking
> into removing all assignments and tests of kvm_x86_ops, one step at a
> time. Until this is done, unfortunately we won't be able to reap the
> performance benefit. But the advantage is that this can be done in many

There's not much performance benefit left from the removal
kvm_x86_ops. It'll only remove a few branches at best (and only if we
don't have to replace the branches on the pointer check with other
branches on a static variable to disambiguate the different cases).

> separate submissions; it doesn't have to be one huge patch.
>
> Once this is done, removing kvm_x86_ops is trivial in the end. It's
> okay if the intermediate step has minimal performance regressions, we
> know what it will look like. I have to order patches with maintenance
> first and performance second, if possible.

The removal of kvm_x86_ops is just a badly needed code cleanup and of
course I agree it must happen sooner than later. I'm just trying to
avoid running into rejects on those further commit cleanups too.

> By the way, we are already planning to make some module parameters
> per-VM instead of global, so this refactoring would also help that effort.
>
> > Said that it will go away and there's no concern about it. It's
> > just that the patchset seems large enough already and it rejects
> > heavily already at every port. I simply stopped at the first self
> > contained step that provides all performance benefits.
>
> That is good enough to prove the feasibility of the idea, so I agree
> that was a good plan.

All right, so I'm not exactly sure what's the plan and if it's ok to
do it over time or if I should go ahead doing all logic changes while
the big patch remains out of tree.

If you apply it and reorder 4/13 and 3/13 before 2/13 in a rebase like
I did locally, it should already be good starting point in my view and
the modpost also can be fixed over time too, the warnings appears
harmless so far.

Thanks,
Andrea

2019-11-05 15:12:04

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 05/11/19 15:56, Andrea Arcangeli wrote:
>>> I think we should:
>>>
>>> 1) whitelist to shut off the warnings on demand
>>
>> Do you mean adding a whitelist to modpost? That would work, though I am
>> not sure if the module maintainer (Jessica Yu) would accept that.
>
> Yes that's exactly what I meant.

Ok, thanks. Jessica, the issue here is that we have two (mutually
exclusive) modules providing the same interface to a third module.

Andrea will check that, when the same symbol is exported by two modules,
the second-loaded module correctly fails insmod. If that is okay, we
will also need modpost not to warn for these symbols in sym_add_exported.

>> The answer is maintainability. My suggestion is that we start looking
>> into removing all assignments and tests of kvm_x86_ops, one step at a
>> time. Until this is done, unfortunately we won't be able to reap the
>> performance benefit. But the advantage is that this can be done in many
>
> There's not much performance benefit left from the removal
> kvm_x86_ops.

Indeed; what I mean is that until then we will have to keep the
retpolines. Not removing kvm_x86_ops leaves an unsustainable mess in
terms of maintainability, therefore we will need to first refactor the
code. Once the refactoring is over, kvm_x86_ops can be dropped easily,
just like kvm_pmu_ops in this version of the series.

The good thing is that the modpost discussion can proceed in parallel.

> The removal of kvm_x86_ops is just a badly needed code cleanup and of
> course I agree it must happen sooner than later. I'm just trying to
> avoid running into rejects on those further commit cleanups too.

>> That is good enough to prove the feasibility of the idea, so I agree
>> that was a good plan.
>
> All right, so I'm not exactly sure what's the plan and if it's ok to
> do it over time or if I should go ahead doing all logic changes while
> the big patch remains out of tree.

Yes, the changes to remove tests and assignments to kvm_x86_ops must
happen first. I understand that the big patch is a conflict magnet, but
once all the refactoring is done it will be very easy to review and it
will get in quickly.

Paolo

2019-11-08 13:58:40

by Jessica Yu

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

+++ Paolo Bonzini [05/11/19 16:10 +0100]:
>On 05/11/19 15:56, Andrea Arcangeli wrote:
>>>> I think we should:
>>>>
>>>> 1) whitelist to shut off the warnings on demand
>>>
>>> Do you mean adding a whitelist to modpost? That would work, though I am
>>> not sure if the module maintainer (Jessica Yu) would accept that.
>>
>> Yes that's exactly what I meant.
>
>Ok, thanks. Jessica, the issue here is that we have two (mutually
>exclusive) modules providing the same interface to a third module.

>Andrea will check that, when the same symbol is exported by two modules,
>the second-loaded module correctly fails insmod.

Hi Paolo, thanks for getting me up to speed.

The module loader already rejects loading a module with
duplicate exported symbols.

> If that is okay, we will also need modpost not to warn for these
> symbols in sym_add_exported.

I think it's certainly doable in modpost, for example we could pass a
list of whitelisted symbols and have modpost read them in and not warn
if it encounters the whitelisted symbols more than once. Modpost will
also have to be modified to accomodate duplicate symbols. I'm not
sure how ugly this would be without seeing the actual patch. And I am
not sure what Masahiro (who takes care of all things kbuild-related)
thinks of this idea. But before implementing all this, is there
absolutely no way around having the duplicated exported symbols? (e.g.,
could the modules be configured/built in a mutally exclusive way? I'm
lacking the context from the rest of the thread, so not sure which are
the problematic modules.)

Thanks,

Jessica

2019-11-08 19:52:13

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 08/11/19 14:56, Jessica Yu wrote:
> And I am
> not sure what Masahiro (who takes care of all things kbuild-related)
> thinks of this idea. But before implementing all this, is there
> absolutely no way around having the duplicated exported symbols? (e.g.,
> could the modules be configured/built in a mutally exclusive way? I'm
> lacking the context from the rest of the thread, so not sure which are
> the problematic modules.)

The problematic modules are kvm_intel and kvm_amd, so we cannot build
them in a mutually exclusive way (but we know it won't make sense to
load both). We will have to build only one of them when built into
vmlinux, but the module case must support building both.

Currently we put the common symbols in kvm.ko, and kvm.ko acts as a kind
of "library" for kvm_intel.ko and kvm_amd.ko. The problem is that
kvm_intel.ko and kvm_amd.ko currently pass a large array of function
pointers to kvm.ko, and Andrea measured a substantial performance
penalty from retpolines when kvm.ko calls back through those pointers.

Therefore he would like to remove kvm.ko, and that would result in
symbols exported from two modules.

I suppose we could use code patching mechanism to avoid the retpolines.
Andrea, what do you think about that? That would have the advantage
that we won't have to remove kvm_x86_ops. :)

Thanks,

Paolo

2019-11-08 20:04:28

by Andrea Arcangeli

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On Fri, Nov 08, 2019 at 08:51:04PM +0100, Paolo Bonzini wrote:
> I suppose we could use code patching mechanism to avoid the retpolines.
> Andrea, what do you think about that? That would have the advantage
> that we won't have to remove kvm_x86_ops. :)

page 17 covers pvops:

https://people.redhat.com/~aarcange/slides/2019-KVM-monolithic.pdf

2019-11-08 21:04:05

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 08/11/19 21:01, Andrea Arcangeli wrote:
> On Fri, Nov 08, 2019 at 08:51:04PM +0100, Paolo Bonzini wrote:
>> I suppose we could use code patching mechanism to avoid the retpolines.
>> Andrea, what do you think about that? That would have the advantage
>> that we won't have to remove kvm_x86_ops. :)
>
> page 17 covers pvops:
>
> https://people.redhat.com/~aarcange/slides/2019-KVM-monolithic.pdf

You can patch call instructions directly using text_poke when
kvm_intel.ko or kvm_amd.ko, I'm not sure why that would be worse for TLB
or RAM usage. The hard part is recording the location of the call sites
using some pushsection/popsection magic.

Paolo

2019-11-08 21:27:50

by Andrea Arcangeli

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On Fri, Nov 08, 2019 at 10:02:52PM +0100, Paolo Bonzini wrote:
> kvm_intel.ko or kvm_amd.ko, I'm not sure why that would be worse for TLB
> or RAM usage. The hard part is recording the location of the call sites

Let's ignore the different code complexity of supporting self
modifying code: kvm.ko and kvm-*.ko will be located in different
pages, hence it'll waste 1 iTLB for every vmexit and 2k of RAM in
average. The L1 icache also will be wasted. It'll simply run slower.

Now about the code complexity, it is even higher than pvops:

KVM pvops
========= =============
1) Changes daily Never change

2) Patched at runtime Patched only at boot time early on
during module load
and multiple times
at every load of kvm-*.ko

3) The patching points to All patch destinations are linked into
code in kernel modules the kernel

Why exactly should we go through such a complication when it runs
slower in the end and it's much more complex to implement and maintain
and in fact even more complex than pvops already is?

Runtime patching the indirect call like pvops do is strictly required
when you are forced to resolve the linking at runtime. The alternative
would be to ship two different Linux kernels for PV and bare
metal. Maintaining a whole new kernel rpm and having to install a
different rpm depending on the hypervisor/bare metal is troublesome so
pvops is worth it.

With kvm-amd and kvm-intel we can avoid the whole runtime patching of
the call sites as already proven by KVM monolithic patchset, and it'll
run faster in the CPU and it'll save RAM, so I'm not exactly sure how
anybody could prefer runtime patching here when the only benefit is a
few mbytes of disk space saved on disk.

Furthermore by linking the thing statically we'll also enable LTO and
other gcc features which would never be possible with those indirect
calls.

Thanks,
Andrea

2019-11-08 23:11:07

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On 08/11/19 22:26, Andrea Arcangeli wrote:
> On Fri, Nov 08, 2019 at 10:02:52PM +0100, Paolo Bonzini wrote:
>> kvm_intel.ko or kvm_amd.ko, I'm not sure why that would be worse for TLB
>> or RAM usage. The hard part is recording the location of the call sites
>
> Let's ignore the different code complexity of supporting self
> modifying code: kvm.ko and kvm-*.ko will be located in different
> pages, hence it'll waste 1 iTLB for every vmexit and 2k of RAM in
> average.

This is unlikely to make a difference, since kvm.o and kvm-intel.o are
overall about 700 KiB in size. You do lose some inlining opportunities
with LTO, but without LTO the L1 cache benefits are debatable too. The
real loss is in the complexity, I agree with you about that.

> Now about the code complexity, it is even higher than pvops:
>
> KVM pvops
> ========= =============
> 1) Changes daily Never change
>
> 2) Patched at runtime Patched only at boot time early on
> during module load
> and multiple times
> at every load of kvm-*.ko
>
> 3) The patching points to All patch destinations are linked into
> code in kernel modules the kernel
>
> Why exactly should we go through such a complication when it runs
> slower in the end and it's much more complex to implement and maintain
> and in fact even more complex than pvops already is?

For completeness, one advantage of patching would be to keep support for
built-in Intel+AMD. The modpost patch should be pretty small, and since
Jessica seemed quite open to it let's do that.

Thanks,

Paolo

> Furthermore by linking the thing statically we'll also enable LTO and
> other gcc features which would never be possible with those indirect
> calls.
>
> Thanks,
> Andrea
>

2019-11-09 03:33:03

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

(+CC: Lucas De Marchi, the kmod maintainer)

On Sat, Nov 9, 2019 at 4:51 AM Paolo Bonzini <[email protected]> wrote:
>
> On 08/11/19 14:56, Jessica Yu wrote:
> > And I am
> > not sure what Masahiro (who takes care of all things kbuild-related)
> > thinks of this idea. But before implementing all this, is there
> > absolutely no way around having the duplicated exported symbols? (e.g.,
> > could the modules be configured/built in a mutally exclusive way? I'm
> > lacking the context from the rest of the thread, so not sure which are
> > the problematic modules.)

I do not think having a white-list in the modpost
is maintainable.

>
> The problematic modules are kvm_intel and kvm_amd, so we cannot build
> them in a mutually exclusive way (but we know it won't make sense to
> load both). We will have to build only one of them when built into
> vmlinux, but the module case must support building both.
>
> Currently we put the common symbols in kvm.ko, and kvm.ko acts as a kind
> of "library" for kvm_intel.ko and kvm_amd.ko. The problem is that
> kvm_intel.ko and kvm_amd.ko currently pass a large array of function
> pointers to kvm.ko, and Andrea measured a substantial performance
> penalty from retpolines when kvm.ko calls back through those pointers.
>
> Therefore he would like to remove kvm.ko, and that would result in
> symbols exported from two modules.

If this is a general demand, I think we can relax
the 'exported twice' warning; we can show the warning
only when the previous export is from vmlinux.


However, I am not sure how the module dependency
should be handled when the same symbol is exported
from multiple modules.


Let's say the same symbol is exported from foo.ko and bar.ko
and foo.ko appears first in modules.order .
In this case, I think the foo.ko should be considered to have a higher
priority than bar.ko
The modpost records MODULE_INFO(depends, foo.ko) correctly,
but modules.{dep, symbols} do not seem to reflect that.
Maybe, depmod does not take multiple-export into consideration ??

If we change this, I want this working consistently.


Masahiro Yamada




> I suppose we could use code patching mechanism to avoid the retpolines.
> Andrea, what do you think about that? That would have the advantage
> that we won't have to remove kvm_x86_ops. :)
>
> Thanks,
>
> Paolo



--
Best Regards
Masahiro Yamada