From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3Nyh-00036B-CK for qemu-devel@nongnu.org; Sun, 28 Jul 2013 06:13:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3NyY-0006fv-DR for qemu-devel@nongnu.org; Sun, 28 Jul 2013 06:13:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51710) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3NyY-0006fr-5L for qemu-devel@nongnu.org; Sun, 28 Jul 2013 06:13:18 -0400 Date: Sun, 28 Jul 2013 13:14:27 +0300 From: "Michael S. Tsirkin" Message-ID: <20130728101427.GA15065@redhat.com> References: <1374681580-17439-1-git-send-email-mst@redhat.com> <1374681580-17439-12-git-send-email-mst@redhat.com> <20130725093248.GA27524@redhat.com> <51F46201.2010106@suse.de> <20130728073028.GF12087@redhat.com> <51F4E689.80405@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <51F4E689.80405@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Anthony Liguori , qemu-devel@nongnu.org, Aurelien Jarno , Gerd Hoffmann On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas F=E4rber wrote: > Am 28.07.2013 09:30, schrieb Michael S. Tsirkin: > > On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas F=E4rber 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 > >>> --- > >>> > >>> 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" > >>> =20 > >>> //#define DEBUG > >>> =20 > >>> @@ -63,7 +64,7 @@ typedef struct CPUStatus { > >>> uint8_t sts[PIIX4_PROC_LEN]; > >>> } CPUStatus; > >>> =20 > >>> -typedef struct PIIX4PMState { > >>> +struct PIIX4PMState { > >>> /*< private >*/ > >>> PCIDevice parent_obj; > >>> /*< public >*/ > >>> @@ -96,7 +97,7 @@ typedef struct PIIX4PMState { > >>> =20 > >>> CPUStatus gpe_cpu; > >>> Notifier cpu_added_notifier; > >>> -} PIIX4PMState; > >>> +}; > >>> =20 > >>> #define TYPE_PIIX4_PM "PIIX4_PM" > >>> =20 > >> > >> 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; > >>> } > >>> =20 > >>> +PIIX4PMState *piix4_pm_find(void) > >>> +{ > >>> + bool ambig; > >>> + Object *o =3D 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. > >=20 > > 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". > >=20 > > Is the point that macros are somehow better than functions so they > > should be used where possible? >=20 > 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 makin= g > Anthony's QOM pattern consistent. >=20 > >>> +} > >>> + > >>> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info) > >>> +{ > >>> + info->s3_disabled =3D s->disable_s3; > >>> + info->s4_disabled =3D s->disable_s4; > >>> + info->s4_val =3D s->s4_val; > >> > >> These three are accessible as qdev/QOM properties. > >> > >>> + > >>> + info->acpi_enable_cmd =3D ACPI_ENABLE; > >>> + info->acpi_disable_cmd =3D ACPI_DISABLE; > >>> + info->gpe0_blk =3D GPE_BASE; > >>> + info->gpe0_blk_len =3D GPE_LEN; > >> > >> So here the issue is that values are constant? > >=20 > > Yes. > >=20 > >>> + info->sci_int =3D 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_bas= e, > >>> 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 =3D 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, NUL= L); > >> > >> This looks fishy. Might belong into a different patch? Implementatio= n > >> didn't change above. > >> > >>> /* TODO: Populate SPD eeprom data. */ > >>> smbus_eeprom_init(smbus, 8, NULL, 0); > >>> pit =3D 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_s= tate, int *piix3_devfn, > >>> return b; > >>> } > >>> =20 > >>> +PCIBus *find_i440fx(void) > >>> +{ > >>> + PCIHostState *s =3D OBJECT_CHECK(PCIHostState, > >>> + object_resolve_path("/machine/i= 440fx", 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 trivia= l > >> addition to the path that's already being used here. You can do: > >> PCIBus *bus =3D PCI_BUS(object_resolve_path("/machine/i440fx/pci.0")= ); > >> where you actually need to access it. > >=20 > >=20 > > I don't mind but I would like to avoid callers hard-coding > > paths, in this case "i440fx". > > Why the aversion to functions? >=20 > 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. >=20 > The suggested "pci.0" is considered stable for -device ...,bus=3Dpci.0 > according to review feedback the Xen people have received for libxl. >=20 > Andreas >=20 > >=20 > >>> + > >>> /* 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_st= ate, int *piix_devfn, > >>> MemoryRegion *pci_memory, > >>> MemoryRegion *ram_memory); > >>> =20 > >>> +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; > >>> =20 > >>> #endif /* QEMU_TYPEDEFS_H */ > >>> > >> > >> > >> --=20 > >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > >> GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FC= rnberg >=20 >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg