[PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[Roger Pau Monne]

CPUID policies from guest being migrated shouldn't have the maximum
leaves shrink, as that would be a guest visible change. The hypervisor
has no knowledge on whether a guest has been migrated or is build from
scratch, and hence it must not blindly shrink the CPUID policy in
recalculate_cpuid_policy. Remove the
x86_cpuid_policy_shrink_max_leaves call from recalculate_cpuid_policy.
Removing such call could be seen as a partial revert of 540d911c28.

Instead let the toolstack shrink the policies for newly created
guests, while keeping the previous values for guests that are migrated
in. Note that guests migrated in without a CPUID policy won't get any
kind of shrinking applied.

Fixes: 540d911c28 ('x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <iwj@xenproject.org>

This is a regression introduced in this release cycle, so we should
consider whether we want to take this patch. It's mostly moving a
shrink call from the hypervisor into the toolstack and making it more
selective.

Main risks would be this shrinking somehow altering the recalculations
of the CPUID policy done by the hypervisor. Removing the shirk itself
in the hypervisor shouldn't cause issues as that wasn't done before,
and reporting empty max leaf should be fine.
---
 tools/libs/guest/xg_cpuid_x86.c | 7 +++++++
 xen/arch/x86/cpuid.c            | 2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 198892ebdf..3ffd5f683b 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -638,6 +638,13 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         }
     }
 
+    /*
+     * Do not try to shrink the policy if restoring, as that could cause
+     * guest visible changes in the maximum leaf fields.
+     */
+    if ( !restore )
+        x86_cpuid_policy_shrink_max_leaves(p);
+
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
     if ( rc )
     {
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 2079a30ae4..8ac55f0806 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -719,8 +719,6 @@ void recalculate_cpuid_policy(struct domain *d)
 
     if ( !p->extd.page1gb )
         p->extd.raw[0x19] = EMPTY_LEAF;
-
-    x86_cpuid_policy_shrink_max_leaves(p);
 }
 
 int init_domain_cpuid_policy(struct domain *d)
-- 
2.33.0

Re: [PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[Andrew Cooper]

On 10/11/2021 17:40, Roger Pau Monne wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 198892ebdf..3ffd5f683b 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -638,6 +638,13 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>          }
>      }
>  
> +    /*
> +     * Do not try to shrink the policy if restoring, as that could cause
> +     * guest visible changes in the maximum leaf fields.
> +     */
> +    if ( !restore )
> +        x86_cpuid_policy_shrink_max_leaves(p);

Nothing in xc_cpuid_apply_policy() changes any of the max leaves, so
this is dead logic.

xc_cpuid_xend_policy() can in principle change max leaves, but that
logic is all horribly broken and I don't recommend touching it.

I'd just drop this hunk entirely, and retain the deletion in the hypervisor.

~Andrew

Re: [PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[Ian Jackson]

Roger Pau Monne writes ("[PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves"):
> CPUID policies from guest being migrated shouldn't have the maximum
> leaves shrink, as that would be a guest visible change. The hypervisor
> has no knowledge on whether a guest has been migrated or is build from
> scratch, and hence it must not blindly shrink the CPUID policy in
> recalculate_cpuid_policy. Remove the
> x86_cpuid_policy_shrink_max_leaves call from recalculate_cpuid_policy.
> Removing such call could be seen as a partial revert of 540d911c28.
...
> This is a regression introduced in this release cycle, so we should
> consider whether we want to take this patch. It's mostly moving a
> shrink call from the hypervisor into the toolstack and making it more
> selective.
> 
> Main risks would be this shrinking somehow altering the recalculations
> of the CPUID policy done by the hypervisor. Removing the shirk itself
> in the hypervisor shouldn't cause issues as that wasn't done before,
> and reporting empty max leaf should be fine.

Thank you.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[Jan Beulich]

On 10.11.2021 19:17, Andrew Cooper wrote:
> On 10/11/2021 17:40, Roger Pau Monne wrote:
>> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
>> index 198892ebdf..3ffd5f683b 100644
>> --- a/tools/libs/guest/xg_cpuid_x86.c
>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>> @@ -638,6 +638,13 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>          }
>>      }
>>  
>> +    /*
>> +     * Do not try to shrink the policy if restoring, as that could cause
>> +     * guest visible changes in the maximum leaf fields.
>> +     */
>> +    if ( !restore )
>> +        x86_cpuid_policy_shrink_max_leaves(p);
> 
> Nothing in xc_cpuid_apply_policy() changes any of the max leaves, so
> this is dead logic.

