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

[Michael S. Tsirkin]

On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
> Am 28.07.2013 09:30, schrieb 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?
> 
> No, the point is reuse: For QOM realize we will very likely need the
> same thing. It was not my decision to make these macros, I'm just making
> Anthony's QOM pattern consistent.
> 
> >>> +}
> >>> +
> >>> +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.
> > 
> > 
> > I don't mind but I would like to avoid callers hard-coding
> > paths, in this case "i440fx".
> > Why the aversion to functions?
> 
> Simply because QMP cannot call functions. It has to work with qom-list
> and qom-get, so this is a test case showing what is missing and can IMO
> easily be addressed for both parties.

Fine but if the function calls QOM things internally
then where's the problem?

> The suggested cast to PCI_BUS() lets you use regular qdev functions btw
> as a shortcut, QMP users would need to iterate children of that node.
> 
> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
> according to review feedback the Xen people have received for libxl.
> 
> Andreas
> 
> > 
> >>> +
> >>>  /* 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
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg