Re: [Qemu-devel] [PATCH 2/3] pc: get rid of builtin pvpanic

[Michael S. Tsirkin]

On Wed, Aug 21, 2013 at 08:03:58PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 21, 2013 at 06:43:15PM +0200, Paolo Bonzini wrote:
> > It is a source of pain, and the previous patch anyway changed the
> > behavior of "-M pc-1.5" compared to the real 1.5.
> > 
> > This also makes it clear that "-device pvpanic" is not enough:
> > it will not expose pvpanic in fw_cfg properly.
> > 
> > No idea how to fix that.
> > 
> > Signed-off-by: Paolo Bonzini <address@hidden>
> 
> That should be the first step. Make the device work properly.
> We'll discuss compatibility when that is clear.

So not nacking yet but I think it's too early to apply.

> > ---
> >  hw/i386/pc_piix.c    |  8 --------
> >  hw/i386/pc_q35.c     |  6 ------
> >  hw/misc/pvpanic.c    | 14 ++------------
> >  include/hw/i386/pc.h |  3 ---
> >  4 files changed, 2 insertions(+), 29 deletions(-)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index b58c255..b80f9a3 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -56,7 +56,6 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 
> > };
> >  static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
> >  static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
> >  
> > -static bool has_pvpanic = true;
> >  static bool has_pci_info = true;
> >  
> >  /* PC hardware initialisation */
> > @@ -240,10 +239,6 @@ static void pc_init1(MemoryRegion *system_memory,
> >      if (pci_enabled) {
> >          pc_pci_device_init(pci_bus);
> >      }
> > -
> > -    if (has_pvpanic) {
> > -        pvpanic_init(isa_bus);
> > -    }
> >  }
> >  
> >  static void pc_init_pci(QEMUMachineInitArgs *args)
> > @@ -269,7 +264,6 @@ static void pc_init_pci_1_5(QEMUMachineInitArgs *args)
> >  
> >  static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
> >  {
> > -    has_pvpanic = false;
> >      x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
> >      pc_init_pci_1_5(args);
> >  }
> > @@ -302,7 +296,6 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs 
> > *args)
> >      const char *kernel_cmdline = args->kernel_cmdline;
> >      const char *initrd_filename = args->initrd_filename;
> >      const char *boot_device = args->boot_device;
> > -    has_pvpanic = false;
> >      has_pci_info = false;
> >      disable_kvm_pv_eoi();
> >      enable_compat_apic_id_mode();
> > @@ -321,7 +314,6 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
> >      const char *kernel_cmdline = args->kernel_cmdline;
> >      const char *initrd_filename = args->initrd_filename;
> >      const char *boot_device = args->boot_device;
> > -    has_pvpanic = false;
> >      has_pci_info = false;
> >      if (cpu_model == NULL)
> >          cpu_model = "486";
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 0b1d2e3..fb403b8 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -46,7 +46,6 @@
> >  /* ICH9 AHCI has 6 ports */
> >  #define MAX_SATA_PORTS     6
> >  
> > -static bool has_pvpanic = true;
> >  static bool has_pci_info = true;
> >  
> >  /* PC hardware initialisation */
> > @@ -210,10 +209,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> >      if (pci_enabled) {
> >          pc_pci_device_init(host_bus);
> >      }
> > -
> > -    if (has_pvpanic) {
> > -        pvpanic_init(isa_bus);
> > -    }
> >  }
> >  
> >  static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
> > @@ -224,7 +219,6 @@ static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
> >  
> >  static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
> >  {
> > -    has_pvpanic = false;
> >      x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
> >      pc_q35_init_1_5(args);
> >  }
> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> > index 7bb49a5..1928cc9 100644
> > --- a/hw/misc/pvpanic.c
> > +++ b/hw/misc/pvpanic.c
> > @@ -101,7 +101,8 @@ static void pvpanic_isa_realizefn(DeviceState *dev, 
> > Error **errp)
> >      isa_register_ioport(d, &s->io, s->ioport);
> >  }
> >  
> > -static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
> > +static void __attribute__((unused)) pvpanic_fw_cfg(ISADevice *dev,
> > +                                                   FWCfgState *fw_cfg)
> >  {
> >      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> >      uint16_t *pvpanic_port = g_malloc(sizeof(*pvpanic_port));
> > @@ -111,17 +112,6 @@ static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState 
> > *fw_cfg)
> >                      sizeof(*pvpanic_port));
> >  }
> >  
> > -void pvpanic_init(ISABus *bus)
> > -{
> > -    ISADevice *dev;
> > -    FWCfgState *fw_cfg = fw_cfg_find();
> > -    if (!fw_cfg) {
> > -        return;
> > -    }
> > -    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
> > -    pvpanic_fw_cfg(dev, fw_cfg);
> > -}
> > -
> >  static Property pvpanic_isa_properties[] = {
> >      DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
> >      DEFINE_PROP_END_OF_LIST(),
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 7fb97b0..064cc81 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -194,9 +194,6 @@ static inline bool isa_ne2000_init(ISABus *bus, int 
> > base, int irq, NICInfo *nd)
> >  /* pc_sysfw.c */
> >  void pc_system_firmware_init(MemoryRegion *rom_memory);
> >  
> > -/* pvpanic.c */
> > -void pvpanic_init(ISABus *bus);
> > -
> >  /* e820 types */
> >  #define E820_RAM        1
> >  #define E820_RESERVED   2
> > -- 
> > 1.8.3.1
> >