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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2022 18:11, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 05:49:42PM +0200, Roger Pau Monné wrote:
>> On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
>>> On 27.04.2022 12:47, Roger Pau Monne wrote:
>>>> +void amd_set_legacy_ssbd(bool enable)
>>>> +{
>>>> +  const struct cpuinfo_x86 *c = &current_cpu_data;
>>>> +  struct ssbd_ls_cfg *status;
>>>> +
>>>> +  if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
>>>> +          BUG_ON(!set_legacy_ssbd(c, enable));
>>>> +          return;
>>>> +  }
>>>> +
>>>> +  BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
>>>> +  BUG_ON(c->cpu_core_id >= ssbd_max_cores);
>>>> +  status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
>>>> +                        c->cpu_core_id];
>>>> +
>>>> +  /*
>>>> +   * Open code a very simple spinlock: this function is used with GIF==0
>>>> +   * and different IF values, so would trigger the checklock detector.
>>>> +   * Instead of trying to workaround the detector, use a very simple lock
>>>> +   * implementation: it's better to reduce the amount of code executed
>>>> +   * with GIF==0.
>>>> +   */
>>>> +  while ( test_and_set_bool(status->locked) )
>>>> +      cpu_relax();
>>>> +  status->count += enable ? 1 : -1;
>>>> +  ASSERT(status->count <= c->x86_num_siblings);
>>>> +  if (enable ? status->count == 1 : !status->count)
>>>> +          BUG_ON(!set_legacy_ssbd(c, enable));
>>>
>>> What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
>>> region?
>>
>> So AFAICT the BUG itself works, the usage of a crash kernel however
>> won't work as it's booted with GIF==0.
>>
>> Maybe we need to issue an stgi as part of BUG_FRAME if required?
>> (maybe that's too naive...)
> 
> Well, better in panic() or kexec_crash() likely.

Yeah, lifting it too early may be detrimental.

Jan