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

[Michael S. Tsirkin]

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 <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.

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