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

[Igor Mammedov]

On Sun, 28 Jul 2013 22:51:57 +0300
"Michael S. Tsirkin" <address@hidden> 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" <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?
> 
> 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.

[...]