by Paolo Bonzini
This series adds a shortcut for network boot, similar to what is present
on bare-metal machines. F11 will prioritize BCV/BEV entries for network
devices over all the other items of the boot list.
After testing with a more recent QEMU I found that it deploys a BCV ROM
that breaks network boot. So this series has two extra patches that
track the PCI class for boot entries; this is then used to filter boot
entries for network boot. A better choice of priorities for BCV and
BEV is also implemented (patch 3).
After Peter Stuge's prod, I reorganized interactive_bootmenu to use a
switch statement, and split the reindentation away in a separate patch.
Paolo
Paolo Bonzini (6):
boot: change definition of IPL_TYPE_* constants
boot: track the PCI class of BCV/BEV boot entries
boot: bump priority of storage-device option ROMs
boot: reorganize interactive_bootmenu
boot: wait for key release before continuing boot
boot: add F11 shortcut for network boot
src/boot.c | 134 ++++++++++++++++++++++++++++++++++++-------------------
src/boot.h | 4 +-
src/optionroms.c | 8 ++--
3 file modificati, 94 inserzioni(+), 52 rimozioni(-)
--
1.7.12.1
by Hiroshi Miura
The rework of EDID support in bochs vgabios
Now populate EDID block in init
You can configure it by CONFIG_VGA_EDID staff.
Developer can easily add modes/dtd in bochs_edid_modes[] and bochs_edid_dtds[],
and validation code select a best resolution for users.
Hiroshi Miura (4):
vgasrc: Add skeleton for VBE function 15h (VBE/DDC Standard)
add VGA_EDID staff
bochsvga: Implement vbe 15h function
bochsvga: validate EDID resolution in init
Makefile | 3 +-
src/vbe.h | 5 ++
vgasrc/Kconfig | 8 +++
vgasrc/bochsedid.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++++
vgasrc/bochsedid.h | 143 ++++++++++++++++++++++++++++++++++++++
vgasrc/bochsvga.c | 9 ++-
vgasrc/stdvga.c | 11 +++
vgasrc/stdvga.h | 2 +
vgasrc/vbe.c | 39 +++++++++++
vgasrc/vgahw.h | 13 ++++
10 files changed, 424 insertions(+), 2 deletions(-)
create mode 100644 vgasrc/bochsedid.c
create mode 100644 vgasrc/bochsedid.h
--
1.7.9.5
by Hiroshi Miura
This patch adds the ability for the Bochs VGA ROM to return information
about a HD wide monitor.
It is constructed from defitions at run time.
Inspired by this patch I digged out from the vgabios mailing list:
http://old.nabble.com/VBE-BIOS-support-for-monitor-information-(EDID)-td2...
Signed-off-by: Hiroshi Miura <miurahr(a)linux.com>
---
Makefile | 3 +-
vgasrc/bochsedid.c | 101 ++++++++++++++++++++++++++++++++++++++
vgasrc/bochsedid.h | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++++
vgasrc/bochsvga.c | 1 +
vgasrc/vgahw.h | 5 ++
5 files changed, 246 insertions(+), 1 deletion(-)
create mode 100644 vgasrc/bochsedid.c
create mode 100644 vgasrc/bochsedid.h
diff --git a/Makefile b/Makefile
index 45ea48a..d3e22b9 100644
--- a/Makefile
+++ b/Makefile
@@ -189,7 +189,8 @@ $(OUT)bios.bin.elf $(OUT)bios.bin: $(OUT)rom.o tools/checkrom.py
SRCVGA=src/output.c src/util.c src/pci.c \
vgasrc/vgabios.c vgasrc/vgafb.c vgasrc/vgafonts.c vgasrc/vbe.c \
vgasrc/stdvga.c vgasrc/stdvgamodes.c vgasrc/stdvgaio.c \
- vgasrc/clext.c vgasrc/bochsvga.c vgasrc/geodevga.c
+ vgasrc/clext.c vgasrc/bochsvga.c vgasrc/geodevga.c \
+ vgasrc/bochsedid.c
CFLAGS16VGA = $(CFLAGS16INC) -Isrc
diff --git a/vgasrc/bochsedid.c b/vgasrc/bochsedid.c
new file mode 100644
index 0000000..ec43d41
--- /dev/null
+++ b/vgasrc/bochsedid.c
@@ -0,0 +1,101 @@
+// VESA VBE EDID capability for virtual display
+//
+// Copyright (C) 2012 Hiroshi Miura <miurahr(a)linux.com>
+//
+// This file may be distributed under the terms of the GNU LGPLv3 license.
+
+#include "vbe.h" // VBE_CAPABILITY_8BIT_DAC
+#include "biosvar.h" // GET_GLOBAL
+#include "util.h" // dprintf
+#include "config.h" // CONFIG_*
+#include "bochsedid.h"
+
+struct edid_detailed_timing edid_dtd_1920x1200 VAR16 = {
+ 0x4B7D, 0xB0, 0xA0, 0x72, 0xB0, 0x2D,
+ 0x40, 0x88, 0xC8, 0x55, 0x75, 0x21,
+ 0x00, 0x00, 0x1C};
+struct edid_monitor_range_limit edid_range_limit VAR16 = {
+ {0x00,0x00,0x00,0xfD,0x00},
+ 50,150, /* Vertical 50Hz - 150Hz */
+ 31,135, /* Horiz 31kHz - 135kHz */
+ 20, /* clock 200MHz */
+ 0, /* no ext */
+ 0x0A,0x20,0x20,
+ 0x2020,
+ 0x20,0x20 /* padding */
+ };
+struct edid_monitor_text edid_serial VAR16 = {
+ {0,0,0,0xFF,0},
+ {'1','2','3','4','5','6','7','8','9','0',0x0A, 0x20, 0x20}
+};
+struct edid_monitor_text edid_name VAR16 = {
+ {0,0,0,0xFC,0},
+ {'S','e','a',' ','V','G','A','B','I','O','S',0x0A, 0x20}
+};
+
+struct edid_info BochsEDID VAR16 = {
+ .header={0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00},
+ .vendor=0x2104, .product=0xCDAB, .serial=0x00000000,
+ .week=36, .year=22, .major_version=1, .minor_version=3,
+ .video_setup=0x0F,
+ /* Video signal interface (analogue, 0.700 : 0.300 : 1.000 V p-p,
+ Video Setup: Blank Level = Black Level, Separate Sync H & V Signals are
+ supported, Composite Sync Signal on Horizontal is supported, Composite
+ Sync Signal on Green Video is supported, Serration on the Vertical Sync
+ is supported) */
+ .screen_width=0x21, .screen_height=0x19,/* 330 mm * 250 mm */
+ .gamma=0x78,/* 2.2 */
+ .feature_flag=0x0F,/* no DMPS states, RGB, display is continuous frequency, prefered mode */
+ .least_chromaticity={0x78, 0xF5},
+ .most_chromaticity={0xA6,0x55,0x48,0x9B,0x26,0x12,0x50,0x54},
+ .established_timing={0xFF,0xEF,0x80},
+ /* 720x400@70Hz, 720x400@88Hz, 640x480@60Hz, 640x480@67Hz, 640x480@72Hz, 640x480@75Hz,
+ 800x600@56Hz, 800x600@60Hz, 800x600@72Hz, 800x600@75Hz, 832x624@75Hz, 1152x870@75Hz,
+ not 1024x768@87Hz(I), 1024x768@60Hz, 1024x768@70Hz, 1024x768@75Hz, 1280x1024@75Hz */
+ .standard_timing={EDID_STD_640x480_85Hz, EDID_STD_800x600_85Hz,
+ EDID_STD_1024x768_85Hz, EDID_STD_1280x720_70Hz,
+ EDID_STD_1280x960_60Hz, EDID_STD_1440x900_60Hz,
+ EDID_STD_1600x1200_60Hz,EDID_STD_NOP},
+};
+
+void bochs_pupulate_edid_block0(u8 next)
+{
+ /* detailed timing block */
+ memcpy_far(get_global_seg(), &(BochsEDID.desc[0]), get_global_seg(),
+ &edid_dtd_1920x1200, sizeof (struct edid_detailed_timing));
+ /* monitor range */
+ memcpy_far(get_global_seg(), &(BochsEDID.desc[1].mrld), get_global_seg(), &edid_range_limit,
+ sizeof(edid_range_limit));
+ /* serial */
+ memcpy_far(get_global_seg(), &(BochsEDID.desc[2]), get_global_seg(), &edid_serial,
+ sizeof (struct edid_monitor_text));
+ /* monitor name */
+ memcpy_far(get_global_seg(), &(BochsEDID.desc[3]), get_global_seg(), &edid_name,
+ sizeof (struct edid_monitor_text));
+
+ SET_VGA(BochsEDID.extensions, next);
+ u8 sum = -checksum_far(get_global_seg(), &BochsEDID, sizeof(BochsEDID));
+ SET_VGA(BochsEDID.checksum, sum);
+}
+
+int bochs_read_edid_block0(u16 unit, u16 block, u16 seg, void *data)
+{
+ memcpy_far(seg, data, get_global_seg(), &BochsEDID, sizeof(BochsEDID));
+ return 0;
+}
+
+int bochs_get_ddc_capabilities(u16 unit)
+{
+ if (unit != 0)
+ return -1;
+
+ return (1 << 8) | VBE_DDC1_PROTOCOL_SUPPORTED;
+}
+
+int bochs_read_edid(u16 unit, u16 block, u16 seg, void *data)
+{
+ if (unit != 0 || block != 0)
+ return -1;
+
+ return bochs_read_edid_block0(unit, block, seg, data);
+}
diff --git a/vgasrc/bochsedid.h b/vgasrc/bochsedid.h
new file mode 100644
index 0000000..b6b2e3a
--- /dev/null
+++ b/vgasrc/bochsedid.h
@@ -0,0 +1,137 @@
+#ifndef __BOCHEDID_H
+#define __BOCHEDID_H
+
+/* EDID infomation definitions */
+/* Detailed Timing Descriptor */
+struct edid_detailed_timing {
+ u16 pixel_clock;
+ u8 horizontal_addressable_low;
+ u8 horizontal_blanking_low;
+ u8 horizontal_high;
+ u8 vertical_addressable_low;
+ u8 vertical_blanking_low;
+ u8 vertical_high;
+ u8 horizontal_front_porch_low;
+ u8 horizontal_sync_pulse_low;
+ u8 vertical_low4;
+ u8 horizontal_vertical_sync_hi;
+ u8 horizontal_video_image_low;
+ u8 vertical_video_image_low;
+ u8 video_image_high;
+ u8 horizontal_border;
+ u8 vertical_border;
+ u8 features;
+} PACKED;
+
+/* Other Monitor Descriptors */
+/*
+ 0xFF: Monitor serial number (text)
+ 0xFE: Unspecified text (text)
+ 0xFC: Monitor name (text)
+ */
+struct edid_monitor_text {
+ u8 header[5];
+ u8 text[13];
+} PACKED;
+
+/* Monitor Range Limits Descriptor */
+/*
+ 0xFD: Monitor range limits. 6- or 13-byte binary descriptor.
+ */
+struct edid_monitor_range_limit {
+ u8 header[5];
+ u8 vertical_rate_min;
+ u8 vertical_rate_max;
+ u8 horizontal_rate_min;
+ u8 horizontal_rate_max;
+ u8 pixel_clock_max;
+ u8 extended_timing_type;
+ u8 reserved;
+ u8 start_frequency;
+ u8 gtf_c_val;
+ u16 gtf_m_val;
+ u8 gtf_k_val;
+ u8 gtf_j_val;
+} PACKED;
+
+/* White point Descriptor */
+/*
+ 0xFB: Additional white point data. 2ÃÂ 5-byte descriptors, padded with 0A 20 20.
+ 0xFA: Additional standard timing identifiers. 6ÃÂ 2-byte descriptors, padded with 0A
+ */
+struct edid_wp_desc {
+ u8 index;
+ u8 least;
+ u8 x;
+ u8 y;
+ u8 gamma;
+} PACKED;
+
+struct edid_white_point {
+ u8 header[5];
+ struct edid_wp_desc desc[2];
+ u8 padding[3];
+} PACKED;
+
+struct edid_additional_timing {
+ u8 header[5];
+ u16 standard_timing[6];
+ u8 padding;
+} PACKED;
+
+union edid_descriptors {
+ struct edid_detailed_timing dtd;
+ struct edid_monitor_range_limit mrld;
+ struct edid_monitor_text mtxtd;
+ struct edid_white_point wpd;
+ struct edid_additional_timing astd;
+} PACKED;
+
+struct edid_info {
+ u8 header[8];
+ u16 vendor;
+ u16 product;
+ u32 serial;
+ u8 week;
+ u8 year;
+ u8 major_version;
+ u8 minor_version;
+ u8 video_setup;
+ u8 screen_width;
+ u8 screen_height;
+ u8 gamma;
+ u8 feature_flag;
+ u8 least_chromaticity[2];
+ u8 most_chromaticity[8];
+ u8 established_timing[3];
+ u16 standard_timing[8];
+ union edid_descriptors desc[4];
+ u8 extensions;
+ u8 checksum;
+} PACKED;
+
+/* EDID Standard Timing Description */
+
+/* First byte: X resolution, divided by 8, less 31 (256âÂÂ2288 pixels) */
+/* bit 7-6, X:Y pixel ratio: 00=16:10; 01=4:3; 10=5:4; 11=16:9 */
+/* bit 5-0, Vertical frequency, less 60 (60âÂÂ123 Hz), nop 01 01 in Big Endian*/
+#define EDID_STD_640x480_85Hz 0x5931
+#define EDID_STD_800x600_85Hz 0x5945
+#define EDID_STD_1024x768_85Hz 0x5961
+#define EDID_STD_1152x864_70Hz 0x4A71
+#define EDID_STD_1280x720_70Hz 0xCA81
+#define EDID_STD_1280x800_70Hz 0x0A81
+#define EDID_STD_1280x960_60Hz 0x4081
+#define EDID_STD_1280x1024_60Hz 0x8081
+#define EDID_STD_1440x900_60Hz 0x0095
+#define EDID_STD_1600x1200_60Hz 0x40A9
+#define EDID_STD_1600x900_60Hz 0xC0A9
+#define EDID_STD_1680x1050_60Hz 0x00B3
+#define EDID_STD_1920x1080_60Hz 0xC0D1
+#define EDID_STD_1920x1200_60Hz 0x00D1
+#define EDID_STD_NOP 0x0101
+
+void bochs_pupulate_edid_block0(u8 next);
+int bochs_get_ddc_capabilities(u16 unit);
+int bochs_read_edid(u16 unit, u16 block, u16 seg, void *data);
+#endif /* bochedid.h */
diff --git a/vgasrc/bochsvga.c b/vgasrc/bochsvga.c
index 2a8aeb1..7b8f430 100644
--- a/vgasrc/bochsvga.c
+++ b/vgasrc/bochsvga.c
@@ -388,6 +388,7 @@ bochsvga_init(void)
SET_VGA(m->mode, 0xffff);
}
}
+ bochs_pupulate_edid_block0(0);
return 0;
}
diff --git a/vgasrc/vgahw.h b/vgasrc/vgahw.h
index 01d2609..130462c 100644
--- a/vgasrc/vgahw.h
+++ b/vgasrc/vgahw.h
@@ -8,6 +8,7 @@
#include "bochsvga.h" // bochsvga_set_mode
#include "stdvga.h" // stdvga_set_mode
#include "geodevga.h" // geodevga_init
+#include "bochsedid.h"
static inline struct vgamode_s *vgahw_find_mode(int mode) {
if (CONFIG_VGA_CIRRUS)
@@ -130,10 +131,14 @@ static inline int vgahw_restore_state(u16 seg, void *data, int states) {
}
static inline int vgahw_get_ddc_capabilities(u16 unit) {
+ if (CONFIG_VGA_BOCHS)
+ return bochs_get_ddc_capabilities(unit);
return stdvga_get_ddc_capabilities(unit);
}
static inline int vgahw_read_edid(u16 unit, u16 block, u16 seg, void *data) {
+ if (CONFIG_VGA_BOCHS)
+ return bochs_read_edid(unit, block, seg, data);
return stdvga_read_edid(unit, block, seg, data);
}
--
1.7.9.5
by Kevin O'Connor
Recent iasl compilers warn about spurious return calls. Remove extra
return calls from the GPE event methods.
Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net>
---
src/acpi-dsdt.dsl | 20 ++------------------
1 file changed, 2 insertions(+), 18 deletions(-)
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index d2575a5..284d70d 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -559,7 +559,6 @@ DefinitionBlock (
PCNT(Local0, 3)
}
}
- Return(One)
}
}
@@ -729,7 +728,6 @@ DefinitionBlock (
}
Increment(Local0)
}
- Return(One)
}
}
@@ -743,54 +741,40 @@ DefinitionBlock (
Name(_HID, "ACPI0006")
Method(_L00) {
- Return(0x01)
}
Method(_E01) {
// PCI hotplug event
- Return(\_SB.PCI0.PCNF())
+ \_SB.PCI0.PCNF()
}
Method(_E02) {
// CPU hotplug event
- Return(\_SB.PRSC())
+ \_SB.PRSC()
}
Method(_L03) {
- Return(0x01)
}
Method(_L04) {
- Return(0x01)
}
Method(_L05) {
- Return(0x01)
}
Method(_L06) {
- Return(0x01)
}
Method(_L07) {
- Return(0x01)
}
Method(_L08) {
- Return(0x01)
}
Method(_L09) {
- Return(0x01)
}
Method(_L0A) {
- Return(0x01)
}
Method(_L0B) {
- Return(0x01)
}
Method(_L0C) {
- Return(0x01)
}
Method(_L0D) {
- Return(0x01)
}
Method(_L0E) {
- Return(0x01)
}
Method(_L0F) {
- Return(0x01)
}
}
}
--
1.7.11.7
by Bruce Rogers
The Pheonix EDD v3.0 spec says that if no dpte is present,
the dpte pointer should contain ffffh:ffffh, not the 0h:0h
which is currently used. This bit me when using NetWare as
a KVM guest.
Signed-off-by: Bruce Rogers <brogers(a)suse.com>
---
src/disk.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/disk.c b/src/disk.c
index 8e1d3ec..0291fe3 100644
--- a/src/disk.c
+++ b/src/disk.c
@@ -615,7 +615,7 @@ disk_1348(struct bregs *regs, struct drive_s *drive_g)
u8 sum = checksum_far(SEG_LOW, &DefaultDPTE, 15);
SET_LOW(DefaultDPTE.checksum, -sum);
} else {
- SET_FARVAR(seg, param_far->dpte.segoff, 0);
+ SET_FARVAR(seg, param_far->dpte.segoff, 0xffffffff);
bdf = GET_GLOBAL(drive_g->cntl_id);
}
--
1.7.7
by Steve Goodrich
Back in July I was working on a problem that involved determining if a drive was bootable (see emails from circa 13 July 2012, "Bootorder Failover (Patch)"). I now have a few extra cycles and I'd like to try to do this the right way. What I'm trying to do is add the "drive_is_bootable()" call to the various boot_add_xxx methods, similar to the following in boot.c:
void
boot_add_hd(struct drive_s *drive_g, const char *desc, int prio)
{
if (!CONFIG_CHECK_BOOTABLE || drive_is_bootable(drive_g)) // <<<<< new code
bootentry_add(IPL_TYPE_HARDDISK, defPrio(prio, DefaultHDPrio)
, (u32)drive_g, desc);
}
The drive_is_bootable() function would read the MBR from the drive, check a few things to ensure that the drive is bootable, and return 0 (fail) or non-zero (success). Per the email thread referenced above, I've got my own copy of process_op() from which I've removed the ASSERT16(). Unfortunately, there are ASSERT16()'s deeper in the call tree -- specifically in the code used by process_floppy_op(), process_ata_op(), and process_cdemu_op().
I think the proper approach would be to put the processor in 16-bit mode and use the original process_op() function (returning to 32-bit mode afterward), but I'm not sure how to do that here. Can anyone offer some guidance here?
Thanks,
-- Steve G.
by Gerd Hoffmann
Add a piix4 memory setup function to make the control flow more clear.
Set the new mtrr_base variable (added by q35 patch set) to the start of
the 32bit pci window to make sure the mtrr entry added covers the I/O
window.
Make the 32bit pci window start at 0x80000000, 0xc0000000 or 0xe000000
(depending on the memory installed) so we can cover the whole window
with a single mtrr entry.
Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com>
---
src/pciinit.c | 18 +++++++++++++++---
1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c
index 93a53e8..28c77ed 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -660,6 +660,18 @@ static void pci_region_map_entries(struct pci_bus *busses, struct pci_region *r)
}
}
+void i440fx_mem_addr_init(struct pci_device *dev, void *arg)
+{
+ u64 *start = (u64 *)arg;
+
+ if (RamSize <= 0x80000000)
+ *start = 0x80000000;
+ else if (RamSize <= 0xc0000000)
+ *start = 0xc0000000;
+
+ mtrr_base = *start;
+}
+
void mch_mem_addr_init(struct pci_device *dev, void *arg)
{
u64 *start = (u64 *)arg;
@@ -670,6 +682,8 @@ void mch_mem_addr_init(struct pci_device *dev, void *arg)
}
static const struct pci_device_id pci_mem_addr_tbl[] = {
+ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82441,
+ i440fx_mem_addr_init),
PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
mch_mem_addr_init),
PCI_DEVICE_END,
@@ -677,9 +691,7 @@ static const struct pci_device_id pci_mem_addr_tbl[] = {
static void pci_bios_map_devices(struct pci_bus *busses)
{
- pcimem_start = RamSize;
-
- /* let's add in mmconfig space on q35 */
+ /* chipset specific memory setup */
pci_find_init_device(pci_mem_addr_tbl, &pcimem_start);
if (pci_bios_init_root_regions(busses)) {
--
1.7.1
by Kevin O'Connor
On Fri, Oct 19, 2012 at 04:41:02PM -0400, Jason Baron wrote:
> From: Isaku Yamahata <yamahata(a)valinux.co.jp>
>
> add dsdt for q35 chipset of qemu.
Thanks.
It looks like the q35 DSDT code was forked from SeaBIOS some time
back. In the last year or so several improvements were made to the
SeaBIOS DSDT code to separate out devices into distinct sections and
to reduce the amount of macro duplication of dsdt code. It would be a
shame to lose these improvements, at least for q35.
Looking at the final q35 dsdt code (patches 6-8), the only thing that
stands out to me as being notably different from the current code is
irq handling. I do wonder if there is an alternative to mass
replacing the dsdt code for a different irq scheme.
-Kevin
by Kevin O'Connor
On Fri, Oct 19, 2012 at 04:41:01PM -0400, Jason Baron wrote:
> From: Isaku Yamahata <yamahata(a)valinux.co.jp>
>
> add q35 initialization functions.
>
Thanks. See comments below.
[...]
> +/* PCI_VENDOR_ID_INTEL && DEVICE_ID_INTEL_Q35_MCH */
> +void mch_mcfg_find(struct pci_device *dev, void *arg)
> +{
> + struct acpi_mcfg *mcfg = arg;
> +
> + mcfg->nr = 1;
> +}
> +
> +/* PCI_VENDOR_ID_INTEL && DEVICE_ID_INTEL_Q35_MCH */
> +void mch_mcfg_init(struct pci_device *dev, void *arg)
> +{
> + u16 bdf = dev->bdf;
> +
> + u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR | Q35_HOST_BRIDGE_PCIEXBAREN;
> + u32 upper = addr >> 32;
> + u32 lower = addr & 0xffffffff;
> +
> + /* at first disable the region. and then update/enable it. */
> + pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, 0);
> + pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR + 4, upper);
> + pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, lower);
> +
> + struct acpi_mcfg *mcfg = arg;
> + struct acpi_mcfg_allocation *alloc = &mcfg->mcfg->allocation[0];
> + alloc->address = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> + alloc->pci_segment = Q35_HOST_PCIE_PCI_SEGMENT;
> + alloc->start_bus_number = Q35_HOST_PCIE_START_BUS_NUMBER;
> + alloc->end_bus_number = Q35_HOST_PCIE_END_BUS_NUMBER;
> +
> + mcfg->e820->start = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> + mcfg->e820->size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> + mcfg->e820->type = E820_RESERVED;
> +}
> +
> static const struct pci_device_id mcfg_find_tbl[] = {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> + mch_mcfg_find),
> PCI_DEVICE_END,
> };
>
> static const struct pci_device_id mcfg_init_tbl[] = {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> + mch_mcfg_init),
> PCI_DEVICE_END,
> };
As in my previous mail, I think mch_mcfg_init (or similar) should do
all the work necessary to build the final acpi mcfg table and not
spread the logic through build_madt/mch_mcfg_find.
[...]
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -12,6 +12,7 @@
> #include "ioport.h" // PORT_ATA1_CMD_BASE
> #include "config.h" // CONFIG_*
> #include "xen.h" // usingXen
> +#include "dev-q35.h" // usingXen
Stale comment.
[...]
> +void mch_mem_addr_init(struct pci_device *dev, void *arg)
> +{
> + u64 *start = (u64 *)arg;
> + /* mmconfig space */
> + *start = Q35_HOST_BRIDGE_PCIEXBAR_ADDR +
> + Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> + mtrr_base = *start;
> +}
> +
> +static const struct pci_device_id pci_mem_addr_tbl[] = {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> + mch_mem_addr_init),
> + PCI_DEVICE_END,
> +};
> +
> static void pci_bios_map_devices(struct pci_bus *busses)
> {
> pcimem_start = RamSize;
>
> + /* let's add in mmconfig space on q35 */
> + pci_find_init_device(pci_mem_addr_tbl, &pcimem_start);
I find this code confusing. Passing a pointer to the global
pcimem_start just obfuscates the assignment, and the setting of
pcimem_start/mtrr_base in pci_bios_map_devices is counter intuitive.
I suggest moving this code up into pci_setup() and performing the
global variable assignments directly.
[...]
> --- a/src/shadow.c
> +++ b/src/shadow.c
> @@ -11,6 +11,7 @@
> #include "pci_ids.h" // PCI_VENDOR_ID_INTEL
> #include "pci_regs.h" // PCI_VENDOR_ID
> #include "xen.h" // usingXen
> +#include "dev-q35.h" // PCI_VENDOR_ID_INTEL
Stale comment.
-Kevin
by Kevin O'Connor
On Fri, Oct 19, 2012 at 04:40:59PM -0400, Jason Baron wrote:
> From: Isaku Yamahata <yamahata(a)valinux.co.jp>
>
> add mcfg table.
> mcfg isn't populated at the moment. dev-q35 will use it later.
>
> [jbaron(a)redhat.com: moved header from post.h -> acpi.h]
Thanks Jason. Sorry for the delay in review.
I find the code flow of this patch to be confusing. The code jumps
from build_mcfg to mch_mcfg_init and mch_mcfg_find (patch 5) setting
intermidiate variables (acpi_mcfg) which are then acted on by all of
build_mcfg and mch_mcfg_*. I think it would be simpler if the code
just detected PCI_DEVICE_ID_INTEL_Q35_MCH and called a function which
did all the work necessary to setup the mcfg acpi table for the q35.
-Kevin
