From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web11.23909.1598548229060095158 for ; Thu, 27 Aug 2020 10:10:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=jVf55n5d; spf=pass (domain: nuviainc.com, ip: 209.85.221.68, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f68.google.com with SMTP id w13so6081914wrk.5 for ; Thu, 27 Aug 2020 10:10:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=heKYo/BqMHx9Meyk0AB/ZmnB3IzhOYYeqqtYq7LW4Fg=; b=jVf55n5d6pwa5EJIfxJC3Alqkp7JtJLeqhFG4HXS4zJL3l1gX39+z49p66Kd4+BDCC 19CRM6X7UmmkLoz5zQVaEFaZOu6vo/UTvkOZOEEk7oaftX1ybabpUB5O8Xin98BYNfc4 IIzPu3nGvFEfVvcRlE/YEJuMhINb3GjqHMxpf6dNHvpJEQIqkZFLIybmySzNlCvYflNo B0RKUtK3bmvwXnEKSMYIOdDDGJ5Lyu+M41RMvc+sv2+uCyhq1fgBk3ANX+ogBW8nXxTb itKudEL6PKKVL2GfoggedTiGsniMjPw5O9w8lwYPFm1E3v/0gzDQ8aGCe9ilgJah97jh 1/6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=heKYo/BqMHx9Meyk0AB/ZmnB3IzhOYYeqqtYq7LW4Fg=; b=B6RJNz5hDZ2aC5NUoyQbKjYtzZyCxB82rrDmAP71AyYfvK1hTnTMJBEIS4m6EtYsPM sBE+fuyUHl9gDpZo+AY+gsQ+e0+ydZQN7fFH6hwRInjmcf7plNRsydc237LYjC0Hl5T6 RiJFo4ON4zN/9gFmupMocil0p5Tos+oXIzdjrA0CCNKunlzrb++xsowEWnQq5jtQbaEy DvFQMH1M58Nz/lR/eP+RoyJ5ewwwAtZGpYuqrbS5Hc+RtLoElMic22HdXuUn8algOhZj cZMwx55e4YMaj9ELpNqlKVXDC2pnWsU9biXTRaCaPeoUUUyibKRAASGgcWNLJ7wYDiHO 5zBQ== X-Gm-Message-State: AOAM531pbQ1kGz1CBTIz8DuESGeqOgij0zb75YqPQEI2w0/Lgg/MhjNZ RluCJYPxJ8tb5CAesfp7Pp694A== X-Google-Smtp-Source: ABdhPJxHlGJydgVQHPw/isoUrkHPpeiB5oYRezJLU9s4yzYfOGyjjGpOfK0gF60U/y+iSvAqnrm24Q== X-Received: by 2002:a5d:554b:: with SMTP id g11mr20870319wrw.169.1598548227384; Thu, 27 Aug 2020 10:10:27 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id d66sm6655844wmc.16.2020.08.27.10.10.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Aug 2020 10:10:26 -0700 (PDT) Date: Thu, 27 Aug 2020 18:10:25 +0100 From: "Leif Lindholm" To: Graeme Gregory Cc: devel@edk2.groups.io, tanmay.jagdale@linaro.org Subject: Re: [edk2-plaforms PATCH 1/1] SbsaQemu: Fix numerous SSDT generation problems Message-ID: <20200827171025.GY1191@vanye> References: <20200827152133.255281-1-graeme@nuviainc.com> MIME-Version: 1.0 In-Reply-To: <20200827152133.255281-1-graeme@nuviainc.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline A few minor typos in the commit message: On Thu, Aug 27, 2020 at 16:21:33 +0100, Graeme Gregory wrote: > 1 - The SBSAQEMU_ACPI_ITOA contained a typo that put bogus characters > in the name if number of CPUs was greater than 10. It is safter to use safter -> safer > the AsciiSPrint function from PrintLib. > > 2 - The _UID fields were bogus, and indicated as bytes in AML instead of > a word. This caused extra Zeros to appear in disassembly. Fixed by Zero's -> Zeros > making them AML_WORD_PREFIX and putting CpuId in little endian. > > 3 - The table was a number of bytes too long causes bogus Zero in missing ", which" > dissassembly at end of table. Re-adjust code slightly to reduce table > size once we know the size of the length field. > > Signed-off-by: Graeme Gregory Reviewed-by: Leif Lindholm I took the liberty to address the typos locally before pushing as b54d65a4a671. Thanks! / Leif > --- > > This patch supercedes "SbsaQemu: Fix CPUID generation in SSDT" > as while fixing review comments on that I found the other issues. > > .../SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 1 + > .../Include/IndustryStandard/SbsaQemuAcpi.h | 5 +--- > .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 25 +++++++++++-------- > 3 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > index cde9d02f7f90..127eef029f3c 100644 > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > @@ -36,6 +36,7 @@ [LibraryClasses] > DxeServicesLib > FdtLib > PcdLib > + PrintLib > UefiDriverEntryPoint > UefiLib > UefiRuntimeServicesTableLib > diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h > index 1a7d9dda2b99..f085765d2677 100644 > --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h > +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h > @@ -50,9 +50,6 @@ > #define SBSAQEMU_ACPI_CPU_DEV_LEN 0x1C > #define SBSAQEMU_ACPI_CPU_DEV_NAME { 'C', '0', '0', '0' } > > -// Macro to convert Integer to Character > -#define SBSAQEMU_ACPI_ITOA(Byte) (0x30 + (Byte > 9 ? (Byte + 1) : Byte)) > - > #define SBSAQEMU_ACPI_CPU_HID { \ > AML_NAME_OP, AML_NAME_CHAR__, 'H', 'I', 'D', \ > AML_STRING_PREFIX, 'A', 'C', 'P', 'I', '0', '0', '0', '7', \ > @@ -60,7 +57,7 @@ > } > > #define SBSAQEMU_ACPI_CPU_UID { \ > - AML_NAME_OP, AML_NAME_CHAR__, 'U', 'I', 'D', AML_BYTE_PREFIX, \ > + AML_NAME_OP, AML_NAME_CHAR__, 'U', 'I', 'D', AML_WORD_PREFIX, \ > AML_ZERO_OP, AML_ZERO_OP \ > } > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > index 06552f4b22f3..47a9bd1d423a 100644 > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -248,6 +249,7 @@ AddSsdtTable ( > UINT32 TableSize; > EFI_PHYSICAL_ADDRESS PageAddress; > UINT8 *New; > + UINT8 *HeaderAddr; > UINT32 CpuId; > UINT32 Offset; > UINT8 ScopeOpName[] = SBSAQEMU_ACPI_SCOPE_NAME; > @@ -283,12 +285,12 @@ AddSsdtTable ( > return EFI_OUT_OF_RESOURCES; > } > > - New = (UINT8 *)(UINTN) PageAddress; > + HeaderAddr = New = (UINT8 *)(UINTN) PageAddress; > ZeroMem (New, TableSize); > > // Add the ACPI Description table header > CopyMem (New, &Header, sizeof (EFI_ACPI_DESCRIPTION_HEADER)); > - ((EFI_ACPI_DESCRIPTION_HEADER*) New)->Length = TableSize; > + > New += sizeof (EFI_ACPI_DESCRIPTION_HEADER); > > // Insert the top level ScopeOp > @@ -296,6 +298,11 @@ AddSsdtTable ( > New++; > Offset = SetPkgLength (New, > (TableSize - sizeof (EFI_ACPI_DESCRIPTION_HEADER) - 1)); > + > + // Adjust TableSize now we know header length of _SB > + TableSize -= (SBSAQEMU_ACPI_SCOPE_OP_MAX_LENGTH - Offset); > + ((EFI_ACPI_DESCRIPTION_HEADER*) HeaderAddr)->Length = TableSize; > + > New += Offset; > CopyMem (New, &ScopeOpName, sizeof (ScopeOpName)); > New += sizeof (ScopeOpName); > @@ -303,21 +310,17 @@ AddSsdtTable ( > // Add new Device structures for the Cores > for (CpuId = 0; CpuId < NumCores; CpuId++) { > SBSAQEMU_ACPI_CPU_DEVICE *CpuDevicePtr; > - UINT8 CpuIdByte1, CpuIdByte2, CpuIdByte3; > > CopyMem (New, &CpuDevice, sizeof (SBSAQEMU_ACPI_CPU_DEVICE)); > CpuDevicePtr = (SBSAQEMU_ACPI_CPU_DEVICE *) New; > > - CpuIdByte1 = CpuId & 0xF; > - CpuIdByte2 = (CpuId >> 4) & 0xF; > - CpuIdByte3 = (CpuId >> 8) & 0xF; > + AsciiSPrint((CHAR8 *)&CpuDevicePtr->dev_name[1], 4, "%03X", CpuId); > > - CpuDevicePtr->dev_name[1] = SBSAQEMU_ACPI_ITOA(CpuIdByte3); > - CpuDevicePtr->dev_name[2] = SBSAQEMU_ACPI_ITOA(CpuIdByte2); > - CpuDevicePtr->dev_name[3] = SBSAQEMU_ACPI_ITOA(CpuIdByte1); > + /* replace character lost by above NULL termination */ > + CpuDevicePtr->hid[0] = AML_NAME_OP; > > - CpuDevicePtr->uid[6] = CpuIdByte1 | CpuIdByte2; > - CpuDevicePtr->uid[7] = CpuIdByte3; > + CpuDevicePtr->uid[6] = CpuId & 0xFF; > + CpuDevicePtr->uid[7] = (CpuId >> 8) & 0xFF; > New += sizeof (SBSAQEMU_ACPI_CPU_DEVICE); > } > > -- > 2.25.1 >