Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature

["Igor Mammedov" <imammedo@redhat.com>]

On Tue, 1 Oct 2019 17:31:17 +0200
"Laszlo Ersek" <lersek@redhat.com> wrote:

> On 09/27/19 13:35, Igor Mammedov wrote:
> > On Tue, 24 Sep 2019 13:34:55 +0200
> > "Laszlo Ersek" <lersek@redhat.com> wrote:  
> 
> >> Going forward, if I understand correctly, the plan is to populate the
> >> new SMRAM with the hotplug SMI handler. (This would likely be put in
> >> place by OvmfPkg/PlatformPei, from NASM source code. The
> >> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
> >> original contents before, and restores them after, the initial SMBASE
> >> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
> >> survive the initial relocation of the cold-plugged VCPUs.)  
> 
> > that's the tricky part, can we safely detect (in SmmRelocateBases)
> > race condition in case of several hotplugged CPUs are executing
> > SMI relocation handler at the same time? (crashing system in case
> > that happens is good enough)  
> 
> Wait, there are *two* initial SMI handlers here, one for cold-plugged
> CPUs, and another for hot-plugged CPUs. Let's just call them coldplug
> and hotplug handlers, for simplicity.
> 
> In chronological order, during boot:
> 
> (1) OvmfPkg/PlatformPei would put the hotplug handler in place, at the
> default SMBASE. This code would lie dormant for a long time.
> 
> (2) UefiCpuPkg/PiSmmCpuDxeSmm backs up the current contents of the
> default SMBASE area, and installs the coldplug handler. This handler is
> then actually used, for relocating SMBASE for the present (cold-plugged)
> CPUs. Finally, UefiCpuPkg/PiSmmCpuDxeSmm restores the original contents
> of the default SMBASE area. This would in effect re-establish the
> hotplug handler from (1).
> 
> (3) At OS runtime, a CPU is hotplugged, and then the hotplug handler
> runs, at the default SMBASE.
> 
> The function SmmRelocateBases() is strictly limited to step (2), and has
> nothing to do with hotplug. Therefore it need not deal with any race
> conditions related to multi-hotplug.
> 
> This is just to clarify that your question applies to the initial SMI
> handler (the hotplug one) that is put in place in step (1), and then
> used in step (3). The question does not apply to SmmRelocateBases().
> 
> 
> With that out of the way, I do think we should be able to detect this,
> with a simple -- haha, famous last words! -- compare-and-exchange (LOCK
> CMPXCHG); a kind of semaphore.
> 
> The initial value of an integer variable in SMRAM should be 1. At the
> start of the hotplug initial SMI handler, the CPU should try to swap
> that with 0. If it succeeds, proceed. If it fails (the variable's value
> is not 1), force a platform reset. Finally, right before the RSM,
> restore 1 (swap it back).
> 
> (It does not matter if another hotplug CPU starts the relocation in SMM
> while the earlier one is left with *only* the RSM instruction in SMM,
> immediately after the swap-back.)
I don't see why it doesn't matter, let me illustrate the case
I'm talking about.

assuming there are 2 hotplugged CPUs with pending SMI and
stray INIT-SIPI-SIPI in flight to CPU2.

    HP CPU1                                        HP CPU2
    save state at 0x30000                            -
    start executing hotplug SMI handler              -
    set new SMBASE                                   -
    -                                         save state at 0x30000
                                             (overwrites CPU1 set SMBASE to default one)
    -                                              .....
                     -----------zzzzzzz----------                                         
    RSM
    restores CPU1 with CPU2 saved state
     incl. CPU2 SMBASE = either default or
     already new CPU2 specific one
    
semaphore is irrelevant in this case as we will loose CPU1 relocation
at best (assuming hotplug handler does nothing else beside relocation)
and if hotplug handler does something else this case will probably
leave FW in inconsistent state.


Safest way to deal with it would be:

  1. wait till all host CPUs are brought into SMM (with hotplug SMI handler = check me in)

  2. wait till all hotplugged CPUs are put in well know state
    (a host cpu should send INIT-SIPI-SIPI with NOP vector to wake up)

  3. a host CPU will serialize hotplugged CPUs relocation by sending
     uincast SMI + INIT-SIPI-SIPI NOP vector to wake up the second time
     (with hotplug SMI handler = relocate_me)

alternatively we could skip step 3 and do following:

 hotplug_SMI_handler()   
     hp_cpu_check_in_map[apic_id] = 1;
     /* wait till another cpu tells me to continue */
     while(hp_cpu_check_in_map[apic_id]) ;;
     ...

 host_cpu_hotplug_all_cpus()
     wait till all hotpluged CPUs are in hp_cpu_check_in_map;
     for(i=0;;) {
        if (hp_cpu_check_in_map[i]) {
            /* relocate this CPU */
            hp_cpu_check_in_map[i] = 0;
        }

        tell QEMU that FW pulled the CPU in;
        /* we can add a flag to QEMU's cpu hotplug MMIO interface to FW do it,
           so that legitimate  GPE handler would tell OS to online only
           firmware vetted CPUs */
     }

> Alternatively, if it's friendlier or simpler, we can spin on the initial
> LOCK CMPXCHG until it succeeds, executing PAUSE in the loop body. That
> should be friendly with KVM too (due to pause loop exiting).
> 
> 
> >> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
> >> for it (perhaps internally to QEMU?),  
> 
> > I'd rather rise SMI from guest side (using IO port write from
> > ACPI cpu hotplug handler). Which in typical case (QEMU negotiated
> > SMI broadcast) will trigger pending SMI on hotplugged CPU as well.
> > (if it's acceptable I can prepare corresponding QEMU patches)  
> 
> What if the OS does not send the SMI (from the GPE handler) at once, and
> boots the new CPU instead?
> 
> Previously, you wrote that the new CPU "won't get access to privileged
> SMRAM so OS can't subvert firmware. The next time SMI broadcast is sent
> the CPU will use SMI handler at default 30000 SMBASE. It's up to us to
> define behavior here (for example relocation handler can put such CPU in
> shutdown state)."
> 
> How can we discover in the hotplug initial SMI handler at 0x3_0000 that
> the CPU executing the code should have been relocated *earlier*, and the
> OS just didn't obey the ACPI GPE code? I think a platform reset would be
> a proper response, but I don't know how to tell, "this CPU should not be
> here".

one can ask QEMU about present CPUs (via cpu hotplug interface)
and compare with present CPU state in FW (OS can hide insert
event there but not the presence).

another way is to keep it pinned in relocation SMI handler
until another CPU tells it to continue with relocation.

In absence of SMI, such CPU will continue to run wild but do we
really care about it?

> > In case of unicast SMIs, a CPU handling guest ACPI triggered
> > SMI, can send SMI to hotpluged CPU as an additional step.  
> 
> I think it's OK if we restrict this feature to "broadcast SMI" only.
> 
> >   
> >> which will remain pending until
> >> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
> >> sent, the VCPU will immediately enter SMM, and run the SMI handler in
> >> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
> >> vector requested in the INIT-SIPI-SIPI.  
> > Firmware can arbitrate/send INIT-SIPI-SIPI to hotplugged CPUs.
> > Enumerating hotplugged CPUs, can be done by reusing CPU hotplug
> > interface described at QEMU/docs/specs/acpi_cpu_hotplug.txt.
> > (preferably in read-only mode)  
> 
> ... I'll have to look at that later :)
> 
> Thanks
> Laszlo
> 
> 
>