Re: [Xen-devel] [PATCH v4 14/24] arm/acpi: Prepare EFI system table for Dom0

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 29 Feb 2016, Jan Beulich wrote:
> >>> On 28.02.16 at 12:19, <zhaoshenglong@xxxxxxxxxx> wrote:
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -1173,6 +1173,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> > *SystemTable)
> >  }
> >  
> >  #if defined (CONFIG_ACPI) && defined (CONFIG_ARM)
> > +#include "../../../common/decompress.h"
> > +#define XZ_EXTERN STATIC
> > +#include "../../../common/xz/crc32.c"
> > +
> >  /* Constant to indicate "Xen" in unicode u16 format */
> >  static const u16 XEN_EFI_FW_VENDOR[] ={0x0058,0x0065,0x006E,0x0000};
> >  
> > @@ -1189,6 +1193,46 @@ int __init estimate_efi_size(int mem_nr_banks)
> >  
> >      return size;
> >  }
> > +
> > +void __init acpi_create_efi_system_table(paddr_t paddr, void 
> > *efi_acpi_table,
> > +                                         struct membank tbl_add[])
> > +{
> > +    u64 table_addr, table_size, offset = 0;
> > +    u8 *base_ptr;
> > +    EFI_CONFIGURATION_TABLE *efi_conf_tbl;
> > +    EFI_SYSTEM_TABLE *efi_sys_tbl;
> > +
> > +    table_addr = paddr + acpi_get_table_offset(tbl_add, TBL_EFIT);
> > +    table_size = sizeof(EFI_SYSTEM_TABLE) + sizeof(EFI_CONFIGURATION_TABLE)
> > +                 + sizeof(XEN_EFI_FW_VENDOR);
> > +    base_ptr = efi_acpi_table + acpi_get_table_offset(tbl_add, TBL_EFIT);
> > +    efi_sys_tbl = (EFI_SYSTEM_TABLE *)base_ptr;
> > +
> > +    efi_sys_tbl->Hdr.Signature = EFI_SYSTEM_TABLE_SIGNATURE;
> > +    /* Specify the revision as 2.5 */
> > +    efi_sys_tbl->Hdr.Revision = (2 << 16 | 50);
> > +    efi_sys_tbl->Hdr.HeaderSize = table_size;
> > +
> > +    efi_sys_tbl->FirmwareRevision = 1;
> > +    efi_sys_tbl->NumberOfTableEntries = 1;
> > +    offset += sizeof(EFI_SYSTEM_TABLE);
> > +    memcpy((u16 *)(base_ptr + offset), XEN_EFI_FW_VENDOR,
> > +           sizeof(XEN_EFI_FW_VENDOR));
> > +    efi_sys_tbl->FirmwareVendor = (CHAR16 *)(table_addr + offset);
> > +
> > +    offset += sizeof(XEN_EFI_FW_VENDOR);
> > +    efi_conf_tbl = (EFI_CONFIGURATION_TABLE *)(base_ptr + offset);
> > +    efi_conf_tbl->VendorGuid = (EFI_GUID)ACPI_20_TABLE_GUID;
> > +    efi_conf_tbl->VendorTable = (VOID *)tbl_add[TBL_RSDP].start;
> > +    efi_sys_tbl->ConfigurationTable = (EFI_CONFIGURATION_TABLE 
> > *)(table_addr
> > +                                                                  + 
> > offset);
> > +    xz_crc32_init();
> > +    efi_sys_tbl->Hdr.CRC32 = xz_crc32((uint8_t *)efi_sys_tbl,
> > +                                      efi_sys_tbl->Hdr.HeaderSize, 0);
> > +
> > +    tbl_add[TBL_EFIT].start = table_addr;
> > +    tbl_add[TBL_EFIT].size = table_size;
> > +}
> >  #endif
> >  
> >  #ifndef CONFIG_ARM /* TODO - runtime service support */
> 
> While the previous addition here was probably fine on its own, here
> it becomes clear that these additions all belong into arch/arm/efi/.

Right, or maybe into xen/arch/arm/domain_build.c


> Also it doesn't look very nice to me to (ab)use xz's CRC32 code
> here; I don't know who has suggested doing so.
 
It was suggested by Julien.

I agree that including ../../../common/xz/crc32.c seems a bit fragile
but introducing another copy of xz_crc32 seems even worse to me (see
http://marc.info/?l=xen-devel&m=144775375427921&w=2). Maybe you have a
better suggestion?

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