Re: [PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD

[Jan Beulich <jbeulich@suse.com>]

On 28.03.2022 17:24, Roger Pau Monné wrote:
> On Mon, Mar 28, 2022 at 04:21:02PM +0200, Jan Beulich wrote:
>> On 15.03.2022 15:18, Roger Pau Monne wrote:
>>> @@ -677,14 +680,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>>>          if ( !cp->extd.virt_ssbd )
>>>              goto gp_fault;
>>>  
>>> -        /*
>>> -         * Only supports SSBD bit, the rest are ignored. Only modify the SSBD
>>> -         * bit in case other bits are set.
>>> -         */
>>> -        if ( val & SPEC_CTRL_SSBD )
>>> -            msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
>>> +        /* Only supports SSBD bit, the rest are ignored. */
>>> +        if ( cpu_has_amd_ssbd )
>>> +        {
>>> +            /* Only modify the SSBD bit in case other bits are set. */
>>
>> While more a comment on the earlier patch introducing this wording, it
>> occurred to me only here that this is ambiguous: It can also be read as
>> "Only modify the SSBD bit as long as other bits are set."
> 
> Hm, no, that's not what I meant. I meant to note that here we are
> careful to only modify the SSBD bit of spec_ctrl, because other bits
> might be used for other purposes.

Right, I understand that's what you mean, and because I understand
the ambiguity also slipped my attention in the earlier patch.

> We can't do:
> 
> msrs->spec_ctrl.raw = SPEC_CTRL_SSBD;
> 
> But maybe this doesn't require a comment, as it seems to raise more
> questions than answer?

I wouldn't mind if (in the earlier patch) you simply dropped the 2nd
sentence. Or alternatively how about "Also only record the SSBD bit
to return for future reads" or something along these lines?

Jan