[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info |
Date: |
Sun, 28 Jul 2013 11:38:17 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 |
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.
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
- Re: [Qemu-devel] [PATCH v3 12/14] pvpanic: add API to access io port, (continued)
[Qemu-devel] [PATCH v3 13/14] hpet: add API to find it, Michael S. Tsirkin, 2013/07/24
[Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/24
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/25
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/27
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info,
Andreas Färber <=
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/28
Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Gerd Hoffmann, 2013/07/25
[Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Michael S. Tsirkin, 2013/07/24