From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44001) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3NQu-0007CU-Au for qemu-devel@nongnu.org; Sun, 28 Jul 2013 05:38:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3NQn-0004Sr-13 for qemu-devel@nongnu.org; Sun, 28 Jul 2013 05:38:32 -0400 Received: from cantor2.suse.de ([195.135.220.15]:56715 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3NQm-0004Sm-KG for qemu-devel@nongnu.org; Sun, 28 Jul 2013 05:38:24 -0400 Message-ID: <51F4E689.80405@suse.de> Date: Sun, 28 Jul 2013 11:38:17 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 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> In-Reply-To: <20130728073028.GF12087@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 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: "Michael S. Tsirkin" Cc: Anthony Liguori , qemu-devel@nongnu.org, Aurelien Jarno , Gerd Hoffmann 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? 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 =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_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 =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, 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 =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_sta= te, int *piix3_devfn, >>> return b; >>> } >>> =20 >>> +PCIBus *find_i440fx(void) >>> +{ >>> + PCIHostState *s =3D OBJECT_CHECK(PCIHostState, >>> + object_resolve_path("/machine/i44= 0fx", 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 =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? 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. 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=3Dpci.0 according to review feedback the Xen people have received for libxl. Andreas >=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_stat= e, 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=FCr= nberg --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg