| Message ID | 1608295996-8464-3-git-send-email-mihai.carabas@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] hw/misc/pvpanic: split-out generic and bus dependent code | expand |
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 259f9c9..e9512ab 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -108,6 +108,7 @@ extern bool pci_available; > #define PCI_DEVICE_ID_REDHAT_MDPY 0x000f > #define PCI_DEVICE_ID_REDHAT_NVME 0x0010 > #define PCI_DEVICE_ID_REDHAT_QXL 0x0100 > +#define PCI_DEVICE_ID_REDHAT_PVPANIC 0x0101 qxl has 0x100 for historical reasons, otherwise devices are simply enumerated upwards. NVME has 0x10, so PVPANIC should simply take the next free which is 0x11. Didn't I mention that in my last mail? thanks, Gerd
> On 22 Dec 2020, at 17:30, Gerd Hoffmann <kraxel@redhat.com> wrote: > > >> >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index 259f9c9..e9512ab 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -108,6 +108,7 @@ extern bool pci_available; >> #define PCI_DEVICE_ID_REDHAT_MDPY 0x000f >> #define PCI_DEVICE_ID_REDHAT_NVME 0x0010 >> #define PCI_DEVICE_ID_REDHAT_QXL 0x0100 >> +#define PCI_DEVICE_ID_REDHAT_PVPANIC 0x0101 > > qxl has 0x100 for historical reasons, otherwise devices are simply > enumerated upwards. NVME has 0x10, so PVPANIC should simply take > the next free which is 0x11. Didn't I mention that in my last mail? > > thanks, > Gerd Sorry, my mistake. I did not read your email carefully. Consider this modification done. Waiting for other feedback a couple of days and I will send a new revision with this update. Thank you, Mihai >
On Fri, 18 Dec 2020 at 13:36, Mihai Carabas <mihai.carabas@oracle.com> wrote: > > Add PCI interface support for PVPANIC device. Create a new file pvpanic-pci.c > where the PCI specific routines reside and update the build system with the new > files and config structure. > > Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com> > --- > docs/specs/pci-ids.txt | 2 ++ > hw/misc/Kconfig | 6 ++++ > hw/misc/meson.build | 1 + > hw/misc/pvpanic-pci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/misc/pvpanic.h | 1 + > include/hw/pci/pci.h | 1 + > 6 files changed, 98 insertions(+) > create mode 100644 hw/misc/pvpanic-pci.c > > diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt > index abbdbca..191681d 100644 > --- a/docs/specs/pci-ids.txt > +++ b/docs/specs/pci-ids.txt > @@ -68,3 +68,5 @@ PCI devices (other than virtio): > All these devices are documented in docs/specs. > > The 0100 device ID is used for the QXL video card device. > + > +The 0101 device ID is used for the PVPanic device. > diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig > index b58e6fd..aca7ffb 100644 > --- a/hw/misc/Kconfig > +++ b/hw/misc/Kconfig > @@ -124,6 +124,12 @@ config IOTKIT_SYSINFO > config PVPANIC_COMMON > bool > > +config PVPANIC_PCI > + bool > + default y if PCI_DEVICES > + depends on PCI > + select PVPANIC_COMMON > + > config PVPANIC_ISA > bool > default y > diff --git a/hw/misc/meson.build b/hw/misc/meson.build > index 8c828ad..f686019 100644 > --- a/hw/misc/meson.build > +++ b/hw/misc/meson.build > @@ -99,6 +99,7 @@ softmmu_ss.add(when: 'CONFIG_ARMSSE_CPUID', if_true: files('armsse-cpuid.c')) > softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c')) > > softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c')) > +softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c')) > softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c')) > softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c')) > softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c')) > diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c > new file mode 100644 > index 0000000..173909a > --- /dev/null > +++ b/hw/misc/pvpanic-pci.c > @@ -0,0 +1,87 @@ > +/* > + * QEMU simulated PCI pvpanic device. > + * > + * Copyright (C) 2020 Oracle > + * > + * Authors: > + * Mihai Carabas <mihai.carabas@oracle.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/log.h" > +#include "qemu/module.h" > +#include "sysemu/runstate.h" > + > +#include "hw/nvram/fw_cfg.h" > +#include "hw/qdev-properties.h" > +#include "hw/misc/pvpanic.h" > +#include "qom/object.h" > +#include "hw/pci/pci.h" > + > +typedef struct PVPanicPCIState PVPanicPCIState; > +DECLARE_INSTANCE_CHECKER(PVPanicPCIState, PVPANIC_PCI_DEVICE, > + TYPE_PVPANIC_PCI) The doc comment for the DECLARE_INSTANCE_CHECKER() macro says "Direct usage of this macro should be avoided, and the complete OBJECT_DECLARE_TYPE macro is recommended instead." > + > +/* > + * PVPanicPCIState for PCI device > + */ > +typedef struct PVPanicPCIState { > + PCIDevice dev; > + PVPanicState pvpanic; > +} PVPanicPCIState; > + > +/* pvpanic pci device*/ Missing space before '*/', but the comment isn't really telling the reader anything, so you could just delete it. > + > +static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp) > +{ > + PVPanicPCIState *s = DO_UPCAST(PVPanicPCIState, dev, dev); Since this is a QOM device, better to use the QOM cast rather than DO_UPCAST(): PVPanicPCIState *s = PVPANIC_PCI_DEVICE(dev); > + PVPanicState *ps = &s->pvpanic; > + > + pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2); Why 2 bytes? > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr); > +} > +static void pvpanic_pci_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass); > + > + device_class_set_props(dc, pvpanic_pci_properties); > + > + pc->realize = pvpanic_pci_realizefn; > + pc->vendor_id = PCI_VENDOR_ID_REDHAT; > + pc->device_id = PCI_DEVICE_ID_REDHAT_PVPANIC; > + pc->revision = 1; > + pc->class_id = PCI_CLASS_SYSTEM_OTHER; > + > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > +} You also need to set the dc->vmsd to a VMState for this device. The ISA pvpanic didn't need one because the pvpanic device itself has no variable state and an ISA device doesn't either, but PCI devices do have guest-modifiable state, so you need a VMState structure that uses a VMSTATE_PCI_DEVICE() line to ensure it gets saved and restored on migration. > +static TypeInfo pvpanic_pci_info = { > + .name = TYPE_PVPANIC_PCI, > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(PVPanicPCIState), > + .class_init = pvpanic_pci_class_init, > + .interfaces = (InterfaceInfo[]) { > + { INTERFACE_PCIE_DEVICE }, > + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, I'm not very familiar with the PCI code, but are we definitely doing enough to be able to claim to be a PCIe device ? > + { } > + } > +}; > + > +static void pvpanic_register_types(void) > +{ > + type_register_static(&pvpanic_pci_info); > +} > + > +type_init(pvpanic_register_types); thanks -- PMM
diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt index abbdbca..191681d 100644 --- a/docs/specs/pci-ids.txt +++ b/docs/specs/pci-ids.txt @@ -68,3 +68,5 @@ PCI devices (other than virtio): All these devices are documented in docs/specs. The 0100 device ID is used for the QXL video card device. + +The 0101 device ID is used for the PVPanic device. diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig index b58e6fd..aca7ffb 100644 --- a/hw/misc/Kconfig +++ b/hw/misc/Kconfig @@ -124,6 +124,12 @@ config IOTKIT_SYSINFO config PVPANIC_COMMON bool +config PVPANIC_PCI + bool + default y if PCI_DEVICES + depends on PCI + select PVPANIC_COMMON + config PVPANIC_ISA bool default y diff --git a/hw/misc/meson.build b/hw/misc/meson.build index 8c828ad..f686019 100644 --- a/hw/misc/meson.build +++ b/hw/misc/meson.build @@ -99,6 +99,7 @@ softmmu_ss.add(when: 'CONFIG_ARMSSE_CPUID', if_true: files('armsse-cpuid.c')) softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c')) softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c')) +softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c')) softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c')) softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c')) softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c')) diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c new file mode 100644 index 0000000..173909a --- /dev/null +++ b/hw/misc/pvpanic-pci.c @@ -0,0 +1,87 @@ +/* + * QEMU simulated PCI pvpanic device. + * + * Copyright (C) 2020 Oracle + * + * Authors: + * Mihai Carabas <mihai.carabas@oracle.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "sysemu/runstate.h" + +#include "hw/nvram/fw_cfg.h" +#include "hw/qdev-properties.h" +#include "hw/misc/pvpanic.h" +#include "qom/object.h" +#include "hw/pci/pci.h" + +typedef struct PVPanicPCIState PVPanicPCIState; +DECLARE_INSTANCE_CHECKER(PVPanicPCIState, PVPANIC_PCI_DEVICE, + TYPE_PVPANIC_PCI) + +/* + * PVPanicPCIState for PCI device + */ +typedef struct PVPanicPCIState { + PCIDevice dev; + PVPanicState pvpanic; +} PVPanicPCIState; + +/* pvpanic pci device*/ + +static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp) +{ + PVPanicPCIState *s = DO_UPCAST(PVPanicPCIState, dev, dev); + PVPanicState *ps = &s->pvpanic; + + pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2); + + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr); +} + +static Property pvpanic_pci_properties[] = { + DEFINE_PROP_UINT8("events", PVPanicState, events, PVPANIC_PANICKED | PVPANIC_CRASHLOADED), + DEFINE_PROP_END_OF_LIST(), +}; + +static void pvpanic_pci_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass); + + device_class_set_props(dc, pvpanic_pci_properties); + + pc->realize = pvpanic_pci_realizefn; + pc->vendor_id = PCI_VENDOR_ID_REDHAT; + pc->device_id = PCI_DEVICE_ID_REDHAT_PVPANIC; + pc->revision = 1; + pc->class_id = PCI_CLASS_SYSTEM_OTHER; + + set_bit(DEVICE_CATEGORY_MISC, dc->categories); +} + +static TypeInfo pvpanic_pci_info = { + .name = TYPE_PVPANIC_PCI, + .parent = TYPE_PCI_DEVICE, + .instance_size = sizeof(PVPanicPCIState), + .class_init = pvpanic_pci_class_init, + .interfaces = (InterfaceInfo[]) { + { INTERFACE_PCIE_DEVICE }, + { INTERFACE_CONVENTIONAL_PCI_DEVICE }, + { } + } +}; + +static void pvpanic_register_types(void) +{ + type_register_static(&pvpanic_pci_info); +} + +type_init(pvpanic_register_types); diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h index 691cf61..de1d0a9 100644 --- a/include/hw/misc/pvpanic.h +++ b/include/hw/misc/pvpanic.h @@ -18,6 +18,7 @@ #include "qom/object.h" #define TYPE_PVPANIC_ISA "pvpanic" +#define TYPE_PVPANIC_PCI "pvpanic-pci" #define PVPANIC_IOPORT_PROP "ioport" diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 259f9c9..e9512ab 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -108,6 +108,7 @@ extern bool pci_available; #define PCI_DEVICE_ID_REDHAT_MDPY 0x000f #define PCI_DEVICE_ID_REDHAT_NVME 0x0010 #define PCI_DEVICE_ID_REDHAT_QXL 0x0100 +#define PCI_DEVICE_ID_REDHAT_PVPANIC 0x0101 #define FMT_PCIBUS PRIx64
Add PCI interface support for PVPANIC device. Create a new file pvpanic-pci.c where the PCI specific routines reside and update the build system with the new files and config structure. Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com> --- docs/specs/pci-ids.txt | 2 ++ hw/misc/Kconfig | 6 ++++ hw/misc/meson.build | 1 + hw/misc/pvpanic-pci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ include/hw/misc/pvpanic.h | 1 + include/hw/pci/pci.h | 1 + 6 files changed, 98 insertions(+) create mode 100644 hw/misc/pvpanic-pci.c