Re: [PATCH v6 3/9] OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper

["Ankur Arora" <ankur.a.arora@oracle.com>]

On 2021-01-29 6:36 p.m., Laszlo Ersek wrote:
> On 01/29/21 01:59, Ankur Arora wrote:
>> Add QemuCpuhpWriteCpuStatus() which will be used to update the QEMU
>> CPU status register. On error, it hangs in a similar fashion as
>> other helper functions.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Aaron Young <aaron.young@oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.h |  6 ++++++
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 22 ++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
>> index 8adaa0ad91f0..804809846890 100644
>> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
>> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
>> @@ -30,6 +30,12 @@ QemuCpuhpReadCpuStatus (
>>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>>     );
>>   
>> +VOID
>> +QemuCpuhpWriteCpuStatus (
>> +  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
>> +  IN UINT8                        CpuStatus
>> +  );
>> +
>>   UINT32
>>   QemuCpuhpReadCommandData (
>>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
>> index f871e50c377b..ed44264de934 100644
>> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
>> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
>> @@ -67,6 +67,28 @@ QemuCpuhpReadCpuStatus (
>>     return CpuStatus;
>>   }
>>   
>> +VOID
>> +QemuCpuhpWriteCpuStatus (
>> +  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
>> +  IN UINT8                        CpuStatus
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +
>> +  Status = MmCpuIo->Io.Write (
>> +                         MmCpuIo,
>> +                         MM_IO_UINT8,
>> +                         ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_R_CPU_STAT,
>> +                         1,
>> +                         &CpuStatus
>> +                         );
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
>> +    ASSERT (FALSE);
>> +    CpuDeadLoop ();
>> +  }
>> +}
>> +
>>   UINT32
>>   QemuCpuhpReadCommandData (
>>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>>
> 
> The code is fine, but please move the new function (both declaration and
> definition) between QemuCpuhpWriteCpuSelector() and QemuCpuhpWriteCommand().
> 
> Reason: the pre-patch order of the functions matches the order of the
> register descriptions in QEMU's "docs/specs/acpi_cpu_hotplug.txt".
> 
> There, we first have a section called "read access", then another called
> "write access". And in each section, registers are listed in increasing
> offset order, within the hotplug register block.

Will fix.

Thanks
Ankur

> 
> Thanks!
> Laszlo
>