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: Wed, 17 May 2023 11:20:14 +0200	[thread overview]
Message-ID: <a53a77e3-6dcd-2668-0f3c-282de93d8b04@suse.com> (raw)
In-Reply-To: <54b35fa0-160e-3035-6c22-65a791ed2f84@citrix.com>

On 16.05.2023 21:31, Andrew Cooper wrote:
> On 16/05/2023 3:53 pm, Jan Beulich wrote:
>> On 16.05.2023 16:16, Andrew Cooper wrote:
>>> On 16/05/2023 3:06 pm, Jan Beulich wrote:
>>>> On 16.05.2023 15:51, Andrew Cooper wrote:
>>>>> On 16/05/2023 2:06 pm, Jan Beulich wrote:
>>>>>> On 15.05.2023 16:42, Andrew Cooper wrote:
>>>>>> Further is even just non-default exposure of all the various bits okay
>>>>>> to other than Dom0? IOW is there indeed no further adjustment necessary
>>>>>> to guest_rdmsr()?
>>>> With your reply further down also sufficiently clarifying things for
>>>> me (in particular pointing the one oversight of mine), the question
>>>> above is the sole part remaining before I'd be okay giving my R-b here.
>>> Oh sorry.  Yes, it is sufficient.  Because VMs (other than dom0) don't
>>> get the ARCH_CAPS CPUID bit, reads of MSR_ARCH_CAPS will #GP.
>>>
>>> Right now, you can set cpuid = "host:arch-caps" in an xl.cfg file.  If
>>> you do this, you get to keep both pieces, as you'll end up advertising
>>> the MSR but with a value of 0 because of the note in patch 4.  libxl
>>> still only understand the xend CPUID format and can't express any MSR
>>> data at all.
>> Hmm, so the CPUID bit being max only results in all the ARCH_CAPS bits
>> getting turned off in the default policy. That is, to enable anything
>> you need to not only enable the CPUID bit, but also the ARCH_CAPS bits
>> you want enabled (with no presents means to do so).
> 
> Correct.
> 
>> 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.

>> 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".

> The thing which is 99% of the complexity with MSR_ARCH_CAPS is getting
> RSBA/RRSBA right.  The moment we advertise MSR_ARCH_CAPS to guests,
> RSBA/RRSBA must be set appropriately for migrate or we're creating a
> security vulnerability in the guest.
> 
> If you're wondering about the block disable, that's because MSRs and
> CPUID are different.  With CPUID, we have
> x86_cpu_policy_clear_out_of_range_leaves() which uses the various
> max_leaf.  e.g. a feat.max_leaf=0 is what causes all of subleaf 1 and 2
> to be zeroed in a policy.
> 
> 
>> But irrespective of that (which is kind of orthogonal) my question was
>> rather with already considering the point in time when the CPUID bit
>> would become 'A'. IOW I was wondering whether at that point having all
>> the individual bits be 'A' is actually going to be correct.
> 
> I've chosen all 'A' for these bits because that is what I expect to be
> correct in due course.  They're all the simple "you're not vulnerable to
> $X" bits, plus eIBRS which in practice is just a qualifying statement on
> IBRS (already fully supported in guests).

Right, upon checking again I agree.

Jan

> The rest of MSR_ARCH_CAPS is pretty much a dumping ground for all of the
> controls we can't give to guests under any circumstance.  (FB_CLEAR_CTRL
> might be an exception - allegedly we might want to give it to guests
> which have passthrough and trust their userspace, but I'm unconvinced of
> this argument and going to insist on concrete numbers from anyone
> wanting to try and implement this usecase.)
> 
> But there certainly could be a feature in there in the future where we
> leave it at 'a' for a while...  It's just feature bitmap data in a
> non-CPUID form factor.
> 
> ~Andrew