Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info

[Michael S. Tsirkin]

On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
> > This adds APIs that will be used to fill in guest info table,
> > implemented using QOM, to various piix components.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> > 
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index c885690..2128f13 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -29,6 +29,7 @@
> >  #include "exec/ioport.h"
> >  #include "hw/nvram/fw_cfg.h"
> >  #include "exec/address-spaces.h"
> > +#include "hw/acpi/piix4.h"
> >  
> >  //#define DEBUG
> >  
> > @@ -63,7 +64,7 @@ typedef struct CPUStatus {
> >      uint8_t sts[PIIX4_PROC_LEN];
> >  } CPUStatus;
> >  
> > -typedef struct PIIX4PMState {
> > +struct PIIX4PMState {
> >      /*< private >*/
> >      PCIDevice parent_obj;
> >      /*< public >*/
> > @@ -96,7 +97,7 @@ typedef struct PIIX4PMState {
> >  
> >      CPUStatus gpe_cpu;
> >      Notifier cpu_added_notifier;
> > -} PIIX4PMState;
> > +};
> >  
> >  #define TYPE_PIIX4_PM "PIIX4_PM"
> >  
> 
> Here add
> 
> #define PIIX4_PM(obj) OBJECT_CHECK(PIIX4PMState, obj, TYPE_PIIX4_PM)
> 
> for the general public ...
> 
> > @@ -458,6 +459,30 @@ static int piix4_pm_initfn(PCIDevice *dev)
> >      return 0;
> >  }
> >  
> > +PIIX4PMState *piix4_pm_find(void)
> > +{
> > +    bool ambig;
> > +    Object *o = object_resolve_path_type("", "PIIX4_PM", &ambig);
> > +
> > +    if (ambig || !o) {
> > +        return NULL;
> > +    }
> > +    return OBJECT_CHECK(PIIX4PMState, o, "PIIX4_PM");
> 
> ... instead of open-coding it where only you use it, please.

I don't really understand in which direction you want to take this.
On the one hand, you are saying "there's one caller of
this function so please open-code it".
On the other hand, you are saying "it does not matter that
there's one user of this macro put it in the header".

Is the point that macros are somehow better than functions so they
should be used where possible?


> > +}
> > +
> > +void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info)
> > +{
> > +        info->s3_disabled = s->disable_s3;
> > +        info->s4_disabled = s->disable_s4;
> > +        info->s4_val = s->s4_val;
> 
> These three are accessible as qdev/QOM properties.
> 
> > +
> > +        info->acpi_enable_cmd = ACPI_ENABLE;
> > +        info->acpi_disable_cmd = ACPI_DISABLE;
> > +        info->gpe0_blk = GPE_BASE;
> > +        info->gpe0_blk_len = GPE_LEN;
> 
> So here the issue is that values are constant?

Yes.

> > +        info->sci_int = 9;
> 
> Magic number - use a define to share it with wherever it is used?
> 
> > +}
> > +
> >  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> >                         qemu_irq sci_irq, qemu_irq smi_irq,
> >                         int kvm_enabled, FWCfgState *fw_cfg)
> > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> > index de87241..14573ab 100644
> > --- a/hw/mips/mips_malta.c
> > +++ b/hw/mips/mips_malta.c
> > @@ -965,7 +965,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
> >      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
> >      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
> >      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
> > -                          isa_get_irq(NULL, 9), NULL, 0, NULL);
> > +                          isa_get_irq(NULL, 9), NULL, 0, NULL, NULL);
> 
> This looks fishy. Might belong into a different patch? Implementation
> didn't change above.
> 
> >      /* TODO: Populate SPD eeprom data.  */
> >      smbus_eeprom_init(smbus, 8, NULL, 0);
> >      pit = pit_init(isa_bus, 0x40, 0, NULL);
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index 3908860..daefdfb 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, 
> > int *piix3_devfn,
> >      return b;
> >  }
> >  
> > +PCIBus *find_i440fx(void)
> > +{
> > +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> > +                                   object_resolve_path("/machine/i440fx", 
> > NULL),
> > +                                   TYPE_PCI_HOST_BRIDGE);
> 
> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
> 
> > +    return s ? s->bus : NULL;
> > +}
> 
> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
> addition to the path that's already being used here. You can do:
> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
> where you actually need to access it.
> 
> Andreas


I don't mind but I would like to avoid callers hard-coding
paths, in this case "i440fx".
Why the aversion to functions?

> > +
> >  /* PIIX3 PCI to ISA bridge */
> >  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> >  {
> > diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> > new file mode 100644
> > index 0000000..2876428
> > --- /dev/null
> > +++ b/include/hw/acpi/piix4.h
> > @@ -0,0 +1,10 @@
> > +#ifndef HW_ACPI_PIIX4_H
> > +#define HW_ACPI_PIIX4_H
> > +
> > +#include "qemu/typedefs.h"
> > +
> > +PIIX4PMState *piix4_pm_find(void);
> > +
> > +void piix4_pm_get_acpi_pm_info(PIIX4PMState *, AcpiPmInfo *);
> > +
> > +#endif
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 7c0bd50..76af5cd 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -186,6 +186,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int 
> > *piix_devfn,
> >                      MemoryRegion *pci_memory,
> >                      MemoryRegion *ram_memory);
> >  
> > +PCIBus *find_i440fx(void);
> >  /* piix4.c */
> >  extern PCIDevice *piix4_dev;
> >  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index cb66e19..7d42693 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -65,6 +65,7 @@ typedef struct QEMUSGList QEMUSGList;
> >  typedef struct SHPCDevice SHPCDevice;
> >  typedef struct FWCfgState FWCfgState;
> >  typedef struct PcGuestInfo PcGuestInfo;
> > +typedef struct PIIX4PMState PIIX4PMState;
> >  typedef struct AcpiPmInfo AcpiPmInfo;
> >  
> >  #endif /* QEMU_TYPEDEFS_H */
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg