From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) by mx.groups.io with SMTP id smtpd.web09.6699.1609717928134635097 for ; Sun, 03 Jan 2021 15:52:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=BA/Bchya; spf=pass (domain: nuviainc.com, ip: 209.85.219.54, mailfrom: rebecca@nuviainc.com) Received: by mail-qv1-f54.google.com with SMTP id et9so12288636qvb.10 for ; Sun, 03 Jan 2021 15:52:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=jP4icVHjZrtuSGUv06T/iNxVh9yHEXeMvNWAnO28fvY=; b=BA/Bchyaq5DGXM6t5xjZH7WGV+URqXfT8fTtthUvy3lpPNw6Lma84zVg2uliRMBut/ z+wHsLX9IMxb0r92Q/hQdHtaD7QWn9HQNE9LdUh40dRbxyzpbQZoGCsO8Z/ahyYKDUI8 GcZAE3Li6+QEPo8zIRjOBJzNyV4nHn8nh/Iu6guSttKVB3AEgWlbw25NcUUHFtJXnmL4 fc35eaqgH4YYdui7S3hyFaKQVwUgh5vSEeATAGfkteRnDDKSOLYEQB4rrDlLf1n0i5c2 poqg1B79Ih+0D7/GJkj/QdS5ZVHb5zbtY/kMAdmSSaA3UCT89gEgcm5UsG0Oox4lZ7tO 8+WQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jP4icVHjZrtuSGUv06T/iNxVh9yHEXeMvNWAnO28fvY=; b=DSmXHto+mIy7gIwptAOzmedK/RWnV2yvd3PMLw992xEh3F0qj+eVydtQq5AU10XLU3 jX97mMdInVxaYbJGsacOcHnBTedMYS3i05blIdFfxXKE4zN5nfK2UrwVtMweUuKtORkQ AdZR1eSibgTTZ0WfkN8U/WxPJvZt4yq09J85V7dhPYUIU1/mt1qScx9Eyevaclm3L5Ff 4nasCghDzdVZNWQ+EkP+YsHB+wW9W6r23AnutwKDXL3L39LJzieu8hdCyG4y0zuVSSCr k88iyh+dcZlgtQKdZqe95FxFqOgUyEQ9eIYEVXFz3wzpYZpoCCAcXAXIJPvBQ6EojIPZ l+fg== X-Gm-Message-State: AOAM531UdcOdzzX6haZ4CZ/BOB3CyF48kl/3Gfsp5whaXzdbLnf3dOAq uKU9tK2C/9UBHQFo9XYaFK750A== X-Google-Smtp-Source: ABdhPJy+SXGWVvmr8G1He3w5ikGHz3pDub3Vp8IDQTAgYlZzu5Xhj/oLpDr+EoqbxsB3Mr6aKOZJsg== X-Received: by 2002:a0c:e74a:: with SMTP id g10mr49908203qvn.3.1609717927301; Sun, 03 Jan 2021 15:52:07 -0800 (PST) Return-Path: Received: from [10.0.10.142] (c-174-52-16-57.hsd1.ut.comcast.net. [174.52.16.57]) by smtp.gmail.com with ESMTPSA id o29sm36826572qtl.7.2021.01.03.15.52.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 03 Jan 2021 15:52:06 -0800 (PST) Subject: Re: [edk2-devel] [PATCH v4 10/10] ArmPkg: Add Universal/Smbios, a generic SMBIOS library for ARM To: Sami Mujawar , "devel@edk2.groups.io" Cc: Michael D Kinney , Liming Gao , Zhiguang Liu , Leif Lindholm , Ard Biesheuvel , nd References: <20201207175427.28712-1-rebecca@nuviainc.com> <20201207175427.28712-11-rebecca@nuviainc.com> <16513B32D4BA613C.12945@groups.io> From: "Rebecca Cran" Message-ID: <438bc3e8-5f04-e7b1-1043-69392cc7183a@nuviainc.com> Date: Sun, 3 Jan 2021 16:52:04 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/29/20 8:10 AM, Sami Mujawar wrote: > Apologies for the delay in reviewing this patch. > Please find my response inline marked [SAMI]. Thanks. I have a few comments: I've replied inline. Where I haven't replied, I've made the changes you suggested. > + 2, // ProcessorManufacture > [SAMI] Would defining a macro PROC_MANUFACTURER_STR_ID or an enum be helpful? Similarly, for other strings used across SMBIOS tables in this patch. > [/SAMI] The string IDs are specific to the table, so I'm not sure we should define a constant to re-use? > + Clidr.Data = ReadCLIDR (); > [SAMI] The ReadCLIDR() and similar functions would run on the current PE. I think this code would not work with a big.LITTLE system or a system that utilises a DSU with different CPUs. > Is the assumption here that all PEs in the system are same? > [/SAMI] Yes, that code currently assumes all CPUs are the same. I'll add code to allow platforms to specify different cache information tables for CPUs that are different. > + FreePool (HandleArray); > [SAMI] Please correct me if I am wrong, from the spec it appears that there can be n handles appended at the end of the table. However, the code above appears to only assign the first handle. > I think GetLinkTypeHandle() should be called before allocating the memory for the SmbiosRecord. That way additional space for the n handles can be allocated. The handle list can then be appended to the end of the TYPE2 table. > So, the table data should look something like: SMBIOS_TABLE_TYPE2 + (n * HANDLES) + StringData. > Does this also mean that the TYPE2 table should be the last table to be populated? Should SmbiosMiscEntryPoint() be modified to schedule the population of TYPE2 table at the end? > [/SAMI] The code fetches the first of the _chassis_ handles, which it's presumed there will only be one. > + //ContainedElements > + (VOID)CopyMem (SmbiosRecord + 1, &ContainedElements, ExtendLength); > [SAMI] If I understand correctly, the Contained Element data is never really copied, right? > [/SAMI] I'm not sure why it wouldn't be copied. Could you elaborate? -- Rebecca Cran