Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default

[Igor Mammedov]

On Sun, 28 Jul 2013 10:57:12 +0300
"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>,
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  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
> 


-- 
Regards,
  Igor