I guess you mean nothing there does anything which would result in
shrinking of the max leaves by calling this function? Yet even if
so, isn't the call here to take care of any earlier changes which
might have resulted in fully blank tail leaves?

Jan

> xc_cpuid_xend_policy() can in principle change max leaves, but that
> logic is all horribly broken and I don't recommend touching it.
> 
> I'd just drop this hunk entirely, and retain the deletion in the hypervisor.
> 
> ~Andrew
> 

Re: [PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[Roger Pau Monné]

On Thu, Nov 11, 2021 at 10:26:29AM +0100, Jan Beulich wrote:
> On 10.11.2021 19:17, Andrew Cooper wrote:
> > On 10/11/2021 17:40, Roger Pau Monne wrote:
> >> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> >> index 198892ebdf..3ffd5f683b 100644
> >> --- a/tools/libs/guest/xg_cpuid_x86.c
> >> +++ b/tools/libs/guest/xg_cpuid_x86.c
> >> @@ -638,6 +638,13 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
> >>          }
> >>      }
> >>  
> >> +    /*
> >> +     * Do not try to shrink the policy if restoring, as that could cause
> >> +     * guest visible changes in the maximum leaf fields.
> >> +     */
> >> +    if ( !restore )
> >> +        x86_cpuid_policy_shrink_max_leaves(p);
> > 
> > Nothing in xc_cpuid_apply_policy() changes any of the max leaves, so
> > this is dead logic.
> 
> I guess you mean nothing there does anything which would result in
> shrinking of the max leaves by calling this function? Yet even if
> so, isn't the call here to take care of any earlier changes which
> might have resulted in fully blank tail leaves?

AFAICT the featureset (optionally) passed in as a parameter could
result in certain leaves being zeroed and thus allow for the max leaf
to shrink.

So while xc_cpuid_apply_policy doesn't change the max leaves fields,
it can potentially zero certain leaves allowing to shrink the reported
max leaf.

Thanks, Roger.

Re: [PATCH for-4.16] x86/cpuid: prevent shrinking migrated policies max leaves

[Andrew Cooper]

On 11/11/2021 14:04, Roger Pau Monné wrote:
> On Thu, Nov 11, 2021 at 10:26:29AM +0100, Jan Beulich wrote:
>> On 10.11.2021 19:17, Andrew Cooper wrote:
>>> On 10/11/2021 17:40, Roger Pau Monne wrote:
>>>> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
>>>> index 198892ebdf..3ffd5f683b 100644
>>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>>> @@ -638,6 +638,13 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>>>          }
>>>>      }
>>>>  
>>>> +    /*
>>>> +     * Do not try to shrink the policy if restoring, as that could cause
>>>> +     * guest visible changes in the maximum leaf fields.
>>>> +     */
>>>> +    if ( !restore )
>>>> +        x86_cpuid_policy_shrink_max_leaves(p);
>>> Nothing in xc_cpuid_apply_policy() changes any of the max leaves, so
>>> this is dead logic.
>> I guess you mean nothing there does anything which would result in
>> shrinking of the max leaves by calling this function? Yet even if
>> so, isn't the call here to take care of any earlier changes which
>> might have resulted in fully blank tail leaves?
> AFAICT the featureset (optionally) passed in as a parameter could
> result in certain leaves being zeroed and thus allow for the max leaf
> to shrink.
>
> So while xc_cpuid_apply_policy doesn't change the max leaves fields,
> it can potentially zero certain leaves allowing to shrink the reported
> max leaf.

Hmm true, although I don't anticipate this having an effect in practice.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>