| Message ID | 1504024193-31105-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 29, 2017 at 05:29:53PM +0100, Igor Druzhinin wrote: > We need to choose ACPI tables properly depending on the device > model version we are running. Previously, this decision was > made by BIOS type specific code in hvmloader, e.g. always load > QEMU traditional specific tables if it's ROMBIOS and always > load QEMU Xen specific tables if it's SeaBIOS. > > This change saves this behavior (for compatibility) but adds > an additional way (xenstore key) to specify the correct > device model if we happen to run a non-default one. Toolstack > bit makes use of it. > > The enforcement of BIOS type depending on QEMU version will > be lifted later when the rest of ROMBIOS compatibility fixes > are in place. > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > Changes in v4: > * Use V1 port location unconditionally as modern versions of > Qemu-trad use it anyway > * Change confusing comments in ioreq.h > > Changes in v3: > * move ACPI table externs into util.h > > Changes in v2: > * fix insufficient allocation size of localent > --- > tools/firmware/hvmloader/ovmf.c | 3 --- > tools/firmware/hvmloader/rombios.c | 3 --- > tools/firmware/hvmloader/seabios.c | 3 --- You forgot to remove the calls to HVM_PARAM_ACPI_IOPORTS_LOCATION from the above files. > diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h > index 2e5809b..cffee6b 100644 > --- a/xen/include/public/hvm/ioreq.h > +++ b/xen/include/public/hvm/ioreq.h > @@ -103,14 +103,14 @@ typedef struct buffered_iopage buffered_iopage_t; > * version number in HVM_PARAM_ACPI_IOPORTS_LOCATION. > */ > > -/* Version 0 (default): Traditional Xen locations. */ > +/* Version 0 (default): Traditional (obsolete) Xen locations. */ Could you please add a note saying this is only keep for migration purposes (being able to migrate from older Xen versions)? Thanks, Roger.
>>> On 29.08.17 at 18:29, <igor.druzhinin@citrix.com> wrote: > --- a/tools/firmware/hvmloader/util.h > +++ b/tools/firmware/hvmloader/util.h > @@ -276,6 +276,9 @@ extern struct e820map memory_map; > bool check_overlap(uint64_t start, uint64_t size, > uint64_t reserved_start, uint64_t reserved_size); > > +extern unsigned char dsdt_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[]; Preferably const, unless this requires to much other code to be changed. > @@ -472,6 +472,8 @@ int libxl__domain_build(libxl__gc *gc, > info->u.hvm.mmio_hole_memkb << 10); > } > } > + localents[i++] = "platform/device-model"; > + localents[i++] = (char *) libxl_device_model_version_to_string(info->device_model_version); If this was hypervisor code, I'd complain about the blank following the closing parenthesis; not sure what the libxl conventions are in this regard. Jan
On Wed, Aug 30, 2017 at 02:31:54AM -0600, Jan Beulich wrote: > >>> On 29.08.17 at 18:29, <igor.druzhinin@citrix.com> wrote: > > --- a/tools/firmware/hvmloader/util.h > > +++ b/tools/firmware/hvmloader/util.h > > @@ -276,6 +276,9 @@ extern struct e820map memory_map; > > bool check_overlap(uint64_t start, uint64_t size, > > uint64_t reserved_start, uint64_t reserved_size); > > > > +extern unsigned char dsdt_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[]; > > Preferably const, unless this requires to much other code to be > changed. > > > @@ -472,6 +472,8 @@ int libxl__domain_build(libxl__gc *gc, > > info->u.hvm.mmio_hole_memkb << 10); > > } > > } > > + localents[i++] = "platform/device-model"; > > + localents[i++] = (char *) libxl_device_model_version_to_string(info->device_model_version); > > If this was hypervisor code, I'd complain about the blank following > the closing parenthesis; not sure what the libxl conventions are > in this regard. > We don't have strict requirement in CODING_STYLE. I normally would prefer not having the space there. I can fix that up while committing but Igor please change it if you need to resend this patch.
On 30/08/17 08:21, Roger Pau Monné wrote: > On Tue, Aug 29, 2017 at 05:29:53PM +0100, Igor Druzhinin wrote: >> We need to choose ACPI tables properly depending on the device >> model version we are running. Previously, this decision was >> made by BIOS type specific code in hvmloader, e.g. always load >> QEMU traditional specific tables if it's ROMBIOS and always >> load QEMU Xen specific tables if it's SeaBIOS. >> >> This change saves this behavior (for compatibility) but adds >> an additional way (xenstore key) to specify the correct >> device model if we happen to run a non-default one. Toolstack >> bit makes use of it. >> >> The enforcement of BIOS type depending on QEMU version will >> be lifted later when the rest of ROMBIOS compatibility fixes >> are in place. >> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> >> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> >> --- >> Changes in v4: >> * Use V1 port location unconditionally as modern versions of >> Qemu-trad use it anyway >> * Change confusing comments in ioreq.h >> >> Changes in v3: >> * move ACPI table externs into util.h >> >> Changes in v2: >> * fix insufficient allocation size of localent >> --- >> tools/firmware/hvmloader/ovmf.c | 3 --- >> tools/firmware/hvmloader/rombios.c | 3 --- >> tools/firmware/hvmloader/seabios.c | 3 --- > > You forgot to remove the calls to HVM_PARAM_ACPI_IOPORTS_LOCATION from > the above files. > I think I did that if you mean the change that I had before. These files now don't touch HVM_PARAM_ACPI_IOPORTS_LOCATION. >> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h >> index 2e5809b..cffee6b 100644 >> --- a/xen/include/public/hvm/ioreq.h >> +++ b/xen/include/public/hvm/ioreq.h >> @@ -103,14 +103,14 @@ typedef struct buffered_iopage buffered_iopage_t; >> * version number in HVM_PARAM_ACPI_IOPORTS_LOCATION. >> */ >> >> -/* Version 0 (default): Traditional Xen locations. */ >> +/* Version 0 (default): Traditional (obsolete) Xen locations. */ > > Could you please add a note saying this is only keep for migration > purposes (being able to migrate from older Xen versions)? > Sure. > Thanks, Roger. >
On Wed, Aug 30, 2017 at 01:24:24PM +0100, Igor Druzhinin wrote: > On 30/08/17 08:21, Roger Pau Monné wrote: > > On Tue, Aug 29, 2017 at 05:29:53PM +0100, Igor Druzhinin wrote: > >> tools/firmware/hvmloader/ovmf.c | 3 --- > >> tools/firmware/hvmloader/rombios.c | 3 --- > >> tools/firmware/hvmloader/seabios.c | 3 --- > > > > You forgot to remove the calls to HVM_PARAM_ACPI_IOPORTS_LOCATION from > > the above files. > > > > I think I did that if you mean the change that I had before. These files > now don't touch HVM_PARAM_ACPI_IOPORTS_LOCATION. Sorry, I was looking at the code without the revert. Roger.
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c index 4ff7f1d..a17a11c 100644 --- a/tools/firmware/hvmloader/ovmf.c +++ b/tools/firmware/hvmloader/ovmf.c @@ -41,9 +41,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 */ diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c index 56b39b7..c736fd9 100644 --- a/tools/firmware/hvmloader/rombios.c +++ b/tools/firmware/hvmloader/rombios.c @@ -42,9 +42,6 @@ #define ROMBIOS_MAXOFFSET 0x0000FFFF #define ROMBIOS_END (ROMBIOS_BEGIN + ROMBIOS_SIZE) -extern unsigned char dsdt_anycpu[], dsdt_15cpu[]; -extern int dsdt_anycpu_len, dsdt_15cpu_len; - static void rombios_setup_e820(void) { /* diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c index 870576a..801516d 100644 --- a/tools/firmware/hvmloader/seabios.c +++ b/tools/firmware/hvmloader/seabios.c @@ -29,9 +29,6 @@ #include <acpi2_0.h> #include <libacpi.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 */ diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index db5f240..ab5448b 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -897,6 +897,23 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, /* Allocate and initialise the acpi info area. */ mem_hole_populate_ram(ACPI_INFO_PHYSICAL_ADDRESS >> PAGE_SHIFT, 1); + /* If the device model is specified switch to the corresponding tables */ + s = xenstore_read("platform/device-model", ""); + if ( !strncmp(s, "qemu_xen_traditional", 21) ) + { + config->dsdt_anycpu = dsdt_anycpu; + config->dsdt_anycpu_len = dsdt_anycpu_len; + config->dsdt_15cpu = dsdt_15cpu; + config->dsdt_15cpu_len = dsdt_15cpu_len; + } + else if ( !strncmp(s, "qemu_xen", 9) ) + { + config->dsdt_anycpu = dsdt_anycpu_qemu_xen; + config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len; + config->dsdt_15cpu = NULL; + config->dsdt_15cpu_len = 0; + } + config->lapic_base_address = LAPIC_BASE_ADDRESS; config->lapic_id = acpi_lapic_id; config->ioapic_base_address = ioapic_base_address; diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index 6062f0b..874916c 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -276,6 +276,9 @@ extern struct e820map memory_map; bool check_overlap(uint64_t start, uint64_t size, uint64_t reserved_start, uint64_t reserved_size); +extern unsigned char dsdt_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[]; +extern int dsdt_anycpu_qemu_xen_len, dsdt_anycpu_len, dsdt_15cpu_len; + struct acpi_config; void hvmloader_acpi_build_tables(struct acpi_config *config, unsigned int physical); diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 1158303..1d24209 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -451,7 +451,7 @@ int libxl__domain_build(libxl__gc *gc, vments[4] = "start_time"; vments[5] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000); - localents = libxl__calloc(gc, 11, sizeof(char *)); + localents = libxl__calloc(gc, 13, sizeof(char *)); i = 0; localents[i++] = "platform/acpi"; localents[i++] = libxl__acpi_defbool_val(info) ? "1" : "0"; @@ -472,6 +472,8 @@ int libxl__domain_build(libxl__gc *gc, info->u.hvm.mmio_hole_memkb << 10); } } + localents[i++] = "platform/device-model"; + localents[i++] = (char *) libxl_device_model_version_to_string(info->device_model_version); break; case LIBXL_DOMAIN_TYPE_PV: diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h index 2e5809b..cffee6b 100644 --- a/xen/include/public/hvm/ioreq.h +++ b/xen/include/public/hvm/ioreq.h @@ -103,14 +103,14 @@ typedef struct buffered_iopage buffered_iopage_t; * version number in HVM_PARAM_ACPI_IOPORTS_LOCATION. */ -/* Version 0 (default): Traditional Xen locations. */ +/* Version 0 (default): Traditional (obsolete) Xen locations. */ #define ACPI_PM1A_EVT_BLK_ADDRESS_V0 0x1f40 #define ACPI_PM1A_CNT_BLK_ADDRESS_V0 (ACPI_PM1A_EVT_BLK_ADDRESS_V0 + 0x04) #define ACPI_PM_TMR_BLK_ADDRESS_V0 (ACPI_PM1A_EVT_BLK_ADDRESS_V0 + 0x08) #define ACPI_GPE0_BLK_ADDRESS_V0 (ACPI_PM_TMR_BLK_ADDRESS_V0 + 0x20) #define ACPI_GPE0_BLK_LEN_V0 0x08 -/* Version 1: Locations preferred by modern Qemu. */ +/* Version 1: Locations preferred by modern Qemu (including Qemu-trad). */ #define ACPI_PM1A_EVT_BLK_ADDRESS_V1 0xb000 #define ACPI_PM1A_CNT_BLK_ADDRESS_V1 (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + 0x04) #define ACPI_PM_TMR_BLK_ADDRESS_V1 (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + 0x08)