Re: [Xen-devel] [PATCH v4 12/19] efi: introduce EFI_RS to ease control on runtime services usage

[Daniel Kiper]

On Thu, Aug 18, 2016 at 05:18:01AM -0600, Jan Beulich wrote:
> >>> On 18.08.16 at 12:30,  wrote:
> > On Wed, Aug 17, 2016 at 10:12:32AM -0600, Jan Beulich wrote:
> >> >>> On 06.08.16 at 01:04,  wrote:
> >>
> >> Apart from the question of this probably better getting merged with
> >> the previous patch ...
> >>
> >> > --- a/xen/common/efi/boot.c
> >> > +++ b/xen/common/efi/boot.c
> >> > @@ -936,6 +936,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> > *SystemTable)
> >> >
> >> >  __set_bit(EFI_BOOT, );
> >> >
> >> > +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
> >> > +__set_bit(EFI_RS, );
> >> > +#endif
> >>
> >> ... this needs to be paralleled by a __clear_bit() when "efi=no-rs"
> >> was given (and then efi_rs_enable) should go away. Oh, looks
> >> like that's the next patch, but I'd then again question the split.
> >
> > Do you suggest that patches 11-13 should be merged into one thing?
> > If it is OK for you I can do that.
> >
> >> > --- a/xen/include/xen/efi.h
> >> > +++ b/xen/include/xen/efi.h
> >> > @@ -12,6 +12,7 @@
> >> >  struct efi {
> >> >  unsigned long flags;/* Bit fields representing available EFI
> > features/properties */
> >> >  #define EFI_BOOT0   /* Were we booted from EFI? */
> >> > +#define EFI_RS  2   /* Can we use runtime services? */
> >>
> >> Why is this not the sequentially next number?
> >
> > I wish that EFI_LOADER (added in patch 16) has number 1 (next to EFI_BOOT).
>
> That'll then too end up more natural by merging the patches.

Potentially we can do that but patch 14 and 15 use efi_enabled().
So, we should merge patches 11-13 if you wish and leave 14-16 as is.

Daniel

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

Re: [Xen-devel] [PATCH v4 12/19] efi: introduce EFI_RS to ease control on runtime services usage

[Jan Beulich]

>>> On 18.08.16 at 12:30,  wrote:
> On Wed, Aug 17, 2016 at 10:12:32AM -0600, Jan Beulich wrote:
>> >>> On 06.08.16 at 01:04,  wrote:
>>
>> Apart from the question of this probably better getting merged with
>> the previous patch ...
>>
>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -936,6 +936,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>> >
>> >  __set_bit(EFI_BOOT, );
>> >
>> > +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
>> > +__set_bit(EFI_RS, );
>> > +#endif
>>
>> ... this needs to be paralleled by a __clear_bit() when "efi=no-rs"
>> was given (and then efi_rs_enable) should go away. Oh, looks
>> like that's the next patch, but I'd then again question the split.
> 
> Do you suggest that patches 11-13 should be merged into one thing?
> If it is OK for you I can do that.
> 
>> > --- a/xen/include/xen/efi.h
>> > +++ b/xen/include/xen/efi.h
>> > @@ -12,6 +12,7 @@
>> >  struct efi {
>> >  unsigned long flags;/* Bit fields representing available EFI 
> features/properties */
>> >  #define EFI_BOOT  0   /* Were we booted from EFI? */
>> > +#define EFI_RS2   /* Can we use runtime services? */
>>
>> Why is this not the sequentially next number?
> 
> I wish that EFI_LOADER (added in patch 16) has number 1 (next to EFI_BOOT).

That'll then too end up more natural by merging the patches.

Jan


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

Re: [Xen-devel] [PATCH v4 12/19] efi: introduce EFI_RS to ease control on runtime services usage

[Daniel Kiper]

On Wed, Aug 17, 2016 at 10:12:32AM -0600, Jan Beulich wrote:
> >>> On 06.08.16 at 01:04,  wrote:
>
> Apart from the question of this probably better getting merged with
> the previous patch ...
>
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -936,6 +936,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> > *SystemTable)
> >
> >  __set_bit(EFI_BOOT, );
> >
> > +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
> > +__set_bit(EFI_RS, );
> > +#endif
>
> ... this needs to be paralleled by a __clear_bit() when "efi=no-rs"
> was given (and then efi_rs_enable) should go away. Oh, looks
> like that's the next patch, but I'd then again question the split.

Do you suggest that patches 11-13 should be merged into one thing?
If it is OK for you I can do that.

> > --- a/xen/include/xen/efi.h
> > +++ b/xen/include/xen/efi.h
> > @@ -12,6 +12,7 @@
> >  struct efi {
> >  unsigned long flags;/* Bit fields representing available EFI 
> > features/properties */
> >  #define EFI_BOOT   0   /* Were we booted from EFI? */
> > +#define EFI_RS 2   /* Can we use runtime services? */
>
> Why is this not the sequentially next number?

I wish that EFI_LOADER (added in patch 16) has number 1 (next to EFI_BOOT).

Daniel

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

Re: [Xen-devel] [PATCH v4 12/19] efi: introduce EFI_RS to ease control on runtime services usage

[Jan Beulich]

>>> On 06.08.16 at 01:04,  wrote:

Apart from the question of this probably better getting merged with
the previous patch ...

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -936,6 +936,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  
>  __set_bit(EFI_BOOT, );
>  
> +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
> +__set_bit(EFI_RS, );
> +#endif

... this needs to be paralleled by a __clear_bit() when "efi=no-rs"
was given (and then efi_rs_enable) should go away. Oh, looks
like that's the next patch, but I'd then again question the split.

> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -12,6 +12,7 @@
>  struct efi {
>  unsigned long flags;/* Bit fields representing available EFI 
> features/properties */
>  #define EFI_BOOT 0   /* Were we booted from EFI? */
> +#define EFI_RS   2   /* Can we use runtime services? */

Why is this not the sequentially next number?

Jan


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

[Xen-devel] [PATCH v4 12/19] efi: introduce EFI_RS to ease control on runtime services usage

[Daniel Kiper]

Suggested-by: Jan Beulich 
Signed-off-by: Daniel Kiper 
---
 xen/arch/x86/domain_page.c |2 +-
 xen/arch/x86/shutdown.c|2 +-
 xen/arch/x86/time.c|2 +-
 xen/common/efi/boot.c  |4 
 xen/include/xen/efi.h  |1 +
 5 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index 71ade05..7541b91 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
  * domain's page tables but current may point at another domain's VCPU.
  * Return NULL as though current is not properly set up yet.
  */
-if ( efi_enabled(EFI_BOOT) && efi_rs_using_pgtables() )
+if ( efi_enabled(EFI_RS) && efi_rs_using_pgtables() )
 return NULL;
 
 /*
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 7ce3761..b429fd0 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -119,7 +119,7 @@ void machine_halt(void)
 static void default_reboot_type(void)
 {
 if ( reboot_type == BOOT_INVALID )
-reboot_type = efi_enabled(EFI_BOOT) ? BOOT_EFI
+reboot_type = efi_enabled(EFI_RS) ? BOOT_EFI
   : acpi_disabled ? BOOT_KBD
   : BOOT_ACPI;
 }
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index b2ecc8e..8d94530 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -686,7 +686,7 @@ static unsigned long get_cmos_time(void)
 static bool_t __read_mostly cmos_rtc_probe;
 boolean_param("cmos-rtc-probe", cmos_rtc_probe);
 
-if ( efi_enabled(EFI_BOOT) )
+if ( efi_enabled(EFI_RS) )
 {
 res = efi_get_time();
 if ( res )
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index edd0434..dd6b0a8 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -936,6 +936,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 
 __set_bit(EFI_BOOT, );
 
+#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
+__set_bit(EFI_RS, );
+#endif
+
 efi_init(ImageHandle, SystemTable);
 
 use_cfg_file = efi_arch_use_config_file(SystemTable);
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index be18e4d..ba14472 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -12,6 +12,7 @@
 struct efi {
 unsigned long flags;/* Bit fields representing available EFI 
features/properties */
 #define EFI_BOOT   0   /* Were we booted from EFI? */
+#define EFI_RS 2   /* Can we use runtime services? */
 unsigned long mps;  /* MPS table */
 unsigned long acpi; /* ACPI table (IA64 ext 0.71) */
 unsigned long acpi20;   /* ACPI table (ACPI 2.0) */
-- 
1.7.10.4


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