| Message ID | 1456658360-16080-15-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 28.02.16 at 12:19, <zhaoshenglong@huawei.com> 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/. 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. Jan
On Mon, 29 Feb 2016, Jan Beulich wrote: > >>> On 28.02.16 at 12:19, <zhaoshenglong@huawei.com> 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?
>>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: > On Mon, 29 Feb 2016, Jan Beulich wrote: >> 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? Well, at some point there was talk of ARM not wanting to ExitBootServices() as early as x86 does. In that case there would be a CRC32 function among the various boot services ones. The other option would be to have a generic crc32() function, and maybe make xz use it. Jan
On Mon, 29 Feb 2016, Jan Beulich wrote: > >>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: > > On Mon, 29 Feb 2016, Jan Beulich wrote: > >> 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? > > Well, at some point there was talk of ARM not wanting to > ExitBootServices() as early as x86 does. In that case there > would be a CRC32 function among the various boot services > ones. > > The other option would be to have a generic crc32() function, > and maybe make xz use it. Either way seems good to me.
On 2016/2/29 22:36, Jan Beulich wrote: >>>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: >> > On Mon, 29 Feb 2016, Jan Beulich wrote: >>> >> 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? > Well, at some point there was talk of ARM not wanting to > ExitBootServices() as early as x86 does. In that case there > would be a CRC32 function among the various boot services > ones. > At this point, I think it already ExitBootServices() while it's creating Dom0. > The other option would be to have a generic crc32() function, > and maybe make xz use it. Ok, I'll go for this way. Thanks,
>>> On 01.03.16 at 03:35, <zhaoshenglong@huawei.com> wrote: > > On 2016/2/29 22:36, Jan Beulich wrote: >>>>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: >>> > On Mon, 29 Feb 2016, Jan Beulich wrote: >>>> >> 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? >> Well, at some point there was talk of ARM not wanting to >> ExitBootServices() as early as x86 does. In that case there >> would be a CRC32 function among the various boot services >> ones. >> > At this point, I think it already ExitBootServices() while it's creating > Dom0. I understand that's the case today, hence my saying "at some point there was talk of ...". >> The other option would be to have a generic crc32() function, >> and maybe make xz use it. > Ok, I'll go for this way. Well, for the avoidance of doubt: With the code moving into an ARM specific file, if Stefano is fine with the xz hack, I certainly won't insist on you adding a separate crc32(). Jan
On Tue, 1 Mar 2016, Jan Beulich wrote: > >>> On 01.03.16 at 03:35, <zhaoshenglong@huawei.com> wrote: > > > > > On 2016/2/29 22:36, Jan Beulich wrote: > >>>>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: > >>> > On Mon, 29 Feb 2016, Jan Beulich wrote: > >>>> >> 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? > >> Well, at some point there was talk of ARM not wanting to > >> ExitBootServices() as early as x86 does. In that case there > >> would be a CRC32 function among the various boot services > >> ones. > >> > > At this point, I think it already ExitBootServices() while it's creating > > Dom0. > > I understand that's the case today, hence my saying "at some > point there was talk of ...". > > >> The other option would be to have a generic crc32() function, > >> and maybe make xz use it. > > Ok, I'll go for this way. > > Well, for the avoidance of doubt: With the code moving into an > ARM specific file, if Stefano is fine with the xz hack, I certainly > won't insist on you adding a separate crc32(). Having a generic crc32() function like you suggested would be nicer than the xz hack. If Shannon is OK with doing that, it would be best.
On 2016/3/1 20:49, Stefano Stabellini wrote: > On Tue, 1 Mar 2016, Jan Beulich wrote: >>>>> > >>> On 01.03.16 at 03:35, <zhaoshenglong@huawei.com> wrote: >> > >>> > > >>> > > On 2016/2/29 22:36, Jan Beulich wrote: >>>>>>> > >>>>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: >>>>>> > >>> > On Mon, 29 Feb 2016, Jan Beulich wrote: >>>>>>>> > >>>> >> 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? >>>> > >> Well, at some point there was talk of ARM not wanting to >>>> > >> ExitBootServices() as early as x86 does. In that case there >>>> > >> would be a CRC32 function among the various boot services >>>> > >> ones. >>>> > >> >>> > > At this point, I think it already ExitBootServices() while it's creating >>> > > Dom0. >> > >> > I understand that's the case today, hence my saying "at some >> > point there was talk of ...". >> > >>>> > >> The other option would be to have a generic crc32() function, >>>> > >> and maybe make xz use it. >>> > > Ok, I'll go for this way. >> > >> > Well, for the avoidance of doubt: With the code moving into an >> > ARM specific file, if Stefano is fine with the xz hack, I certainly >> > won't insist on you adding a separate crc32(). > Having a generic crc32() function like you suggested would be nicer than > the xz hack. If Shannon is OK with doing that, it would be best. Since this will introduce more changes, I want to stay with current way. Thanks,
On Fri, 4 Mar 2016, Shannon Zhao wrote: > On 2016/3/1 20:49, Stefano Stabellini wrote: > > On Tue, 1 Mar 2016, Jan Beulich wrote: > >>>>> > >>> On 01.03.16 at 03:35, <zhaoshenglong@huawei.com> wrote: > >> > > >>> > > > >>> > > On 2016/2/29 22:36, Jan Beulich wrote: > >>>>>>> > >>>>> On 29.02.16 at 15:25, <stefano.stabellini@eu.citrix.com> wrote: > >>>>>> > >>> > On Mon, 29 Feb 2016, Jan Beulich wrote: > >>>>>>>> > >>>> >> 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? > >>>> > >> Well, at some point there was talk of ARM not wanting to > >>>> > >> ExitBootServices() as early as x86 does. In that case there > >>>> > >> would be a CRC32 function among the various boot services > >>>> > >> ones. > >>>> > >> > >>> > > At this point, I think it already ExitBootServices() while it's creating > >>> > > Dom0. > >> > > >> > I understand that's the case today, hence my saying "at some > >> > point there was talk of ...". > >> > > >>>> > >> The other option would be to have a generic crc32() function, > >>>> > >> and maybe make xz use it. > >>> > > Ok, I'll go for this way. > >> > > >> > Well, for the avoidance of doubt: With the code moving into an > >> > ARM specific file, if Stefano is fine with the xz hack, I certainly > >> > won't insist on you adding a separate crc32(). > > Having a generic crc32() function like you suggested would be nicer than > > the xz hack. If Shannon is OK with doing that, it would be best. > Since this will introduce more changes, I want to stay with current way. OK
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 6ad420c..09f9770 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1686,6 +1686,8 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo) return rc; acpi_map_other_tables(d); + acpi_create_efi_system_table(d->arch.efi_acpi_gpa, d->arch.efi_acpi_table, + tbl_add); return 0; } diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 535c685..238c5fd 100644 --- 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 */ diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index b759813..2d65796 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -53,6 +53,9 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); int estimate_efi_size(int mem_nr_banks); +void acpi_create_efi_system_table(paddr_t paddr, void *efi_acpi_table, + struct membank tbl_add[]); + int construct_dom0(struct domain *d); void discard_initial_modules(void);