[PATCH 1/1] KVM: pass through CPUID(0x80000006)

[PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Jon Cargille]

From: Eric Northup <digitaleric@gmail.com>

Return L2 cache and TLB information to guests.
They could have been set before, but the defaults that KVM returns will be
necessary for usermode that doesn't supply their own CPUID tables.

Signed-off-by: Eric Northup <digitaleric@google.com>
Signed-off-by: Eric Northup <digitaleric@gmail.com>
Signed-off-by: Jon Cargille <jcargill@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b072..4a8d67303a42c 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -734,6 +734,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->ecx &= kvm_cpuid_8000_0001_ecx_x86_features;
 		cpuid_mask(&entry->ecx, CPUID_8000_0001_ECX);
 		break;
+	case 0x80000006:
+		/* L2 cache and TLB: pass through host info. */
+		break;
 	case 0x80000007: /* Advanced power management */
 		/* invariant TSC is CPUID.80000007H:EDX[8] */
 		entry->edx &= (1 << 8);
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Sean Christopherson]

On Tue, Apr 14, 2020 at 06:23:20PM -0700, Jon Cargille wrote:
> From: Eric Northup <digitaleric@gmail.com>
> 
> Return L2 cache and TLB information to guests.
> They could have been set before, but the defaults that KVM returns will be
> necessary for usermode that doesn't supply their own CPUID tables.

I don't follow the changelog.  The code makes sense, but I don't understand
the justification.  This only affects KVM_GET_SUPPORTED_CPUID, i.e. what's
advertised to userspace, it doesn't directly change CPUID emulation in any
way.  The "They could have been set before" blurb is especially confusing.

I assume you want to say something like:

  Return the host's L2 cache and TLB information for CPUID.0x80000006
  instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
  This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
  directly into KVM_SET_CPUID2 (without breaking the guest).

> Signed-off-by: Eric Northup <digitaleric@google.com>
> Signed-off-by: Eric Northup <digitaleric@gmail.com>
> Signed-off-by: Jon Cargille <jcargill@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>

Jim's tag is unnecessary, unless he was a middleman between Eric and Jon,
in which case Jim's tag should also come between Eric's and Jon's.

Only one of Eric's signoffs is needed (the one that matches the From: tag,
i.e. is the official author).  I'm guessing Google would prefer the author
to be the @google.com address.

> ---
>  arch/x86/kvm/cpuid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b072..4a8d67303a42c 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -734,6 +734,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>  		entry->ecx &= kvm_cpuid_8000_0001_ecx_x86_features;
>  		cpuid_mask(&entry->ecx, CPUID_8000_0001_ECX);
>  		break;
> +	case 0x80000006:
> +		/* L2 cache and TLB: pass through host info. */
> +		break;
>  	case 0x80000007: /* Advanced power management */
>  		/* invariant TSC is CPUID.80000007H:EDX[8] */
>  		entry->edx &= (1 << 8);
> -- 
> 2.25.1.481.gfbce0eb801-goog
> 

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Sean Christopherson]

On Tue, Apr 14, 2020 at 07:37:26PM -0700, Sean Christopherson wrote:
> On Tue, Apr 14, 2020 at 06:23:20PM -0700, Jon Cargille wrote:
> > From: Eric Northup <digitaleric@gmail.com>
> > 
> > Return L2 cache and TLB information to guests.
> > They could have been set before, but the defaults that KVM returns will be
> > necessary for usermode that doesn't supply their own CPUID tables.
> 
> I don't follow the changelog.  The code makes sense, but I don't understand
> the justification.  This only affects KVM_GET_SUPPORTED_CPUID, i.e. what's
> advertised to userspace, it doesn't directly change CPUID emulation in any
> way.  The "They could have been set before" blurb is especially confusing.
> 
> I assume you want to say something like:
> 
>   Return the host's L2 cache and TLB information for CPUID.0x80000006
>   instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
>   This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
>   directly into KVM_SET_CPUID2 (without breaking the guest).
> 
> > Signed-off-by: Eric Northup <digitaleric@google.com>
> > Signed-off-by: Eric Northup <digitaleric@gmail.com>
> > Signed-off-by: Jon Cargille <jcargill@google.com>
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> 
> Jim's tag is unnecessary, unless he was a middleman between Eric and Jon,
> in which case Jim's tag should also come between Eric's and Jon's.
> 
> Only one of Eric's signoffs is needed (the one that matches the From: tag,
> i.e. is the official author).  I'm guessing Google would prefer the author
> to be the @google.com address.

Ah, Eric's @google.com mail bounced.  Maybe do:

  Signed-off-by: Eric Northup (Google) <digitaleric@gmail.com>

to clarify the work was done for Google without having a double signoff
and/or a dead email.

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Eric Northup]

