| Message ID | 0f04f568-e55a-ef20-aa97-fbb199dfae37@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xvmalloc() / x86 xstate area / x86 CPUID / AMX beginnings | expand |
On 23.11.2020 15:32, Jan Beulich wrote: > A maximum extended leaf input value with the high half different from > 0x8000 should not be considered valid - all leaves should be cleared in > this case. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Integrate into series. While most other parts of this series are to be delayed until (at least) 4.16, I consider this one a bug fix. Jan > --- a/tools/tests/cpu-policy/test-cpu-policy.c > +++ b/tools/tests/cpu-policy/test-cpu-policy.c > @@ -516,11 +516,22 @@ static void test_cpuid_out_of_range_clea > }, > }, > { > + .name = "no extd", > + .nr_markers = 0, > + .p = { > + /* Clears all markers. */ > + .extd.max_leaf = 0, > + > + .extd.vendor_ebx = 0xc2, > + .extd.raw_fms = 0xc2, > + }, > + }, > + { > .name = "extd", > .nr_markers = 1, > .p = { > /* Retains marker in leaf 0. Clears others. */ > - .extd.max_leaf = 0, > + .extd.max_leaf = 0x80000000, > .extd.vendor_ebx = 0xc2, > > .extd.raw_fms = 0xc2, > --- a/xen/lib/x86/cpuid.c > +++ b/xen/lib/x86/cpuid.c > @@ -232,7 +232,9 @@ void x86_cpuid_policy_clear_out_of_range > ARRAY_SIZE(p->xstate.raw) - 1); > } > > - zero_leaves(p->extd.raw, (p->extd.max_leaf & 0xffff) + 1, > + zero_leaves(p->extd.raw, > + ((p->extd.max_leaf >> 16) == 0x8000 > + ? (p->extd.max_leaf & 0xffff) + 1 : 0), > ARRAY_SIZE(p->extd.raw) - 1); > } > >
On Mon, Nov 23, 2020 at 03:32:35PM +0100, Jan Beulich wrote: > A maximum extended leaf input value with the high half different from > 0x8000 should not be considered valid - all leaves should be cleared in > this case. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 23/11/2020 14:32, Jan Beulich wrote: > A maximum extended leaf input value with the high half different from > 0x8000 should not be considered valid - all leaves should be cleared in > this case. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Integrate into series. > > --- a/tools/tests/cpu-policy/test-cpu-policy.c > +++ b/tools/tests/cpu-policy/test-cpu-policy.c > @@ -516,11 +516,22 @@ static void test_cpuid_out_of_range_clea > }, > }, > { > + .name = "no extd", > + .nr_markers = 0, > + .p = { > + /* Clears all markers. */ > + .extd.max_leaf = 0, > + > + .extd.vendor_ebx = 0xc2, > + .extd.raw_fms = 0xc2, > + }, > + }, > + { > .name = "extd", > .nr_markers = 1, > .p = { > /* Retains marker in leaf 0. Clears others. */ > - .extd.max_leaf = 0, > + .extd.max_leaf = 0x80000000, > .extd.vendor_ebx = 0xc2, > > .extd.raw_fms = 0xc2, > --- a/xen/lib/x86/cpuid.c > +++ b/xen/lib/x86/cpuid.c > @@ -232,7 +232,9 @@ void x86_cpuid_policy_clear_out_of_range > ARRAY_SIZE(p->xstate.raw) - 1); > } > > - zero_leaves(p->extd.raw, (p->extd.max_leaf & 0xffff) + 1, > + zero_leaves(p->extd.raw, > + ((p->extd.max_leaf >> 16) == 0x8000 > + ? (p->extd.max_leaf & 0xffff) + 1 : 0), > ARRAY_SIZE(p->extd.raw) - 1); Honestly, this is unnecessary complexity and overhead, and the logic is already hard enough to follow. There won't be extd.max_leaf with the high half != 0x8000 in real policies, because of how we fill them. Nor ought there to be, given the intended meaning of this part of the union. I think we simply forbid this case, rather than taking extra complexity to cope with it. Approximately all VMs will have 0x80000008 as a minimum, and I don't think catering to pre-64bit Intel CPUs is worth our effort either. ~Andrew
On 15.04.2021 14:48, Andrew Cooper wrote: > On 23/11/2020 14:32, Jan Beulich wrote: >> --- a/xen/lib/x86/cpuid.c >> +++ b/xen/lib/x86/cpuid.c >> @@ -232,7 +232,9 @@ void x86_cpuid_policy_clear_out_of_range >> ARRAY_SIZE(p->xstate.raw) - 1); >> } >> >> - zero_leaves(p->extd.raw, (p->extd.max_leaf & 0xffff) + 1, >> + zero_leaves(p->extd.raw, >> + ((p->extd.max_leaf >> 16) == 0x8000 >> + ? (p->extd.max_leaf & 0xffff) + 1 : 0), >> ARRAY_SIZE(p->extd.raw) - 1); > > Honestly, this is unnecessary complexity and overhead, and the logic is > already hard enough to follow. > > There won't be extd.max_leaf with the high half != 0x8000 in real > policies, because of how we fill them. Nor ought there to be, given the > intended meaning of this part of the union. > > I think we simply forbid this case, rather than taking extra complexity > to cope with it. Approximately all VMs will have 0x80000008 as a > minimum, and I don't think catering to pre-64bit Intel CPUs is worth our > effort either. "Forbid" has got to mean something other than "assume all input is fine". Aiui guest config files can restrict the maximum sub-leaves exposed to a guest. If we don't want to handle the case here, where else to you suggest we at least and complain about the broken assumption? (IOW I'd be okay dropping this patch if your "forbid" actually means some enforcement elsewhere, and then perhaps a comment pointing there ahead of the zero_leaves() invocation here.) Jan
--- a/tools/tests/cpu-policy/test-cpu-policy.c +++ b/tools/tests/cpu-policy/test-cpu-policy.c @@ -516,11 +516,22 @@ static void test_cpuid_out_of_range_clea }, }, { + .name = "no extd", + .nr_markers = 0, + .p = { + /* Clears all markers. */ + .extd.max_leaf = 0, + + .extd.vendor_ebx = 0xc2, + .extd.raw_fms = 0xc2, + }, + }, + { .name = "extd", .nr_markers = 1, .p = { /* Retains marker in leaf 0. Clears others. */ - .extd.max_leaf = 0, + .extd.max_leaf = 0x80000000, .extd.vendor_ebx = 0xc2, .extd.raw_fms = 0xc2, --- a/xen/lib/x86/cpuid.c +++ b/xen/lib/x86/cpuid.c @@ -232,7 +232,9 @@ void x86_cpuid_policy_clear_out_of_range ARRAY_SIZE(p->xstate.raw) - 1); } - zero_leaves(p->extd.raw, (p->extd.max_leaf & 0xffff) + 1, + zero_leaves(p->extd.raw, + ((p->extd.max_leaf >> 16) == 0x8000 + ? (p->extd.max_leaf & 0xffff) + 1 : 0), ARRAY_SIZE(p->extd.raw) - 1); }
A maximum extended leaf input value with the high half different from 0x8000 should not be considered valid - all leaves should be cleared in this case. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Integrate into series.