From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::341; helo=mail-wm1-x341.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 522EB21962301 for ; Mon, 7 Jan 2019 06:09:38 -0800 (PST) Received: by mail-wm1-x341.google.com with SMTP id b11so1007805wmj.1 for ; Mon, 07 Jan 2019 06:09:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=bqPuKT54ZOQPJTuVqHjYa5eyAnZUR0dB9t3Me/HVsfU=; b=L5yHxnjZ9mQ24vc1HUIZBkhbBxNIXRrqsVhDCMk1kMUGW9uitDINbwGX8SWsQZurzj /BAY4tbMucvfHfFCjXjyydGdbqagQUzNbL6XbaCJrheJ/yjJvU6/VwPWUnYlF/PVPl8v bzi6ULwnCGt7EAnaB1aiO1oFEaUkLGyQug0Pk= 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=bqPuKT54ZOQPJTuVqHjYa5eyAnZUR0dB9t3Me/HVsfU=; b=TlME5jEIaOwLak3qH41qD57aVMvd0B59TIObF7OZa9H3k4stkJlBLaIwl4FuN8WeSs BACL5VUH2Ers5Av0Sp4dFNQJdDWTgIYDt4Kis4nCtLwdcfInfUFf7PYIR6/PrHT/+NZh peJVvGI/pnms4nuYnEuPDwFJFPJDCZnvJ00KyRFEek5cH81MWtBPx0v3mo8AWtpvu6fv DpVOWyhCmf1fn4wW9UHET/GDVFrXGolAVu+yagFhxqxiDsFnW3OB8x/JWH1NbTAmSOIZ bf/Z0ytudK5pvgWw4NFNxa0UddLl6S6yhYAk39afM6JF68nduEv8KET7PxGQ65e85m7O oLSg== X-Gm-Message-State: AJcUukcJsUr/vF6nEO/1fsSL2H/VE4tzrY/F5abmIveMLY+lFQjyMOwm kW0WAQcAI05n5R8k00EiXwzzew== X-Google-Smtp-Source: ALg8bN70K89bO+hCp0D/PpEOYoe53961jsOTOd6cPKzvPxVQkLJ5g8+wwaiCkenWMcth6SyDIzYfXg== X-Received: by 2002:a1c:541a:: with SMTP id i26mr9057813wmb.128.1546870175167; Mon, 07 Jan 2019 06:09:35 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id q14sm43309911wrw.39.2019.01.07.06.09.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 07 Jan 2019 06:09:34 -0800 (PST) Date: Mon, 7 Jan 2019 14:09:32 +0000 From: Leif Lindholm To: AKASHI Takahiro Cc: Alexander Graf , Heinrich Schuchardt , trini@konsulko.com, robdclark@gmail.com, u-boot@lists.denx.de, edk2-devel@lists.01.org, Jian J Wang , Hao Wu , Ruiyu Ni , Star Zeng , Andrew Fish , Laszlo Ersek , Michael D Kinney , Ard Biesheuvel Message-ID: <20190107140932.uefkly3a3jzlyjjf@bivouac.eciton.net> References: <20181214101043.14067-1-takahiro.akashi@linaro.org> <20181214101043.14067-3-takahiro.akashi@linaro.org> <20181217011626.GC14562@linaro.org> <84b6f3fd-ed68-a541-7727-69e5392984e6@suse.de> <20181225083024.GC14405@linaro.org> MIME-Version: 1.0 In-Reply-To: <20181225083024.GC14405@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Jan 2019 14:09:38 -0000 X-List-Received-Date: Mon, 07 Jan 2019 14:09:38 -0000 X-List-Received-Date: Mon, 07 Jan 2019 14:09:38 -0000 X-List-Received-Date: Mon, 07 Jan 2019 14:09:38 -0000 X-List-Received-Date: Mon, 07 Jan 2019 14:09:38 -0000 X-List-Received-Date: Mon, 07 Jan 2019 14:09:38 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Apologies for late reply, back from holidays today. On Tue, Dec 25, 2018 at 05:30:25PM +0900, AKASHI Takahiro wrote: > > >>> +struct efi_key_descriptor { > > >>> + efi_key key; > > >> > > >> Hello Takahiro, > > >> > > >> with the patch I can start the EFI shell. But I am still trying to check > > >> the different definitions in this file. > > >> > > >> As mentioned in prior mail the size of enum is not defined in the C > > >> spec. So better use u32. > > > > > > Sure, but I still wonder whether this should be u32 or u16 (or even u8) > > > as UEFI spec doesn't clearly define this. > > > > Enums may hold up to INT_MAX, so just make it u32. > > OK. > > > > > > >>> + u16 unicode; > > >>> + u16 shifted_unicode; > > >>> + u16 alt_gr_unicode; > > >>> + u16 shifted_alt_gr_unicode; > > >>> + u16 modifier; > > >>> + u16 affected_attribute; > > >>> +}; > > >>> + > > >>> +struct efi_hii_keyboard_layout { > > >>> + u16 layout_length; > > >>> + efi_guid_t guid; > > >> > > >> A patch to change the alignment of efi_guid_t to __alinged(8) has been > > >> merged into efi-next. > > > > > > I have one concern here; > > > This structure will also be used as a data format/layout in a file, > > > a package list, given the fact that UEFI defines ExportPackageLists(). > > > So extra six bytes after layout_length, for example, may not be very > > > useful in general. > > > I'm not very much sure if UEFI spec intends so. > > > > I'm not sure I fully follow. We ideally should be compatible with edk2, > > no? So if the spec isn't clear, let's make sure we clarify the spec to > > the behavior edk2 exposes today. The spec cannot simply be changed to be incompatible with a previous version of the spec, regardless of what EDK2 happens to do. (But...) > I'm not sure that EDK2 is very careful about data alignment. > Regarding guid, in MdePkg/Include/Base.h, > /// > /// 128 bit buffer containing a unique identifier value. > /// Unless otherwise specified, aligned on a 64 bit boundary. > /// > typedef struct { > UINT32 Data1; > UINT16 Data2; > UINT16 Data3; > UINT8 Data4[8]; > } GUID; > in MdePkg/Include/Uefi/UefiBaseType.h, > typedef GUID EFI_GUID; > > There is no explicit "aligned()" attribute contrary to Heinrich's patch. No, so the alignment when building for (any) ARM architecture will end up being 32-bit. Which I agree does not seem to live up to the specification's requirement on 64-bit alignment where nothing else is said. Since, obviously, u-boot and edk2 disagreeing about the layout of structs that are exposed to external applications/drivers would defeat this whole exercise, I think we should start by taking this question to edk2-devel (which I have). > Regarding hii_keyboard_layout, > in Include/Uefi/UefiInternalFormRepresentation.h, > typedef struct { > UINT16 LayoutLength; > EFI_GUID Guid; > UINT32 LayoutDescriptorStringOffset; > UINT8 DescriptorCount; > // EFI_KEY_DESCRIPTOR Descriptors[]; > } EFI_HII_KEYBOARD_LAYOUT; > > No "packed" attribute, so neither in my code. There is a #pragma pack(1) near the start of this file and a #pragma pack() near the end. Interestingly, UEFI 2.7a describes the EFI_HII_KEYBOARD_LAYOUT struct as containing the EFI_KEY_DESCRIPTOR array at the end, whereas the EDK2 code above has it commented it out. EDK2 itself treats the EFI_HII_KEYBOARD_LAYOUT as a header, which is discarded. So, continuning the (But...) My understanding is this: - The EDK2 implementation does not conform to the specification; it completely packs the EFI_HII_KEYBOARD_LAYOUT, which the specification does not mention anything about. Since this code is well in the wild, and drivers tested against the current layout need to continuw eorking, I expect the only possible solution is to update the specification to say EFI_HII_KEYBOARD_LAYOUT must be packed. - The default EDK2 definition of GUID (and hence EFI_GUID) gives it a 32-bit alignment requirement also on 64-bit architectures. In practice, I expect this would only affect (some of the) GUIDs that are members of structs used in UEFI interfaces. But that may already be too common an occurrence to audit and address in EDK2. Does the spec need to change on this also? Can the TianoCore MdeModulePkg Maintainers on cc please comment? (I also cc:d the other stewards as well as Ard, in case they have further input.) > > >>> + u32 layout_descriptor_string_offset; > > >>> + u8 descriptor_count; > > >>> + struct efi_key_descriptor descriptors[]; > > >>> +}; > > >>> + > > >>> +struct efi_hii_package_list_header { > > >>> + efi_guid_t package_list_guid; > > >>> + u32 package_length; > > >>> +} __packed; > > >> > > >> You are defining several structures as __packed. It is unclear to me > > >> where I can find this in the UEFI spec. Looking at EDK2 I could not find > > >> the same __packed attributes. > > > > > > To be honest, I don't know. I just copied these lines from > > > the original patch from Leif & Rob. > > > My guess here is that such data structures are also used as data > > > formats/layouts as part of a package list in a *file* and that no paddings > > > are necessary. Same as I explained above. > > > > > > # Same comments to yours below. > > > > > > I hope that Leif will confirm (or deny) it. > > > > Leif? :) > > I'd like to echo, "Leif?" I think the __packed bits were added by Rob, presumably in order to get the Shell (built with EDK2 headers) working. Regards, Leif