On Tue, Apr 14, 2020 at 7:51 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Apr 14, 2020 at 07:37:26PM -0700, Sean Christopherson wrote:
> > On Tue, Apr 14, 2020 at 06:23:20PM -0700, Jon Cargille wrote:
> > > From: Eric Northup <digitaleric@gmail.com>
> > >
> > > Return L2 cache and TLB information to guests.
> > > They could have been set before, but the defaults that KVM returns will be
> > > necessary for usermode that doesn't supply their own CPUID tables.
> >
> > I don't follow the changelog.  The code makes sense, but I don't understand
> > the justification.  This only affects KVM_GET_SUPPORTED_CPUID, i.e. what's
> > advertised to userspace, it doesn't directly change CPUID emulation in any
> > way.  The "They could have been set before" blurb is especially confusing.
> >
> > I assume you want to say something like:
> >
> >   Return the host's L2 cache and TLB information for CPUID.0x80000006
> >   instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
> >   This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
> >   directly into KVM_SET_CPUID2 (without breaking the guest).

This is a much better commit message, thanks.

> >
> > > Signed-off-by: Eric Northup <digitaleric@google.com>
> > > Signed-off-by: Eric Northup <digitaleric@gmail.com>
> > > Signed-off-by: Jon Cargille <jcargill@google.com>
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> >
> > Jim's tag is unnecessary, unless he was a middleman between Eric and Jon,
> > in which case Jim's tag should also come between Eric's and Jon's.
> >
> > Only one of Eric's signoffs is needed (the one that matches the From: tag,
> > i.e. is the official author).  I'm guessing Google would prefer the author
> > to be the @google.com address.
>
> Ah, Eric's @google.com mail bounced.  Maybe do:
>
>   Signed-off-by: Eric Northup (Google) <digitaleric@gmail.com>
>
> to clarify the work was done for Google without having a double signoff
> and/or a dead email.

That works for me. Thanks for upstreaming these patches Jon!

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Paolo Bonzini]

On 15/04/20 07:27, Eric Northup wrote:
>>>   Return the host's L2 cache and TLB information for CPUID.0x80000006
>>>   instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
>>>   This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
>>>   directly into KVM_SET_CPUID2 (without breaking the guest).
> This is a much better commit message, thanks.
> 

Queued, thanks.

Paolo

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Jon Cargille]

> I assume you want to say something like:

That's a much better commit message--thank you, Sean!

> Jim's tag is unnecessary, unless he was a middleman between Eric and Jon,

I appreciate the feedback; I was trying to capture that Jim "was in
the patch's delivery path."
(per submitting-patches.rst), but it sounds like that is intended for
a more explicit middle-man
relationship than I had understood. Jim reviewed it internally before
sending, which sounds
like it should be expressed as an "Acked-by" instead; is that accurate?

>> Only one of Eric's signoffs is needed (the one that matches the From: tag,
> Ah, Eric's @google.com mail bounced.  Maybe do:
>
>   Signed-off-by: Eric Northup (Google) <digitaleric@gmail.com>

Gotcha, thanks. Yes, when I conferred with Eric on submitting his
commits, he had wanted to
acknowledge that the work was done while he was at Google (e.g. his
old Google email addr),
and  I wanted to include current contact information for him as well.

Your suggestion to combine into a single Signed-off-by is a good one,
and I'll use that form
in the future.

Thanks much,

Jon

On Tue, Apr 14, 2020 at 7:51 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Apr 14, 2020 at 07:37:26PM -0700, Sean Christopherson wrote:
> > On Tue, Apr 14, 2020 at 06:23:20PM -0700, Jon Cargille wrote:
> > > From: Eric Northup <digitaleric@gmail.com>
> > >
> > > Return L2 cache and TLB information to guests.
> > > They could have been set before, but the defaults that KVM returns will be
> > > necessary for usermode that doesn't supply their own CPUID tables.
> >
> > I don't follow the changelog.  The code makes sense, but I don't understand
> > the justification.  This only affects KVM_GET_SUPPORTED_CPUID, i.e. what's
> > advertised to userspace, it doesn't directly change CPUID emulation in any
> > way.  The "They could have been set before" blurb is especially confusing.
> >
> > I assume you want to say something like:
> >
> >   Return the host's L2 cache and TLB information for CPUID.0x80000006
> >   instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
> >   This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
> >   directly into KVM_SET_CPUID2 (without breaking the guest).
> >
> > > Signed-off-by: Eric Northup <digitaleric@google.com>
> > > Signed-off-by: Eric Northup <digitaleric@gmail.com>
> > > Signed-off-by: Jon Cargille <jcargill@google.com>
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> >
> > Jim's tag is unnecessary, unless he was a middleman between Eric and Jon,
> > in which case Jim's tag should also come between Eric's and Jon's.
> >
> > Only one of Eric's signoffs is needed (the one that matches the From: tag,
> > i.e. is the official author).  I'm guessing Google would prefer the author
> > to be the @google.com address.
>
> Ah, Eric's @google.com mail bounced.  Maybe do:
>
>   Signed-off-by: Eric Northup (Google) <digitaleric@gmail.com>
>
> to clarify the work was done for Google without having a double signoff
> and/or a dead email.

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Sean Christopherson]

