On 19/11/25 00:18, Gustavo Romero wrote:
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 <[email protected]>
Reviewed-by: Gerd Hoffmann <[email protected]>
Reviewed-by: Peter Maydell <[email protected]>
Signed-off-by: Mihai Carabas <[email protected]>
---
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
+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.).
Thank you, this is the relevant info I was looking for :)
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).
Also interesting for PCI_BASE_ADDRESS_SPACE_IO:
Devices that map control functions into I/O Space must not
consume more than 256 bytes per I/O Base Address register.
Regards,
Phil.
