2017-08-01 13:35+0200, Paolo Bonzini:
> On 01/08/2017 13:18, David Hildenbrand wrote:
> >
> >>> Can't we rewrite that a little bit, avoiding that "best" handling
> >>> (introducing guest_cpuid_disable_invpcid() and guest_cpuid_has_invpcid())
> >>>
> >>> bool invpcid_enabled = guest_cpuid_has_pcid(vcpu) &&
> >>> guest_cpuid_has_invpcid();
> >>>
> >>> if (!invpcid_enabled) {
> >>> secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> >>> /* make sure there is no no INVPCID without PCID */
> >>> guest_cpuid_disable_invpcid(vcpu);
> >>> }
> >>
> >> I don't know... if you need a comment, it means the different structure
> >> of the code doesn't spare many doubts from the reader. And the code
> >> doesn't become much simpler since you have to handle "nested" anyway.
> >> What I tried to do was to mimic as much as possible the rdtscp case, but
> >> it cannot be exactly the same due to the interaction between PCID and
> >> INVPCID.
> >
> > It's more about the handling of best here, which can be avoided quite
> > easily as I showed (by encapsulating how cpuids are looked up/modified).
>
> Yeah, I don't like either option. :) Luckily there is a second maintainer!
With three people, we'll just have three options! :)
I'd go with Paolo's version, because it is efficient and also follows
patterns the bit clearing in kvm_update_cpuid().
---
I would like the wrappers if they were more generic, e.g.:
guest_cpuid_has(vcpu, X86_FEATURE_INVPCID);
guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID);
To do that, we need a mapping from X86_FEATURE to (leaf, subleaf,
register) triple, which can be done with cpuid_leafs.
I haven't tried to compile the idea below, but if the optimizer is good,
then we should have only slightly worse byte code as we do now thanks to
x86_feature being a compile-time constant.
Do you it's less ugly than the other two options?
static inline bool x86_feature_cpuid(int x86_feature, int *id, int *count, int
*register)
{
static struct {int x86_leaf; int id; int count; int register;} cpuid[]
= {
{CPUID_1_EDX, 1, 0, 3},
{CPUID_8000_0001_EDX, 0x80000001, 0, 3},
{CPUID_8086_0001_EDX, 0x80860001, 0, 3},
{CPUID_1_ECX, 1, 0, 2},
... // you get the idea, it's tedious :)
};
for (size_t i = 0; i < ARRAY_SIZE(cpuid); i++)
if (cpuid[i].x86_leaf == x86_feature / 32) {
*id = cpuid[i].id;
*count = cpuid[i].count;
*register = cpuid[i].register;
return true;
}
return false;
}
static inline bool guest_cpuid_has(struct kvm_vcpu *vcpu, int x86_feature)
{
int id, count, register;
u32 *reg;
struct kvm_cpuid_entry2 *best;
if (!x86_feature_cpuid(x86_feature, &id, &count, ®ister))
return false;
if (!(best = kvm_find_cpuid_entry(id, count)))
return false;
reg = &best->eax + register; // rewrite to be safer
return *reg & bit(x86_feature);
}
Similar for guest_cpuid_clear and we can also factor out the common part
for getting the register (all up to the last line, with NULL as failure)
