Add a helper to retrieve cpuid_maxphyaddr() instead of manually
calculating the value in the emulator via raw CPUID output. In addition
to consolidating logic, this also paves the way toward simplifying
kvm_cpuid(), whose somewhat confusing return value exists purely to
support the emulator's maxphyaddr calculation.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_emulate.h | 1 +
arch/x86/kvm/emulate.c | 10 +---------
arch/x86/kvm/x86.c | 6 ++++++
3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index bf5f5e476f65..ded06515d30f 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -222,6 +222,7 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt, u32 *eax, u32 *ebx,
u32 *ecx, u32 *edx, bool check_limit);
+ int (*get_cpuid_maxphyaddr)(struct x86_emulate_ctxt *ctxt);
bool (*guest_has_long_mode)(struct x86_emulate_ctxt *ctxt);
bool (*guest_has_movbe)(struct x86_emulate_ctxt *ctxt);
bool (*guest_has_fxsr)(struct x86_emulate_ctxt *ctxt);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index dd19fb3539e0..bf02ed51e90f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4244,16 +4244,8 @@ static int check_cr_write(struct x86_emulate_ctxt *ctxt)
ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
if (efer & EFER_LMA) {
- u64 maxphyaddr;
- u32 eax, ebx, ecx, edx;
+ int maxphyaddr = ctxt->ops->get_cpuid_maxphyaddr(ctxt);
- eax = 0x80000008;
- ecx = 0;
- if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx,
- &edx, false))
- maxphyaddr = eax & 0xff;
- else
- maxphyaddr = 36;
rsvd = rsvd_bits(maxphyaddr, 63);
if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
rsvd &= ~X86_CR3_PCID_NOFLUSH;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ddd1d296bd20..5467ee71c25b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6209,6 +6209,11 @@ static bool emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
return kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx, check_limit);
}
+static int emulator_get_cpuid_maxphyaddr(struct x86_emulate_ctxt *ctxt)
+{
+ return cpuid_maxphyaddr(emul_to_vcpu(ctxt));
+}
+
static bool emulator_guest_has_long_mode(struct x86_emulate_ctxt *ctxt)
{
return guest_cpuid_has(emul_to_vcpu(ctxt), X86_FEATURE_LM);
@@ -6301,6 +6306,7 @@ static const struct x86_emulate_ops emulate_ops = {
.fix_hypercall = emulator_fix_hypercall,
.intercept = emulator_intercept,
.get_cpuid = emulator_get_cpuid,
+ .get_cpuid_maxphyaddr= emulator_get_cpuid_maxphyaddr,
.guest_has_long_mode = emulator_guest_has_long_mode,
.guest_has_movbe = emulator_guest_has_movbe,
.guest_has_fxsr = emulator_guest_has_fxsr,
--
2.24.1
On 02/03/20 20:57, Sean Christopherson wrote:
> Add a helper to retrieve cpuid_maxphyaddr() instead of manually
> calculating the value in the emulator via raw CPUID output. In addition
> to consolidating logic, this also paves the way toward simplifying
> kvm_cpuid(), whose somewhat confusing return value exists purely to
> support the emulator's maxphyaddr calculation.
>
> No functional change intended.
I don't think this is a particularly useful change. Yes, it's not
intuitive but is it more than a matter of documentation (and possibly
moving the check_cr_write snippet into a separate function)?
Paolo
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/include/asm/kvm_emulate.h | 1 +
> arch/x86/kvm/emulate.c | 10 +---------
> arch/x86/kvm/x86.c | 6 ++++++
> 3 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index bf5f5e476f65..ded06515d30f 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -222,6 +222,7 @@ struct x86_emulate_ops {
>
> bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt, u32 *eax, u32 *ebx,
> u32 *ecx, u32 *edx, bool check_limit);
> + int (*get_cpuid_maxphyaddr)(struct x86_emulate_ctxt *ctxt);
> bool (*guest_has_long_mode)(struct x86_emulate_ctxt *ctxt);
> bool (*guest_has_movbe)(struct x86_emulate_ctxt *ctxt);
> bool (*guest_has_fxsr)(struct x86_emulate_ctxt *ctxt);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index dd19fb3539e0..bf02ed51e90f 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4244,16 +4244,8 @@ static int check_cr_write(struct x86_emulate_ctxt *ctxt)
>
> ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
> if (efer & EFER_LMA) {
> - u64 maxphyaddr;
> - u32 eax, ebx, ecx, edx;
> + int maxphyaddr = ctxt->ops->get_cpuid_maxphyaddr(ctxt);
>
> - eax = 0x80000008;
> - ecx = 0;
> - if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx,
> - &edx, false))
> - maxphyaddr = eax & 0xff;
> - else
> - maxphyaddr = 36;
> rsvd = rsvd_bits(maxphyaddr, 63);
> if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
> rsvd &= ~X86_CR3_PCID_NOFLUSH;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..5467ee71c25b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6209,6 +6209,11 @@ static bool emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
> return kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx, check_limit);
> }
>
> +static int emulator_get_cpuid_maxphyaddr(struct x86_emulate_ctxt *ctxt)
> +{
> + return cpuid_maxphyaddr(emul_to_vcpu(ctxt));
> +}
> +
> static bool emulator_guest_has_long_mode(struct x86_emulate_ctxt *ctxt)
> {
> return guest_cpuid_has(emul_to_vcpu(ctxt), X86_FEATURE_LM);
> @@ -6301,6 +6306,7 @@ static const struct x86_emulate_ops emulate_ops = {
> .fix_hypercall = emulator_fix_hypercall,
> .intercept = emulator_intercept,
> .get_cpuid = emulator_get_cpuid,
> + .get_cpuid_maxphyaddr= emulator_get_cpuid_maxphyaddr,
> .guest_has_long_mode = emulator_guest_has_long_mode,
> .guest_has_movbe = emulator_guest_has_movbe,
> .guest_has_fxsr = emulator_guest_has_fxsr,
>
On 03.03.20 09:48, Paolo Bonzini wrote:
> On 02/03/20 20:57, Sean Christopherson wrote:
>> Add a helper to retrieve cpuid_maxphyaddr() instead of manually
>> calculating the value in the emulator via raw CPUID output. In addition
>> to consolidating logic, this also paves the way toward simplifying
>> kvm_cpuid(), whose somewhat confusing return value exists purely to
>> support the emulator's maxphyaddr calculation.
>>
>> No functional change intended.
>
> I don't think this is a particularly useful change. Yes, it's not
> intuitive but is it more than a matter of documentation (and possibly
> moving the check_cr_write snippet into a separate function)?
Besides the non obvious return value of the current function, this
approach also avoids leaving cpuid traces for querying maxphyaddr, which
is also not very intuitive IMHO.
Jan
>
> Paolo
>
>> Signed-off-by: Sean Christopherson <[email protected]>
>> ---
>> arch/x86/include/asm/kvm_emulate.h | 1 +
>> arch/x86/kvm/emulate.c | 10 +---------
>> arch/x86/kvm/x86.c | 6 ++++++
>> 3 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
>> index bf5f5e476f65..ded06515d30f 100644
>> --- a/arch/x86/include/asm/kvm_emulate.h
>> +++ b/arch/x86/include/asm/kvm_emulate.h
>> @@ -222,6 +222,7 @@ struct x86_emulate_ops {
>>
>> bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt, u32 *eax, u32 *ebx,
>> u32 *ecx, u32 *edx, bool check_limit);
>> + int (*get_cpuid_maxphyaddr)(struct x86_emulate_ctxt *ctxt);
>> bool (*guest_has_long_mode)(struct x86_emulate_ctxt *ctxt);
>> bool (*guest_has_movbe)(struct x86_emulate_ctxt *ctxt);
>> bool (*guest_has_fxsr)(struct x86_emulate_ctxt *ctxt);
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index dd19fb3539e0..bf02ed51e90f 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -4244,16 +4244,8 @@ static int check_cr_write(struct x86_emulate_ctxt *ctxt)
>>
>> ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
>> if (efer & EFER_LMA) {
>> - u64 maxphyaddr;
>> - u32 eax, ebx, ecx, edx;
>> + int maxphyaddr = ctxt->ops->get_cpuid_maxphyaddr(ctxt);
>>
>> - eax = 0x80000008;
>> - ecx = 0;
>> - if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx,
>> - &edx, false))
>> - maxphyaddr = eax & 0xff;
>> - else
>> - maxphyaddr = 36;
>> rsvd = rsvd_bits(maxphyaddr, 63);
>> if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
>> rsvd &= ~X86_CR3_PCID_NOFLUSH;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index ddd1d296bd20..5467ee71c25b 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6209,6 +6209,11 @@ static bool emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
>> return kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx, check_limit);
>> }
>>
>> +static int emulator_get_cpuid_maxphyaddr(struct x86_emulate_ctxt *ctxt)
>> +{
>> + return cpuid_maxphyaddr(emul_to_vcpu(ctxt));
>> +}
>> +
>> static bool emulator_guest_has_long_mode(struct x86_emulate_ctxt *ctxt)
>> {
>> return guest_cpuid_has(emul_to_vcpu(ctxt), X86_FEATURE_LM);
>> @@ -6301,6 +6306,7 @@ static const struct x86_emulate_ops emulate_ops = {
>> .fix_hypercall = emulator_fix_hypercall,
>> .intercept = emulator_intercept,
>> .get_cpuid = emulator_get_cpuid,
>> + .get_cpuid_maxphyaddr= emulator_get_cpuid_maxphyaddr,
>> .guest_has_long_mode = emulator_guest_has_long_mode,
>> .guest_has_movbe = emulator_guest_has_movbe,
>> .guest_has_fxsr = emulator_guest_has_fxsr,
>>
>
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
On 03/03/20 10:48, Jan Kiszka wrote:
>>
>> I don't think this is a particularly useful change.? Yes, it's not
>> intuitive but is it more than a matter of documentation (and possibly
>> moving the check_cr_write snippet into a separate function)?
>
> Besides the non obvious return value of the current function, this
> approach also avoids leaving cpuid traces for querying maxphyaddr, which
> is also not very intuitive IMHO.
There are already other cases where we leave CPUID traces. We can just
stop tracing if check_limit (which should be renamed to from_guest) is
true; there are other internal cases which call ctxt->ops->get_cpuid,
such as vendor_intel, and those should also use check_limit==true and
check the return value of ctxt->ops->get_cpuid.
Paolo
On Tue, Mar 03, 2020 at 09:48:03AM +0100, Paolo Bonzini wrote:
> On 02/03/20 20:57, Sean Christopherson wrote:
> > Add a helper to retrieve cpuid_maxphyaddr() instead of manually
> > calculating the value in the emulator via raw CPUID output. In addition
> > to consolidating logic, this also paves the way toward simplifying
> > kvm_cpuid(), whose somewhat confusing return value exists purely to
> > support the emulator's maxphyaddr calculation.
> >
> > No functional change intended.
>
> I don't think this is a particularly useful change. Yes, it's not
> intuitive but is it more than a matter of documentation (and possibly
> moving the check_cr_write snippet into a separate function)?
I really don't like duplicating the maxphyaddr logic. I'm paranoid
something will come along and change the "effective" maxphyaddr and we'll
forget all about the emulator, e.g. SEV, TME and paravirt XO all dance
around maxphyaddr.
On 03/03/20 17:28, Sean Christopherson wrote:
>> I don't think this is a particularly useful change. Yes, it's not
>> intuitive but is it more than a matter of documentation (and possibly
>> moving the check_cr_write snippet into a separate function)?
>
> I really don't like duplicating the maxphyaddr logic. I'm paranoid
> something will come along and change the "effective" maxphyaddr and we'll
> forget all about the emulator, e.g. SEV, TME and paravirt XO all dance
> around maxphyaddr.
Well, I'm paranoid about breaking everything else... :)
Adding a separate emulator_get_maxphyaddr function would at least
simplify grepping, since searching for 0x80000008 isn't exactly intuitive.
Paolo
On Tue, Mar 03, 2020 at 11:14:22AM +0100, Paolo Bonzini wrote:
> On 03/03/20 10:48, Jan Kiszka wrote:
> >>
> >> I don't think this is a particularly useful change.? Yes, it's not
> >> intuitive but is it more than a matter of documentation (and possibly
> >> moving the check_cr_write snippet into a separate function)?
> >
> > Besides the non obvious return value of the current function, this
> > approach also avoids leaving cpuid traces for querying maxphyaddr, which
> > is also not very intuitive IMHO.
>
> There are already other cases where we leave CPUID traces. We can just
> stop tracing if check_limit (which should be renamed to from_guest) is
> true; there are other internal cases which call ctxt->ops->get_cpuid,
> such as vendor_intel, and those should also use check_limit==true and
> check the return value of ctxt->ops->get_cpuid.
No, the vendor checks that use get_cpuid() shouldn't do check_limit=true,
they're looking for an exact match on the vendor.
Not that it matters. @check_limit only comes into play on a vendor check
if CPUID.0 doesn't exist, and @check_limit only effects the output if
CPUID.0 _does_ exist. I.e. the output for CPUID.0 is unaffected by
@check_limit.