On Sun, 28 Jul 2013 12:11:42 +0300
"Michael S. Tsirkin" <m...@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" <m...@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 <imamm...@redhat.com>,
> > > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imamm...@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
>
--
Regards,
Igor
