From: Yin Fengwei <[email protected]>
ACRN Hypervisor reports hypervisor features via CPUID leaf 0x40000001
which is similar to KVM. A VM can check if it's the privileged VM using
the feature bits. The Service VM is the only privileged VM by design.
Signed-off-by: Yin Fengwei <[email protected]>
Signed-off-by: Shuo Liu <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Fengwei Yin <[email protected]>
Cc: Zhi Wang <[email protected]>
Cc: Zhenyu Wang <[email protected]>
Cc: Yu Wang <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
arch/x86/include/asm/acrn.h | 9 +++++++++
arch/x86/kernel/cpu/acrn.c | 19 ++++++++++++++++++-
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/acrn.h b/arch/x86/include/asm/acrn.h
index ff259b69cde7..a2d4aea3a80d 100644
--- a/arch/x86/include/asm/acrn.h
+++ b/arch/x86/include/asm/acrn.h
@@ -2,7 +2,16 @@
#ifndef _ASM_X86_ACRN_H
#define _ASM_X86_ACRN_H
+/*
+ * This CPUID returns feature bitmaps in EAX.
+ * Guest VM uses this to detect the appropriate feature bit.
+ */
+#define ACRN_CPUID_FEATURES 0x40000001
+/* Bit 0 indicates whether guest VM is privileged */
+#define ACRN_FEATURE_PRIVILEGED_VM BIT(0)
+
void acrn_setup_intr_handler(void (*handler)(void));
void acrn_remove_intr_handler(void);
+bool acrn_is_privileged_vm(void);
#endif /* _ASM_X86_ACRN_H */
diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
index e0c181781905..5528bfc913ea 100644
--- a/arch/x86/kernel/cpu/acrn.c
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -19,9 +19,26 @@
#include <asm/idtentry.h>
#include <asm/irq_regs.h>
+static u32 acrn_cpuid_base(void)
+{
+ static u32 acrn_cpuid_base;
+
+ if (!acrn_cpuid_base && boot_cpu_has(X86_FEATURE_HYPERVISOR))
+ acrn_cpuid_base = hypervisor_cpuid_base("ACRNACRNACRN", 0);
+
+ return acrn_cpuid_base;
+}
+
+bool acrn_is_privileged_vm(void)
+{
+ return cpuid_eax(acrn_cpuid_base() | ACRN_CPUID_FEATURES) &
+ ACRN_FEATURE_PRIVILEGED_VM;
+}
+EXPORT_SYMBOL_GPL(acrn_is_privileged_vm);
+
static u32 __init acrn_detect(void)
{
- return hypervisor_cpuid_base("ACRNACRNACRN", 0);
+ return acrn_cpuid_base();
}
static void __init acrn_init_platform(void)
--
2.28.0
On Mon, Oct 19, 2020 at 02:17:49PM +0800, [email protected] wrote:
> +bool acrn_is_privileged_vm(void)
> +{
> + return cpuid_eax(acrn_cpuid_base() | ACRN_CPUID_FEATURES) &
> + ACRN_FEATURE_PRIVILEGED_VM;
I asked in the previous review why that acrn_cpuid_base() is used here,
you said that the base might vary. Looking at hypervisor_cpuid_base(),
it searches in the range [0x40000000, 0x40010000] with an 0x100 offset.
So you're saying that ACRN_CPUID_FEATURES is the first leaf beyond the
base. Close?
If so, why isn't the code doing this?
return cpuid_eax(acrn_cpuid_base() + 1)...
and why doesn't it have a comment above it explaining that the base can
change and it needs to be discovered each time?
> +EXPORT_SYMBOL_GPL(acrn_is_privileged_vm);
Also, that acrn_is_privileged_vm() silly helper is used only once and
I don't like the exported symbols pollution we're doing. So make that
function give you the eax of ACRN_CPUID_FEATURES and callers can do
their testing themselves.
When it turns out that code patterns get repeated, you can then
aggregate stuff into a helper.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris,
On Mon 2.Nov'20 at 15:37:07 +0100, Borislav Petkov wrote:
>On Mon, Oct 19, 2020 at 02:17:49PM +0800, [email protected] wrote:
>> +bool acrn_is_privileged_vm(void)
>> +{
>> + return cpuid_eax(acrn_cpuid_base() | ACRN_CPUID_FEATURES) &
>> + ACRN_FEATURE_PRIVILEGED_VM;
>
>I asked in the previous review why that acrn_cpuid_base() is used here,
>you said that the base might vary. Looking at hypervisor_cpuid_base(),
>it searches in the range [0x40000000, 0x40010000] with an 0x100 offset.
>
>So you're saying that ACRN_CPUID_FEATURES is the first leaf beyond the
>base. Close?
Yes.
>
>If so, why isn't the code doing this?
>
> return cpuid_eax(acrn_cpuid_base() + 1)...
>
>and why doesn't it have a comment above it explaining that the base can
>change and it needs to be discovered each time?
The code just followed KVM style (see kvm_arch_para_features()).
I can change to use
cpuid_eax(acrn_cpuid_base() + 1)...
If you prefer to.
hypervisor_cpuid_base() implies the base is variable, no? We use
this function to detect the base.
>
>> +EXPORT_SYMBOL_GPL(acrn_is_privileged_vm);
>
>Also, that acrn_is_privileged_vm() silly helper is used only once and
>I don't like the exported symbols pollution we're doing. So make that
>function give you the eax of ACRN_CPUID_FEATURES and callers can do
>their testing themselves.
OK. Then i will define acrn_cpuid_base() as a static inline function in
asm/acrn.h for callers.
>
>When it turns out that code patterns get repeated, you can then
>aggregate stuff into a helper.
Got it. Thanks.
Thanks
shuo
On Tue, Nov 03, 2020 at 02:27:18PM +0800, Shuo A Liu wrote:
> The code just followed KVM style (see kvm_arch_para_features()).
Do you see Documentation/virt/kvm/cpuid.rst?
Now where is yours explaining what your hypervisor is doing?
> I can change to use
> cpuid_eax(acrn_cpuid_base() + 1)...
> If you prefer to.
Yes please.
> hypervisor_cpuid_base() implies the base is variable, no? We use
> this function to detect the base.
Yes, but you need to document all that and make it clear and
understandable. If Linux is supposed to run as an acrn guest, that
interface better be documented just like KVM does.
Also, if there's a bug in the KVM guest/host interface, we might be able
to fix it modulo ABI. Is that possible with acrn?
I'm guessing the answer to that is yes if I'm looking at
https://github.com/projectacrn/acrn-hypervisor
?
> OK. Then i will define acrn_cpuid_base() as a static inline function
> in asm/acrn.h for callers.
Yah, that function is simple enough.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue 3.Nov'20 at 11:25:38 +0100, Borislav Petkov wrote:
>On Tue, Nov 03, 2020 at 02:27:18PM +0800, Shuo A Liu wrote:
>> The code just followed KVM style (see kvm_arch_para_features()).
>
>Do you see Documentation/virt/kvm/cpuid.rst?
OK. It documents the leaf number.
>
>Now where is yours explaining what your hypervisor is doing?
Currently, it is in arch/x86/include/asm/acrn.h.
>
>> I can change to use
>> cpuid_eax(acrn_cpuid_base() + 1)...
>> If you prefer to.
>
>Yes please.
Sure.
If the leaf numbers be documented explicitly (like kvm), i think i can
use them as eax of cpuid_eax() directly (back to your first comment).
cpuid_eax(ACRN_CPUID_FEATURES)...
If you looking at implementation of acrn-hypervisor, you will found the
leaf number is hardcoded in the hypervisor. So, they also can be
documented explicitly.
>
>> hypervisor_cpuid_base() implies the base is variable, no? We use
>> this function to detect the base.
>
>Yes, but you need to document all that and make it clear and
>understandable. If Linux is supposed to run as an acrn guest, that
>interface better be documented just like KVM does.
OK. I can add a similar cpuid.rst for acrn.
>
>Also, if there's a bug in the KVM guest/host interface, we might be able
>to fix it modulo ABI. Is that possible with acrn?
>
>I'm guessing the answer to that is yes if I'm looking at
>
>https://github.com/projectacrn/acrn-hypervisor
>
>?
Yes. Fix patches are always welcome.
>
>> OK. Then i will define acrn_cpuid_base() as a static inline function
>> in asm/acrn.h for callers.
>
>Yah, that function is simple enough.
Thanks
shuo
On Wed, Nov 04, 2020 at 11:50:27AM +0800, Shuo A Liu wrote:
> On Tue 3.Nov'20 at 11:25:38 +0100, Borislav Petkov wrote:
> > On Tue, Nov 03, 2020 at 02:27:18PM +0800, Shuo A Liu wrote:
> > > The code just followed KVM style (see kvm_arch_para_features()).
> >
> > Do you see Documentation/virt/kvm/cpuid.rst?
>
> OK. It documents the leaf number.
It also says
"Note also that old hosts set eax value to 0x0. This should be
interpreted as if the value was 0x40000001."
Does this hold true for the acrn HV? The fact that I'm asking about
all those things should give you a hint that documenting the API is
important.
> > Now where is yours explaining what your hypervisor is doing?
>
> Currently, it is in arch/x86/include/asm/acrn.h.
Yeah, you can't expect people to go scrape it from headers though - it
should be concentrated in a doc file.
> If the leaf numbers be documented explicitly (like kvm), i think i can
> use them as eax of cpuid_eax() directly (back to your first comment).
Which means, you don't need to do the logical OR-ing which kvm does
because of what I pasted above about eax being 0 on old hosts. Now we're
getting somewhere...
> cpuid_eax(ACRN_CPUID_FEATURES)...
>
> If you looking at implementation of acrn-hypervisor, you will found the
> leaf number is hardcoded in the hypervisor. So, they also can be
> documented explicitly.
Ok.
> OK. I can add a similar cpuid.rst for acrn.
Yes please.
> Yes. Fix patches are always welcome.
Ok, good, the thing is open. You could put that in the doc too, along
with the link so that people can find it.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed 4.Nov'20 at 19:51:57 +0100, Borislav Petkov wrote:
>On Wed, Nov 04, 2020 at 11:50:27AM +0800, Shuo A Liu wrote:
>> On Tue 3.Nov'20 at 11:25:38 +0100, Borislav Petkov wrote:
>> > On Tue, Nov 03, 2020 at 02:27:18PM +0800, Shuo A Liu wrote:
>> > > The code just followed KVM style (see kvm_arch_para_features()).
>> >
>> > Do you see Documentation/virt/kvm/cpuid.rst?
>>
>> OK. It documents the leaf number.
>
>It also says
>
>"Note also that old hosts set eax value to 0x0. This should be
>interpreted as if the value was 0x40000001."
Ok.
>
>Does this hold true for the acrn HV? The fact that I'm asking about
>all those things should give you a hint that documenting the API is
>important.
No, acrn HV is always return eax with maximum leaf number.
Let me add the document.
>
>> > Now where is yours explaining what your hypervisor is doing?
>>
>> Currently, it is in arch/x86/include/asm/acrn.h.
>
>Yeah, you can't expect people to go scrape it from headers though - it
>should be concentrated in a doc file.
>
>> If the leaf numbers be documented explicitly (like kvm), i think i can
>> use them as eax of cpuid_eax() directly (back to your first comment).
>
>Which means, you don't need to do the logical OR-ing which kvm does
>because of what I pasted above about eax being 0 on old hosts. Now we're
>getting somewhere...
Yes.
>
>> cpuid_eax(ACRN_CPUID_FEATURES)...
>>
>> If you looking at implementation of acrn-hypervisor, you will found the
>> leaf number is hardcoded in the hypervisor. So, they also can be
>> documented explicitly.
>
>Ok.
>
>> OK. I can add a similar cpuid.rst for acrn.
>
>Yes please.
>
>> Yes. Fix patches are always welcome.
>
>Ok, good, the thing is open. You could put that in the doc too, along
>with the link so that people can find it.
OK.
Thanks
shuo