On Mon, Jul 29, 2013 at 04:47:57PM +0200, Igor Mammedov wrote:
> It turns out that some 32 bit windows guests crash
> if 64 bit PCI hole size is >2G.
> Limit it to 2G for piix and q35 by default.
> User may override default 64-bit PCI hole size by
> using "pci-hole64-size" property.
>
> Examples:
> -global i440FX-pcihost.pci-hole64-size=4G
>
> -global q35-pcihost.pci-hole64-size=4G
>
> Reported-by: Igor Mammedov <imamm...@redhat.com>,
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> v2:
> * make pcihole[64]-(start|end) properties read-only
> * add user manageable pci-hole64-size property
> * use object_resolve_path_type(TYPE_PCI_HOST_BRIDGE)
> to avoid hard codding paths for each supported
> chipset
> * use macros for property names to avoid hadr-codded
> names duplication
> ---
> hw/i386/pc.c | 56 ++++++++++++++++----------
> hw/i386/pc_piix.c | 14 +------
> hw/pci-host/piix.c | 95 ++++++++++++++++++++++++++++++++++++++++++--
> hw/pci-host/q35.c | 88 ++++++++++++++++++++++++++++++++++-------
> include/hw/i386/pc.h | 14 +++++-
> include/hw/pci-host/q35.h | 2 +
> 6 files changed, 211 insertions(+), 58 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b0b98a8..a2b9d88 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -55,6 +55,7 @@
> #include "hw/acpi/acpi.h"
> #include "hw/cpu/icc_bus.h"
> #include "hw/boards.h"
> +#include "hw/pci/pci_host.h"
>
> /* debug PC/ISA interrupts */
> //#define DEBUG_IRQ
> @@ -1003,15 +1004,27 @@ typedef struct PcRomPciInfo {
> static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> {
> PcRomPciInfo *info;
> + Object *pci_info;
> + bool ambiguous = false;
> +
> if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> return;
> }
> + pci_info = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE,
> &ambiguous);
> + g_assert(!ambiguous);
> + if (!pci_info) {
> + return;
> + }
>
> info = g_malloc(sizeof *info);
> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> + PCI_HOST_PROP_PCI_HOLE_START, NULL));
> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> + PCI_HOST_PROP_PCI_HOLE_END, NULL));
> + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> + PCI_HOST_PROP_PCI_HOLE64_START, NULL));
> + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> + PCI_HOST_PROP_PCI_HOLE64_END, NULL));
> /* Pass PCI hole info to guest via a side channel.
> * Required so guest PCI enumeration does the right thing. */
> fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> @@ -1037,29 +1050,28 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t
> below_4g_mem_size,
> PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> PcGuestInfo *guest_info = &guest_info_state->info;
>
> - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> - if (sizeof(hwaddr) == 4) {
> - guest_info->pci_info.w64.begin = 0;
> - guest_info->pci_info.w64.end = 0;
> - } else {
> - /*
> - * BIOS does not set MTRR entries for the 64 bit window, so no need
> to
> - * align address to power of two. Align address at 1G, this makes
> sure
> - * it can be exactly covered with a PAT entry even when using huge
> - * pages.
> - */
> - guest_info->pci_info.w64.begin =
> - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> - (0x1ULL << 62);
> - assert(guest_info->pci_info.w64.begin <=
> guest_info->pci_info.w64.end);
> - }
> -
> guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> return guest_info;
> }
>
> +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start,
> + uint64_t pci_hole64_size)
> +{
> + if ((sizeof(hwaddr) == 4) || (!pci_hole64_size)) {
> + return;
> + }
> + /*
> + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> + * align address to power of two. Align address at 1G, this makes sure
> + * it can be exactly covered with a PAT entry even when using huge
> + * pages.
> + */
> + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> + pci_info->w64.end = pci_info->w64.begin + pci_hole64_size;
Experimenting with various windows versions,
it seems the following values also work, and
allow more space by default:
begin = ROUND_UP(pci_hole64_start, 0x1ULL << 32)
size = (1 << 35)
This might be because 32 bit x86 can use PAE to address 36
bit address space.
I haven't yet checked that resources are actually assigned
though.
> + assert(pci_info->w64.begin <= pci_info->w64.end);
> +}
> +
> void pc_acpi_init(const char *default_dsdt)
> {
> char *filename;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b58c255..ab25458 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> guest_info->has_pci_info = has_pci_info;
>
> - /* Set PCI window size the way seabios has always done it. */
> - /* Power of 2 so bios can cover it with a single MTRR */
> - if (ram_size <= 0x80000000)
> - guest_info->pci_info.w32.begin = 0x80000000;
> - else if (ram_size <= 0xc0000000)
> - guest_info->pci_info.w32.begin = 0xc0000000;
> - else
> - guest_info->pci_info.w32.begin = 0xe0000000;
> -
> /* allocate ram and load rom/bios */
> if (!xen_enabled()) {
> fw_cfg = pc_memory_init(system_memory,
> @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> system_memory, system_io, ram_size,
> below_4g_mem_size,
> 0x100000000ULL - below_4g_mem_size,
> - 0x100000000ULL + above_4g_mem_size,
> - (sizeof(hwaddr) == 4
> - ? 0
> - : ((uint64_t)1 << 62)),
> + above_4g_mem_size,
> pci_memory, ram_memory);
> } else {
> pci_bus = NULL;
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 3f539ec..dc1718f 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -32,6 +32,8 @@
> #include "hw/xen/xen.h"
> #include "hw/pci-host/pam.h"
> #include "sysemu/sysemu.h"
> +#include "hw/i386/ioapic.h"
> +#include "qapi/visitor.h"
>
> /*
> * I440FX chipset data sheet.
> @@ -44,6 +46,8 @@
>
> typedef struct I440FXState {
> PCIHostState parent_obj;
> + PcPciInfo pci_info;
> + uint64_t pci_hole64_size;
> } I440FXState;
>
> #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> @@ -207,14 +211,71 @@ static const VMStateDescription vmstate_i440fx = {
> }
> };
>
> +static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> + uint32_t value = s->pci_info.w32.begin;
> +
> + visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> + uint32_t value = s->pci_info.w32.end;
> +
> + visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
> + void *opaque, const char
> *name,
> + Error **errp)
> +{
> + I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +
> + visit_type_uint64(v, &s->pci_info.w64.begin, name, errp);
> +}
> +
> +static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
> +
> + visit_type_uint64(v, &s->pci_info.w64.end, name, errp);
> +}
> +
> static void i440fx_pcihost_initfn(Object *obj)
> {
> PCIHostState *s = PCI_HOST_BRIDGE(obj);
> + I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
>
> memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
> "pci-conf-idx", 4);
> memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
> "pci-conf-data", 4);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> + i440fx_pcihost_get_pci_hole_start,
> + NULL, NULL, NULL, NULL);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
> + i440fx_pcihost_get_pci_hole_end,
> + NULL, NULL, NULL, NULL);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
> + i440fx_pcihost_get_pci_hole64_start,
> + NULL, NULL, NULL, NULL);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
> + i440fx_pcihost_get_pci_hole64_end,
> + NULL, NULL, NULL, NULL);
> +
> + d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> }
>
> static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> @@ -247,8 +308,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> ram_addr_t ram_size,
> hwaddr pci_hole_start,
> hwaddr pci_hole_size,
> - hwaddr pci_hole64_start,
> - hwaddr pci_hole64_size,
> + ram_addr_t above_4g_mem_size,
> MemoryRegion *pci_address_space,
> MemoryRegion *ram_memory)
> {
> @@ -259,6 +319,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> PIIX3State *piix3;
> PCII440FXState *f;
> unsigned i;
> + I440FXState *i440fx;
>
> dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> s = PCI_HOST_BRIDGE(dev);
> @@ -274,14 +335,31 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> f->system_memory = address_space_mem;
> f->pci_address_space = pci_address_space;
> f->ram_memory = ram_memory;
> +
> + i440fx = I440FX_PCI_HOST_BRIDGE(dev);
> + /* Set PCI window size the way seabios has always done it. */
> + /* Power of 2 so bios can cover it with a single MTRR */
> + if (ram_size <= 0x80000000) {
> + i440fx->pci_info.w32.begin = 0x80000000;
> + } else if (ram_size <= 0xc0000000) {
> + i440fx->pci_info.w32.begin = 0xc0000000;
> + } else {
> + i440fx->pci_info.w32.begin = 0xe0000000;
> + }
> +
> memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole",
> f->pci_address_space,
> pci_hole_start, pci_hole_size);
> memory_region_add_subregion(f->system_memory, pci_hole_start,
> &f->pci_hole);
> +
> + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size,
> + i440fx->pci_hole64_size);
> memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> f->pci_address_space,
> - pci_hole64_start, pci_hole64_size);
> - if (pci_hole64_size) {
> - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> + i440fx->pci_info.w64.begin,
> + i440fx->pci_hole64_size);
> + if (i440fx->pci_hole64_size) {
> + memory_region_add_subregion(f->system_memory,
> + i440fx->pci_info.w64.begin,
> &f->pci_hole_64bit);
> }
> memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> @@ -629,6 +707,12 @@ static const char
> *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> return "0000";
> }
>
> +static Property i440fx_props[] = {
> + DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, I440FXState,
> + pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -638,6 +722,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass,
> void *data)
> dc->realize = i440fx_pcihost_realize;
> dc->fw_name = "pci";
> dc->no_user = 1;
> + dc->props = i440fx_props;
> }
>
> static const TypeInfo i440fx_pcihost_info = {
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 6b1b3b7..6ae432b 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -29,6 +29,7 @@
> */
> #include "hw/hw.h"
> #include "hw/pci-host/q35.h"
> +#include "qapi/visitor.h"
>
> /****************************************************************************
> * Q35 host
> @@ -64,9 +65,49 @@ static const char *q35_host_root_bus_path(PCIHostState
> *host_bridge,
> return "0000";
> }
>
> +static void q35_host_get_pci_hole_start(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> + uint32_t value = s->mch.pci_info.w32.begin;
> +
> + visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void q35_host_get_pci_hole_end(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> + uint32_t value = s->mch.pci_info.w32.end;
> +
> + visit_type_uint32(v, &value, name, errp);
> +}
> +
> +static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +
> + visit_type_uint64(v, &s->mch.pci_info.w64.begin, name, errp);
> +}
> +
> +static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +
> + visit_type_uint64(v, &s->mch.pci_info.w64.end, name, errp);
> +}
> +
> static Property mch_props[] = {
> DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
> MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> + DEFINE_PROP_SIZE(PCI_HOST_PROP_PCI_HOLE64_SIZE, Q35PCIHost,
> + mch.pci_hole64_size, DEFAULT_PCI_HOLE64_SIZE),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -95,6 +136,31 @@ static void q35_host_initfn(Object *obj)
> object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
> qdev_prop_set_uint32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
> qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> + q35_host_get_pci_hole_start,
> + NULL, NULL, NULL, NULL);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_END, "int",
> + q35_host_get_pci_hole_end,
> + NULL, NULL, NULL, NULL);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_START, "int",
> + q35_host_get_pci_hole64_start,
> + NULL, NULL, NULL, NULL);
> +
> + object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
> + q35_host_get_pci_hole64_end,
> + NULL, NULL, NULL, NULL);
> +
> + /* Leave enough space for the biggest MCFG BAR */
> + /* TODO: this matches current bios behaviour, but
> + * it's not a power of two, which means an MTRR
> + * can't cover it exactly.
> + */
> + s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> + MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> + s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> }
>
> static const TypeInfo q35_host_info = {
> @@ -252,17 +318,8 @@ static void mch_reset(DeviceState *qdev)
> static int mch_init(PCIDevice *d)
> {
> int i;
> - hwaddr pci_hole64_size;
> MCHPCIState *mch = MCH_PCI_DEVICE(d);
>
> - /* Leave enough space for the biggest MCFG BAR */
> - /* TODO: this matches current bios behaviour, but
> - * it's not a power of two, which means an MTRR
> - * can't cover it exactly.
> - */
> - mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> - MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> -
> /* setup pci memory regions */
> memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
> mch->pci_address_space,
> @@ -270,15 +327,16 @@ static int mch_init(PCIDevice *d)
> 0x100000000ULL - mch->below_4g_mem_size);
> memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
> &mch->pci_hole);
> - pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> - ((uint64_t)1 << 62));
> +
> + pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL +
> mch->above_4g_mem_size,
> + mch->pci_hole64_size);
> memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
> mch->pci_address_space,
> - 0x100000000ULL + mch->above_4g_mem_size,
> - pci_hole64_size);
> - if (pci_hole64_size) {
> + mch->pci_info.w64.begin,
> + mch->pci_hole64_size);
> + if (mch->pci_hole64_size) {
> memory_region_add_subregion(mch->system_memory,
> - 0x100000000ULL + mch->above_4g_mem_size,
> + mch->pci_info.w64.begin,
> &mch->pci_hole_64bit);
> }
> /* smram */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 7fb97b0..e375b4f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -18,7 +18,6 @@ typedef struct PcPciInfo {
> } PcPciInfo;
>
> struct PcGuestInfo {
> - PcPciInfo pci_info;
> bool has_pci_info;
> FWCfgState *fw_cfg;
> };
> @@ -101,6 +100,16 @@ void pc_acpi_init(const char *default_dsdt);
> PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> ram_addr_t above_4g_mem_size);
>
> +#define PCI_HOST_PROP_PCI_HOLE_START "pci-hole-start"
> +#define PCI_HOST_PROP_PCI_HOLE_END "pci-hole-end"
> +#define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
> +#define PCI_HOST_PROP_PCI_HOLE64_END "pci-hole64-end"
> +#define PCI_HOST_PROP_PCI_HOLE64_SIZE "pci-hole64-size"
> +#define DEFAULT_PCI_HOLE64_SIZE (1ULL << 31)
> +
> +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start,
> + uint64_t pci_hole64_size);
> +
> FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> const char *kernel_filename,
> const char *kernel_cmdline,
> @@ -150,8 +159,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int
> *piix_devfn,
> ram_addr_t ram_size,
> hwaddr pci_hole_start,
> hwaddr pci_hole_size,
> - hwaddr pci_hole64_start,
> - hwaddr pci_hole64_size,
> + ram_addr_t above_4g_mem_size,
> MemoryRegion *pci_memory,
> MemoryRegion *ram_memory);
>
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 3cb631e..6eb7ab6 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,9 +55,11 @@ typedef struct MCHPCIState {
> MemoryRegion smram_region;
> MemoryRegion pci_hole;
> MemoryRegion pci_hole_64bit;
> + PcPciInfo pci_info;
> uint8_t smm_enabled;
> ram_addr_t below_4g_mem_size;
> ram_addr_t above_4g_mem_size;
> + uint64_t pci_hole64_size;
> PcGuestInfo *guest_info;
> } MCHPCIState;
>
> --
> 1.7.1
