| Message ID | 1456412174-20162-14-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: > --- a/tools/firmware/hvmloader/hvmloader.c > +++ b/tools/firmware/hvmloader/hvmloader.c > @@ -365,8 +365,26 @@ int main(void) > > if ( bios->acpi_build_tables ) > { > + const struct hvm_modlist_entry *acpi_module; > + acpi_module = get_module_entry(hvm_start_info, "acpi"); > printf("Loading ACPI ...\n"); > - bios->acpi_build_tables(); > + if ( acpi_module ) > + { > + uint32_t paddr = acpi_module->paddr; > + bios->acpi_build_tables((void*)paddr, > + acpi_module->size); > + } Hmm, so far it was the build process which ensured the right ACPI tables would be used with the corresponding BIOS. The disconnect that gets introduced here worries me a little, since things having got out of sync may be rather hard to diagnose (as they may surface only much later). > +#ifdef ENABLE_ROMBIOS > + else if ( bios == &rombios_config ) > + { > + bios->acpi_build_tables(0, 0); > + } > +#endif > + else > + { > + printf("no ACPI DSDT image found\n"); > + BUG(); > + } Is this really fatal? It's not like "no ACPI" == "end of the world", is it? Jan
On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote: > >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: > > --- a/tools/firmware/hvmloader/hvmloader.c > > +++ b/tools/firmware/hvmloader/hvmloader.c > > @@ -365,8 +365,26 @@ int main(void) > > > > if ( bios->acpi_build_tables ) > > { > > + const struct hvm_modlist_entry *acpi_module; > > + acpi_module = get_module_entry(hvm_start_info, "acpi"); > > printf("Loading ACPI ...\n"); > > - bios->acpi_build_tables(); > > + if ( acpi_module ) > > + { > > + uint32_t paddr = acpi_module->paddr; > > + bios->acpi_build_tables((void*)paddr, > > + acpi_module->size); > > + } > > Hmm, so far it was the build process which ensured the right ACPI > tables would be used with the corresponding BIOS. The disconnect > that gets introduced here worries me a little, since things having > got out of sync may be rather hard to diagnose (as they may > surface only much later). So, my ultimate goal with this series was to be able to create a guest with QEMU's Q35 machine, which would need a different ACPI tables. Also, I would say that the ACPI tables are already disconnected from the thing they describe, the device model QEMU. I don't think there is much information about the BIOS backed into the DSDT table. > > +#ifdef ENABLE_ROMBIOS > > + else if ( bios == &rombios_config ) > > + { > > + bios->acpi_build_tables(0, 0); > > + } > > +#endif > > + else > > + { > > + printf("no ACPI DSDT image found\n"); > > + BUG(); > > + } > > Is this really fatal? It's not like "no ACPI" == "end of the world", > is it? I guest it's not fatal, I'll give it a try and just print a warning.
>>> On 03.03.16 at 18:59, <anthony.perard@citrix.com> wrote: > On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote: >> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: >> > --- a/tools/firmware/hvmloader/hvmloader.c >> > +++ b/tools/firmware/hvmloader/hvmloader.c >> > @@ -365,8 +365,26 @@ int main(void) >> > >> > if ( bios->acpi_build_tables ) >> > { >> > + const struct hvm_modlist_entry *acpi_module; >> > + acpi_module = get_module_entry(hvm_start_info, "acpi"); >> > printf("Loading ACPI ...\n"); >> > - bios->acpi_build_tables(); >> > + if ( acpi_module ) >> > + { >> > + uint32_t paddr = acpi_module->paddr; >> > + bios->acpi_build_tables((void*)paddr, >> > + acpi_module->size); >> > + } >> >> Hmm, so far it was the build process which ensured the right ACPI >> tables would be used with the corresponding BIOS. The disconnect >> that gets introduced here worries me a little, since things having >> got out of sync may be rather hard to diagnose (as they may >> surface only much later). > > So, my ultimate goal with this series was to be able to create a guest with > QEMU's Q35 machine, which would need a different ACPI tables. > > Also, I would say that the ACPI tables are already disconnected from the > thing they describe, the device model QEMU. I don't think there is much > information about the BIOS backed into the DSDT table. But then why would the Q35 model need a different one? Or the other way around, why wouldn't that other one be usable with with the current machine type (or more generally, why couldn't we have one that fits all machine types we mean to support)? Jan
On Fri, Mar 04, 2016 at 01:39:38AM -0700, Jan Beulich wrote: > >>> On 03.03.16 at 18:59, <anthony.perard@citrix.com> wrote: > > On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote: > >> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: > >> > --- a/tools/firmware/hvmloader/hvmloader.c > >> > +++ b/tools/firmware/hvmloader/hvmloader.c > >> > @@ -365,8 +365,26 @@ int main(void) > >> > > >> > if ( bios->acpi_build_tables ) > >> > { > >> > + const struct hvm_modlist_entry *acpi_module; > >> > + acpi_module = get_module_entry(hvm_start_info, "acpi"); > >> > printf("Loading ACPI ...\n"); > >> > - bios->acpi_build_tables(); > >> > + if ( acpi_module ) > >> > + { > >> > + uint32_t paddr = acpi_module->paddr; > >> > + bios->acpi_build_tables((void*)paddr, > >> > + acpi_module->size); > >> > + } > >> > >> Hmm, so far it was the build process which ensured the right ACPI > >> tables would be used with the corresponding BIOS. The disconnect > >> that gets introduced here worries me a little, since things having > >> got out of sync may be rather hard to diagnose (as they may > >> surface only much later). > > > > So, my ultimate goal with this series was to be able to create a guest with > > QEMU's Q35 machine, which would need a different ACPI tables. > > > > Also, I would say that the ACPI tables are already disconnected from the > > thing they describe, the device model QEMU. I don't think there is much > > information about the BIOS backed into the DSDT table. > > But then why would the Q35 model need a different one? Or the > other way around, why wouldn't that other one be usable with > with the current machine type (or more generally, why couldn't > we have one that fits all machine types we mean to support)? I have not though about using the same one for both. I could try to change the tables we have to make it work with both. But that for another time. Thanks,
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h index 4c6d8ad..2c39637 100644 --- a/tools/firmware/hvmloader/config.h +++ b/tools/firmware/hvmloader/config.h @@ -29,7 +29,7 @@ struct bios_config { void (*e820_setup)(void); - void (*acpi_build_tables)(void); + void (*acpi_build_tables)(void* addr, uint32_t size); void (*create_mp_tables)(void); void (*create_smbios_tables)(void); void (*create_pir_tables)(void); diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index cc52302..e7b0139 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -365,8 +365,26 @@ int main(void) if ( bios->acpi_build_tables ) { + const struct hvm_modlist_entry *acpi_module; + acpi_module = get_module_entry(hvm_start_info, "acpi"); printf("Loading ACPI ...\n"); - bios->acpi_build_tables(); + if ( acpi_module ) + { + uint32_t paddr = acpi_module->paddr; + bios->acpi_build_tables((void*)paddr, + acpi_module->size); + } +#ifdef ENABLE_ROMBIOS + else if ( bios == &rombios_config ) + { + bios->acpi_build_tables(0, 0); + } +#endif + else + { + printf("no ACPI DSDT image found\n"); + BUG(); + } } acpi_enable_sci(); diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c index 89a005e..c1612ea 100644 --- a/tools/firmware/hvmloader/ovmf.c +++ b/tools/firmware/hvmloader/ovmf.c @@ -40,9 +40,6 @@ #define LOWCHUNK_MAXOFFSET 0x0000FFFF #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000 -extern unsigned char dsdt_anycpu_qemu_xen[]; -extern int dsdt_anycpu_qemu_xen_len; - #define OVMF_INFO_MAX_TABLES 4 struct ovmf_info { char signature[14]; /* XenHVMOVMF\0\0\0\0 */ @@ -114,11 +111,11 @@ static void ovmf_load(const struct bios_config *config, memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length); } -static void ovmf_acpi_build_tables(void) +static void ovmf_acpi_build_tables(void *addr, uint32_t length) { struct acpi_config config = { - .dsdt_anycpu = dsdt_anycpu_qemu_xen, - .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len, + .dsdt_anycpu = addr, + .dsdt_anycpu_len = length, .dsdt_15cpu = NULL, .dsdt_15cpu_len = 0 }; diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c index 2ded844..732a7ff 100644 --- a/tools/firmware/hvmloader/rombios.c +++ b/tools/firmware/hvmloader/rombios.c @@ -172,7 +172,8 @@ static void reset_bios_checksum(void) *((uint8_t *)(ROMBIOS_BEGIN + ROMBIOS_MAXOFFSET)) = -checksum; } -static void rombios_acpi_build_tables(void) +static void rombios_acpi_build_tables(void *unused_addr, + uint32_t unused_size) { struct acpi_config config = { .dsdt_anycpu = dsdt_anycpu, diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c index 5d896e3..b55a7c9 100644 --- a/tools/firmware/hvmloader/seabios.c +++ b/tools/firmware/hvmloader/seabios.c @@ -27,9 +27,6 @@ #include "smbios_types.h" #include "acpi/acpi2_0.h" -extern unsigned char dsdt_anycpu_qemu_xen[]; -extern int dsdt_anycpu_qemu_xen_len; - struct seabios_info { char signature[14]; /* XenHVMSeaBIOS\0 */ uint8_t length; /* Length of this struct */ @@ -87,12 +84,12 @@ static void add_table(uint32_t t) info->tables_nr++; } -static void seabios_acpi_build_tables(void) +static void seabios_acpi_build_tables(void *addr, uint32_t length) { uint32_t rsdp = (uint32_t)scratch_alloc(sizeof(struct acpi_20_rsdp), 0); struct acpi_config config = { - .dsdt_anycpu = dsdt_anycpu_qemu_xen, - .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len, + .dsdt_anycpu = addr, + .dsdt_anycpu_len = length, .dsdt_15cpu = NULL, .dsdt_15cpu_len = 0, };
... and use it with both SeaBIOS and OVMF. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Changes in V3: - handle rombios --- tools/firmware/hvmloader/config.h | 2 +- tools/firmware/hvmloader/hvmloader.c | 20 +++++++++++++++++++- tools/firmware/hvmloader/ovmf.c | 9 +++------ tools/firmware/hvmloader/rombios.c | 3 ++- tools/firmware/hvmloader/seabios.c | 9 +++------ 5 files changed, 28 insertions(+), 15 deletions(-)