Re: [Xen-devel] [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.05.18 at 19:27, <andrew.cooper3@xxxxxxxxxx> wrote:
> c/s f9616884e (a backport of c/s 0d703a701 "x86/feature: Definitions for
> Indirect Branch Controls") missed a CPUID adjustment when calculating the raw
> featureset.  This impacts host administrator diagnostics.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>

I continue to think this should remain to be a separate patch.

> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
> However, it breaks levelling of guests, making it impossible for the toolstack
> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
> 
> The dom0 issue referenced in the commit message was fixed by the hunk
> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
> be for levelling purposes) OR'd into the leaf.
> 
> One final item which was missed in backport was the need to ignore the
> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
> the mask has been applied.

This last paragraph at least partly contradicts the first talking about tool 
stack
chosen hiding of STIBP. The intended net effect, aiui, is
- expose STIBP independent of tool stack choice when the tool stack has
  elected to expose IBRSB,
- hide STIBP according to tool stack choice when IBRSB is also hidden.

A similar implication (as mentioned before, and see below) is supposed to
exist from IBRSB to IBPB, I think.

Also this aspect wasn't missed in the original backport, but done wrongly: The
ORing in ahead of the masking was meant to take care of this, utilizing what
calculate_{hvm,pv}_featureset() do (overlooking the fact that the feature sets
may have the bits set while the tool stack may have cleared them for the given
domain).

> @@ -1188,7 +1191,6 @@ void pv_cpuid(struct cpu_user_regs *regs)
>  
>      case 0x80000008:
>          a = paddr_bits | (vaddr_bits << 8);
> -        b |= cpufeat_mask(X86_FEATURE_IBPB);
>          b &= pv_featureset[FEATURESET_e8b];
>          break;

You didn't address my respective v1 review comment, neither by adding code
here, nor verbally: How is Dom0 supposed to know of IBPB being available if
IBSRB is (in hardware), but IBPB isn't? The whole purpose of the respective
chunk of code in calculate_pv_featureset() is just that. And if we  I think 
this similarly
should be done for DomU-s, i.e. also in the HVM variant of this code.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel