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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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 <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


>>> +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.