Re: [Xen-devel] [PATCH for-4.5 V7 14/14] Add ARM EFI boot support

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Thu, Sep 25, 2014 at 3:49 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 25.09.14 at 03:42, <roy.franz@xxxxxxxxxx> wrote:
>> @@ -1060,6 +1071,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>      for( ; ; ); /* not reached */
>>  }
>>
>> +#ifndef CONFIG_ARM /* TODO - runtime service support */
>>  #ifndef USE_SET_VIRTUAL_ADDRESS_MAP
>>  static __init void copy_mapping(unsigned long mfn, unsigned long end,
>>                                  bool_t (*is_valid)(unsigned long smfn,
>> @@ -1285,3 +1297,4 @@ void __init efi_init_memory(void)
>>          efi_l4_pgtable[i] = idle_pg_table[i];
>>  #endif
>>  }
>> +#endif
>
> So you want from moving this code block out in its entirety back to
> leaving it in its entirety, despite there being clearly x86-specific
> pieces. That's a little sad...
>
This code is all EFI runtime service related, and not used by ARM at
all.  I have deliberately left
refactoring of the runtime services code until the refactoring can
actually be _used_ by ARM, as I view
changing this code as out of scope for this patchset, and the
refactoring would likely need to be tweaked
when there is a second user.

>> --- a/xen/common/efi/efi.h
>> +++ b/xen/common/efi/efi.h
>> @@ -28,7 +28,9 @@ extern EFI_RUNTIME_SERVICES *efi_rs;
>>  extern UINTN efi_memmap_size, efi_mdesc_size;
>>  extern void *efi_memmap;
>>
>> +#ifndef CONFIG_ARM
>>  extern l4_pgentry_t *efi_l4_pgtable;
>> +#endif
>
> #ifdef CONFIG_X86 please.
sure

>
>> --- a/xen/common/efi/runtime.c
>> +++ b/xen/common/efi/runtime.c
>> @@ -4,17 +4,22 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/irq.h>
>>  #include <xen/time.h>
>> -#include <asm/mc146818rtc.h>
>> +
>> +extern spinlock_t rtc_lock;
>
> No extern declarations in .c files for things being defined in other .c
> files please.

OK, I'll have to look at making asm/mc146818rtc.h a common file, or
moving the extern declaration from that x86 specific include file to a
common one.
(or #ifdef CONFIG_X86 this and the spinlock, but that's not nice either.)

>
>> @@ -24,7 +29,6 @@ unsigned int __read_mostly efi_fw_revision;
>>  const CHAR16 *__read_mostly efi_fw_vendor;
>>
>>  EFI_RUNTIME_SERVICES *__read_mostly efi_rs;
>> -static DEFINE_SPINLOCK(efi_rs_lock);
>>
>>  UINTN __read_mostly efi_memmap_size;
>>  UINTN __read_mostly efi_mdesc_size;
>> @@ -41,10 +45,11 @@ struct efi __read_mostly efi = {
>>       .smbios = EFI_INVALID_TABLE_ADDR,
>>  };
>>
>> -l4_pgentry_t *__read_mostly efi_l4_pgtable;
>> -
>>  const struct efi_pci_rom *__read_mostly efi_pci_roms;
>>
>> +#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
>> +static DEFINE_SPINLOCK(efi_rs_lock);
>> +l4_pgentry_t *__read_mostly efi_l4_pgtable;
>>  unsigned long efi_rs_enter(void)
>
> Blank lines missing.
>
>> @@ -135,7 +140,9 @@ void efi_reset_system(bool_t warm)
>>  }
>>
>>  #endif
>> +#endif
>
> Please attach a /* CONFIG_ARM */ comment to the right one of
> those two (at once making clear without having to consult the
> actual file that the #ifndef below can't be folded with this one).
>
> Jan

OK, I didn't see any other #endifs commented, so I wasn't sure that
the policy on that
was.  I agree this makes things clearer.

>
>> +#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
>>  int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>  {
>>      unsigned int i, n;
>
>

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