Re: [PATCH 2/3] hw/misc/pvpanic: add PCI interface support

[Peter Maydell]

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