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

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Phil,

On 11/18/25 11:16, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> (old patch merged as commit d097b3dcb62)
> 
> On 27/1/21 15:59, Mihai Carabas 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>
>> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>> ---
>>   docs/specs/pci-ids.txt    |  1 +
>>   hw/misc/Kconfig           |  6 +++
>>   hw/misc/meson.build       |  1 +
>>   hw/misc/pvpanic-pci.c     | 94 +++++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/misc/pvpanic.h |  1 +
>>   include/hw/pci/pci.h      |  1 +
>>   6 files changed, 104 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..5e407a6 100644
>> --- a/docs/specs/pci-ids.txt
>> +++ b/docs/specs/pci-ids.txt
>> @@ -64,6 +64,7 @@ PCI devices (other than virtio):
>>   1b36:000d  PCI xhci usb host adapter
>>   1b36:000f  mdpy (mdev sample device), linux/samples/vfio-mdev/mdpy.c
>>   1b36:0010  PCIe NVMe device (-device nvme)
>> +1b36:0011  PCI PVPanic device (-device pvpanic-pci)
>>   All these devices are documented in docs/specs.
>> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
>> index 23bc978..19c216f 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
>>       depends on ISA_BUS
>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>> index edaaec2..6292839 100644
>> --- a/hw/misc/meson.build
>> +++ b/hw/misc/meson.build
>> @@ -100,6 +100,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..d629639
>> --- /dev/null
>> +++ b/hw/misc/pvpanic-pci.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * 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 "migration/vmstate.h"
>> +#include "hw/misc/pvpanic.h"
>> +#include "qom/object.h"
>> +#include "hw/pci/pci.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(PVPanicPCIState, PVPANIC_PCI_DEVICE)
>> +
>> +/*
>> + * PVPanicPCIState for PCI device
>> + */
>> +typedef struct PVPanicPCIState {
>> +    PCIDevice dev;
>> +    PVPanicState pvpanic;
>> +} PVPanicPCIState;
>> +
>> +static const VMStateDescription vmstate_pvpanic_pci = {
>> +    .name = "pvpanic-pci",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_PCI_DEVICE(dev, PVPanicPCIState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
>> +{
>> +    PVPanicPCIState *s = PVPANIC_PCI_DEVICE(dev);
>> +    PVPanicState *ps = &s->pvpanic;
>> +
>> +    pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2);
> 
> Why registering 2-bytes of I/O when the underlying device is 1-byte
> wide? Is this to allow 16-bit I/O instructions?

TL;DR: I think that 2 should actually be 16 :)

First, IMO, pvpanic_setup_io() should be called pvpanic_setup_mmio() because
the registered memory region in question is of type PCI_BASE_ADDRESS_SPACE_MEMORY,
not of type PCI_BASE_ADDRESS_SPACE_IO, hence the BAR associated to
this memory region (ps->mr) will map a MMIO region, not an I/O region.

But I think I see why it looks confusing to me: QEMU puts both a Memory Space
and an I/O Space (PCI terms here) into pci_dev->io_regions[] (which is ok).
The former defines a MMIO region and the latter an I/O (x86 old-style) region.

I'm just saying that in case you were thinking of inb/outb/insw/outsw &
friends when you said "to allow 16-bit I/O instructions", which are mean
to work with PCI_BASE_ADDRESS_SPACE_IO, not with PCI_BASE_ADDRESS_SPACE_MEMORY
regions, where normal CPU load/store instructions (mov, ldr, str, ld, std, etc.)
should be used instead.

For such normal a CPU loads/stores the granularity would be, in general, 1 byte,
so CPU can load/store 1 byte or more on most PCI devices (in MMIO regions), hence:

       .impl = {
           .min_access_size = 1,
           .max_access_size = 1,
       },


looks correct to me.

That said, I understand that pvpanic_setup_io is defining a 2 bytes
PCI_BASE_ADDRESS_SPACE_MEMORY (Memory Space) and the minimum size of this
region should actually be 16 bytes, not 2 bytes.

If you take a look at the PCI spec 3.0, in section 6.2.5.1, "Address Maps",
it says, about "Base Address Register for Memory":

"A 32-bit register can be implemented to support a single memory size that
is a power of 2 from 16 bytes to 2 GB."

So the minimum size of a Memory Space is 16 bytes, because bits from 0 to 3
in the BAR are not used for address decoding purposes (they define the type
of the space, number of bits in the address space, etc.).

Also, in pci_register_bar() the size is used to set the wmask for the
BAR register:

wmask = ~(size - 1);

and with size = 2, if my math is right, gives 0xfffffffe, which is invalid
(it should be 0xfffffff0 for the minimum size: bits from 0 to 3 should
be zeroed (falgs) and bit 4 and upwards should be one. The first bit set as
1 starting from the least significant bit determines the size of the MMIO region,
and in 0xfffffff0 bit 4 is the first one that is 1, which kind tells us the
minimum size should really be 16 bytes for a MMIO region).

So that's my 2 cents on it :)


Cheers,
Gustavo

>> +
>> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr);
>> +}
>> +
>> +static Property pvpanic_pci_properties[] = {
>> +    DEFINE_PROP_UINT8("events", PVPanicPCIState, pvpanic.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;
>> +    dc->vmsd = &vmstate_pvpanic_pci;
>> +
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static TypeInfo pvpanic_pci_info = {
>> +    .name          = TYPE_PVPANIC_PCI_DEVICE,
>> +    .parent        = TYPE_PCI_DEVICE,
>> +    .instance_size = sizeof(PVPanicPCIState),
>> +    .class_init    = pvpanic_pci_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { 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 abc9dde..ca3c5bb 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_DEVICE "pvpanic"
>> +#define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
>>   #define PVPANIC_IOPORT_PROP "ioport"
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 259f9c9..66db084 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -107,6 +107,7 @@ extern bool pci_available;
>>   #define PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE 0x000e
>>   #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
>>   #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
>> +#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0011
>>   #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>>   #define FMT_PCIBUS                      PRIx64
> 
>