Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper]

On Wed, Jul 06, 2016 at 06:00:47AM -0600, Jan Beulich wrote:
> >>> On 06.07.16 at 12:27,  wrote:
> > On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote:
> >> >>> On 05.07.16 at 20:33,  wrote:
> >> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
> >> >> >>> On 25.05.16 at 18:45,  wrote:
> >> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >> >> >> >>> On 15.04.16 at 14:33,  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Jan Beulich]

>>> On 06.07.16 at 12:27,  wrote:
> On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote:
>> >>> On 05.07.16 at 20:33,  wrote:
>> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
>> >> >>> On 25.05.16 at 18:45,  wrote:
>> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
>> >> >> >>> On 15.04.16 at 14:33,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper]

On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote:
> >>> On 05.07.16 at 20:33,  wrote:
> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
> >> >>> On 25.05.16 at 18:45,  wrote:
> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >> >> >>> On 15.04.16 at 14:33,  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Jan Beulich]

>>> On 05.07.16 at 20:33,  wrote:
> On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
>> >>> On 25.05.16 at 18:45,  wrote:
>> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
>> >> >>> On 15.04.16 at 14:33,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper]

On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 18:45,  wrote:
> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33,  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper]

On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 18:45,  wrote:
> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33,  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Jan Beulich]

>>> On 25.05.16 at 18:45,  wrote:
> On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
>> >>> On 15.04.16 at 14:33,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper]

On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33,  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Jan Beulich]

>>> On 15.04.16 at 14:33,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Daniel Kiper]

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 
---
 xen/arch/x86/efi/stub.c |8 
 xen/arch/x86/xen.lds.S  |2 --
 2 files changed, 8 insertions(+), 2 deletions(-)

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 */
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel