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

[Igor Mammedov]

On Sun, 28 Jul 2013 12:11:42 +0300
"Michael S. Tsirkin" <address@hidden> 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" <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.
> 
> Well because you are specifying end as 0xffffffffffffffff
> so it's not a multiple of 1G?
> 
> > > 
> > > > 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/
> 
> 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