Re: [PATCH 6/6] x86/boot: Expose MSR_ARCH_CAPS data in guest max policies

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 6/6] x86/boot: Expose MSR_ARCH_CAPS data in guest max policies
Date: Mon, 22 May 2023 09:31:04 +0200	[thread overview]
Message-ID: <9bc8f75b-e46f-48fc-3083-aac30995ec29@suse.com> (raw)
In-Reply-To: <897bac23-b17d-ec4b-613b-d4d1b4c77d58@citrix.com>

On 19.05.2023 17:52, Andrew Cooper wrote:
> On 17/05/2023 10:20 am, Jan Beulich wrote:
>> On 16.05.2023 21:31, Andrew Cooper wrote:
>>> On 16/05/2023 3:53 pm, Jan Beulich wrote:
>>>> I guess that's no
>>>> different from other max-only features with dependents, but I wonder
>>>> whether that's good behavior.
>>> It's not really something you get a choice over.
>>>
>>> Default is always less than max, so however you choose to express these
>>> concepts, when you're opting-in you're always having to put information
>>> back in which was previously stripped out.
>> But my point is towards the amount of data you need to specify manually.
>> I would find it quite helpful if default-on sub-features became available
>> automatically once the top-level feature was turned on. I guess so far
>> we don't have many such cases, but here you add a whole bunch.
> 
> I'm not suggesting specifying it manually.  That would be a dumb UX move.
> 
> But you absolutely cannot have "user turns on EIBRS" meaning "turn on
> ARCH_CAPS too", because a) that requires creating the reverse feature
> map which is massively larger than the forward feature map, and b) it
> creates a vulnerability in the guest, and c) it's ambiguous - e.g. what
> does turning on a sub-mode of AVX-512 mean for sibling sub-modes?

Feels like a misunderstanding at this point, because what you're describing
above is not what I was referring to. Instead I was specifically referring
to "cpuid=...,arch-caps" not having any effect beyond the turning on of the
MSR itself (with all-zero content). This is even worse with libxl not even
having a way right now to express something like "arch-caps=..." to enable
some of the sub-features for guests.

To explicitly answer the AVX512 part of your reply: Turning on a sub-mode
alone could be useful as well (with the effect of also turning on the
main feature, and with or without [policy question] also turning on other
default-on subfeatures of AVX512). But again - that direction of possible
"implications" isn't what my earlier reply was about. As you say, reverse
maps would then also be needed, whereas for what I'm suggesting only the
deep-deps we already have are necessary: We'd grab the main feature's
dependencies and AND that with the default mask before ORing into the
domain's policy.

> Whichever algorithm you want, you still need *something* to know that
> ARCH_CAPS is special and how to arrange the defaults given a non-default
> overarching setting.

Afaict right now that would be achieved by the same 'a', 'A', '!a", and
"!A" annotations, suitably placed on every of the sub-features.

Jan

> When the toolstack infrastructure grows the ability to say no to the
> user, it will be able to identify explicit user settings which cannot be
> fulfilled.  (And with a bit more complicated logic, why.)
> 
>>>> Wouldn't it make more sense for the
>>>> individual bits' exposure qualifiers to become meaningful one to
>>>> qualifying feature is enabled? I.e. here this would then mean that
>>>> some ARCH_CAPS bits may become available, while others may require
>>>> explicit turning on (assuming they weren't all 'A').
>>> I'm afraid I don't follow.  You could make some bits of MSR_ARCH_CAPS
>>> itself 'a' vs 'A', and that would have the expected effect (for any VM
>>> where arch_caps was visible).
>> Visible by default, you mean. Whereas I'm considering the case where
>> the CPUID bit is default-off, and turning it on for a guest doesn't at
>> the same time turn on all the 'A' bits in ARCH_CAPS (which hardware
>> offers, or which we synthesize).
>>
>> Something similar could be seen / utilized for AMX, where in my
>> pending series I set all the bits to 'a', requiring every individual
>> bit to be turned on along with turning on AMX-TILE. Yet it would be
>> more user friendly if only the top-level bit needed enabling manually,
>> with available sub-features then becoming available "automatically".
> 
> I think I've covered all of this in the reply above?
> 
> ~Andrew