Hi,
On 1/28/20 6:26 PM, Andre Przywara wrote:
> On Mon, 25 Nov 2019 10:30:24 +0000
> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi,
>
>> From: Julien Thierry <julien.thierry@xxxxxxx>
>>
>> The PCI Local Bus Specification, Rev. 3.0, Section 6.2.5.1. "Address Maps"
>> states: "Devices that map control functions into I/O Space must not consume
>> more than 256 bytes per I/O Base Address register."
>>
>> Yet all the PCI devices allocate IO ports of IOPORT_SIZE (= 1024 bytes).
>>
>> Fix this by having PCI devices use 256 bytes ports for IO BARs.
>>
>> There is no hard requirement on the size of the memory region described
>> by memory BARs. However, the region must be big enough to hold the
>> virtio common interface described in [1], which is 20 bytes, and other
>> MSI-X and/or device specific configuration. To be consistent, let's also
>> limit the memory region described by BAR1 to 256. This is the same size
>> used by BAR2 for each of the two MSI-X vectors.
> So the I/O port size is surely fine, QEMU seems to get away with 64 or even 32 bytes.
> But QEMU also reports a memory region size of 4K, is that something we need to consider?
BAR 1 (memory BAR) maps the same control functions that BAR 0 (io BAR) maps, so it
made sense to me for them to have the same size. I think QEMU reports the BAR size
as 4K because of this recommendation p[1]:
" Devices are free to consume more address space than required, but decoding down
to a 4 KB space for memory is suggested for devices that need less than that amount"
We also don't follow this recommendation for BAR 2 which maps the MSIX capability
table and PBA.
[1] PCI Local Bus Specification, Revision 3.0, page 226.
>
>> [1] VIRTIO Version 1.0 Committee Specification 04, section 4.4.8.
> I think that should read section 4.1.4.8.
Oops, that's true, I'll fix it in the next iteration of the patch.
Thanks,
Alex
>
> The rest looks OK.
>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
>
> Cheers,
> Andre
>
>> Cc: julien.thierry.kdev@xxxxxxxxx
>> Signed-off-by: Julien Thierry <julien.thierry@xxxxxxx>
>> [Added rationale for changing BAR1 size to PCI_IO_SIZE]
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>> ---
>> hw/vesa.c | 4 ++--
>> include/kvm/ioport.h | 1 -
>> pci.c | 2 +-
>> virtio/pci.c | 15 +++++++--------
>> 4 files changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/vesa.c b/hw/vesa.c
>> index 70ab59974f76..0191e9264666 100644
>> --- a/hw/vesa.c
>> +++ b/hw/vesa.c
>> @@ -62,8 +62,8 @@ struct framebuffer *vesa__init(struct kvm *kvm)
>>
>> if (!kvm->cfg.vnc && !kvm->cfg.sdl && !kvm->cfg.gtk)
>> return NULL;
>> - r = pci_get_io_port_block(IOPORT_SIZE);
>> - r = ioport__register(kvm, r, &vesa_io_ops, IOPORT_SIZE, NULL);
>> + r = pci_get_io_port_block(PCI_IO_SIZE);
>> + r = ioport__register(kvm, r, &vesa_io_ops, PCI_IO_SIZE, NULL);
>> if (r < 0)
>> return ERR_PTR(r);
>>
>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h
>> index b10fcd5b4412..8c86b7151f25 100644
>> --- a/include/kvm/ioport.h
>> +++ b/include/kvm/ioport.h
>> @@ -14,7 +14,6 @@
>>
>> /* some ports we reserve for own use */
>> #define IOPORT_DBG 0xe0
>> -#define IOPORT_SIZE 0x400
>>
>> struct kvm;
>>
>> diff --git a/pci.c b/pci.c
>> index 32a07335a765..b4677434c50c 100644
>> --- a/pci.c
>> +++ b/pci.c
>> @@ -20,7 +20,7 @@ static u16 io_port_blocks = PCI_IOPORT_START;
>>
>> u16 pci_get_io_port_block(u32 size)
>> {
>> - u16 port = ALIGN(io_port_blocks, IOPORT_SIZE);
>> + u16 port = ALIGN(io_port_blocks, PCI_IO_SIZE);
>>
>> io_port_blocks = port + size;
>> return port;
>> diff --git a/virtio/pci.c b/virtio/pci.c
>> index d73414abde05..eeb5b5efa6e1 100644
>> --- a/virtio/pci.c
>> +++ b/virtio/pci.c
>> @@ -421,7 +421,7 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
>> {
>> struct virtio_pci *vpci = ptr;
>> int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> - u16 port = vpci->port_addr + (addr & (IOPORT_SIZE - 1));
>> + u16 port = vpci->port_addr + (addr & (PCI_IO_SIZE - 1));
>>
>> kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> }
>> @@ -435,17 +435,16 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>> vpci->kvm = kvm;
>> vpci->dev = dev;
>>
>> - BUILD_BUG_ON(!is_power_of_two(IOPORT_SIZE));
>> BUILD_BUG_ON(!is_power_of_two(PCI_IO_SIZE));
>>
>> - r = pci_get_io_port_block(IOPORT_SIZE);
>> - r = ioport__register(kvm, r, &virtio_pci__io_ops, IOPORT_SIZE, vdev);
>> + r = pci_get_io_port_block(PCI_IO_SIZE);
>> + r = ioport__register(kvm, r, &virtio_pci__io_ops, PCI_IO_SIZE, vdev);
>> if (r < 0)
>> return r;
>> vpci->port_addr = (u16)r;
>>
>> - vpci->mmio_addr = pci_get_mmio_block(IOPORT_SIZE);
>> - r = kvm__register_mmio(kvm, vpci->mmio_addr, IOPORT_SIZE, false,
>> + vpci->mmio_addr = pci_get_mmio_block(PCI_IO_SIZE);
>> + r = kvm__register_mmio(kvm, vpci->mmio_addr, PCI_IO_SIZE, false,
>> virtio_pci__io_mmio_callback, vpci);
>> if (r < 0)
>> goto free_ioport;
>> @@ -475,8 +474,8 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>> | PCI_BASE_ADDRESS_SPACE_MEMORY),
>> .status = cpu_to_le16(PCI_STATUS_CAP_LIST),
>> .capabilities = (void *)&vpci->pci_hdr.msix - (void *)&vpci->pci_hdr,
>> - .bar_size[0] = cpu_to_le32(IOPORT_SIZE),
>> - .bar_size[1] = cpu_to_le32(IOPORT_SIZE),
>> + .bar_size[0] = cpu_to_le32(PCI_IO_SIZE),
>> + .bar_size[1] = cpu_to_le32(PCI_IO_SIZE),
>> .bar_size[2] = cpu_to_le32(PCI_IO_SIZE*2),
>> };
>>
