On Thu, May 30, 2013 at 02:25:41PM +0200, Laszlo Ersek wrote:
> I can't offer any opinion about the values you put into w32 and w64, but I
> have some remarks.
>
> First, please consider passing -O/path/to/some/order_file to
> git-format-patch, so that .h files show up at the top of each patch.
>
> On 05/30/13 13:07, Michael S. Tsirkin wrote:
> > Will be used to pass hole ranges to guests.
> >
> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > ---
> > hw/i386/pc.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > hw/i386/pc_piix.c | 14 +++++++++++++-
> > hw/i386/pc_q35.c | 6 +++++-
> > hw/pci-host/q35.c | 4 ++++
> > include/hw/i386/pc.h | 19 ++++++++++++++++++-
> > include/hw/pci-host/q35.h | 2 ++
> > include/qemu/typedefs.h | 1 +
> > 7 files changed, 81 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 4844a6b..c233161 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -978,6 +978,41 @@ void pc_cpus_init(const char *cpu_model, DeviceState
> > *icc_bridge)
> > }
> > }
> >
> > +typedef struct PcGuestInfoState {
> > + PcGuestInfo info;
> > + Notifier machine_done;
> > +} PcGuestInfoState;
> > +
> > +static
> > +void pc_guest_info_machine_done(Notifier *notifier, void *data)
> > +{
> > + PcGuestInfoState *guest_info_state = container_of(notifier,
> > + PcGuestInfoState,
> > + machine_done);
> > +}
> > +
> > +PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > + ram_addr_t above_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 {
> > + guest_info->pci_info.w64.begin = 0x100000000ULL +
> > above_4g_mem_size;
> > + 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_acpi_init(const char *default_dsdt)
> > {
> > char *filename;
> > @@ -1019,7 +1054,8 @@ FWCfgState *pc_memory_init(MemoryRegion
> > *system_memory,
> > ram_addr_t below_4g_mem_size,
> > ram_addr_t above_4g_mem_size,
> > MemoryRegion *rom_memory,
> > - MemoryRegion **ram_memory)
> > + MemoryRegion **ram_memory,
> > + PcGuestInfo *guest_info)
> > {
> > int linux_boot, i;
> > MemoryRegion *ram, *option_rom_mr;
> > @@ -1071,6 +1107,7 @@ FWCfgState *pc_memory_init(MemoryRegion
> > *system_memory,
> > for (i = 0; i < nb_option_roms; i++) {
> > rom_add_option(option_rom[i].name, option_rom[i].bootindex);
> > }
> > + guest_info->fw_cfg = fw_cfg;
> > return fw_cfg;
> > }
>
> I find this suboptimal style, ie. passing "guest_info" to pc_memory_init()
> just so that pc_memory_init() can set guest_info->fw_cfg to fw_cfg, when
> pc_memory_init() returns fw_cfg anyway.
Well otherwise all callers have to remember to set it.
> More "philosophically":
>
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index b4c8a74..1bf5219 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -9,8 +9,20 @@
> > #include "net/net.h"
> > #include "hw/i386/ioapic.h"
> >
> > +#include "qemu/range.h"
> > +
> > /* PC-style peripherals (also used by other machines). */
> >
> > +typedef struct PcPciInfo {
> > + Range w32;
> > + Range w64;
> > +} PcPciInfo;
> > +
> > +struct PcGuestInfo {
> > + PcPciInfo pci_info;
> > + FWCfgState *fw_cfg;
> > +};
>
> Are you sure you need a private link to the global fw_cfg object? The pvpanic
> series added a global lookup possibility in commit 10a584b2
> (object_resolve_path()).
>
> Anyway these are just subjective style notes.
Yes.
I personally prefer not using global lookups: passing a pointer makes
dependencies clearer IMO.
If we do switch to that, I think it's cleaner to have a wrapper so
that everyone does not need to hard-code strings like /machine/fw_cfg.
> > +
> > /* parallel.c */
> > static inline bool parallel_init(ISABus *bus, int index, CharDriverState
> > *chr)
> > {
> > @@ -80,6 +92,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> > level);
> > void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
> > void pc_hot_add_cpu(const int64_t id, Error **errp);
> > 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);
> > +
> > FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > const char *kernel_filename,
> > const char *kernel_cmdline,
> > @@ -87,7 +103,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > ram_addr_t below_4g_mem_size,
> > ram_addr_t above_4g_mem_size,
> > MemoryRegion *rom_memory,
> > - MemoryRegion **ram_memory);
> > + MemoryRegion **ram_memory,
> > + PcGuestInfo *guest_info);
> > qemu_irq *pc_allocate_cpu_irq(void);
> > DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> > void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>
> Going forward:
>
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index d547548..eaff0b6 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -90,6 +90,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > MemoryRegion *rom_memory;
> > DeviceState *icc_bridge;
> > FWCfgState *fw_cfg = NULL;
> > + PcGuestInfo *guest_info;
> >
> > icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> > object_property_add_child(qdev_get_machine(), "icc-bridge",
> > @@ -119,12 +120,23 @@ static void pc_init1(MemoryRegion *system_memory,
> > rom_memory = system_memory;
> > }
> >
> > + guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > +
> > + /* Set PCI window size the way seabios has always done it. */
> > + /* TODO: consider just starting at below_4g_mem_size */
> > + 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,
> > kernel_filename, kernel_cmdline, initrd_filename,
> > below_4g_mem_size, above_4g_mem_size,
> > - rom_memory, &ram_memory);
> > + rom_memory, &ram_memory, guest_info);
> > }
> >
> > gsi_state = g_malloc0(sizeof(*gsi_state));
>
> On PIIX you *almost* leak the guest_info structure at once; the only link to
> it is the machine-done-notifier registered in pc_guest_info_init(). Is this
> intended? (I guess a later patch in the series will change this.)
Yes. Machine done notifiers generally aren't cleaned up:
there is qemu_add_machine_init_done_notifier but not
qemu_del_machine_init_done_notifier,
so there's no way to free it.
Same applies to all other such notifiers.
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 7888dfe..32d6357 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -77,6 +77,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> > ICH9LPCState *ich9_lpc;
> > PCIDevice *ahci;
> > DeviceState *icc_bridge;
> > + PcGuestInfo *guest_info;
> >
> > icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> > object_property_add_child(qdev_get_machine(), "icc-bridge",
> > @@ -105,11 +106,13 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> > rom_memory = get_system_memory();
> > }
> >
> > + guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > +
> > /* allocate ram and load rom/bios */
> > if (!xen_enabled()) {
> > pc_memory_init(get_system_memory(), kernel_filename,
> > kernel_cmdline,
> > initrd_filename, below_4g_mem_size,
> > above_4g_mem_size,
> > - rom_memory, &ram_memory);
> > + rom_memory, &ram_memory, guest_info);
> > }
> >
> > /* irq lines */
> > @@ -131,6 +134,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> > q35_host->mch.address_space_io = get_system_io();
> > q35_host->mch.below_4g_mem_size = below_4g_mem_size;
> > q35_host->mch.above_4g_mem_size = above_4g_mem_size;
> > + q35_host->mch.guest_info = guest_info;
> > /* pci */
> > qdev_init_nofail(DEVICE(q35_host));
> > host_bus = q35_host->host.pci.bus;
>
> OK, a direct (owner) link is established here; which gives birth to the next
> question:
>
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 24df6b5..63c64dd 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -244,6 +244,10 @@ static int mch_init(PCIDevice *d)
> > hwaddr pci_hole64_size;
> > MCHPCIState *mch = MCH_PCI_DEVICE(d);
> >
> > + /* Leave enough space for the biggest MCFG BAR */
> > + 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, "pci-hole",
> > mch->pci_address_space,
>
>
> > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > index e182c82..b083831 100644
> > --- a/include/hw/pci-host/q35.h
> > +++ b/include/hw/pci-host/q35.h
> > @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
> > uint8_t smm_enabled;
> > ram_addr_t below_4g_mem_size;
> > ram_addr_t above_4g_mem_size;
> > + PcGuestInfo *guest_info;
> > } MCHPCIState;
>
> ... how does this relate to migration? (I'm not suggesting anything, I'm just
> curious.)
>
> Thanks,
> Laszlo
As far as I can tell it doesn't relate to migration in any way.
--
MST
