On Thu, Apr 22, 2021 at 11:58:45AM +0200, Jan Beulich wrote:
> On 22.04.2021 11:42, Roger Pau Monné wrote:
> > On Wed, Apr 14, 2021 at 03:49:02PM +0200, Jan Beulich wrote:
> >> On 13.04.2021 16:01, Roger Pau Monne wrote:
> >>> @@ -944,3 +945,130 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch,
> >>> const xc_cpu_policy_t host,
> >>>
> >>> return false;
> >>> }
> >>> +
> >>> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t
> >>> val2)
> >>> +{
> >>> + uint64_t val = val1 & val2;;
> >>
> >> For arbitrary MSRs this isn't going to do any good. If only very
> >> specific MSRs are assumed to make it here, I think this wants
> >> commenting on.
> >
> > I've added: "MSRs passed to level_msr are expected to be bitmaps of
> > features"
>
> How does such a comment help? I.e. how does the caller tell which MSRs
> to pass here and which to deal with anyother way?
All MSRs should be passed to level_msr, but it's handling logic would
need to be expanded to support MSRs that are not feature bitmaps.
It might be best to restore the previous switch and handle each MSR
specifically?
>From your reply to v1 I wrongly misunderstood that you initially
wanted to handle all MSRs as bitmaps:
https://lore.kernel.org/xen-devel/f66e61d5-e4a0-cba3-f15c-73ca54ac4...@suse.com/
> >>> + xen_cpuid_leaf_t *out)
> >>> +{
> >>> + *out = (xen_cpuid_leaf_t){ };
> >>> +
> >>> + switch ( l1->leaf )
> >>> + {
> >>> + case 0x1:
> >>> + case 0x80000001:
> >>> + out->c = l1->c & l2->c;
> >>> + out->d = l1->d & l2->d;
> >>> + return true;
> >>> +
> >>> + case 0xd:
> >>> + if ( l1->subleaf != 1 )
> >>> + break;
> >>> + out->a = l1->a & l2->a;
> >>> + return true;
> >>
> >> Could you explain your thinking behind this (a code comment would
> >> likely help)? You effectively discard everything except subleaf 1
> >> by returning false in that case, don't you?
> >
> > Yes, the intent is to only level the features bitfield found in
> > subleaf 1.
> >
> > I was planning for level_leaf so far in this series to deal with the
> > feature leaves part of the featureset only. I guess you would also
> > like to leverage other parts of the xstate leaf, like the max_size or
> > the supported bits in xss_{low,high}?
>
> The latter is clearly one of the things to consider, yes (alongside
> the respective bits in sub-leaf 0 for XCR0). Sub-leaves > 1 may also
> need dealing with ECX. Yet then again some or all of this may need
> handling elsewhere, not the least because of the unusual handling of
> leaf 0xd in the hypervisor. What gets checked and/or adjusted where
> needs to be settled upon, and then the different parts of code would
> imo better cross-reference each other.
There's a comment in recalculate_xstate that mentions that Da1 leaf is
the only piece of information preserved, and that everything else is
derived from feature state. I don't think it makes sense to try to
level anything apart from Da1 if it's going to be discarded by
recalculate_xstate anyway?
I can add a comment here regarding why only Da1 is taken into account
for leveling so far.
Thanks, Roger.
