From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Kiper Subject: Re: [PATCH v2 09/23] efi: create efi_enabled() Date: Sat, 22 Aug 2015 14:33:59 +0200 Message-ID: <20150822123359.GX7143__6980.61368098042$1440246985$gmane$org@olila.local.net-space.pl> References: <1437402558-7313-1-git-send-email-daniel.kiper@oracle.com> <1437402558-7313-10-git-send-email-daniel.kiper@oracle.com> <55D60BD9020000780009C3CE@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZT80B-0005yg-Cn for xen-devel@lists.xenproject.org; Sat, 22 Aug 2015 12:34:27 +0000 Content-Disposition: inline In-Reply-To: <55D60BD9020000780009C3CE@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Juergen Gross , grub-devel@gnu.org, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, phcoder@gmail.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, keir@xen.org, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org On Thu, Aug 20, 2015 at 09:18:17AM -0600, Jan Beulich wrote: > >>> On 20.07.15 at 16:29, wrote: > > --- a/xen/arch/x86/efi/stub.c > > +++ b/xen/arch/x86/efi/stub.c > > @@ -4,9 +4,14 @@ > > #include > > #include > > > > -#ifndef efi_enabled > > -const bool_t efi_enabled = 0; > > -#endif > > +struct efi __read_mostly efi = { > > + .flags = 0, /* Initialized later. */ > > + .acpi = EFI_INVALID_TABLE_ADDR, > > + .acpi20 = EFI_INVALID_TABLE_ADDR, > > + .mps = EFI_INVALID_TABLE_ADDR, > > + .smbios = EFI_INVALID_TABLE_ADDR, > > + .smbios3 = EFI_INVALID_TABLE_ADDR > > +}; > > How is this change related to the subject of the patch? I need to add this struct because... > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -191,8 +191,6 @@ SECTIONS > > .pad : { > > . = ALIGN(MB(16)); > > } :text > > -#else > > - efi = .; > > #endif > > Same here. ...this creates efi symbol to just satisfy linker and I am removing it. However, existing solution does not allocate space for this symbol and any references to acpi20, etc. does not make sense. As I saw any efi.* references are protected by relevant ifs but we should not do that because it makes code very fragile. If somebody does not know how efi symbol is created he/she may assume that it always represent valid structure and do invalid references somewhere. So, I still think that stub.c should define efi struct properly even if we assume that flags should not be there. However, I agree that this could be separate patch. By the way why did you choose so strange way to satisfy liker needs? > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -717,6 +717,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > > *SystemTable) > > char *option_str; > > bool_t use_cfg_file; > > > > +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */ > > + set_bit(EFI_PLATFORM, &efi.flags); > > +#endif > > Just for this to work? I don't see the need for all the pointers in > the stub case - why can't this be a separate variable? We don't Could be but if we create struct with so generic name like just simple efi it suggest that this is good place to put flags there. If it is not how to call it? efi_flags? Or maybe we should rename efi to efi_tables too. Then everything will be clear. Daniel