From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f194.google.com (mail-qk1-f194.google.com [209.85.222.194]) by mx.groups.io with SMTP id smtpd.web11.3881.1570750439741415493 for ; Thu, 10 Oct 2019 16:33:59 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@semihalf-com.20150623.gappssmtp.com header.s=20150623 header.b=QeHlUTlX; spf=none, err=SPF record not found (domain: semihalf.com, ip: 209.85.222.194, mailfrom: mw@semihalf.com) Received: by mail-qk1-f194.google.com with SMTP id z67so7218685qkb.12 for ; Thu, 10 Oct 2019 16:33:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=ufaUjNDx0UwQCC1GG+HEfSKYXSofpC1N6YxV+xRsRZc=; b=QeHlUTlXyKK4pRgJ8e72jcaF9e990o2CQ9xOb383TGV0PjNQGw1evFcYST5sLRyqLb pWlKcUuhOsUIH8hMEmUVtzZBmD3y629Vl8bqv9tSj8Uir/tThE7c4Y7ryP6BegUEXts4 qSjZzomE5BApf1FC48Oy/1iqKyjdehDQW3yibpFf6GopZMyCVMl/oKeEpWkB0w5Sn3iC lDeubzG4rL3s9zx88OtjbYkhWnGfE2Fll+4Lejq+3wYUFIz8oabjWCxEMeklCMBVb7jx ZcwBKvWUpga/6b3RbvAh9jUszAFk5zoNQVTyHVXOwqPD5K5fDq5tLHDRDqpnYhH6A8br JZZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=ufaUjNDx0UwQCC1GG+HEfSKYXSofpC1N6YxV+xRsRZc=; b=ln2SFgy0ZEpG386agmx/y2JN0gCgeGVySNBsskMooImfe1A7o77G+ABpCgU+fj9ZzJ P/eHchES9tAVOBWf79qwwQrUnUueDSaKzoo/xsRrBGhxbnKjpcLPHcQX1kmHUxgHUZ1z up+h9FvyVPtkPARyzdH55B4sbVFL7Fz82ZMfuj2GOGSRw7iFcfTqDCYI47mSyCE9YKYR 9lfspnUK4zuuMOSfhC9Nkr5CYovDmGJSkkdj70Zb1SYWAwXrXwMLRe4mXfFwbMm02Usd PwdnpSr/w3ieW8Re892zSPJ7CVi3qBloS7SUjbIVTKm1+cuZVonauC2ruMMETK/DgH1u 9Kdw== X-Gm-Message-State: APjAAAUtBo0M473s0ZxsjrGeS+CdKdxKcX/7D03BF/0/vNqqqyPxGTGD XhqJZKsv+Hav3uF4imgN5rx3VNcyuY3yzsQwUej6dA== X-Google-Smtp-Source: APXvYqzSt1ob6y3xeQ8tm4zzZo8bbS7nPS3/8nwxlj317SIBN/6rvHZdyb516TqCZU/lvfJkXZs7PquaWMpVAELmbOA= X-Received: by 2002:a37:4cd5:: with SMTP id z204mr12323575qka.153.1570750438746; Thu, 10 Oct 2019 16:33:58 -0700 (PDT) MIME-Version: 1.0 References: <1570686139-25182-1-git-send-email-mw@semihalf.com> <1570686139-25182-9-git-send-email-mw@semihalf.com> <20191010230430.GX25504@bivouac.eciton.net> In-Reply-To: <20191010230430.GX25504@bivouac.eciton.net> From: "Marcin Wojtas" Date: Fri, 11 Oct 2019 01:33:49 +0200 Message-ID: Subject: Re: [edk2-platforms: PATCH v3 8/9] Marvell/Drivers: SmbiosPlatformDxe: Load SMBIOS strings from PCD To: Leif Lindholm Cc: edk2-devel-groups-io , Ard Biesheuvel , "jsd@semihalf.com" , Grzegorz Jaszczyk , Kostya Porotchkin , Patryk Duda Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Leif, pt., 11 pa=C5=BA 2019 o 01:04 Leif Lindholm napi= sa=C5=82(a): > > On Thu, Oct 10, 2019 at 07:42:18AM +0200, Marcin Wojtas wrote: > > From: Patryk Duda > > > > This patch implements convenient way of changing strings included > > in SMBIOS Table1, Table2, Table3. > > > > Strings can be altered by defining following PCDs: > > gMarvellTokenSpaceGuid.PcdProductManufacturer > > gMarvellTokenSpaceGuid.PcdProductPlatformName > > gMarvellTokenSpaceGuid.PcdProductVersion > > gMarvellTokenSpaceGuid.PcdProductSerial > > > > This patch adds also limit for length of string which can be increased > > if necessary in future. > > > > Signed-off-by: Patryk Duda > > --- > > Silicon/Marvell/Marvell.dec | 6 += + > > Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 4 + > > Silicon/Marvell/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 79 += ++++++++++++++++--- > > 3 files changed, 78 insertions(+), 11 deletions(-) > > > > diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec > > index d337d3e..a84b056 100644 > > --- a/Silicon/Marvell/Marvell.dec > > +++ b/Silicon/Marvell/Marvell.dec > > @@ -169,6 +169,12 @@ > > gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x0 }|VOID*|0x3000034 > > gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x3000035 > > > > +#Platform description > > + gMarvellTokenSpaceGuid.PcdProductManufacturer|"Marvell = \0"|VOID*|0x50000100 > > + gMarvellTokenSpaceGuid.PcdProductPlatformName|"Marvell Development B= oard \0"|VOID*|0x50000101 > > + gMarvellTokenSpaceGuid.PcdProductSerial|"Serial Not Set = \0"|VOID*|0x50000103 > > + gMarvellTokenSpaceGuid.PcdProductVersion|"Revision unknown = \0"|VOID*|0x50000102 > > + > > I'm not too pleased about this random number of spaces. I'm cool with > the strings, but they should be treated as such, not magical data > structures. In v4 the trailing spaces will be removed from the defaults (as well as "\0= "). > > +STATIC > > +VOID > > +MvSmbiosCopyStrings ( > > + VOID > > + ) > > +{ > > + EFI_STATUS Status; > > + > > + ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductManufacturer), > > + MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE); > > + ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductPlatformName), > > + MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE); > > + ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductVersion), > > + MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE); > > + ASSERT (AsciiStrnLenS ((CHAR8 *)PcdGetPtr (PcdProductSerial), > > + MV_SMBIOS_STRING_MAX_SIZE) < MV_SMBIOS_STRING_MAX_SIZE); > > Especially given the current design, these ASSERTs seem a bit > ... unhelpful. In fact, this whole MAX_SIZE thing seems ... restricted > by the implementation, not by external constraints. What is the > benefit? Not having to do a bunch of pointer conversions at > SetVirtualAddressMap? > The default static tables require constant initializers and the placeholders should have some predefined size in current approach. So max of 32 characters was picked and with the asserts, ensuring the user will not exceed it. Do you think they should be removed? Best regards, Marcin > > > + > > + Status =3D AsciiStrCpyS (mSysInfoManufacturer, > > + MV_SMBIOS_STRING_MAX_SIZE, > > + (CHAR8 *)PcdGetPtr (PcdProductManufacturer)); > > + ASSERT_EFI_ERROR (Status); > > + Status =3D AsciiStrCpyS (mSysInfoProductName, > > + MV_SMBIOS_STRING_MAX_SIZE, > > + (CHAR8 *)PcdGetPtr (PcdProductPlatformName)); > > + ASSERT_EFI_ERROR (Status); > > + Status =3D AsciiStrCpyS (mSysInfoVersion, > > + MV_SMBIOS_STRING_MAX_SIZE, > > + (CHAR8 *)PcdGetPtr (PcdProductVersion)); > > + ASSERT_EFI_ERROR (Status); > > + Status =3D AsciiStrCpyS (mSysInfoSerial, > > + MV_SMBIOS_STRING_MAX_SIZE, > > + (CHAR8 *)PcdGetPtr (PcdProductSerial)); > > + ASSERT_EFI_ERROR (Status); > > +} > > + > > +/** > > Install all structures from the DefaultTables structure > > > > @param Smbios SMBIOS protocol > > @@ -760,6 +815,8 @@ SmbiosInstallAllStructures ( > > FirmwareMajorRevisionNumber =3D (PcdGet32 (PcdFirmwareRevision) >> 1= 6) & 0xFF; > > FirmwareMinorRevisionNumber =3D PcdGet32 (PcdFirmwareRevision) & 0xF= F; > > > > + MvSmbiosCopyStrings(); > > + > > // > > // Update Firmware Revision, CPU and DRAM frequencies. > > // > > -- > > 2.7.4 > >