| Message ID | 1374996553-21828-7-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > "pci_hole64_end " property. > > Examples: > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff IMO that's pretty bad as user interfaces go. In particular if you are not careful you can make end < start. Why not set the size, and include a patch that let people write "1G" or "2G" for convenience? > Reported-by: Igor Mammedov <imammedo@redhat.com>, > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > hw/i386/pc_piix.c | 14 +---------- > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > hw/pci-host/q35.c | 32 +++++++++++++++---------- > include/hw/i386/pc.h | 5 ++-- > include/hw/pci-host/q35.h | 1 + > 6 files changed, 94 insertions(+), 54 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index b0b98a8..acaeb6c 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > { > PcRomPciInfo *info; > + Object *pci_info; > + > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > return; > } > + pci_info = object_resolve_path("/machine/i440fx", NULL); > + if (!pci_info) { > + pci_info = object_resolve_path("/machine/q35", NULL); > + if (!pci_info) { > + return; > + } > + } So is the path /machine/i440fx? /machine/i440FX? /machine/i440FX-pcihost? There's no way to check this code is right without actually running it. How about i44fx_get_pci_info so we can have this knowledge of paths localized in specific chipset code? > > 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_hole_start", NULL)); > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > + "pci_hole_end", NULL)); Looks wrong. object_property_get_int returns a signed int64. w32 is unsigned. Happens to work but I think we need an explicit API. Property names are hard-coded string literals. Please add macros to set and get them so we can avoid duplicating code. E.g. #define PCI_HOST_PROPS... static inline get_ > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, > + "pci_hole64_start", NULL)); > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, > + "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,31 @@ 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) > +{ > + if (sizeof(hwaddr) == 4) { > + memset(&pci_info->w64, sizeof(pci_info->w64), 0); > + 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); > + if (!pci_info->w64.end) { > + /* set default end if it wasn't specified, + 2 Gb past start */ > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); > + } > + 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 4624d04..59a42c5 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -32,6 +32,7 @@ > #include "hw/xen/xen.h" > #include "hw/pci-host/pam.h" > #include "sysemu/sysemu.h" > +#include "hw/i386/ioapic.h" > > /* > * I440FX chipset data sheet. > @@ -44,6 +45,7 @@ > > typedef struct I440FXState { > PCIHostState parent_obj; > + PcPciInfo pci_info; > } I440FXState; > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > PIIX3State *piix3; > PCII440FXState *f; > unsigned i; > + I440FXState *i440fx; > + uint64_t pci_hole64_size; > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); > s = PCI_HOST_BRIDGE(dev); > @@ -274,14 +277,30 @@ 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(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); > + pci_hole64_size = range_size(i440fx->pci_info.w64); > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > f->pci_address_space, > - pci_hole64_start, pci_hole64_size); > + i440fx->pci_info.w64.begin, pci_hole64_size); > if (pci_hole64_size) { > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > return "0000"; > } > > +static Property i440fx_props[] = { > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > + IO_APIC_DEFAULT_ADDRESS), > + DEFINE_PROP_END_OF_LIST(), > +}; > + So we have 4 properties. One of them pci_hole64_end is supposed to be set to a value. Others should not be touched under any circuimstances. Of course if you query properties you have no way to know which is which and what are the legal values. Ouch. > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -638,6 +666,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..a6936e6 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge, > static Property mch_props[] = { > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr, > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT), > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost, > + mch.pci_info.w64.begin, 0), > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost, > + mch.pci_info.w64.end, 0), > + /* 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. > + */ > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost, > + mch.pci_info.w32.begin, > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + > + MCH_HOST_BRIDGE_PCIEXBAR_MAX), > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost, > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d) > 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 +277,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); > + pci_hole64_size = range_size(mch->pci_info.w64); > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64", > mch->pci_address_space, > - 0x100000000ULL + mch->above_4g_mem_size, > + mch->pci_info.w64.begin, > pci_hole64_size); > if (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..ab747b7 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; > }; > @@ -100,6 +99,7 @@ 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); > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start); > > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > const char *kernel_filename, > @@ -150,8 +150,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..3a9e04b 100644 > --- a/include/hw/pci-host/q35.h > +++ b/include/hw/pci-host/q35.h > @@ -55,6 +55,7 @@ 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; > -- > 1.8.3.1
On Sun, 28 Jul 2013 10:57:12 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > "pci_hole64_end " property. > > > > Examples: > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > IMO that's pretty bad as user interfaces go. > In particular if you are not careful you can make > end < start. > Why not set the size, and include a patch that > let people write "1G" or "2G" for convenience? sure as convenience why not, on top of this patches. > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > hw/i386/pc_piix.c | 14 +---------- > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > include/hw/i386/pc.h | 5 ++-- > > include/hw/pci-host/q35.h | 1 + > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index b0b98a8..acaeb6c 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > { > > PcRomPciInfo *info; > > + Object *pci_info; > > + > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > return; > > } > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > + if (!pci_info) { > > + pci_info = object_resolve_path("/machine/q35", NULL); > > + if (!pci_info) { > > + return; > > + } > > + } > > > So is the path /machine/i440fx? /machine/i440FX? > /machine/i440FX-pcihost? > There's no way to check this code is right without > actually running it. that drawback of dynamic lookup, QOM paths supposed to be stable. > > How about i44fx_get_pci_info so we can have this > knowledge of paths localized in specific chipset code? I've seen objections from afaerber about this approach, so dropped this idea. > > > > > 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_hole_start", NULL)); > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > > + "pci_hole_end", NULL)); > > Looks wrong. > object_property_get_int returns a signed int64. > w32 is unsigned. > Happens to work but I think we need an explicit API. I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ but not need for extra API, with fixed property definition i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > Property names are hard-coded string literals. > Please add macros to set and get them > so we can avoid duplicating code. > E.g. sure. > > #define PCI_HOST_PROPS... > static inline get_ > > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, > > + "pci_hole64_start", NULL)); > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, > > + "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,31 @@ 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) > > +{ > > + if (sizeof(hwaddr) == 4) { > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0); > > + 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); > > + if (!pci_info->w64.end) { > > + /* set default end if it wasn't specified, + 2 Gb past start */ > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); > > + } > > + 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 4624d04..59a42c5 100644 > > --- a/hw/pci-host/piix.c > > +++ b/hw/pci-host/piix.c > > @@ -32,6 +32,7 @@ > > #include "hw/xen/xen.h" > > #include "hw/pci-host/pam.h" > > #include "sysemu/sysemu.h" > > +#include "hw/i386/ioapic.h" > > > > /* > > * I440FX chipset data sheet. > > @@ -44,6 +45,7 @@ > > > > typedef struct I440FXState { > > PCIHostState parent_obj; > > + PcPciInfo pci_info; > > } I440FXState; > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > > @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > PIIX3State *piix3; > > PCII440FXState *f; > > unsigned i; > > + I440FXState *i440fx; > > + uint64_t pci_hole64_size; > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); > > s = PCI_HOST_BRIDGE(dev); > > @@ -274,14 +277,30 @@ 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(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); > > + pci_hole64_size = range_size(i440fx->pci_info.w64); > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > > f->pci_address_space, > > - pci_hole64_start, pci_hole64_size); > > + i440fx->pci_info.w64.begin, pci_hole64_size); > > if (pci_hole64_size) { > > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > > + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > > return "0000"; > > } > > > > +static Property i440fx_props[] = { > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > > + IO_APIC_DEFAULT_ADDRESS), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > So we have 4 properties. One of them pci_hole64_end > is supposed to be set to a value. > Others should not be touched under any circuimstances. > Of course if you query properties you have no way > to know which is which and what are the legal values. > Ouch. read-only properties are possible but we would have to drop usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, user better not to touch these properties since they are mostly internal API. but if we say it's internal properties then enforcing read-only might be overkill. For user friendly property "pci_hole64_size" would be nice to have. > > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -638,6 +666,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..a6936e6 100644 > > --- a/hw/pci-host/q35.c > > +++ b/hw/pci-host/q35.c > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge, > > static Property mch_props[] = { > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr, > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT), > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost, > > + mch.pci_info.w64.begin, 0), > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost, > > + mch.pci_info.w64.end, 0), > > + /* 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. > > + */ > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost, > > + mch.pci_info.w32.begin, > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX), > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost, > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d) > > 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 +277,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); > > + pci_hole64_size = range_size(mch->pci_info.w64); > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64", > > mch->pci_address_space, > > - 0x100000000ULL + mch->above_4g_mem_size, > > + mch->pci_info.w64.begin, > > pci_hole64_size); > > if (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..ab747b7 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; > > }; > > @@ -100,6 +99,7 @@ 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); > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start); > > > > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > > const char *kernel_filename, > > @@ -150,8 +150,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..3a9e04b 100644 > > --- a/include/hw/pci-host/q35.h > > +++ b/include/hw/pci-host/q35.h > > @@ -55,6 +55,7 @@ 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; > > -- > > 1.8.3.1 >
On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > On Sun, 28 Jul 2013 10:57:12 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > > "pci_hole64_end " property. > > > > > > Examples: > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > > > IMO that's pretty bad as user interfaces go. > > In particular if you are not careful you can make > > end < start. > > Why not set the size, and include a patch that > > let people write "1G" or "2G" for convenience? > sure as convenience why not, on top of this patches. Well because you are specifying end as 0xffffffffffffffff so it's not a multiple of 1G? > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > > hw/i386/pc_piix.c | 14 +---------- > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > > include/hw/i386/pc.h | 5 ++-- > > > include/hw/pci-host/q35.h | 1 + > > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index b0b98a8..acaeb6c 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > > { > > > PcRomPciInfo *info; > > > + Object *pci_info; > > > + > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > > return; > > > } > > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > > + if (!pci_info) { > > > + pci_info = object_resolve_path("/machine/q35", NULL); > > > + if (!pci_info) { > > > + return; > > > + } > > > + } > > > > > > So is the path /machine/i440fx? /machine/i440FX? > > /machine/i440FX-pcihost? > > There's no way to check this code is right without > > actually running it. > that drawback of dynamic lookup, > QOM paths supposed to be stable. > > > > > How about i44fx_get_pci_info so we can have this > > knowledge of paths localized in specific chipset code? > I've seen objections from afaerber about this approach, so dropped > this idea. > > > > > > > > > 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_hole_start", NULL)); > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > > > + "pci_hole_end", NULL)); > > > > Looks wrong. > > object_property_get_int returns a signed int64. > > w32 is unsigned. > > Happens to work but I think we need an explicit API. > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ Not these are 64 bit values, but they need to be unsigned not signed. > but not need for extra API, with fixed property definition > i.e. s/UINT64/UNIT32/ property set code will take care about limits. If you replace these with UINT32 you won't be able to specify values >4G. > > > > Property names are hard-coded string literals. > > Please add macros to set and get them > > so we can avoid duplicating code. > > E.g. > sure. > > > > > #define PCI_HOST_PROPS... > > static inline get_ > > > > > > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, > > > + "pci_hole64_start", NULL)); > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, > > > + "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,31 @@ 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) > > > +{ > > > + if (sizeof(hwaddr) == 4) { > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0); > > > + 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); > > > + if (!pci_info->w64.end) { > > > + /* set default end if it wasn't specified, + 2 Gb past start */ > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); > > > + } > > > + 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 4624d04..59a42c5 100644 > > > --- a/hw/pci-host/piix.c > > > +++ b/hw/pci-host/piix.c > > > @@ -32,6 +32,7 @@ > > > #include "hw/xen/xen.h" > > > #include "hw/pci-host/pam.h" > > > #include "sysemu/sysemu.h" > > > +#include "hw/i386/ioapic.h" > > > > > > /* > > > * I440FX chipset data sheet. > > > @@ -44,6 +45,7 @@ > > > > > > typedef struct I440FXState { > > > PCIHostState parent_obj; > > > + PcPciInfo pci_info; > > > } I440FXState; > > > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > > > @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > > PIIX3State *piix3; > > > PCII440FXState *f; > > > unsigned i; > > > + I440FXState *i440fx; > > > + uint64_t pci_hole64_size; > > > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); > > > s = PCI_HOST_BRIDGE(dev); > > > @@ -274,14 +277,30 @@ 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(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); > > > + pci_hole64_size = range_size(i440fx->pci_info.w64); > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > > > f->pci_address_space, > > > - pci_hole64_start, pci_hole64_size); > > > + i440fx->pci_info.w64.begin, pci_hole64_size); > > > if (pci_hole64_size) { > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > > > + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > > > return "0000"; > > > } > > > > > > +static Property i440fx_props[] = { > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > > > + IO_APIC_DEFAULT_ADDRESS), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > + > > > > So we have 4 properties. One of them pci_hole64_end > > is supposed to be set to a value. > > Others should not be touched under any circuimstances. > > Of course if you query properties you have no way > > to know which is which and what are the legal values. > > Ouch. > read-only properties are possible but we would have to drop > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, Or add DEFINE_PROP_UINT64_RO for this? > user better not to touch these properties since they are mostly internal API. > but if we say it's internal properties then enforcing read-only might be > overkill. > For user friendly property "pci_hole64_size" would be nice to have. So at the moment I do qemu -device i440FX-pcihost,help and this will get all properties. If we add some properties that user can not set they should not appear in this output. > > > > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > @@ -638,6 +666,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..a6936e6 100644 > > > --- a/hw/pci-host/q35.c > > > +++ b/hw/pci-host/q35.c > > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge, > > > static Property mch_props[] = { > > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr, > > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT), > > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost, > > > + mch.pci_info.w64.begin, 0), > > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost, > > > + mch.pci_info.w64.end, 0), > > > + /* 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. > > > + */ > > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost, > > > + mch.pci_info.w32.begin, > > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + > > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX), > > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost, > > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS), > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > > > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d) > > > 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 +277,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); > > > + pci_hole64_size = range_size(mch->pci_info.w64); > > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64", > > > mch->pci_address_space, > > > - 0x100000000ULL + mch->above_4g_mem_size, > > > + mch->pci_info.w64.begin, > > > pci_hole64_size); > > > if (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..ab747b7 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; > > > }; > > > @@ -100,6 +99,7 @@ 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); > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start); > > > > > > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > > > const char *kernel_filename, > > > @@ -150,8 +150,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..3a9e04b 100644 > > > --- a/include/hw/pci-host/q35.h > > > +++ b/include/hw/pci-host/q35.h > > > @@ -55,6 +55,7 @@ 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; > > > -- > > > 1.8.3.1 > > > > > -- > Regards, > Igor
On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > On Sun, 28 Jul 2013 10:57:12 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > > "pci_hole64_end " property. > > > > > > Examples: > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > > > IMO that's pretty bad as user interfaces go. > > In particular if you are not careful you can make > > end < start. > > Why not set the size, and include a patch that > > let people write "1G" or "2G" for convenience? > sure as convenience why not, on top of this patches. > > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > > hw/i386/pc_piix.c | 14 +---------- > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > > include/hw/i386/pc.h | 5 ++-- > > > include/hw/pci-host/q35.h | 1 + > > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index b0b98a8..acaeb6c 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > > { > > > PcRomPciInfo *info; > > > + Object *pci_info; > > > + > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > > return; > > > } > > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > > + if (!pci_info) { > > > + pci_info = object_resolve_path("/machine/q35", NULL); > > > + if (!pci_info) { > > > + return; > > > + } > > > + } > > > > > > So is the path /machine/i440fx? /machine/i440FX? > > /machine/i440FX-pcihost? > > There's no way to check this code is right without > > actually running it. > that drawback of dynamic lookup, > QOM paths supposed to be stable. > > > > > How about i44fx_get_pci_info so we can have this > > knowledge of paths localized in specific chipset code? > I've seen objections from afaerber about this approach, so dropped > this idea. Could we lookup TYPE_PCI_HOST? This will make pc code independent of specific machine.
Am 28.07.2013 11:11, schrieb Michael S. Tsirkin: > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: >> On Sun, 28 Jul 2013 10:57:12 +0300 >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote: >>>> >>>> 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_hole_start", NULL)); >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, >>>> + "pci_hole_end", NULL)); >>> >>> Looks wrong. >>> object_property_get_int returns a signed int64. >>> w32 is unsigned. >>> Happens to work but I think we need an explicit API. That's how QAPI works internally today for any uint64 visitor/property. uint64_t is cast to int64_t and back in visitors. So I'd hope something like uint64_t val = (uint64_t) object_property_get_int() would work equally well - CC'ing Michael. >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > Not these are 64 bit values, but they need to be > unsigned not signed. > >> but not need for extra API, with fixed property definition >> i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > If you replace these with UINT32 you won't be able to > specify values >4G. > >>> Property names are hard-coded string literals. >>> Please add macros to set and get them >>> so we can avoid duplicating code. >>> E.g. >> sure. >> >>> >>> #define PCI_HOST_PROPS... >>> static inline get_ [...] >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, >>>> return "0000"; >>>> } >>>> >>>> +static Property i440fx_props[] = { >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, >>>> + IO_APIC_DEFAULT_ADDRESS), >>>> + DEFINE_PROP_END_OF_LIST(), >>>> +}; >>>> + >>> >>> So we have 4 properties. One of them pci_hole64_end >>> is supposed to be set to a value. >>> Others should not be touched under any circuimstances. >>> Of course if you query properties you have no way >>> to know which is which and what are the legal values. >>> Ouch. >> read-only properties are possible but we would have to drop >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > Or add DEFINE_PROP_UINT64_RO for this? > >> user better not to touch these properties since they are mostly internal API. >> but if we say it's internal properties then enforcing read-only might be >> overkill. >> For user friendly property "pci_hole64_size" would be nice to have. > > So at the moment I do > > qemu -device i440FX-pcihost,help > > and this will get all properties. > > If we add some properties that user can not set > they should not appear in this output. [snip] Igor, you can simply use dynamic properties with NULL as setter argument for object_property_add*() to achieve that effect. Andreas
On Sun, 28 Jul 2013 12:11:42 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > > On Sun, 28 Jul 2013 10:57:12 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > > > "pci_hole64_end " property. > > > > > > > > Examples: > > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > IMO that's pretty bad as user interfaces go. > > > In particular if you are not careful you can make > > > end < start. > > > Why not set the size, and include a patch that > > > let people write "1G" or "2G" for convenience? > > sure as convenience why not, on top of this patches. > > Well because you are specifying end as 0xffffffffffffffff > so it's not a multiple of 1G? > > > > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > > > hw/i386/pc_piix.c | 14 +---------- > > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > > > include/hw/i386/pc.h | 5 ++-- > > > > include/hw/pci-host/q35.h | 1 + > > > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index b0b98a8..acaeb6c 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > > > { > > > > PcRomPciInfo *info; > > > > + Object *pci_info; > > > > + > > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > > > return; > > > > } > > > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > > > + if (!pci_info) { > > > > + pci_info = object_resolve_path("/machine/q35", NULL); > > > > + if (!pci_info) { > > > > + return; > > > > + } > > > > + } > > > > > > > > > So is the path /machine/i440fx? /machine/i440FX? > > > /machine/i440FX-pcihost? > > > There's no way to check this code is right without > > > actually running it. > > that drawback of dynamic lookup, > > QOM paths supposed to be stable. > > > > > > > > How about i44fx_get_pci_info so we can have this > > > knowledge of paths localized in specific chipset code? > > I've seen objections from afaerber about this approach, so dropped > > this idea. > > > > > > > > > > > > > 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_hole_start", NULL)); > > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > > > > + "pci_hole_end", NULL)); > > > > > > Looks wrong. > > > object_property_get_int returns a signed int64. > > > w32 is unsigned. > > > Happens to work but I think we need an explicit API. > > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > Not these are 64 bit values, but they need to be > unsigned not signed. > > > but not need for extra API, with fixed property definition > > i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > If you replace these with UINT32 you won't be able to > specify values >4G. does 32 bit PCI hole has right to be more than 4Gb? > > > > > > > Property names are hard-coded string literals. > > > Please add macros to set and get them > > > so we can avoid duplicating code. > > > E.g. > > sure. > > > > > > > > #define PCI_HOST_PROPS... > > > static inline get_ > > > > > > > > > > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, > > > > + "pci_hole64_start", NULL)); > > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, > > > > + "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,31 @@ 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) > > > > +{ > > > > + if (sizeof(hwaddr) == 4) { > > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0); > > > > + 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); > > > > + if (!pci_info->w64.end) { > > > > + /* set default end if it wasn't specified, + 2 Gb past start */ > > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); > > > > + } > > > > + 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 4624d04..59a42c5 100644 > > > > --- a/hw/pci-host/piix.c > > > > +++ b/hw/pci-host/piix.c > > > > @@ -32,6 +32,7 @@ > > > > #include "hw/xen/xen.h" > > > > #include "hw/pci-host/pam.h" > > > > #include "sysemu/sysemu.h" > > > > +#include "hw/i386/ioapic.h" > > > > > > > > /* > > > > * I440FX chipset data sheet. > > > > @@ -44,6 +45,7 @@ > > > > > > > > typedef struct I440FXState { > > > > PCIHostState parent_obj; > > > > + PcPciInfo pci_info; > > > > } I440FXState; > > > > > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > > > > @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > > > PIIX3State *piix3; > > > > PCII440FXState *f; > > > > unsigned i; > > > > + I440FXState *i440fx; > > > > + uint64_t pci_hole64_size; > > > > > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); > > > > s = PCI_HOST_BRIDGE(dev); > > > > @@ -274,14 +277,30 @@ 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(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); > > > > + pci_hole64_size = range_size(i440fx->pci_info.w64); > > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > > > > f->pci_address_space, > > > > - pci_hole64_start, pci_hole64_size); > > > > + i440fx->pci_info.w64.begin, pci_hole64_size); > > > > if (pci_hole64_size) { > > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > > > > + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > > > > return "0000"; > > > > } > > > > > > > > +static Property i440fx_props[] = { > > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > > > > + IO_APIC_DEFAULT_ADDRESS), > > > > + DEFINE_PROP_END_OF_LIST(), > > > > +}; > > > > + > > > > > > So we have 4 properties. One of them pci_hole64_end > > > is supposed to be set to a value. > > > Others should not be touched under any circuimstances. > > > Of course if you query properties you have no way > > > to know which is which and what are the legal values. > > > Ouch. > > read-only properties are possible but we would have to drop > > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > Or add DEFINE_PROP_UINT64_RO for this? it might be the way to do it. > > > user better not to touch these properties since they are mostly internal API. > > but if we say it's internal properties then enforcing read-only might be > > overkill. > > For user friendly property "pci_hole64_size" would be nice to have. > > So at the moment I do > > qemu -device i440FX-pcihost,help > > and this will get all properties. > > If we add some properties that user can not set > they should not appear in this output. with QOM all around I wouldn't say so, it could be property only for internal purposes and QOM properties do not care about whether they are visible or not to user so far. I guess we could document it in code like do not touch /internal or ... > > > > > > > > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) > > > > { > > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > > @@ -638,6 +666,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..a6936e6 100644 > > > > --- a/hw/pci-host/q35.c > > > > +++ b/hw/pci-host/q35.c > > > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge, > > > > static Property mch_props[] = { > > > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr, > > > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT), > > > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost, > > > > + mch.pci_info.w64.begin, 0), > > > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost, > > > > + mch.pci_info.w64.end, 0), > > > > + /* 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. > > > > + */ > > > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost, > > > > + mch.pci_info.w32.begin, > > > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + > > > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX), > > > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost, > > > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS), > > > > DEFINE_PROP_END_OF_LIST(), > > > > }; > > > > > > > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d) > > > > 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 +277,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); > > > > + pci_hole64_size = range_size(mch->pci_info.w64); > > > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64", > > > > mch->pci_address_space, > > > > - 0x100000000ULL + mch->above_4g_mem_size, > > > > + mch->pci_info.w64.begin, > > > > pci_hole64_size); > > > > if (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..ab747b7 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; > > > > }; > > > > @@ -100,6 +99,7 @@ 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); > > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start); > > > > > > > > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > > > > const char *kernel_filename, > > > > @@ -150,8 +150,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..3a9e04b 100644 > > > > --- a/include/hw/pci-host/q35.h > > > > +++ b/include/hw/pci-host/q35.h > > > > @@ -55,6 +55,7 @@ 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; > > > > -- > > > > 1.8.3.1 > > > > > > > > > -- > > Regards, > > Igor >
On Sun, 28 Jul 2013 12:13:17 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > > On Sun, 28 Jul 2013 10:57:12 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > > > "pci_hole64_end " property. > > > > > > > > Examples: > > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > IMO that's pretty bad as user interfaces go. > > > In particular if you are not careful you can make > > > end < start. > > > Why not set the size, and include a patch that > > > let people write "1G" or "2G" for convenience? > > sure as convenience why not, on top of this patches. > > > > > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > > > hw/i386/pc_piix.c | 14 +---------- > > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > > > include/hw/i386/pc.h | 5 ++-- > > > > include/hw/pci-host/q35.h | 1 + > > > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index b0b98a8..acaeb6c 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > > > { > > > > PcRomPciInfo *info; > > > > + Object *pci_info; > > > > + > > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > > > return; > > > > } > > > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > > > + if (!pci_info) { > > > > + pci_info = object_resolve_path("/machine/q35", NULL); > > > > + if (!pci_info) { > > > > + return; > > > > + } > > > > + } > > > > > > > > > So is the path /machine/i440fx? /machine/i440FX? > > > /machine/i440FX-pcihost? > > > There's no way to check this code is right without > > > actually running it. > > that drawback of dynamic lookup, > > QOM paths supposed to be stable. > > > > > > > > How about i44fx_get_pci_info so we can have this > > > knowledge of paths localized in specific chipset code? > > I've seen objections from afaerber about this approach, so dropped > > this idea. > > Could we lookup TYPE_PCI_HOST? This will make pc code > independent of specific machine. sure, thanks for idea. > > -- > MST
On Sun, 28 Jul 2013 12:17:47 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 28.07.2013 11:11, schrieb Michael S. Tsirkin: > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > >> On Sun, 28 Jul 2013 10:57:12 +0300 > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote: > >>>> > >>>> 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_hole_start", NULL)); > >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > >>>> + "pci_hole_end", NULL)); > >>> > >>> Looks wrong. > >>> object_property_get_int returns a signed int64. > >>> w32 is unsigned. > >>> Happens to work but I think we need an explicit API. > > That's how QAPI works internally today for any uint64 visitor/property. > uint64_t is cast to int64_t and back in visitors. > > So I'd hope something like > uint64_t val = (uint64_t) object_property_get_int() > would work equally well - CC'ing Michael. > > >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > > > Not these are 64 bit values, but they need to be > > unsigned not signed. > > > >> but not need for extra API, with fixed property definition > >> i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > > > If you replace these with UINT32 you won't be able to > > specify values >4G. > > > >>> Property names are hard-coded string literals. > >>> Please add macros to set and get them > >>> so we can avoid duplicating code. > >>> E.g. > >> sure. > >> > >>> > >>> #define PCI_HOST_PROPS... > >>> static inline get_ > [...] > >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > >>>> return "0000"; > >>>> } > >>>> > >>>> +static Property i440fx_props[] = { > >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > >>>> + IO_APIC_DEFAULT_ADDRESS), > >>>> + DEFINE_PROP_END_OF_LIST(), > >>>> +}; > >>>> + > >>> > >>> So we have 4 properties. One of them pci_hole64_end > >>> is supposed to be set to a value. > >>> Others should not be touched under any circuimstances. > >>> Of course if you query properties you have no way > >>> to know which is which and what are the legal values. > >>> Ouch. > >> read-only properties are possible but we would have to drop > >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > > > Or add DEFINE_PROP_UINT64_RO for this? > > > >> user better not to touch these properties since they are mostly internal API. > >> but if we say it's internal properties then enforcing read-only might be > >> overkill. > >> For user friendly property "pci_hole64_size" would be nice to have. > > > > So at the moment I do > > > > qemu -device i440FX-pcihost,help > > > > and this will get all properties. > > > > If we add some properties that user can not set > > they should not appear in this output. > [snip] > > Igor, you can simply use dynamic properties with NULL as setter argument > for object_property_add*() to achieve that effect. Yes, I can but it's more boiler plate code just for restricting single property. And if we have "pci_hole64_size"/default then user set "pci_hole64_end" would not have any effect, since "pci_hole64_size" would override it. > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On Sun, Jul 28, 2013 at 07:40:32PM +0200, Igor Mammedov wrote: > On Sun, 28 Jul 2013 12:17:47 +0200 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 28.07.2013 11:11, schrieb Michael S. Tsirkin: > > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > > >> On Sun, 28 Jul 2013 10:57:12 +0300 > > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > > >> > > >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote: > > >>>> > > >>>> 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_hole_start", NULL)); > > >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > > >>>> + "pci_hole_end", NULL)); > > >>> > > >>> Looks wrong. > > >>> object_property_get_int returns a signed int64. > > >>> w32 is unsigned. > > >>> Happens to work but I think we need an explicit API. > > > > That's how QAPI works internally today for any uint64 visitor/property. > > uint64_t is cast to int64_t and back in visitors. > > > > So I'd hope something like > > uint64_t val = (uint64_t) object_property_get_int() > > would work equally well - CC'ing Michael. > > > > >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > > > > > Not these are 64 bit values, but they need to be > > > unsigned not signed. > > > > > >> but not need for extra API, with fixed property definition > > >> i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > > > > > If you replace these with UINT32 you won't be able to > > > specify values >4G. > > > > > >>> Property names are hard-coded string literals. > > >>> Please add macros to set and get them > > >>> so we can avoid duplicating code. > > >>> E.g. > > >> sure. > > >> > > >>> > > >>> #define PCI_HOST_PROPS... > > >>> static inline get_ > > [...] > > >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > > >>>> return "0000"; > > >>>> } > > >>>> > > >>>> +static Property i440fx_props[] = { > > >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > > >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > > >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > > >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > > >>>> + IO_APIC_DEFAULT_ADDRESS), > > >>>> + DEFINE_PROP_END_OF_LIST(), > > >>>> +}; > > >>>> + > > >>> > > >>> So we have 4 properties. One of them pci_hole64_end > > >>> is supposed to be set to a value. > > >>> Others should not be touched under any circuimstances. > > >>> Of course if you query properties you have no way > > >>> to know which is which and what are the legal values. > > >>> Ouch. > > >> read-only properties are possible but we would have to drop > > >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > > > > > Or add DEFINE_PROP_UINT64_RO for this? > > > > > >> user better not to touch these properties since they are mostly internal API. > > >> but if we say it's internal properties then enforcing read-only might be > > >> overkill. > > >> For user friendly property "pci_hole64_size" would be nice to have. > > > > > > So at the moment I do > > > > > > qemu -device i440FX-pcihost,help > > > > > > and this will get all properties. > > > > > > If we add some properties that user can not set > > > they should not appear in this output. > > [snip] > > > > Igor, you can simply use dynamic properties with NULL as setter argument > > for object_property_add*() to achieve that effect. > Yes, I can but it's more boiler plate code just for restricting single > property. And if we have "pci_hole64_size"/default then user set > "pci_hole64_end" would not have any effect, since "pci_hole64_size" > would override it. I don't think we want user to control low level properties such and _start and _end. _size might be unavoidable but let's limit it to that. > > Andreas > > > > -- > > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > > > -- > Regards, > Igor
On Sun, Jul 28, 2013 at 07:33:27PM +0200, Igor Mammedov wrote: > On Sun, 28 Jul 2013 12:11:42 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > > > On Sun, 28 Jul 2013 10:57:12 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > > > > "pci_hole64_end " property. > > > > > > > > > > Examples: > > > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > > > IMO that's pretty bad as user interfaces go. > > > > In particular if you are not careful you can make > > > > end < start. > > > > Why not set the size, and include a patch that > > > > let people write "1G" or "2G" for convenience? > > > sure as convenience why not, on top of this patches. > > > > Well because you are specifying end as 0xffffffffffffffff > > so it's not a multiple of 1G? > > > > > > > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > > > > hw/i386/pc_piix.c | 14 +---------- > > > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > > > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > > > > include/hw/i386/pc.h | 5 ++-- > > > > > include/hw/pci-host/q35.h | 1 + > > > > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > > index b0b98a8..acaeb6c 100644 > > > > > --- a/hw/i386/pc.c > > > > > +++ b/hw/i386/pc.c > > > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > > > > { > > > > > PcRomPciInfo *info; > > > > > + Object *pci_info; > > > > > + > > > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > > > > return; > > > > > } > > > > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > > > > + if (!pci_info) { > > > > > + pci_info = object_resolve_path("/machine/q35", NULL); > > > > > + if (!pci_info) { > > > > > + return; > > > > > + } > > > > > + } > > > > > > > > > > > > So is the path /machine/i440fx? /machine/i440FX? > > > > /machine/i440FX-pcihost? > > > > There's no way to check this code is right without > > > > actually running it. > > > that drawback of dynamic lookup, > > > QOM paths supposed to be stable. > > > > > > > > > > > How about i44fx_get_pci_info so we can have this > > > > knowledge of paths localized in specific chipset code? > > > I've seen objections from afaerber about this approach, so dropped > > > this idea. > > > > > > > > > > > > > > > > > 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_hole_start", NULL)); > > > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > > > > > + "pci_hole_end", NULL)); > > > > > > > > Looks wrong. > > > > object_property_get_int returns a signed int64. > > > > w32 is unsigned. > > > > Happens to work but I think we need an explicit API. > > > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > > > Not these are 64 bit values, but they need to be > > unsigned not signed. > > > > > but not need for extra API, with fixed property definition > > > i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > > > If you replace these with UINT32 you won't be able to > > specify values >4G. > does 32 bit PCI hole has right to be more than 4Gb? No but the 64 bit one does. 32 one shouldn't be user controllable at all. > > > > > > > > > > Property names are hard-coded string literals. > > > > Please add macros to set and get them > > > > so we can avoid duplicating code. > > > > E.g. > > > sure. > > > > > > > > > > > #define PCI_HOST_PROPS... > > > > static inline get_ > > > > > > > > > > > > > > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, > > > > > + "pci_hole64_start", NULL)); > > > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, > > > > > + "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,31 @@ 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) > > > > > +{ > > > > > + if (sizeof(hwaddr) == 4) { > > > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0); > > > > > + 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); > > > > > + if (!pci_info->w64.end) { > > > > > + /* set default end if it wasn't specified, + 2 Gb past start */ > > > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); > > > > > + } > > > > > + 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 4624d04..59a42c5 100644 > > > > > --- a/hw/pci-host/piix.c > > > > > +++ b/hw/pci-host/piix.c > > > > > @@ -32,6 +32,7 @@ > > > > > #include "hw/xen/xen.h" > > > > > #include "hw/pci-host/pam.h" > > > > > #include "sysemu/sysemu.h" > > > > > +#include "hw/i386/ioapic.h" > > > > > > > > > > /* > > > > > * I440FX chipset data sheet. > > > > > @@ -44,6 +45,7 @@ > > > > > > > > > > typedef struct I440FXState { > > > > > PCIHostState parent_obj; > > > > > + PcPciInfo pci_info; > > > > > } I440FXState; > > > > > > > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > > > > > @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > > > > PIIX3State *piix3; > > > > > PCII440FXState *f; > > > > > unsigned i; > > > > > + I440FXState *i440fx; > > > > > + uint64_t pci_hole64_size; > > > > > > > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); > > > > > s = PCI_HOST_BRIDGE(dev); > > > > > @@ -274,14 +277,30 @@ 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(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); > > > > > + pci_hole64_size = range_size(i440fx->pci_info.w64); > > > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > > > > > f->pci_address_space, > > > > > - pci_hole64_start, pci_hole64_size); > > > > > + i440fx->pci_info.w64.begin, pci_hole64_size); > > > > > if (pci_hole64_size) { > > > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > > > > > + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > > > > > return "0000"; > > > > > } > > > > > > > > > > +static Property i440fx_props[] = { > > > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > > > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > > > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > > > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > > > > > + IO_APIC_DEFAULT_ADDRESS), > > > > > + DEFINE_PROP_END_OF_LIST(), > > > > > +}; > > > > > + > > > > > > > > So we have 4 properties. One of them pci_hole64_end > > > > is supposed to be set to a value. > > > > Others should not be touched under any circuimstances. > > > > Of course if you query properties you have no way > > > > to know which is which and what are the legal values. > > > > Ouch. > > > read-only properties are possible but we would have to drop > > > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > > > Or add DEFINE_PROP_UINT64_RO for this? > it might be the way to do it. > > > > > > user better not to touch these properties since they are mostly internal API. > > > but if we say it's internal properties then enforcing read-only might be > > > overkill. > > > For user friendly property "pci_hole64_size" would be nice to have. > > > > So at the moment I do > > > > qemu -device i440FX-pcihost,help > > > > and this will get all properties. > > > > If we add some properties that user can not set > > they should not appear in this output. > with QOM all around I wouldn't say so, it could be property only for internal > purposes and QOM properties do not care about whether they are visible or > not to user so far. I guess we could document it in code like do not > touch /internal or ... > > > > > > > > > > > > > > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) > > > > > { > > > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > > > @@ -638,6 +666,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..a6936e6 100644 > > > > > --- a/hw/pci-host/q35.c > > > > > +++ b/hw/pci-host/q35.c > > > > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge, > > > > > static Property mch_props[] = { > > > > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr, > > > > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT), > > > > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost, > > > > > + mch.pci_info.w64.begin, 0), > > > > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost, > > > > > + mch.pci_info.w64.end, 0), > > > > > + /* 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. > > > > > + */ > > > > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost, > > > > > + mch.pci_info.w32.begin, > > > > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + > > > > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX), > > > > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost, > > > > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS), > > > > > DEFINE_PROP_END_OF_LIST(), > > > > > }; > > > > > > > > > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d) > > > > > 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 +277,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); > > > > > + pci_hole64_size = range_size(mch->pci_info.w64); > > > > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64", > > > > > mch->pci_address_space, > > > > > - 0x100000000ULL + mch->above_4g_mem_size, > > > > > + mch->pci_info.w64.begin, > > > > > pci_hole64_size); > > > > > if (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..ab747b7 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; > > > > > }; > > > > > @@ -100,6 +99,7 @@ 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); > > > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start); > > > > > > > > > > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > > > > > const char *kernel_filename, > > > > > @@ -150,8 +150,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..3a9e04b 100644 > > > > > --- a/include/hw/pci-host/q35.h > > > > > +++ b/include/hw/pci-host/q35.h > > > > > @@ -55,6 +55,7 @@ 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; > > > > > -- > > > > > 1.8.3.1 > > > > > > > > > > > > > -- > > > Regards, > > > Igor > > > > > -- > Regards, > Igor
On Sun, 28 Jul 2013 22:51:57 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Jul 28, 2013 at 07:33:27PM +0200, Igor Mammedov wrote: > > On Sun, 28 Jul 2013 12:11:42 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > > > > On Sun, 28 Jul 2013 10:57:12 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Sun, Jul 28, 2013 at 09:29:13AM +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 boundaries by using > > > > > > "pci_hole64_end " property. > > > > > > > > > > > > Examples: > > > > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > > > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff > > > > > > > > > > IMO that's pretty bad as user interfaces go. > > > > > In particular if you are not careful you can make > > > > > end < start. > > > > > Why not set the size, and include a patch that > > > > > let people write "1G" or "2G" for convenience? > > > > sure as convenience why not, on top of this patches. > > > > > > Well because you are specifying end as 0xffffffffffffffff > > > so it's not a multiple of 1G? > > > > > > > > > > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>, > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > --- > > > > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------ > > > > > > hw/i386/pc_piix.c | 14 +---------- > > > > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++---- > > > > > > hw/pci-host/q35.c | 32 +++++++++++++++---------- > > > > > > include/hw/i386/pc.h | 5 ++-- > > > > > > include/hw/pci-host/q35.h | 1 + > > > > > > 6 files changed, 94 insertions(+), 54 deletions(-) > > > > > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > > > index b0b98a8..acaeb6c 100644 > > > > > > --- a/hw/i386/pc.c > > > > > > +++ b/hw/i386/pc.c > > > > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { > > > > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) > > > > > > { > > > > > > PcRomPciInfo *info; > > > > > > + Object *pci_info; > > > > > > + > > > > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) { > > > > > > return; > > > > > > } > > > > > > + pci_info = object_resolve_path("/machine/i440fx", NULL); > > > > > > + if (!pci_info) { > > > > > > + pci_info = object_resolve_path("/machine/q35", NULL); > > > > > > + if (!pci_info) { > > > > > > + return; > > > > > > + } > > > > > > + } > > > > > > > > > > > > > > > So is the path /machine/i440fx? /machine/i440FX? > > > > > /machine/i440FX-pcihost? > > > > > There's no way to check this code is right without > > > > > actually running it. > > > > that drawback of dynamic lookup, > > > > QOM paths supposed to be stable. > > > > > > > > > > > > > > How about i44fx_get_pci_info so we can have this > > > > > knowledge of paths localized in specific chipset code? > > > > I've seen objections from afaerber about this approach, so dropped > > > > this idea. > > > > > > > > > > > > > > > > > > > > > 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_hole_start", NULL)); > > > > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > > > > > > + "pci_hole_end", NULL)); > > > > > > > > > > Looks wrong. > > > > > object_property_get_int returns a signed int64. > > > > > w32 is unsigned. > > > > > Happens to work but I think we need an explicit API. > > > > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > > > > > Not these are 64 bit values, but they need to be > > > unsigned not signed. > > > > > > > but not need for extra API, with fixed property definition > > > > i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > > > > > If you replace these with UINT32 you won't be able to > > > specify values >4G. > > does 32 bit PCI hole has right to be more than 4Gb? > > No but the 64 bit one does. 32 one shouldn't be user > controllable at all. > > > > > > > > > > > > > > Property names are hard-coded string literals. > > > > > Please add macros to set and get them > > > > > so we can avoid duplicating code. > > > > > E.g. > > > > sure. > > > > > > > > > > > > > > #define PCI_HOST_PROPS... > > > > > static inline get_ > > > > > > > > > > > > > > > > > > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, > > > > > > + "pci_hole64_start", NULL)); > > > > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, > > > > > > + "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,31 @@ 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) > > > > > > +{ > > > > > > + if (sizeof(hwaddr) == 4) { > > > > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0); > > > > > > + 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); > > > > > > + if (!pci_info->w64.end) { > > > > > > + /* set default end if it wasn't specified, + 2 Gb past start */ > > > > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); > > > > > > + } > > > > > > + 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 4624d04..59a42c5 100644 > > > > > > --- a/hw/pci-host/piix.c > > > > > > +++ b/hw/pci-host/piix.c > > > > > > @@ -32,6 +32,7 @@ > > > > > > #include "hw/xen/xen.h" > > > > > > #include "hw/pci-host/pam.h" > > > > > > #include "sysemu/sysemu.h" > > > > > > +#include "hw/i386/ioapic.h" > > > > > > > > > > > > /* > > > > > > * I440FX chipset data sheet. > > > > > > @@ -44,6 +45,7 @@ > > > > > > > > > > > > typedef struct I440FXState { > > > > > > PCIHostState parent_obj; > > > > > > + PcPciInfo pci_info; > > > > > > } I440FXState; > > > > > > > > > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > > > > > > @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > > > > > PIIX3State *piix3; > > > > > > PCII440FXState *f; > > > > > > unsigned i; > > > > > > + I440FXState *i440fx; > > > > > > + uint64_t pci_hole64_size; > > > > > > > > > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); > > > > > > s = PCI_HOST_BRIDGE(dev); > > > > > > @@ -274,14 +277,30 @@ 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(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); > > > > > > + pci_hole64_size = range_size(i440fx->pci_info.w64); > > > > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > > > > > > f->pci_address_space, > > > > > > - pci_hole64_start, pci_hole64_size); > > > > > > + i440fx->pci_info.w64.begin, pci_hole64_size); > > > > > > if (pci_hole64_size) { > > > > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > > > > > > + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > > > > > > return "0000"; > > > > > > } > > > > > > > > > > > > +static Property i440fx_props[] = { > > > > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > > > > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > > > > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > > > > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > > > > > > + IO_APIC_DEFAULT_ADDRESS), > > > > > > + DEFINE_PROP_END_OF_LIST(), > > > > > > +}; > > > > > > + > > > > > > > > > > So we have 4 properties. One of them pci_hole64_end > > > > > is supposed to be set to a value. > > > > > Others should not be touched under any circuimstances. > > > > > Of course if you query properties you have no way > > > > > to know which is which and what are the legal values. > > > > > Ouch. > > > > read-only properties are possible but we would have to drop > > > > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > > > > > Or add DEFINE_PROP_UINT64_RO for this? > > it might be the way to do it. I've meant to do it only for 32-bit PCI hole, I'm sorry for being unclear. [...]
On Mon, Jul 29, 2013 at 09:55:32AM +0200, Igor Mammedov wrote: > > > > > but not need for extra API, with fixed property definition > > > > > i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > > > > > > > If you replace these with UINT32 you won't be able to > > > > specify values >4G. > > > does 32 bit PCI hole has right to be more than 4Gb? > > > > No but the 64 bit one does. 32 one shouldn't be user > > controllable at all. ... > I've meant to do it only for 32-bit PCI hole, I'm sorry for being unclear. > > [...] That's OK then.
Quoting Andreas Färber (2013-07-28 05:17:47) > Am 28.07.2013 11:11, schrieb Michael S. Tsirkin: > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: > >> On Sun, 28 Jul 2013 10:57:12 +0300 > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote: > >>>> > >>>> 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_hole_start", NULL)); > >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, > >>>> + "pci_hole_end", NULL)); > >>> > >>> Looks wrong. > >>> object_property_get_int returns a signed int64. > >>> w32 is unsigned. > >>> Happens to work but I think we need an explicit API. > > That's how QAPI works internally today for any uint64 visitor/property. > uint64_t is cast to int64_t and back in visitors. > > So I'd hope something like > uint64_t val = (uint64_t) object_property_get_int() > would work equally well - CC'ing Michael. It actually depends on the 'wire'/'serialized' encoding of the underlying visitor implementation. In this case we're 'serializing' into a QObject via QmpOutputVisitor, where all integers are actually stored as a QInt/int64 anyway, so this cast is unavoidable in this case regardless of what QAPI interface we use: we'll always end up storing as an int64, requiring us to re-cast to get back to original value. Best we can achieve is burying the cast deeper (or significantly re-working how QObject/QInt works) There is additional bounds checking performed prior to serialization if the serialized type is less than 64 bits though, so we'd probably want to add fixed-width accessors if we found ourselves in a situation where we needed to cast to a smaller datatype than the original. It's also worth noting that visiting uint64 types using int64 visitor interfaces isn't universally guaranteed to work: for certain visitor implementations the serialized encodings may not be compatible with one another. But in this case we should be good. > > >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > > > Not these are 64 bit values, but they need to be > > unsigned not signed. > > > >> but not need for extra API, with fixed property definition > >> i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > > > If you replace these with UINT32 you won't be able to > > specify values >4G. > > > >>> Property names are hard-coded string literals. > >>> Please add macros to set and get them > >>> so we can avoid duplicating code. > >>> E.g. > >> sure. > >> > >>> > >>> #define PCI_HOST_PROPS... > >>> static inline get_ > [...] > >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, > >>>> return "0000"; > >>>> } > >>>> > >>>> +static Property i440fx_props[] = { > >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), > >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), > >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), > >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, > >>>> + IO_APIC_DEFAULT_ADDRESS), > >>>> + DEFINE_PROP_END_OF_LIST(), > >>>> +}; > >>>> + > >>> > >>> So we have 4 properties. One of them pci_hole64_end > >>> is supposed to be set to a value. > >>> Others should not be touched under any circuimstances. > >>> Of course if you query properties you have no way > >>> to know which is which and what are the legal values. > >>> Ouch. > >> read-only properties are possible but we would have to drop > >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo, > > > > Or add DEFINE_PROP_UINT64_RO for this? > > > >> user better not to touch these properties since they are mostly internal API. > >> but if we say it's internal properties then enforcing read-only might be > >> overkill. > >> For user friendly property "pci_hole64_size" would be nice to have. > > > > So at the moment I do > > > > qemu -device i440FX-pcihost,help > > > > and this will get all properties. > > > > If we add some properties that user can not set > > they should not appear in this output. > [snip] > > Igor, you can simply use dynamic properties with NULL as setter argument > for object_property_add*() to achieve that effect. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index b0b98a8..acaeb6c 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo { static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info) { PcRomPciInfo *info; + Object *pci_info; + if (!guest_info->has_pci_info || !guest_info->fw_cfg) { return; } + pci_info = object_resolve_path("/machine/i440fx", NULL); + if (!pci_info) { + pci_info = object_resolve_path("/machine/q35", NULL); + 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_hole_start", NULL)); + info->w32_max = cpu_to_le64(object_property_get_int(pci_info, + "pci_hole_end", NULL)); + info->w64_min = cpu_to_le64(object_property_get_int(pci_info, + "pci_hole64_start", NULL)); + info->w64_max = cpu_to_le64(object_property_get_int(pci_info, + "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,31 @@ 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) +{ + if (sizeof(hwaddr) == 4) { + memset(&pci_info->w64, sizeof(pci_info->w64), 0); + 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); + if (!pci_info->w64.end) { + /* set default end if it wasn't specified, + 2 Gb past start */ + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31); + } + 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 4624d04..59a42c5 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -32,6 +32,7 @@ #include "hw/xen/xen.h" #include "hw/pci-host/pam.h" #include "sysemu/sysemu.h" +#include "hw/i386/ioapic.h" /* * I440FX chipset data sheet. @@ -44,6 +45,7 @@ typedef struct I440FXState { PCIHostState parent_obj; + PcPciInfo pci_info; } I440FXState; #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ @@ -247,8 +249,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 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, PIIX3State *piix3; PCII440FXState *f; unsigned i; + I440FXState *i440fx; + uint64_t pci_hole64_size; dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST); s = PCI_HOST_BRIDGE(dev); @@ -274,14 +277,30 @@ 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(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); + pci_hole64_size = range_size(i440fx->pci_info.w64); memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", f->pci_address_space, - pci_hole64_start, pci_hole64_size); + i440fx->pci_info.w64.begin, pci_hole64_size); if (pci_hole64_size) { - memory_region_add_subregion(f->system_memory, pci_hole64_start, + 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 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge, return "0000"; } +static Property i440fx_props[] = { + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0), + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0), + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0), + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end, + IO_APIC_DEFAULT_ADDRESS), + DEFINE_PROP_END_OF_LIST(), +}; + static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -638,6 +666,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..a6936e6 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge, static Property mch_props[] = { DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr, MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT), + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost, + mch.pci_info.w64.begin, 0), + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost, + mch.pci_info.w64.end, 0), + /* 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. + */ + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost, + mch.pci_info.w32.begin, + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + + MCH_HOST_BRIDGE_PCIEXBAR_MAX), + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost, + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS), DEFINE_PROP_END_OF_LIST(), }; @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d) 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 +277,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); + pci_hole64_size = range_size(mch->pci_info.w64); memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64", mch->pci_address_space, - 0x100000000ULL + mch->above_4g_mem_size, + mch->pci_info.w64.begin, pci_hole64_size); if (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..ab747b7 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; }; @@ -100,6 +99,7 @@ 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); +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start); FWCfgState *pc_memory_init(MemoryRegion *system_memory, const char *kernel_filename, @@ -150,8 +150,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..3a9e04b 100644 --- a/include/hw/pci-host/q35.h +++ b/include/hw/pci-host/q35.h @@ -55,6 +55,7 @@ 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;