[kvmtool,07/16] pci: Fix ioport allocation size

Message ID	20191125103033.22694-8-alexandru.elisei@arm.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=umKK=ZR=vger.kernel.org=kvm-owner@kernel.org> From: Alexandru Elisei <alexandru.elisei@arm.com> To: kvm@vger.kernel.org Cc: will@kernel.org, julien.thierry.kdev@gmail.com, andre.przywara@arm.com, sami.mujawar@arm.com, lorenzo.pieralisi@arm.com Subject: [PATCH kvmtool 07/16] pci: Fix ioport allocation size Date: Mon, 25 Nov 2019 10:30:24 +0000 Message-Id: <20191125103033.22694-8-alexandru.elisei@arm.com> In-Reply-To: <20191125103033.22694-1-alexandru.elisei@arm.com> References: <20191125103033.22694-1-alexandru.elisei@arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: kvm-owner@vger.kernel.org Precedence: bulk
Series	Add writable BARs and PCIE 1.1 support \| expand [kvmtool,00/16] Add writable BARs and PCIE 1.1 support [kvmtool,01/16] Makefile: Use correct objcopy binary when cross-compiling for x86_64 [kvmtool,02/16] pci: Fix BAR resource sizing arbitration [kvmtool,03/16] Remove pci-shmem device [kvmtool,04/16] Check that a PCI device's memory size is power of two [kvmtool,05/16] arm: pci.c: Advertise only PCI bus 0 in the DT [kvmtool,06/16] ioport: pci: Move port allocations to PCI devices [kvmtool,07/16] pci: Fix ioport allocation size [kvmtool,08/16] arm/pci: Fix PCI IO region [kvmtool,09/16] arm/pci: Do not use first PCI IO space bytes for devices [kvmtool,10/16] virtio/pci: Make memory and IO BARs independent [kvmtool,11/16] virtio/pci: Ignore MMIO and I/O accesses when they are disabled [kvmtool,12/16] Use independent read/write locks for ioport and mmio [kvmtool,13/16] vfio: Add support for BAR configuration [kvmtool,14/16] virtio/pci: Add support for BAR configuration [kvmtool,15/16] arm/fdt: Remove 'linux,pci-probe-only' property [kvmtool,16/16] Add PCI Express 1.1 support

Message ID

20191125103033.22694-8-alexandru.elisei@arm.com (mailing list archive)

State

New, archived

Headers

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: kvm@vger.kernel.org
Cc: will@kernel.org, julien.thierry.kdev@gmail.com,
        andre.przywara@arm.com, sami.mujawar@arm.com,
        lorenzo.pieralisi@arm.com
Subject: [PATCH kvmtool 07/16] pci: Fix ioport allocation size
Date: Mon, 25 Nov 2019 10:30:24 +0000
Message-Id: <20191125103033.22694-8-alexandru.elisei@arm.com>
In-Reply-To: <20191125103033.22694-1-alexandru.elisei@arm.com>
References: <20191125103033.22694-1-alexandru.elisei@arm.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

Series

Add writable BARs and PCIE 1.1 support | expand

Commit Message

Alexandru Elisei Nov. 25, 2019, 10:30 a.m. UTC

From: Julien Thierry <julien.thierry@arm.com>

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.

[1] VIRTIO Version 1.0 Committee Specification 04, section 4.4.8.

Cc: julien.thierry.kdev@gmail.com
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
[Added rationale for changing BAR1 size to PCI_IO_SIZE]
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 hw/vesa.c            |  4 ++--
 include/kvm/ioport.h |  1 -
 pci.c                |  2 +-
 virtio/pci.c         | 15 +++++++--------
 4 files changed, 10 insertions(+), 12 deletions(-)

Comments

Andre Przywara Jan. 28, 2020, 6:26 p.m. UTC | #1

On Mon, 25 Nov 2019 10:30:24 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi,

> From: Julien Thierry <julien.thierry@arm.com>
> 
> 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?
 
> [1] VIRTIO Version 1.0 Committee Specification 04, section 4.4.8.

I think that should read section 4.1.4.8.

The rest looks OK.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> Cc: julien.thierry.kdev@gmail.com
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> [Added rationale for changing BAR1 size to PCI_IO_SIZE]
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  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),
>  	};
>

Alexandru Elisei Jan. 29, 2020, 11:11 a.m. UTC | #2

Hi,

On 1/28/20 6:26 PM, Andre Przywara wrote:
> On Mon, 25 Nov 2019 10:30:24 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> From: Julien Thierry <julien.thierry@arm.com>
>>
>> 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@arm.com>
>
> Cheers,
> Andre
>
>> Cc: julien.thierry.kdev@gmail.com
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> [Added rationale for changing BAR1 size to PCI_IO_SIZE]
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  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),
>>  	};
>>

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),
 	};

[kvmtool,07/16] pci: Fix ioport allocation size

Commit Message

Comments

Patch