| Message ID | 1460723596-13261-10-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > 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. I do not view this as a valid reason for the change. > Additionally, following patch adds efi struct flags member which > is used during runtime to differentiate between legacy BIOS and > EFI platforms and multiboot2 and EFI native loader. So, efi symbol > have to proper representation in ELF and PE Xen image. Hence, > define efi struct in xen/arch/x86/efi/stub.c and remove efi > symbol from ld script. Only this one is, afaic. The only request here would be to replace "following" by e.g. "a subsequent", to make the description independent of whether the two patches get committed together. > --- a/xen/arch/x86/efi/stub.c > +++ b/xen/arch/x86/efi/stub.c > @@ -8,6 +8,14 @@ > const bool_t efi_enabled = 0; > #endif > > +struct efi __read_mostly efi = { > + .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 > +}; I don't view duplicating this here as a good approach - you'd better move the existing instance elsewhere. If this was a temporary thing (until a later patch), it might be acceptable, but since building without EFI support will need to remain an option (for people using older tool chains), I don't expect a later patch to remove this. Jan
On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > > 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. > > I do not view this as a valid reason for the change. Why? > > Additionally, following patch adds efi struct flags member which > > is used during runtime to differentiate between legacy BIOS and > > EFI platforms and multiboot2 and EFI native loader. So, efi symbol > > have to proper representation in ELF and PE Xen image. Hence, > > define efi struct in xen/arch/x86/efi/stub.c and remove efi > > symbol from ld script. > > Only this one is, afaic. The only request here would be to replace > "following" by e.g. "a subsequent", to make the description > independent of whether the two patches get committed together. OK. > > --- a/xen/arch/x86/efi/stub.c > > +++ b/xen/arch/x86/efi/stub.c > > @@ -8,6 +8,14 @@ > > const bool_t efi_enabled = 0; > > #endif > > > > +struct efi __read_mostly efi = { > > + .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 > > +}; > > I don't view duplicating this here as a good approach - you'd better > move the existing instance elsewhere. If this was a temporary thing > (until a later patch), it might be acceptable, but since building without > EFI support will need to remain an option (for people using older tool > chains), I don't expect a later patch to remove this. Do you think about separate C file which should contain efi struct and should be included in stub.c and runtime.c? Or anything else? Daniel
>>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> > 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. >> >> I do not view this as a valid reason for the change. > > Why? Because there are no accesses to the structure in non-EFI builds? Even if it's just a small table, I'm generally opposed to adding dead code or data. I simply do not like the attitude of "memory is cheap" these days. Following that model leads to quite a bit of useless bloat. Plus no matter whether memory is cheap, cache and TLB bandwidth are precious, and both may get pressure added by such dead elements. >> > --- a/xen/arch/x86/efi/stub.c >> > +++ b/xen/arch/x86/efi/stub.c >> > @@ -8,6 +8,14 @@ >> > const bool_t efi_enabled = 0; >> > #endif >> > >> > +struct efi __read_mostly efi = { >> > + .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 >> > +}; >> >> I don't view duplicating this here as a good approach - you'd better >> move the existing instance elsewhere. If this was a temporary thing >> (until a later patch), it might be acceptable, but since building without >> EFI support will need to remain an option (for people using older tool >> chains), I don't expect a later patch to remove this. > > Do you think about separate C file which should contain efi struct > and should be included in stub.c and runtime.c? Or anything else? A separate file seems to be overkill. Just move it to some other existing file; I'm sure some sensible place can be found. Jan
On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: > >>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: > > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> > 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. > >> > >> I do not view this as a valid reason for the change. > > > > Why? > > Because there are no accesses to the structure in non-EFI builds? > Even if it's just a small table, I'm generally opposed to adding dead > code or data. I simply do not like the attitude of "memory is cheap" > these days. Following that model leads to quite a bit of useless I concur! > bloat. Plus no matter whether memory is cheap, cache and TLB > bandwidth are precious, and both may get pressure added by such > dead elements. OK, but in the future please add a few words of comment in such cases because it is not obvious why just looking at code. Daniel
On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: > >>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: > > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> > 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. > >> > >> I do not view this as a valid reason for the change. > > > > Why? > > Because there are no accesses to the structure in non-EFI builds? > Even if it's just a small table, I'm generally opposed to adding dead > code or data. I simply do not like the attitude of "memory is cheap" > these days. Following that model leads to quite a bit of useless > bloat. Plus no matter whether memory is cheap, cache and TLB > bandwidth are precious, and both may get pressure added by such > dead elements. > > >> > --- a/xen/arch/x86/efi/stub.c > >> > +++ b/xen/arch/x86/efi/stub.c > >> > @@ -8,6 +8,14 @@ > >> > const bool_t efi_enabled = 0; > >> > #endif > >> > > >> > +struct efi __read_mostly efi = { > >> > + .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 > >> > +}; > >> > >> I don't view duplicating this here as a good approach - you'd better > >> move the existing instance elsewhere. If this was a temporary thing > >> (until a later patch), it might be acceptable, but since building without > >> EFI support will need to remain an option (for people using older tool > >> chains), I don't expect a later patch to remove this. > > > > Do you think about separate C file which should contain efi struct > > and should be included in stub.c and runtime.c? Or anything else? > > A separate file seems to be overkill. Just move it to some other > existing file; I'm sure some sensible place can be found. This solution is not perfect, however, I cannot find better place for efi struct. If you have one then drop me a line. Daniel
>>> On 05.07.16 at 20:33, <daniel.kiper@oracle.com> wrote: > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: >> >>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> >> > --- a/xen/arch/x86/efi/stub.c >> >> > +++ b/xen/arch/x86/efi/stub.c >> >> > @@ -8,6 +8,14 @@ >> >> > const bool_t efi_enabled = 0; >> >> > #endif >> >> > >> >> > +struct efi __read_mostly efi = { >> >> > + .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 >> >> > +}; >> >> >> >> I don't view duplicating this here as a good approach - you'd better >> >> move the existing instance elsewhere. If this was a temporary thing >> >> (until a later patch), it might be acceptable, but since building without >> >> EFI support will need to remain an option (for people using older tool >> >> chains), I don't expect a later patch to remove this. >> > >> > Do you think about separate C file which should contain efi struct >> > and should be included in stub.c and runtime.c? Or anything else? >> >> A separate file seems to be overkill. Just move it to some other >> existing file; I'm sure some sensible place can be found. > > This solution is not perfect, however, I cannot find better place for > efi struct. If you have one then drop me a line. common/kernel.c or common/lib.c. Jan
On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote: > >>> On 05.07.16 at 20:33, <daniel.kiper@oracle.com> wrote: > > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: > >> >>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: > >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> >> > --- a/xen/arch/x86/efi/stub.c > >> >> > +++ b/xen/arch/x86/efi/stub.c > >> >> > @@ -8,6 +8,14 @@ > >> >> > const bool_t efi_enabled = 0; > >> >> > #endif > >> >> > > >> >> > +struct efi __read_mostly efi = { > >> >> > + .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 > >> >> > +}; > >> >> > >> >> I don't view duplicating this here as a good approach - you'd better > >> >> move the existing instance elsewhere. If this was a temporary thing > >> >> (until a later patch), it might be acceptable, but since building without > >> >> EFI support will need to remain an option (for people using older tool > >> >> chains), I don't expect a later patch to remove this. > >> > > >> > Do you think about separate C file which should contain efi struct > >> > and should be included in stub.c and runtime.c? Or anything else? > >> > >> A separate file seems to be overkill. Just move it to some other > >> existing file; I'm sure some sensible place can be found. > > > > This solution is not perfect, however, I cannot find better place for > > efi struct. If you have one then drop me a line. > > common/kernel.c or common/lib.c. This means that we must delete efi struct initialization from xen/common/efi/runtime.c and xen/arch/x86/efi/stub.c and put it in one of both files mentioned by you. Is it OK for you? Daniel
>>> On 06.07.16 at 12:27, <daniel.kiper@oracle.com> wrote: > On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote: >> >>> On 05.07.16 at 20:33, <daniel.kiper@oracle.com> wrote: >> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: >> >> >>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: >> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: >> >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> >> >> > --- a/xen/arch/x86/efi/stub.c >> >> >> > +++ b/xen/arch/x86/efi/stub.c >> >> >> > @@ -8,6 +8,14 @@ >> >> >> > const bool_t efi_enabled = 0; >> >> >> > #endif >> >> >> > >> >> >> > +struct efi __read_mostly efi = { >> >> >> > + .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 >> >> >> > +}; >> >> >> >> >> >> I don't view duplicating this here as a good approach - you'd better >> >> >> move the existing instance elsewhere. If this was a temporary thing >> >> >> (until a later patch), it might be acceptable, but since building without >> >> >> EFI support will need to remain an option (for people using older tool >> >> >> chains), I don't expect a later patch to remove this. >> >> > >> >> > Do you think about separate C file which should contain efi struct >> >> > and should be included in stub.c and runtime.c? Or anything else? >> >> >> >> A separate file seems to be overkill. Just move it to some other >> >> existing file; I'm sure some sensible place can be found. >> > >> > This solution is not perfect, however, I cannot find better place for >> > efi struct. If you have one then drop me a line. >> >> common/kernel.c or common/lib.c. > > This means that we must delete efi struct initialization from > xen/common/efi/runtime.c and xen/arch/x86/efi/stub.c and put > it in one of both files mentioned by you. Is it OK for you? Note how in my original reply I said "move the existing instance elsewhere". Jan
On Wed, Jul 06, 2016 at 06:00:47AM -0600, Jan Beulich wrote: > >>> On 06.07.16 at 12:27, <daniel.kiper@oracle.com> wrote: > > On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote: > >> >>> On 05.07.16 at 20:33, <daniel.kiper@oracle.com> wrote: > >> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: > >> >> >>> On 25.05.16 at 18:45, <daniel.kiper@oracle.com> wrote: > >> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >> >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> >> >> > --- a/xen/arch/x86/efi/stub.c > >> >> >> > +++ b/xen/arch/x86/efi/stub.c > >> >> >> > @@ -8,6 +8,14 @@ > >> >> >> > const bool_t efi_enabled = 0; > >> >> >> > #endif > >> >> >> > > >> >> >> > +struct efi __read_mostly efi = { > >> >> >> > + .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 > >> >> >> > +}; > >> >> >> > >> >> >> I don't view duplicating this here as a good approach - you'd better > >> >> >> move the existing instance elsewhere. If this was a temporary thing > >> >> >> (until a later patch), it might be acceptable, but since building without > >> >> >> EFI support will need to remain an option (for people using older tool > >> >> >> chains), I don't expect a later patch to remove this. > >> >> > > >> >> > Do you think about separate C file which should contain efi struct > >> >> > and should be included in stub.c and runtime.c? Or anything else? > >> >> > >> >> A separate file seems to be overkill. Just move it to some other > >> >> existing file; I'm sure some sensible place can be found. > >> > > >> > This solution is not perfect, however, I cannot find better place for > >> > efi struct. If you have one then drop me a line. > >> > >> common/kernel.c or common/lib.c. > > > > This means that we must delete efi struct initialization from > > xen/common/efi/runtime.c and xen/arch/x86/efi/stub.c and put > > it in one of both files mentioned by you. Is it OK for you? > > Note how in my original reply I said "move the existing instance > elsewhere". OK, thanks. I will do that. Daniel
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 07c2bd0..e6c99b5 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -8,6 +8,14 @@ const bool_t efi_enabled = 0; #endif +struct efi __read_mostly efi = { + .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 +}; + void __init efi_init_memory(void) { } void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 6802da1..6376bfa 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -227,8 +227,6 @@ SECTIONS .pad : { . = ALIGN(MB(16)); } :text -#else - efi = .; #endif /* Sections to be discarded */
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. Additionally, following patch adds efi struct flags member which is used during runtime to differentiate between legacy BIOS and EFI platforms and multiboot2 and EFI native loader. So, efi symbol have to proper representation in ELF and PE Xen image. Hence, define efi struct in xen/arch/x86/efi/stub.c and remove efi symbol from ld script. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- xen/arch/x86/efi/stub.c | 8 ++++++++ xen/arch/x86/xen.lds.S | 2 -- 2 files changed, 8 insertions(+), 2 deletions(-)