On Wed, Apr 15, 2020 at 10:22:16AM -0700, Jon Cargille wrote:
> > I assume you want to say something like:
> 
> That's a much better commit message--thank you, Sean!
> 
> > Jim's tag is unnecessary, unless he was a middleman between Eric and Jon,
> 
> I appreciate the feedback; I was trying to capture that Jim "was in the
> patch's delivery path." (per submitting-patches.rst), but it sounds like that
> is intended for a more explicit middle-man relationship than I had
> understood.

Yep, exactly.

> Jim reviewed it internally before sending, which sounds like it should be
> expressed as an "Acked-by" instead; is that accurate?

Or Reviewed-by.  The proper (and easiest) way to handle this is to use
whatever tag Jim (or any other reviewer) provides, e.g. submitting-patches
states, under 12) When to use Acked-by:, Cc:, and Co-developed-by:, states:

  If a person has had the opportunity to comment on a patch, but has not
  provided such comments, you may optionally add a ``Cc:`` tag to the patch.
  This is the only tag which might be added without an explicit action by the
  person it names

I.e. all *-by tags are only supposed to be used with explicit permission
from the named person.  This doesn't mean the person has to literally write
Reviewed-by or whatever (though that's usually the case), but it does mean
you should confirm it's ok to add a tag, e.g. if someone replies "LGTM" and
you want to interpret that as a Reviewed-by or Acked-by, explicitly ask if
it's ok to add the tag.

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Takahiro Itazuri]

Please forgive me if this is an absurd question.

Date:   Tue, 14 Apr 2020 19:37:26 -0700
From:   Sean Christopherson <sean.j.christopherson@intel.com>
>   Return the host's L2 cache and TLB information for CPUID.0x80000006
>   instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
>   This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
>   directly into KVM_SET_CPUID2 (without breaking the guest).

I noticed that CPUID 0x80000005 also returns cache information (L1 Cache
and TLB Information) when looking at AMD APM, while it is marked
reserved on Intel SDM. What do you think about passing through CPUID
0x80000005 to guests?

To be honest, I'm not sure if it is harmless from security and
performance perspectives in the first place.

Regard security aspect, I'm a bit concerned that it could help malicious
guests to know something to allow cache side channel attacks. However,
CPUID 0x80000006 has already passed through L2 Cache and TLB and L3
Cache Information. If passing through CPUID 0x80000006 is really fine,
I'm guessing it is the case with CPUID 0x80000005 as well.

In terms of performance, as far as I know, some softwares utilizes cache
information to achieve better performance. To simply put, by letting
guests know cache information, they may gain some benefits. Having said
that, if I understand correctly, guests can be scheduled on CPUs that do
not belong to the same group of CPUs that they run last time, unless
guests are pinned to a specific set of host physical CPUs. In such
cases, guests may not benefit from using cache information.

If I'm missing something or say something wrong, I'd appreciate it if
you could correct me. If it sounds no problem, I'd like to send a patch
for it.

Best regards,
Takahiro Itazuri

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Sean Christopherson]

Trimmed the Cc to remove folks that no longer directly work on any of this stuff.

On Fri, Jul 07, 2023, Takahiro Itazuri wrote:
> Please forgive me if this is an absurd question.
> 
> Date:   Tue, 14 Apr 2020 19:37:26 -0700
> From:   Sean Christopherson <sean.j.christopherson@intel.com>
> >   Return the host's L2 cache and TLB information for CPUID.0x80000006
> >   instead of zeroing out the entry as part of KVM_GET_SUPPORTED_CPUID.
> >   This allows a userspace VMM to feed KVM_GET_SUPPORTED_CPUID's output
> >   directly into KVM_SET_CPUID2 (without breaking the guest).

Ha, this confused me for a bit.  While past me did technically write this changelog,
I was just massaging someone else's words.

I'm honestly a bit dubious of the claim that providing a zeroed out 0x80000006
would break the guest.  I'm pretty sure I chose that phrase based Eric's original
wording that KVM's "defaults" would be "necessary".

  Return L2 cache and TLB information to guests.
  They could have been set before, but the defaults that KVM returns will be
  necessary for usermode that doesn't supply their own CPUID tables.

I don't think it actually matters (see below), it's just a rather odd justification.

> I noticed that CPUID 0x80000005 also returns cache information (L1 Cache
> and TLB Information) when looking at AMD APM, while it is marked
> reserved on Intel SDM. What do you think about passing through CPUID
> 0x80000005 to guests?
> 
> To be honest, I'm not sure if it is harmless from security and
> performance perspectives in the first place.
>
> Regard security aspect, I'm a bit concerned that it could help malicious
> guests to know something to allow cache side channel attacks. However,
> CPUID 0x80000006 has already passed through L2 Cache and TLB and L3
> Cache Information. If passing through CPUID 0x80000006 is really fine,
> I'm guessing it is the case with CPUID 0x80000005 as well.

It's definitely harmless from a security perspective.  Userspace already has
access to this information as CPUID is NOT a priveleged instructions.  And the
kernel also publishes this information in sysfs, e.g. /sys/devices/system/cpu/cpuN/cache,
and AFAIK that's not typically restricted.

KVM must assume that any and all information visible to userspace is also visible
to the guest, e.g. even if KVM wanted to police CPUID, nothing would prevent
userspace from providing a paravirtual interface to the guest to enumerate
cache and TLB topology.

> In terms of performance, as far as I know, some softwares utilizes cache
> information to achieve better performance. To simply put, by letting
> guests know cache information, they may gain some benefits. Having said
> that, if I understand correctly, guests can be scheduled on CPUs that do
> not belong to the same group of CPUs that they run last time, unless
> guests are pinned to a specific set of host physical CPUs. In such
> cases, guests may not benefit from using cache information.

I would be quite surprised if homogeneous, a.k.a. non-hybrid, CPUs ever have
variable cache/TLB properties across cores.  Hybrid CPUs might be a different
story, but even then I gotta imagine that userspace software already has problems,
e.g. userspace processes will encounter variable cache/TLB behavior unless
userspace is affining all tasks.

Regardless, the decision on whether or not to report cache information via
KVM_GET_SUPPORTED_CPUID was made long, long ago, as KVM has enumerated CPUID.0x4
since basically forever.  So really this only affects TLB info, and since KVM
already spits out 0x80000006, it's just L1 TLB info.

I'm mildly tempted to remove 0x80000006, for similar reasons as commit 45e966fcca03
("KVM: x86: Do not return host topology information from KVM_GET_SUPPORTED_CPUID"),
but I suspect that would do more harm than good, e.g. Linux falls back to
0x80000005 and 0x80000006 when running on AMD without extended topology info.

> If I'm missing something or say something wrong, I'd appreciate it if
> you could correct me. If it sounds no problem, I'd like to send a patch
> for it.

I think it makes sense to enumerate 0x80000005.  Reporting 0x80000006 but not
0x80000005 seems to be the *worst* behavior, so as I see it, the decision is
really between adding 0x80000005 and removing 0x80000006.  Adding 0x80000005
appears to be the least risky choice given that KVM has reported 0x80000006 for
over three years.

Re: [PATCH 1/1] KVM: pass through CPUID(0x80000006)

[Takahiro Itazuri]

On Wed, 12 Jul 2023, Sean Christopherson wrote:
> Trimmed the Cc to remove folks that no longer directly work on any of this stuff.

I apologize for it and appreciate your reply on this.

> > Regard security aspect, I'm a bit concerned that it could help malicious
> > guests to know something to allow cache side channel attacks. However,
> > CPUID 0x80000006 has already passed through L2 Cache and TLB and L3
> > Cache Information. If passing through CPUID 0x80000006 is really fine,
> > I'm guessing it is the case with CPUID 0x80000005 as well.
> 
> It's definitely harmless from a security perspective.  Userspace already has
> access to this information as CPUID is NOT a priveleged instructions.  And the
> kernel also publishes this information in sysfs, e.g. /sys/devices/system/cpu/cpuN/cache,
> and AFAIK that's not typically restricted.

I'm releaved to hear that.

> I'm mildly tempted to remove 0x80000006, for similar reasons as commit 45e966fcca03
> ("KVM: x86: Do not return host topology information from KVM_GET_SUPPORTED_CPUID"),
> but I suspect that would do more harm than good, e.g. Linux falls back to
> 0x80000005 and 0x80000006 when running on AMD without extended topology info.

Actually I also saw the commit and I was a bit confused about which
leaves to pass through. As you mentioned, CPUID is accessible from
userspace and VMM can query it if they want.

> I think it makes sense to enumerate 0x80000005.  Reporting 0x80000006 but not
> 0x80000005 seems to be the *worst* behavior, so as I see it, the decision is
> really between adding 0x80000005 and removing 0x80000006.  Adding 0x80000005
> appears to be the least risky choice given that KVM has reported 0x80000006 for
> over three years.

I'm on the same page that either reporting both or none of them is
better. I'll create a patch for the least risky one.

Best regards,
Takahiro Itazuri