[PATCH kvmtool 00/16] Support PCI BAR configuration

[PATCH kvmtool 00/16] Support PCI BAR configuration

[Julien Thierry]

Hi,

This series add support for guests writting to PCI BARs. Edk2 does this
and is not aware of the "linux,pci-probe-only" property in the chosen node.

- Patches 1 to 3 do miscelaneous fixes
- Patch 4 fixes the way we deal with BAR sizing
- Patches 5 to 8 fixes the allocation/assignment of PCI IO BARs
- Patches 9 to 12 make it possible to remap ioport and mmio regions
  from vcpu threads, without pausing the entire VM
- Patches 13 to 16 adds the support for writting to BARs

Cheers,

Julien

-->

Julien Thierry (15):
  Makefile: Only compile vesa for archs that need it
  brlock: Always pass argument to br_read_lock/unlock
  brlock: fix build with KVM_BRLOCK_DEBUG
  ioport: pci: Move port allocations to PCI devices
  pci: Fix ioport allocation size
  arm/pci: Fix PCI IO region
  arm/pci: Do not use first PCI IO space bytes for devices
  brlock: Use rwlock instead of pause
  ref_cnt: Add simple ref counting API
  mmio: Allow mmio callbacks to be called without locking
  ioport: Allow ioport callbacks to be called without locking
  vfio: Add support for BAR configuration
  virtio/pci: Make memory and IO BARs independent
  virtio/pci: update virtio mapping when PCI BARs are reconfigured
  arm/fdt: Remove PCI probe only property

Sami Mujawar (1):
  pci: Fix BAR resource sizing arbitration

 Makefile                     |   3 +-
 arm/fdt.c                    |   1 -
 arm/include/arm-common/pci.h |   1 +
 arm/kvm.c                    |   3 +
 arm/pci.c                    |  24 ++++-
 hw/pci-shmem.c               |   3 +-
 hw/vesa.c                    |   4 +-
 include/kvm/brlock.h         |  21 +---
 include/kvm/ioport.h         |   5 -
 include/kvm/kvm.h            |   2 +
 include/kvm/pci.h            |   2 +
 include/kvm/ref_cnt.h        |  53 ++++++++++
 include/kvm/virtio-pci.h     |   1 +
 ioport.c                     |  80 ++++++++--------
 kvm.c                        |   4 +
 mmio.c                       |  30 ++++--
 pci.c                        |  59 +++++++++++-
 vfio/core.c                  |   6 +-
 vfio/pci.c                   |  82 +++++++++++++++-
 virtio/pci.c                 | 223 ++++++++++++++++++++++++++++++++++++-------
 20 files changed, 491 insertions(+), 116 deletions(-)
 create mode 100644 include/kvm/ref_cnt.h

--
1.9.1

[PATCH kvmtool 01/16] Makefile: Only compile vesa for archs that need it

[Julien Thierry]

The vesa framebuffer is only used by architectures that explicitly
require it (i.e. x86). Compile it out for architectures not using it, as
its current implementation might not work for them.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c4faff6..288e467 100644
--- a/Makefile
+++ b/Makefile
@@ -94,7 +94,6 @@ OBJS	+= util/read-write.o
 OBJS	+= util/util.o
 OBJS	+= virtio/9p.o
 OBJS	+= virtio/9p-pdu.o
-OBJS	+= hw/vesa.o
 OBJS	+= hw/pci-shmem.o
 OBJS	+= kvm-ipc.o
 OBJS	+= builtin-sandbox.o
@@ -219,6 +218,8 @@ else
 endif
 
 ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
+	OBJS	+= hw/vesa.o
+
 	CFLAGS_GTK3 := $(shell pkg-config --cflags gtk+-3.0 2>/dev/null)
 	LDFLAGS_GTK3 := $(shell pkg-config --libs gtk+-3.0 2>/dev/null)
 	ifeq ($(call try-build,$(SOURCE_GTK3),$(CFLAGS) $(CFLAGS_GTK3),$(LDFLAGS) $(LDFLAGS_GTK3)),y)
-- 
1.9.1

Re: [PATCH kvmtool 01/16] Makefile: Only compile vesa for archs that need it

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:02 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

> The vesa framebuffer is only used by architectures that explicitly
> require it (i.e. x86). Compile it out for architectures not using it, as
> its current implementation might not work for them.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

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

Cheers,
Andre.

> ---
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index c4faff6..288e467 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -94,7 +94,6 @@ OBJS	+= util/read-write.o
>  OBJS	+= util/util.o
>  OBJS	+= virtio/9p.o
>  OBJS	+= virtio/9p-pdu.o
> -OBJS	+= hw/vesa.o
>  OBJS	+= hw/pci-shmem.o
>  OBJS	+= kvm-ipc.o
>  OBJS	+= builtin-sandbox.o
> @@ -219,6 +218,8 @@ else
>  endif
>  
>  ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
> +	OBJS	+= hw/vesa.o
> +
>  	CFLAGS_GTK3 := $(shell pkg-config --cflags gtk+-3.0 2>/dev/null)
>  	LDFLAGS_GTK3 := $(shell pkg-config --libs gtk+-3.0 2>/dev/null)
>  	ifeq ($(call try-build,$(SOURCE_GTK3),$(CFLAGS) $(CFLAGS_GTK3),$(LDFLAGS) $(LDFLAGS_GTK3)),y)

[PATCH kvmtool 02/16] brlock: Always pass argument to br_read_lock/unlock

[Julien Thierry]

The kvm argument is not passed to br_read_lock/unlock, this works for
the barrier implementation because the argument is not used. This ever
breaks if another lock implementation is used.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 ioport.c | 4 ++--
 mmio.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ioport.c b/ioport.c
index 505e822..a6dc65e 100644
--- a/ioport.c
+++ b/ioport.c
@@ -184,7 +184,7 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
 	void *ptr = data;
 	struct kvm *kvm = vcpu->kvm;
 
-	br_read_lock();
+	br_read_lock(kvm);
 	entry = ioport_search(&ioport_tree, port);
 	if (!entry)
 		goto out;
@@ -201,7 +201,7 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
 	}
 
 out:
-	br_read_unlock();
+	br_read_unlock(kvm);
 
 	if (ret)
 		return true;
diff --git a/mmio.c b/mmio.c
index c648bec..61e1d47 100644
--- a/mmio.c
+++ b/mmio.c
@@ -124,7 +124,7 @@ bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u
 {
 	struct mmio_mapping *mmio;
 
-	br_read_lock();
+	br_read_lock(vcpu->kvm);
 	mmio = mmio_search(&mmio_tree, phys_addr, len);
 
 	if (mmio)
@@ -135,7 +135,7 @@ bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u
 				to_direction(is_write),
 				(unsigned long long)phys_addr, len);
 	}
-	br_read_unlock();
+	br_read_unlock(vcpu->kvm);
 
 	return true;
 }
-- 
1.9.1

Re: [PATCH kvmtool 02/16] brlock: Always pass argument to br_read_lock/unlock

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:03 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

> The kvm argument is not passed to br_read_lock/unlock, this works for
> the barrier implementation because the argument is not used. This ever
> breaks if another lock implementation is used.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

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

Cheers,
Andre.

> ---
>  ioport.c | 4 ++--
>  mmio.c   | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/ioport.c b/ioport.c
> index 505e822..a6dc65e 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -184,7 +184,7 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
>  	void *ptr = data;
>  	struct kvm *kvm = vcpu->kvm;
>  
> -	br_read_lock();
> +	br_read_lock(kvm);
>  	entry = ioport_search(&ioport_tree, port);
>  	if (!entry)
>  		goto out;
> @@ -201,7 +201,7 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
>  	}
>  
>  out:
> -	br_read_unlock();
> +	br_read_unlock(kvm);
>  
>  	if (ret)
>  		return true;
> diff --git a/mmio.c b/mmio.c
> index c648bec..61e1d47 100644
> --- a/mmio.c
> +++ b/mmio.c
> @@ -124,7 +124,7 @@ bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u
>  {
>  	struct mmio_mapping *mmio;
>  
> -	br_read_lock();
> +	br_read_lock(vcpu->kvm);
>  	mmio = mmio_search(&mmio_tree, phys_addr, len);
>  
>  	if (mmio)
> @@ -135,7 +135,7 @@ bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u
>  				to_direction(is_write),
>  				(unsigned long long)phys_addr, len);
>  	}
> -	br_read_unlock();
> +	br_read_unlock(vcpu->kvm);
>  
>  	return true;
>  }

[PATCH kvmtool 03/16] brlock: fix build with KVM_BRLOCK_DEBUG

[Julien Thierry]

Build breaks when using KVM_BRLOCK_DEBUG because the header was seamingly
conceived to be included in a single .c file...

Fix this by moving the definition of the read/write lock into the kvm
struct.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 include/kvm/brlock.h | 10 ++++------
 include/kvm/kvm.h    |  4 ++++
 kvm.c                |  4 ++++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/kvm/brlock.h b/include/kvm/brlock.h
index 29f72e0..1862210 100644
--- a/include/kvm/brlock.h
+++ b/include/kvm/brlock.h
@@ -21,13 +21,11 @@
 
 #include "kvm/rwsem.h"
 
-DECLARE_RWSEM(brlock_sem);
+#define br_read_lock(kvm)	down_read(&(kvm)->brlock_sem);
+#define br_read_unlock(kvm)	up_read(&(kvm)->brlock_sem);
 
-#define br_read_lock(kvm)	down_read(&brlock_sem);
-#define br_read_unlock(kvm)	up_read(&brlock_sem);
-
-#define br_write_lock(kvm)	down_write(&brlock_sem);
-#define br_write_unlock(kvm)	up_write(&brlock_sem);
+#define br_write_lock(kvm)	down_write(&(kvm)->brlock_sem);
+#define br_write_unlock(kvm)	up_write(&(kvm)->brlock_sem);
 
 #else
 
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 1edacfd..7a73818 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -81,6 +81,10 @@ struct kvm {
 	int                     nr_disks;
 
 	int			vm_state;
+
+#ifdef KVM_BRLOCK_DEBUG
+	pthread_rwlock_t	brlock_sem;
+#endif
 };
 
 void kvm__set_dir(const char *fmt, ...);
diff --git a/kvm.c b/kvm.c
index d5249a0..57c4ff9 100644
--- a/kvm.c
+++ b/kvm.c
@@ -160,6 +160,10 @@ struct kvm *kvm__new(void)
 	kvm->sys_fd = -1;
 	kvm->vm_fd = -1;
 
+#ifdef KVM_BRLOCK_DEBUG
+	kvm->brlock_sem = (pthread_rwlock_t) PTHREAD_RWLOCK_INITIALIZER;
+#endif
+
 	return kvm;
 }
 
-- 
1.9.1

Re: [PATCH kvmtool 03/16] brlock: fix build with KVM_BRLOCK_DEBUG

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:04 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

> Build breaks when using KVM_BRLOCK_DEBUG because the header was seamingly
> conceived to be included in a single .c file...
> 
> Fix this by moving the definition of the read/write lock into the kvm
> struct.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

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

Cheers,
Andre.

> ---
>  include/kvm/brlock.h | 10 ++++------
>  include/kvm/kvm.h    |  4 ++++
>  kvm.c                |  4 ++++
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/include/kvm/brlock.h b/include/kvm/brlock.h
> index 29f72e0..1862210 100644
> --- a/include/kvm/brlock.h
> +++ b/include/kvm/brlock.h
> @@ -21,13 +21,11 @@
>  
>  #include "kvm/rwsem.h"
>  
> -DECLARE_RWSEM(brlock_sem);
> +#define br_read_lock(kvm)	down_read(&(kvm)->brlock_sem);
> +#define br_read_unlock(kvm)	up_read(&(kvm)->brlock_sem);
>  
> -#define br_read_lock(kvm)	down_read(&brlock_sem);
> -#define br_read_unlock(kvm)	up_read(&brlock_sem);
> -
> -#define br_write_lock(kvm)	down_write(&brlock_sem);
> -#define br_write_unlock(kvm)	up_write(&brlock_sem);
> +#define br_write_lock(kvm)	down_write(&(kvm)->brlock_sem);
> +#define br_write_unlock(kvm)	up_write(&(kvm)->brlock_sem);
>  
>  #else
>  
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 1edacfd..7a73818 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -81,6 +81,10 @@ struct kvm {
>  	int                     nr_disks;
>  
>  	int			vm_state;
> +
> +#ifdef KVM_BRLOCK_DEBUG
> +	pthread_rwlock_t	brlock_sem;
> +#endif
>  };
>  
>  void kvm__set_dir(const char *fmt, ...);
> diff --git a/kvm.c b/kvm.c
> index d5249a0..57c4ff9 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -160,6 +160,10 @@ struct kvm *kvm__new(void)
>  	kvm->sys_fd = -1;
>  	kvm->vm_fd = -1;
>  
> +#ifdef KVM_BRLOCK_DEBUG
> +	kvm->brlock_sem = (pthread_rwlock_t) PTHREAD_RWLOCK_INITIALIZER;
> +#endif
> +
>  	return kvm;
>  }
>  

[PATCH kvmtool 04/16] pci: Fix BAR resource sizing arbitration

[Julien Thierry]

From: Sami Mujawar <sami.mujawar@arm.com>

According to the 'PCI Local Bus Specification, Revision 3.0,
February 3, 2004, Section 6.2.5.1, Implementation Notes, page 227'

    "Software saves the original value of the Base Address register,
    writes 0 FFFF FFFFh to the register, then reads it back. Size
    calculation can be done from the 32-bit value read by first
    clearing encoding information bits (bit 0 for I/O, bits 0-3 for
    memory), inverting all 32 bits (logical NOT), then incrementing
    by 1. The resultant 32-bit value is the memory/I/O range size
    decoded by the register. Note that the upper 16 bits of the result
    is ignored if the Base Address register is for I/O and bits 16-31
    returned zero upon read."

kvmtool was returning the actual BAR resource size which would be
incorrect as the software software drivers would invert all 32 bits
(logical NOT), then incrementing by 1. This ends up with a very large
resource size (in some cases more than 4GB) due to which drivers
assert/fail to work.

e.g if the BAR resource size was 0x1000, kvmtool would return 0x1000
instead of 0xFFFFF00x.

Fixed pci__config_wr() to return the size of the BAR in accordance with
the PCI Local Bus specification, Implementation Notes.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 pci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/pci.c b/pci.c
index 689869c..9edefa5 100644
--- a/pci.c
+++ b/pci.c
@@ -8,6 +8,9 @@
 #include <linux/err.h>
 #include <assert.h>
 
+/* Macro to check that a value is a power of 2 */
+#define power_of_2(pow) (((pow) != 0) && (((pow) & ((pow) - 1)) == 0))
+
 static u32 pci_config_address_bits;
 
 /* This is within our PCI gap - in an unused area.
@@ -173,9 +176,51 @@ void pci__config_wr(struct kvm *kvm, union pci_config_address addr, void *data,
 	 * BAR there next time it reads from it. When the kernel got the size it
 	 * would write the address back.
 	 */
-	if (bar < 6 && ioport__read32(data) == 0xFFFFFFFF) {
-		u32 sz = pci_hdr->bar_size[bar];
-		memcpy(base + offset, &sz, sizeof(sz));
+	if (bar < 6) {
+		/*
+		 * According to the PCI local bus specification REV 3.0:
+		 * The number of upper bits that a device actually implements
+		 * depends on how much of the address space the device will
+		 * respond to. A device that wants a 1 MB memory address space
+		 * (using a 32-bit base address register) would build the top
+		 * 12 bits of the address register, hardwiring the other bits
+		 * to 0.
+		 * Furthermore software can determine how much address space the
+		 * device requires by writing a value of all 1's to the register
+		 * and then reading the value back. The device will return 0's in
+		 * all don't-care address bits, effectively specifying the address
+		 * space required.
+		 *
+		 * The following code emulates this by storing the value written
+		 * to the BAR, applying the size mask to clear the lower bits,
+		 * restoring the information bits and then updating the BAR value.
+		 */
+		u32 bar_value;
+		/* Get the size mask */
+		u32 sz = ~(pci_hdr->bar_size[bar] - 1);
+		/* Extract the info bits */
+		u32 info = pci_hdr->bar[bar] & 0xF;
+
+		/* Store the value written by software */
+		memcpy(base + offset, data, size);
+
+		/* Apply the size mask to the bar value to clear the lower bits */
+		bar_value = pci_hdr->bar[bar] & sz;
+
+		/* Warn if the bar size is not a power of 2 */
+		WARN_ON(!power_of_2(pci_hdr->bar_size[bar]));
+
+		/* Restore the info bits */
+		if ((info & 0x1) == 0x1) {
+			/* BAR for I/O */
+			bar_value = ((bar_value & ~0x3) | 0x1);
+		} else {
+			/* BAR for Memory */
+			bar_value = ((bar_value & ~0xF) | info);
+		}
+
+		/* Store the final BAR value */
+		pci_hdr->bar[bar] = bar_value;
 	} else {
 		memcpy(base + offset, data, size);
 	}
-- 
1.9.1

Re: [PATCH kvmtool 04/16] pci: Fix BAR resource sizing arbitration

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:05 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> According to the 'PCI Local Bus Specification, Revision 3.0,
> February 3, 2004, Section 6.2.5.1, Implementation Notes, page 227'
> 
>     "Software saves the original value of the Base Address register,
>     writes 0 FFFF FFFFh to the register, then reads it back. Size
>     calculation can be done from the 32-bit value read by first
>     clearing encoding information bits (bit 0 for I/O, bits 0-3 for
>     memory), inverting all 32 bits (logical NOT), then incrementing
>     by 1. The resultant 32-bit value is the memory/I/O range size
>     decoded by the register. Note that the upper 16 bits of the result
>     is ignored if the Base Address register is for I/O and bits 16-31
>     returned zero upon read."
> 
> kvmtool was returning the actual BAR resource size which would be
> incorrect as the software software drivers would invert all 32 bits
> (logical NOT), then incrementing by 1. This ends up with a very large
> resource size (in some cases more than 4GB) due to which drivers
> assert/fail to work.
> 
> e.g if the BAR resource size was 0x1000, kvmtool would return 0x1000
> instead of 0xFFFFF00x.

Ouch!

> 
> Fixed pci__config_wr() to return the size of the BAR in accordance with
> the PCI Local Bus specification, Implementation Notes.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> ---
>  pci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/pci.c b/pci.c
> index 689869c..9edefa5 100644
> --- a/pci.c
> +++ b/pci.c
> @@ -8,6 +8,9 @@
>  #include <linux/err.h>
>  #include <assert.h>
>  
> +/* Macro to check that a value is a power of 2 */
> +#define power_of_2(pow) (((pow) != 0) && (((pow) & ((pow) - 1)) == 0))

That is probably correct, but a bit hard to read. What about defining a
static function instead? That would allow this to be written in multiple
lines. And to improve readability, the prototype should probably be:
static bool is_power_of_two(u32 value)

> +
>  static u32 pci_config_address_bits;
>  
>  /* This is within our PCI gap - in an unused area.
> @@ -173,9 +176,51 @@ void pci__config_wr(struct kvm *kvm, union pci_config_address addr, void *data,
>  	 * BAR there next time it reads from it. When the kernel got the size it
>  	 * would write the address back.
>  	 */
> -	if (bar < 6 && ioport__read32(data) == 0xFFFFFFFF) {
> -		u32 sz = pci_hdr->bar_size[bar];
> -		memcpy(base + offset, &sz, sizeof(sz));
> +	if (bar < 6) {
> +		/*
> +		 * According to the PCI local bus specification REV 3.0:
> +		 * The number of upper bits that a device actually implements
> +		 * depends on how much of the address space the device will
> +		 * respond to. A device that wants a 1 MB memory address space
> +		 * (using a 32-bit base address register) would build the top
> +		 * 12 bits of the address register, hardwiring the other bits
> +		 * to 0.
> +		 * Furthermore software can determine how much address space the
> +		 * device requires by writing a value of all 1's to the register
> +		 * and then reading the value back. The device will return 0's in
> +		 * all don't-care address bits, effectively specifying the address
> +		 * space required.
> +		 *
> +		 * The following code emulates this by storing the value written
> +		 * to the BAR, applying the size mask to clear the lower bits,
> +		 * restoring the information bits and then updating the BAR value.
> +		 */
> +		u32 bar_value;
> +		/* Get the size mask */
> +		u32 sz = ~(pci_hdr->bar_size[bar] - 1);

It's a bit confusing to read just "sz", when it's actually a mask.
Actually there is only one user, so to make the comment there more
meaningful, you can just use the mask expression there, and get rid of
"sz" altogether.

> +		/* Extract the info bits */
> +		u32 info = pci_hdr->bar[bar] & 0xF;

Just a nit, but I find it a bit confusing to see those variable
definitions mixed with comments, especially with those short type names
and after the big introductory comment. Can we just put the comment on the
same line as the declaration, at least?

> +
> +		/* Store the value written by software */
> +		memcpy(base + offset, data, size);
> +
> +		/* Apply the size mask to the bar value to clear the lower bits */
> +		bar_value = pci_hdr->bar[bar] & sz;
> +
> +		/* Warn if the bar size is not a power of 2 */
> +		WARN_ON(!power_of_2(pci_hdr->bar_size[bar]));

Is this actually useful? This would put out a warning to the console, but
is there something an admin could do about it? Looks more like a kvmtool
issue (for instance the VESA and pci-shmem devices don't use power-of-2
sizes at the moment)?

> +
> +		/* Restore the info bits */
> +		if ((info & 0x1) == 0x1) {
> +			/* BAR for I/O */
> +			bar_value = ((bar_value & ~0x3) | 0x1);
> +		} else {
> +			/* BAR for Memory */
> +			bar_value = ((bar_value & ~0xF) | info);
> +		}
> +
> +		/* Store the final BAR value */
> +		pci_hdr->bar[bar] = bar_value;
>  	} else {
>  		memcpy(base + offset, data, size);
>  	}

[PATCH kvmtool 05/16] ioport: pci: Move port allocations to PCI devices

[Julien Thierry]

The dynamic ioport allocation with IOPORT_EMPTY is currently only used
by PCI devices. Other devices use fixed ports for which they request
registration to the ioport API.

PCI ports need to be in the PCI IO space and there is no reason ioport
API should know a PCI port is being allocated and needs to be placed in
PCI IO space. This currently just happens to be the case.

Move the responsability of dynamic allocation of ioports from the ioport
API to PCI.

In the future, if other types of devices also need dynamic ioport
allocation, they'll have to figure out the range of ports they are
allowed to use.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 hw/pci-shmem.c       |  3 ++-
 hw/vesa.c            |  4 ++--
 include/kvm/ioport.h |  3 ---
 include/kvm/pci.h    |  2 ++
 ioport.c             | 18 ------------------
 pci.c                |  8 ++++++++
 vfio/core.c          |  6 ++++--
 virtio/pci.c         |  3 ++-
 8 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/hw/pci-shmem.c b/hw/pci-shmem.c
index f92bc75..a0c5ba8 100644
--- a/hw/pci-shmem.c
+++ b/hw/pci-shmem.c
@@ -357,7 +357,8 @@ int pci_shmem__init(struct kvm *kvm)
 		return 0;
 
 	/* Register MMIO space for MSI-X */
-	r = ioport__register(kvm, IOPORT_EMPTY, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
+	r = pci_get_io_port_block(IOPORT_SIZE);
+	r = ioport__register(kvm, r, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
 	if (r < 0)
 		return r;
 	ivshmem_registers = (u16)r;
diff --git a/hw/vesa.c b/hw/vesa.c
index f3c5114..404a8a3 100644
--- a/hw/vesa.c
+++ b/hw/vesa.c
@@ -60,8 +60,8 @@ struct framebuffer *vesa__init(struct kvm *kvm)
 
 	if (!kvm->cfg.vnc && !kvm->cfg.sdl && !kvm->cfg.gtk)
 		return NULL;
-
-	r = ioport__register(kvm, IOPORT_EMPTY, &vesa_io_ops, IOPORT_SIZE, NULL);
+	r = pci_get_io_space_block(IOPORT_SIZE);
+	r = ioport__register(kvm, r, &vesa_io_ops, IOPORT_SIZE, NULL);
 	if (r < 0)
 		return ERR_PTR(r);
 
diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h
index db52a47..b10fcd5 100644
--- a/include/kvm/ioport.h
+++ b/include/kvm/ioport.h
@@ -14,11 +14,8 @@
 
 /* some ports we reserve for own use */
 #define IOPORT_DBG			0xe0
-#define IOPORT_START			0x6200
 #define IOPORT_SIZE			0x400
 
-#define IOPORT_EMPTY			USHRT_MAX
-
 struct kvm;
 
 struct ioport {
diff --git a/include/kvm/pci.h b/include/kvm/pci.h
index a86c15a..bdbd183 100644
--- a/include/kvm/pci.h
+++ b/include/kvm/pci.h
@@ -19,6 +19,7 @@
 #define PCI_CONFIG_DATA		0xcfc
 #define PCI_CONFIG_BUS_FORWARD	0xcfa
 #define PCI_IO_SIZE		0x100
+#define PCI_IOPORT_START	0x6200
 #define PCI_CFG_SIZE		(1ULL << 24)
 
 struct kvm;
@@ -153,6 +154,7 @@ int pci__init(struct kvm *kvm);
 int pci__exit(struct kvm *kvm);
 struct pci_device_header *pci__find_dev(u8 dev_num);
 u32 pci_get_io_space_block(u32 size);
+u16 pci_get_io_port_block(u32 size);
 void pci__assign_irq(struct device_header *dev_hdr);
 void pci__config_wr(struct kvm *kvm, union pci_config_address addr, void *data, int size);
 void pci__config_rd(struct kvm *kvm, union pci_config_address addr, void *data, int size);
diff --git a/ioport.c b/ioport.c
index a6dc65e..a72e403 100644
--- a/ioport.c
+++ b/ioport.c
@@ -16,24 +16,8 @@
 
 #define ioport_node(n) rb_entry(n, struct ioport, node)
 
-DEFINE_MUTEX(ioport_mutex);
-
-static u16			free_io_port_idx; /* protected by ioport_mutex */
-
 static struct rb_root		ioport_tree = RB_ROOT;
 
-static u16 ioport__find_free_port(void)
-{
-	u16 free_port;
-
-	mutex_lock(&ioport_mutex);
-	free_port = IOPORT_START + free_io_port_idx * IOPORT_SIZE;
-	free_io_port_idx++;
-	mutex_unlock(&ioport_mutex);
-
-	return free_port;
-}
-
 static struct ioport *ioport_search(struct rb_root *root, u64 addr)
 {
 	struct rb_int_node *node;
@@ -85,8 +69,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
 	int r;
 
 	br_write_lock(kvm);
-	if (port == IOPORT_EMPTY)
-		port = ioport__find_free_port();
 
 	entry = ioport_search(&ioport_tree, port);
 	if (entry) {
diff --git a/pci.c b/pci.c
index 9edefa5..cd749db 100644
--- a/pci.c
+++ b/pci.c
@@ -19,6 +19,14 @@ static u32 pci_config_address_bits;
  * PCI isn't currently supported.)
  */
 static u32 io_space_blocks		= KVM_PCI_MMIO_AREA;
+static u16 io_port_blocks		= PCI_IOPORT_START;
+
+u16 pci_get_io_port_block(u32 size)
+{
+	u16 port = ALIGN(io_port_blocks, IOPORT_SIZE);
+	io_port_blocks = port + size;
+	return port;
+}
 
 /*
  * BARs must be naturally aligned, so enforce this in the allocator.
diff --git a/vfio/core.c b/vfio/core.c
index 17b5b0c..0ed1e6f 100644
--- a/vfio/core.c
+++ b/vfio/core.c
@@ -202,8 +202,10 @@ static int vfio_setup_trap_region(struct kvm *kvm, struct vfio_device *vdev,
 				  struct vfio_region *region)
 {
 	if (region->is_ioport) {
-		int port = ioport__register(kvm, IOPORT_EMPTY, &vfio_ioport_ops,
-					    region->info.size, region);
+		int port = pci_get_io_port_block(region->info.size);
+
+		port = ioport__register(kvm, port, &vfio_ioport_ops,
+					region->info.size, region);
 		if (port < 0)
 			return port;
 
diff --git a/virtio/pci.c b/virtio/pci.c
index 5a5fc6e..c8e16dd 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -420,7 +420,8 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	vpci->kvm = kvm;
 	vpci->dev = dev;
 
-	r = ioport__register(kvm, IOPORT_EMPTY, &virtio_pci__io_ops, IOPORT_SIZE, vdev);
+	r = pci_get_io_port_block(IOPORT_SIZE);
+	r = ioport__register(kvm, r, &virtio_pci__io_ops, IOPORT_SIZE, vdev);
 	if (r < 0)
 		return r;
 	vpci->port_addr = (u16)r;
-- 
1.9.1

Re: [PATCH kvmtool 05/16] ioport: pci: Move port allocations to PCI devices

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:06 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

> The dynamic ioport allocation with IOPORT_EMPTY is currently only used
> by PCI devices. Other devices use fixed ports for which they request
> registration to the ioport API.
> 
> PCI ports need to be in the PCI IO space and there is no reason ioport
> API should know a PCI port is being allocated and needs to be placed in
> PCI IO space. This currently just happens to be the case.
> 
> Move the responsability of dynamic allocation of ioports from the ioport
> API to PCI.
> 
> In the future, if other types of devices also need dynamic ioport
> allocation, they'll have to figure out the range of ports they are
> allowed to use.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> ---
>  hw/pci-shmem.c       |  3 ++-
>  hw/vesa.c            |  4 ++--
>  include/kvm/ioport.h |  3 ---
>  include/kvm/pci.h    |  2 ++
>  ioport.c             | 18 ------------------
>  pci.c                |  8 ++++++++
>  vfio/core.c          |  6 ++++--
>  virtio/pci.c         |  3 ++-
>  8 files changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/pci-shmem.c b/hw/pci-shmem.c
> index f92bc75..a0c5ba8 100644
> --- a/hw/pci-shmem.c
> +++ b/hw/pci-shmem.c
> @@ -357,7 +357,8 @@ int pci_shmem__init(struct kvm *kvm)
>  		return 0;
>  
>  	/* Register MMIO space for MSI-X */
> -	r = ioport__register(kvm, IOPORT_EMPTY, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
> +	r = pci_get_io_port_block(IOPORT_SIZE);
> +	r = ioport__register(kvm, r, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
>  	if (r < 0)
>  		return r;
>  	ivshmem_registers = (u16)r;
> diff --git a/hw/vesa.c b/hw/vesa.c
> index f3c5114..404a8a3 100644
> --- a/hw/vesa.c
> +++ b/hw/vesa.c
> @@ -60,8 +60,8 @@ struct framebuffer *vesa__init(struct kvm *kvm)
>  
>  	if (!kvm->cfg.vnc && !kvm->cfg.sdl && !kvm->cfg.gtk)
>  		return NULL;
> -
> -	r = ioport__register(kvm, IOPORT_EMPTY, &vesa_io_ops, IOPORT_SIZE, NULL);
> +	r = pci_get_io_space_block(IOPORT_SIZE);

I am confused. This is still registering I/O ports, right? And this
(misnamed) function is about MMIO?
So should it read r = pci_get_io_port_block(IOPORT_SIZE); ?

> +	r = ioport__register(kvm, r, &vesa_io_ops, IOPORT_SIZE, NULL);
>  	if (r < 0)
>  		return ERR_PTR(r);
>  
> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h
> index db52a47..b10fcd5 100644
> --- a/include/kvm/ioport.h
> +++ b/include/kvm/ioport.h
> @@ -14,11 +14,8 @@
>  
>  /* some ports we reserve for own use */
>  #define IOPORT_DBG			0xe0
> -#define IOPORT_START			0x6200
>  #define IOPORT_SIZE			0x400
>  
> -#define IOPORT_EMPTY			USHRT_MAX
> -
>  struct kvm;
>  
>  struct ioport {
> diff --git a/include/kvm/pci.h b/include/kvm/pci.h
> index a86c15a..bdbd183 100644
> --- a/include/kvm/pci.h
> +++ b/include/kvm/pci.h
> @@ -19,6 +19,7 @@
>  #define PCI_CONFIG_DATA		0xcfc
>  #define PCI_CONFIG_BUS_FORWARD	0xcfa
>  #define PCI_IO_SIZE		0x100
> +#define PCI_IOPORT_START	0x6200
>  #define PCI_CFG_SIZE		(1ULL << 24)
>  
>  struct kvm;
> @@ -153,6 +154,7 @@ int pci__init(struct kvm *kvm);
>  int pci__exit(struct kvm *kvm);
>  struct pci_device_header *pci__find_dev(u8 dev_num);
>  u32 pci_get_io_space_block(u32 size);

So I think this was already misnamed, but with your new function below
becomes utterly confusing. Can we rename this to pci_get_mmio_space_block?

> +u16 pci_get_io_port_block(u32 size);
>  void pci__assign_irq(struct device_header *dev_hdr);
>  void pci__config_wr(struct kvm *kvm, union pci_config_address addr, void *data, int size);
>  void pci__config_rd(struct kvm *kvm, union pci_config_address addr, void *data, int size);
> diff --git a/ioport.c b/ioport.c
> index a6dc65e..a72e403 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -16,24 +16,8 @@
>  
>  #define ioport_node(n) rb_entry(n, struct ioport, node)
>  
> -DEFINE_MUTEX(ioport_mutex);
> -
> -static u16			free_io_port_idx; /* protected by ioport_mutex */
> -
>  static struct rb_root		ioport_tree = RB_ROOT;
>  
> -static u16 ioport__find_free_port(void)
> -{
> -	u16 free_port;
> -
> -	mutex_lock(&ioport_mutex);
> -	free_port = IOPORT_START + free_io_port_idx * IOPORT_SIZE;
> -	free_io_port_idx++;
> -	mutex_unlock(&ioport_mutex);
> -
> -	return free_port;
> -}
> -
>  static struct ioport *ioport_search(struct rb_root *root, u64 addr)
>  {
>  	struct rb_int_node *node;
> @@ -85,8 +69,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
>  	int r;
>  
>  	br_write_lock(kvm);
> -	if (port == IOPORT_EMPTY)
> -		port = ioport__find_free_port();
>  
>  	entry = ioport_search(&ioport_tree, port);
>  	if (entry) {
> diff --git a/pci.c b/pci.c
> index 9edefa5..cd749db 100644
> --- a/pci.c
> +++ b/pci.c
> @@ -19,6 +19,14 @@ static u32 pci_config_address_bits;
>   * PCI isn't currently supported.)
>   */
>  static u32 io_space_blocks		= KVM_PCI_MMIO_AREA;
> +static u16 io_port_blocks		= PCI_IOPORT_START;
> +
> +u16 pci_get_io_port_block(u32 size)
> +{
> +	u16 port = ALIGN(io_port_blocks, IOPORT_SIZE);

Nit: Can we please have an empty line after the variable declaration?

Cheers,
Andre.

> +	io_port_blocks = port + size;
> +	return port;
> +}
>  
>  /*
>   * BARs must be naturally aligned, so enforce this in the allocator.
> diff --git a/vfio/core.c b/vfio/core.c
> index 17b5b0c..0ed1e6f 100644
> --- a/vfio/core.c
> +++ b/vfio/core.c
> @@ -202,8 +202,10 @@ static int vfio_setup_trap_region(struct kvm *kvm, struct vfio_device *vdev,
>  				  struct vfio_region *region)
>  {
>  	if (region->is_ioport) {
> -		int port = ioport__register(kvm, IOPORT_EMPTY, &vfio_ioport_ops,
> -					    region->info.size, region);
> +		int port = pci_get_io_port_block(region->info.size);
> +
> +		port = ioport__register(kvm, port, &vfio_ioport_ops,
> +					region->info.size, region);
>  		if (port < 0)
>  			return port;
>  
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 5a5fc6e..c8e16dd 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -420,7 +420,8 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>  	vpci->kvm = kvm;
>  	vpci->dev = dev;
>  
> -	r = ioport__register(kvm, IOPORT_EMPTY, &virtio_pci__io_ops, IOPORT_SIZE, vdev);
> +	r = pci_get_io_port_block(IOPORT_SIZE);
> +	r = ioport__register(kvm, r, &virtio_pci__io_ops, IOPORT_SIZE, vdev);
>  	if (r < 0)
>  		return r;
>  	vpci->port_addr = (u16)r;

Re: [PATCH kvmtool 05/16] ioport: pci: Move port allocations to PCI devices

[Julien Thierry]

Hi,

On 04/04/2019 14:45, Andre Przywara wrote:
> On Thu, 7 Mar 2019 08:36:06 +0000
> Julien Thierry <julien.thierry@arm.com> wrote:
> 
> Hi,
> 
>> The dynamic ioport allocation with IOPORT_EMPTY is currently only used
>> by PCI devices. Other devices use fixed ports for which they request
>> registration to the ioport API.
>>
>> PCI ports need to be in the PCI IO space and there is no reason ioport
>> API should know a PCI port is being allocated and needs to be placed in
>> PCI IO space. This currently just happens to be the case.
>>
>> Move the responsability of dynamic allocation of ioports from the ioport
>> API to PCI.
>>
>> In the future, if other types of devices also need dynamic ioport
>> allocation, they'll have to figure out the range of ports they are
>> allowed to use.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> ---
>>  hw/pci-shmem.c       |  3 ++-
>>  hw/vesa.c            |  4 ++--
>>  include/kvm/ioport.h |  3 ---
>>  include/kvm/pci.h    |  2 ++
>>  ioport.c             | 18 ------------------
>>  pci.c                |  8 ++++++++
>>  vfio/core.c          |  6 ++++--
>>  virtio/pci.c         |  3 ++-
>>  8 files changed, 20 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/pci-shmem.c b/hw/pci-shmem.c
>> index f92bc75..a0c5ba8 100644
>> --- a/hw/pci-shmem.c
>> +++ b/hw/pci-shmem.c
>> @@ -357,7 +357,8 @@ int pci_shmem__init(struct kvm *kvm)
>>  		return 0;
>>  
>>  	/* Register MMIO space for MSI-X */
>> -	r = ioport__register(kvm, IOPORT_EMPTY, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
>> +	r = pci_get_io_port_block(IOPORT_SIZE);
>> +	r = ioport__register(kvm, r, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
>>  	if (r < 0)
>>  		return r;
>>  	ivshmem_registers = (u16)r;
>> diff --git a/hw/vesa.c b/hw/vesa.c
>> index f3c5114..404a8a3 100644
>> --- a/hw/vesa.c
>> +++ b/hw/vesa.c
>> @@ -60,8 +60,8 @@ struct framebuffer *vesa__init(struct kvm *kvm)
>>  
>>  	if (!kvm->cfg.vnc && !kvm->cfg.sdl && !kvm->cfg.gtk)
>>  		return NULL;
>> -
>> -	r = ioport__register(kvm, IOPORT_EMPTY, &vesa_io_ops, IOPORT_SIZE, NULL);
>> +	r = pci_get_io_space_block(IOPORT_SIZE);
> 
> I am confused. This is still registering I/O ports, right? And this
> (misnamed) function is about MMIO?
> So should it read r = pci_get_io_port_block(IOPORT_SIZE); ?
> 
>> +	r = ioport__register(kvm, r, &vesa_io_ops, IOPORT_SIZE, NULL);
>>  	if (r < 0)
>>  		return ERR_PTR(r);
>>  
>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h
>> index db52a47..b10fcd5 100644
>> --- a/include/kvm/ioport.h
>> +++ b/include/kvm/ioport.h
>> @@ -14,11 +14,8 @@
>>  
>>  /* some ports we reserve for own use */
>>  #define IOPORT_DBG			0xe0
>> -#define IOPORT_START			0x6200
>>  #define IOPORT_SIZE			0x400
>>  
>> -#define IOPORT_EMPTY			USHRT_MAX
>> -
>>  struct kvm;
>>  
>>  struct ioport {
>> diff --git a/include/kvm/pci.h b/include/kvm/pci.h
>> index a86c15a..bdbd183 100644
>> --- a/include/kvm/pci.h
>> +++ b/include/kvm/pci.h
>> @@ -19,6 +19,7 @@
>>  #define PCI_CONFIG_DATA		0xcfc
>>  #define PCI_CONFIG_BUS_FORWARD	0xcfa
>>  #define PCI_IO_SIZE		0x100
>> +#define PCI_IOPORT_START	0x6200
>>  #define PCI_CFG_SIZE		(1ULL << 24)
>>  
>>  struct kvm;
>> @@ -153,6 +154,7 @@ int pci__init(struct kvm *kvm);
>>  int pci__exit(struct kvm *kvm);
>>  struct pci_device_header *pci__find_dev(u8 dev_num);
>>  u32 pci_get_io_space_block(u32 size);
> 
> So I think this was already misnamed, but with your new function below
> becomes utterly confusing. Can we rename this to pci_get_mmio_space_block?

Yes, seems fair enough. I'll add a patch to rename that.

> 
>> +u16 pci_get_io_port_block(u32 size);
>>  void pci__assign_irq(struct device_header *dev_hdr);
>>  void pci__config_wr(struct kvm *kvm, union pci_config_address addr, void *data, int size);
>>  void pci__config_rd(struct kvm *kvm, union pci_config_address addr, void *data, int size);
>> diff --git a/ioport.c b/ioport.c
>> index a6dc65e..a72e403 100644
>> --- a/ioport.c
>> +++ b/ioport.c
>> @@ -16,24 +16,8 @@
>>  
>>  #define ioport_node(n) rb_entry(n, struct ioport, node)
>>  
>> -DEFINE_MUTEX(ioport_mutex);
>> -
>> -static u16			free_io_port_idx; /* protected by ioport_mutex */
>> -
>>  static struct rb_root		ioport_tree = RB_ROOT;
>>  
>> -static u16 ioport__find_free_port(void)
>> -{
>> -	u16 free_port;
>> -
>> -	mutex_lock(&ioport_mutex);
>> -	free_port = IOPORT_START + free_io_port_idx * IOPORT_SIZE;
>> -	free_io_port_idx++;
>> -	mutex_unlock(&ioport_mutex);
>> -
>> -	return free_port;
>> -}
>> -
>>  static struct ioport *ioport_search(struct rb_root *root, u64 addr)
>>  {
>>  	struct rb_int_node *node;
>> @@ -85,8 +69,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
>>  	int r;
>>  
>>  	br_write_lock(kvm);
>> -	if (port == IOPORT_EMPTY)
>> -		port = ioport__find_free_port();
>>  
>>  	entry = ioport_search(&ioport_tree, port);
>>  	if (entry) {
>> diff --git a/pci.c b/pci.c
>> index 9edefa5..cd749db 100644
>> --- a/pci.c
>> +++ b/pci.c
>> @@ -19,6 +19,14 @@ static u32 pci_config_address_bits;
>>   * PCI isn't currently supported.)
>>   */
>>  static u32 io_space_blocks		= KVM_PCI_MMIO_AREA;
>> +static u16 io_port_blocks		= PCI_IOPORT_START;
>> +
>> +u16 pci_get_io_port_block(u32 size)
>> +{
>> +	u16 port = ALIGN(io_port_blocks, IOPORT_SIZE);
> 
> Nit: Can we please have an empty line after the variable declaration?
> 

We can :) .

Thanks,

-- 
Julien Thierry

[PATCH kvmtool 06/16] pci: Fix ioport allocation size

[Julien Thierry]

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.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 hw/pci-shmem.c       |  4 ++--
 hw/vesa.c            |  4 ++--
 include/kvm/ioport.h |  1 -
 pci.c                |  2 +-
 virtio/pci.c         | 14 +++++++-------
 5 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/pci-shmem.c b/hw/pci-shmem.c
index a0c5ba8..2e1474b 100644
--- a/hw/pci-shmem.c
+++ b/hw/pci-shmem.c
@@ -357,8 +357,8 @@ int pci_shmem__init(struct kvm *kvm)
 		return 0;
 
 	/* Register MMIO space for MSI-X */
-	r = pci_get_io_port_block(IOPORT_SIZE);
-	r = ioport__register(kvm, r, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
+	r = pci_get_io_port_block(PCI_IO_SIZE);
+	r = ioport__register(kvm, r, &shmem_pci__io_ops, PCI_IO_SIZE, NULL);
 	if (r < 0)
 		return r;
 	ivshmem_registers = (u16)r;
diff --git a/hw/vesa.c b/hw/vesa.c
index 404a8a3..71935d5 100644
--- a/hw/vesa.c
+++ b/hw/vesa.c
@@ -60,8 +60,8 @@ struct framebuffer *vesa__init(struct kvm *kvm)
 
 	if (!kvm->cfg.vnc && !kvm->cfg.sdl && !kvm->cfg.gtk)
 		return NULL;
-	r = pci_get_io_space_block(IOPORT_SIZE);
-	r = ioport__register(kvm, r, &vesa_io_ops, IOPORT_SIZE, NULL);
+	r = pci_get_io_space_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 b10fcd5..8c86b71 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 cd749db..228a628 100644
--- a/pci.c
+++ b/pci.c
@@ -23,7 +23,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 c8e16dd..5a6c0d0 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -406,7 +406,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);
 }
@@ -420,14 +420,14 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	vpci->kvm = kvm;
 	vpci->dev = dev;
 
-	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_io_space_block(IOPORT_SIZE);
-	r = kvm__register_mmio(kvm, vpci->mmio_addr, IOPORT_SIZE, false,
+	vpci->mmio_addr = pci_get_io_space_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;
@@ -457,8 +457,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),
 	};
 
-- 
1.9.1

Re: [PATCH kvmtool 06/16] pci: Fix ioport allocation size

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:07 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

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

Good catch!

> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

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

Cheers,
Andre.

> ---
>  hw/pci-shmem.c       |  4 ++--
>  hw/vesa.c            |  4 ++--
>  include/kvm/ioport.h |  1 -
>  pci.c                |  2 +-
>  virtio/pci.c         | 14 +++++++-------
>  5 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/pci-shmem.c b/hw/pci-shmem.c
> index a0c5ba8..2e1474b 100644
> --- a/hw/pci-shmem.c
> +++ b/hw/pci-shmem.c
> @@ -357,8 +357,8 @@ int pci_shmem__init(struct kvm *kvm)
>  		return 0;
>  
>  	/* Register MMIO space for MSI-X */
> -	r = pci_get_io_port_block(IOPORT_SIZE);
> -	r = ioport__register(kvm, r, &shmem_pci__io_ops, IOPORT_SIZE, NULL);
> +	r = pci_get_io_port_block(PCI_IO_SIZE);
> +	r = ioport__register(kvm, r, &shmem_pci__io_ops, PCI_IO_SIZE, NULL);
>  	if (r < 0)
>  		return r;
>  	ivshmem_registers = (u16)r;
> diff --git a/hw/vesa.c b/hw/vesa.c
> index 404a8a3..71935d5 100644
> --- a/hw/vesa.c
> +++ b/hw/vesa.c
> @@ -60,8 +60,8 @@ struct framebuffer *vesa__init(struct kvm *kvm)
>  
>  	if (!kvm->cfg.vnc && !kvm->cfg.sdl && !kvm->cfg.gtk)
>  		return NULL;
> -	r = pci_get_io_space_block(IOPORT_SIZE);
> -	r = ioport__register(kvm, r, &vesa_io_ops, IOPORT_SIZE, NULL);
> +	r = pci_get_io_space_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 b10fcd5..8c86b71 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 cd749db..228a628 100644
> --- a/pci.c
> +++ b/pci.c
> @@ -23,7 +23,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 c8e16dd..5a6c0d0 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -406,7 +406,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);
>  }
> @@ -420,14 +420,14 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>  	vpci->kvm = kvm;
>  	vpci->dev = dev;
>  
> -	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_io_space_block(IOPORT_SIZE);
> -	r = kvm__register_mmio(kvm, vpci->mmio_addr, IOPORT_SIZE, false,
> +	vpci->mmio_addr = pci_get_io_space_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;
> @@ -457,8 +457,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),
>  	};
>  

[PATCH kvmtool 07/16] arm/pci: Fix PCI IO region

[Julien Thierry]

Current PCI IO region that is exposed through the DT contains ports that
are reserved by non-PCI devices.

Use the proper PCI IO start so that the region exposed through DT can
actually be used to reassign device BARs.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arm/include/arm-common/pci.h |  1 +
 arm/kvm.c                    |  3 +++
 arm/pci.c                    | 21 ++++++++++++++++++---
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/arm/include/arm-common/pci.h b/arm/include/arm-common/pci.h
index 9008a0e..aea42b8 100644
--- a/arm/include/arm-common/pci.h
+++ b/arm/include/arm-common/pci.h
@@ -1,6 +1,7 @@
 #ifndef ARM_COMMON__PCI_H
 #define ARM_COMMON__PCI_H
 
+void pci__arm_init(struct kvm *kvm);
 void pci__generate_fdt_nodes(void *fdt);
 
 #endif /* ARM_COMMON__PCI_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index b824f63..44d7796 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -6,6 +6,7 @@
 #include "kvm/fdt.h"
 
 #include "arm-common/gic.h"
+#include "arm-common/pci.h"
 
 #include <linux/kernel.h>
 #include <linux/kvm.h>
@@ -86,6 +87,8 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 	/* Create the virtual GIC. */
 	if (gic__create(kvm, kvm->cfg.arch.irqchip))
 		die("Failed to create virtual GIC");
+
+	pci__arm_init(kvm);
 }
 
 #define FDT_ALIGN	SZ_2M
diff --git a/arm/pci.c b/arm/pci.c
index 557cfa9..83238ca 100644
--- a/arm/pci.c
+++ b/arm/pci.c
@@ -1,3 +1,5 @@
+#include "linux/sizes.h"
+
 #include "kvm/devices.h"
 #include "kvm/fdt.h"
 #include "kvm/kvm.h"
@@ -7,6 +9,11 @@
 
 #include "arm-common/pci.h"
 
+#define ARM_PCI_IO_START ALIGN(PCI_IOPORT_START, SZ_4K)
+
+/* Must be a multiple of 4k */
+#define ARM_PCI_IO_SIZE ((ARM_MMIO_AREA - ARM_PCI_IO_START) & ~(SZ_4K - 1))
+
 /*
  * An entry in the interrupt-map table looks like:
  * <pci unit address> <pci interrupt pin> <gic phandle> <gic interrupt>
@@ -24,6 +31,14 @@ struct of_interrupt_map_entry {
 	struct of_gic_irq		gic_irq;
 } __attribute__((packed));
 
+void pci__arm_init(struct kvm *kvm)
+{
+	u32 align_pad = ARM_PCI_IO_START - PCI_IOPORT_START;
+
+	/* Make PCI port allocation start at a properly aligned address */
+	pci_get_io_space_block(align_pad);
+}
+
 void pci__generate_fdt_nodes(void *fdt)
 {
 	struct device_header *dev_hdr;
@@ -40,10 +55,10 @@ void pci__generate_fdt_nodes(void *fdt)
 			.pci_addr = {
 				.hi	= cpu_to_fdt32(of_pci_b_ss(OF_PCI_SS_IO)),
 				.mid	= 0,
-				.lo	= 0,
+				.lo	= cpu_to_fdt32(ARM_PCI_IO_START),
 			},
-			.cpu_addr	= cpu_to_fdt64(KVM_IOPORT_AREA),
-			.length		= cpu_to_fdt64(ARM_IOPORT_SIZE),
+			.cpu_addr	= cpu_to_fdt64(ARM_PCI_IO_START),
+			.length		= cpu_to_fdt64(ARM_PCI_IO_SIZE),
 		},
 		{
 			.pci_addr = {
-- 
1.9.1

[PATCH kvmtool 08/16] arm/pci: Do not use first PCI IO space bytes for devices

[Julien Thierry]

Linux has this convention that the lower 0x1000 bytes of the IO space
should not be used. (cf PCIBIOS_MIN_IO).

Just allocate those bytes to prevent future allocation assigning it to
devices.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arm/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arm/pci.c b/arm/pci.c
index 83238ca..559e0cf 100644
--- a/arm/pci.c
+++ b/arm/pci.c
@@ -37,6 +37,9 @@ void pci__arm_init(struct kvm *kvm)
 
 	/* Make PCI port allocation start at a properly aligned address */
 	pci_get_io_space_block(align_pad);
+
+	/* Convention, don't allocate first 0x1000 bytes of PCI IO */
+	pci_get_io_space_block(0x1000);
 }
 
 void pci__generate_fdt_nodes(void *fdt)
-- 
1.9.1

Re: [PATCH kvmtool 08/16] arm/pci: Do not use first PCI IO space bytes for devices

[Andre Przywara]

On Thu, 7 Mar 2019 08:36:09 +0000
Julien Thierry <julien.thierry@arm.com> wrote:

Hi,

> Linux has this convention that the lower 0x1000 bytes of the IO space
> should not be used. (cf PCIBIOS_MIN_IO).
> 
> Just allocate those bytes to prevent future allocation assigning it to
> devices.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> ---
>  arm/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arm/pci.c b/arm/pci.c
> index 83238ca..559e0cf 100644
> --- a/arm/pci.c
> +++ b/arm/pci.c
> @@ -37,6 +37,9 @@ void pci__arm_init(struct kvm *kvm)
>  
>  	/* Make PCI port allocation start at a properly aligned address */
>  	pci_get_io_space_block(align_pad);
> +
> +	/* Convention, don't allocate first 0x1000 bytes of PCI IO */
> +	pci_get_io_space_block(0x1000);

Is this the same problem with mixing up I/O and MMIO space as in the other patch?
io_space means MMIO, right?

Cheers,
Andre.

>  }
>  
>  void pci__generate_fdt_nodes(void *fdt)

Re: [PATCH kvmtool 08/16] arm/pci: Do not use first PCI IO space bytes for devices

[Julien Thierry]

Hi Andre,

(sorry for the delay in reply)

On 05/04/2019 16:31, Andre Przywara wrote:
> On Thu, 7 Mar 2019 08:36:09 +0000
> Julien Thierry <julien.thierry@arm.com> wrote:
> 
> Hi,
> 
>> Linux has this convention that the lower 0x1000 bytes of the IO space
>> should not be used. (cf PCIBIOS_MIN_IO).
>>
>> Just allocate those bytes to prevent future allocation assigning it to
>> devices.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> ---
>>  arm/pci.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arm/pci.c b/arm/pci.c
>> index 83238ca..559e0cf 100644
>> --- a/arm/pci.c
>> +++ b/arm/pci.c
>> @@ -37,6 +37,9 @@ void pci__arm_init(struct kvm *kvm)
>>  
>>  	/* Make PCI port allocation start at a properly aligned address */
>>  	pci_get_io_space_block(align_pad);
>> +
>> +	/* Convention, don't allocate first 0x1000 bytes of PCI IO */
>> +	pci_get_io_space_block(0x1000);
> 
> Is this the same problem with mixing up I/O and MMIO space as in the other patch?
> io_space means MMIO, right?
> 

Oh yes, you're right. Thanks for catching that (and in the other patch
as well).

However, fixing it unveiled a bug which apparently requires me to change
a bunch of things w.r.t. how we handle the configuration. At boot time,
Linux (without probe only) reassigns bars without disabling the device
response (it assumes that none of the devices it can configure are being
used/accessed).

This means that during the reassignment, bars from different or same
devices can temporarily alias/overlap each other during boot time. And
the current handling of PCI io/mmio region doesn't support that.

I'll rework this to make things a little bit more flexible.

Thanks,

-- 
Julien Thierry

[PATCH kvmtool 09/16] brlock: Use rwlock instead of pause

[Julien Thierry]

Pausing all vcpus when reconfiguring something at run time is a big
overhead. Use rwlock to allow vcpu not accessing ressources being
reconfigured to continue running.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 include/kvm/brlock.h | 11 -----------
 include/kvm/kvm.h    |  2 --
 2 files changed, 13 deletions(-)

diff --git a/include/kvm/brlock.h b/include/kvm/brlock.h
index 1862210..7d81056 100644
--- a/include/kvm/brlock.h
+++ b/include/kvm/brlock.h
@@ -17,8 +17,6 @@
 #define barrier()		__asm__ __volatile__("": : :"memory")
 #endif
 
-#ifdef KVM_BRLOCK_DEBUG
-
 #include "kvm/rwsem.h"
 
 #define br_read_lock(kvm)	down_read(&(kvm)->brlock_sem);
@@ -27,13 +25,4 @@
 #define br_write_lock(kvm)	down_write(&(kvm)->brlock_sem);
 #define br_write_unlock(kvm)	up_write(&(kvm)->brlock_sem);
 
-#else
-
-#define br_read_lock(kvm)	barrier()
-#define br_read_unlock(kvm)	barrier()
-
-#define br_write_lock(kvm)	kvm__pause(kvm)
-#define br_write_unlock(kvm)	kvm__continue(kvm)
-#endif
-
 #endif
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 7a73818..2f1679e 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -82,9 +82,7 @@ struct kvm {
 
 	int			vm_state;
 
-#ifdef KVM_BRLOCK_DEBUG
 	pthread_rwlock_t	brlock_sem;
-#endif
 };
 
 void kvm__set_dir(const char *fmt, ...);
-- 
1.9.1

[PATCH kvmtool 10/16] ref_cnt: Add simple ref counting API

[Julien Thierry]

Provide a simple API with structure and function for reference counting.
This is inspired by the linux kref.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 include/kvm/ref_cnt.h | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 include/kvm/ref_cnt.h

diff --git a/include/kvm/ref_cnt.h b/include/kvm/ref_cnt.h
new file mode 100644
index 0000000..1c8194c
--- /dev/null
+++ b/include/kvm/ref_cnt.h
@@ -0,0 +1,53 @@
+#ifndef KVM__REF_CNT_H
+#define KVM__REF_CNT_H
+
+#include "kvm/mutex.h"
+
+#ifdef __ATOMIC_ACQUIRE
+
+#define KVM_ATOMIC_ACQUIRE __ATOMIC_ACQUIRE
+#define KVM_ATOMIC_RELEASE __ATOMIC_RELEASE
+
+#define kvm_atomic_add_fetch(ptr, val, memorder)	\
+	__atomic_add_fetch((ptr), (val), (memorder))
+
+#define kvm_atomic_sub_fetch(ptr, val, memorder)	\
+	__atomic_sub_fetch((ptr), (val), (memorder))
+#else
+
+#define KVM_ATOMIC_ACQUIRE 0
+#define KVM_ATOMIC_RELEASE 0
+
+#define kvm_atomic_add_fetch(ptr, val, memorder)	\
+	__sync_fetch_and_add((ptr), (val))
+
+#define kvm_atomic_sub_fetch(ptr, val, memorder)	\
+	__sync_fetch_and_sub((ptr), (val))
+
+#endif
+
+struct ref_cnt {
+	int cnt;
+};
+
+#define REF_CNT_INIT (struct ref_cnt) { .cnt = 1 }
+
+static inline void ref_cnt_init(struct ref_cnt *ref_cnt)
+{
+	ref_cnt->cnt = 1;
+}
+
+static inline void ref_get(struct ref_cnt *ref_cnt)
+{
+	kvm_atomic_add_fetch(&ref_cnt->cnt, 1, KVM_ATOMIC_ACQUIRE);
+
+}
+
+static inline void ref_put(struct ref_cnt *ref_cnt,
+			   void (*release)(struct ref_cnt *ref_cnt))
+{
+	if (!kvm_atomic_sub_fetch(&ref_cnt->cnt, 1, KVM_ATOMIC_RELEASE))
+		release(ref_cnt);
+}
+
+#endif /* KVM__REF_CNT_H */
-- 
1.9.1

[PATCH kvmtool 11/16] mmio: Allow mmio callbacks to be called without locking

[Julien Thierry]

A vcpu might reconfigure some memory mapped region while other vcpus
access another one. Currently, edit to one mmio region blocks all other
mmio accesses.

Execute mmio callbacks outside of locking, protecting mmio data with
ref counting to prevent data being deleted while callback runs.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 mmio.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/mmio.c b/mmio.c
index 61e1d47..03e1a76 100644
--- a/mmio.c
+++ b/mmio.c
@@ -2,6 +2,7 @@
 #include "kvm/kvm-cpu.h"
 #include "kvm/rbtree-interval.h"
 #include "kvm/brlock.h"
+#include "kvm/ref_cnt.h"
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -19,10 +20,18 @@ struct mmio_mapping {
 	struct rb_int_node	node;
 	void			(*mmio_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
 	void			*ptr;
+	struct ref_cnt		ref_cnt;
 };
 
 static struct rb_root mmio_tree = RB_ROOT;
 
+static void mmio_release(struct ref_cnt *ref_cnt)
+{
+	struct mmio_mapping * mmio = container_of(ref_cnt, struct mmio_mapping, ref_cnt);
+
+	free(mmio);
+}
+
 static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
 {
 	struct rb_int_node *node;
@@ -75,6 +84,7 @@ int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool c
 		.node = RB_INT_INIT(phys_addr, phys_addr + phys_addr_len),
 		.mmio_fn = mmio_fn,
 		.ptr	= ptr,
+		.ref_cnt = REF_CNT_INIT,
 	};
 
 	if (coalesce) {
@@ -89,6 +99,7 @@ int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool c
 		}
 	}
 	br_write_lock(kvm);
+	/* Pass ref to tree, no need to put ref */
 	ret = mmio_insert(&mmio_tree, mmio);
 	br_write_unlock(kvm);
 
@@ -106,6 +117,7 @@ bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr)
 		br_write_unlock(kvm);
 		return false;
 	}
+	/* Taking the ref from the tree */
 
 	zone = (struct kvm_coalesced_mmio_zone) {
 		.addr	= phys_addr,
@@ -114,9 +126,10 @@ bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr)
 	ioctl(kvm->vm_fd, KVM_UNREGISTER_COALESCED_MMIO, &zone);
 
 	rb_int_erase(&mmio_tree, &mmio->node);
+	ref_put(&mmio->ref_cnt, mmio_release);
+
 	br_write_unlock(kvm);
 
-	free(mmio);
 	return true;
 }
 
@@ -127,15 +140,20 @@ bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u
 	br_read_lock(vcpu->kvm);
 	mmio = mmio_search(&mmio_tree, phys_addr, len);
 
-	if (mmio)
-		mmio->mmio_fn(vcpu, phys_addr, data, len, is_write, mmio->ptr);
-	else {
+	if (!mmio) {
+		br_read_unlock(vcpu->kvm);
 		if (vcpu->kvm->cfg.mmio_debug)
 			fprintf(stderr,	"Warning: Ignoring MMIO %s at %016llx (length %u)\n",
 				to_direction(is_write),
 				(unsigned long long)phys_addr, len);
+		return true;
 	}
+
+	ref_get(&mmio->ref_cnt);
 	br_read_unlock(vcpu->kvm);
 
+	mmio->mmio_fn(vcpu, phys_addr, data, len, is_write, mmio->ptr);
+	ref_put(&mmio->ref_cnt, mmio_release);
+
 	return true;
 }
-- 
1.9.1

[PATCH kvmtool 12/16] ioport: Allow ioport callbacks to be called without locking

[Julien Thierry]

A vcpu might reconfigure some ioports while other vcpus access another one.
Currently, edit to one port blocks all other ioport accesses.

Execute ioport callbacks outside of locking. protecting ioport data with
ref counting to prevent data being deleted while callback runs.

Since ioport struct is being passed to all ioport callbacks, wrap it in
another structure that will get ref counted to avoid modifying all ioport
devices.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 include/kvm/ioport.h |  1 -
 ioport.c             | 68 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h
index 8c86b71..a73f78c 100644
--- a/include/kvm/ioport.h
+++ b/include/kvm/ioport.h
@@ -18,7 +18,6 @@
 struct kvm;
 
 struct ioport {
-	struct rb_int_node		node;
 	struct ioport_operations	*ops;
 	void				*priv;
 	struct device_header		dev_hdr;
diff --git a/ioport.c b/ioport.c
index a72e403..2c3fe93 100644
--- a/ioport.c
+++ b/ioport.c
@@ -5,6 +5,7 @@
 #include "kvm/brlock.h"
 #include "kvm/rbtree-interval.h"
 #include "kvm/mutex.h"
+#include "kvm/ref_cnt.h"
 
 #include <linux/kvm.h>	/* for KVM_EXIT_* */
 #include <linux/types.h>
@@ -14,11 +15,25 @@
 #include <stdlib.h>
 #include <stdio.h>
 
-#define ioport_node(n) rb_entry(n, struct ioport, node)
+#define ioport_node(n) rb_entry(n, struct ioport_data, node)
 
 static struct rb_root		ioport_tree = RB_ROOT;
 
-static struct ioport *ioport_search(struct rb_root *root, u64 addr)
+struct ioport_data {
+	struct rb_int_node	node;
+	struct ioport		ioport;
+	struct ref_cnt		ref_cnt;
+};
+
+static void ioport_release(struct ref_cnt  *ref_cnt)
+{
+	struct ioport_data *data = container_of(ref_cnt,
+						struct ioport_data, ref_cnt);
+
+	free(data);
+}
+
+static struct ioport_data *ioport_search(struct rb_root *root, u64 addr)
 {
 	struct rb_int_node *node;
 
@@ -29,12 +44,12 @@ static struct ioport *ioport_search(struct rb_root *root, u64 addr)
 	return ioport_node(node);
 }
 
-static int ioport_insert(struct rb_root *root, struct ioport *data)
+static int ioport_insert(struct rb_root *root, struct ioport_data *data)
 {
 	return rb_int_insert(root, &data->node);
 }
 
-static void ioport_remove(struct rb_root *root, struct ioport *data)
+static void ioport_remove(struct rb_root *root, struct ioport_data *data)
 {
 	rb_int_erase(root, &data->node);
 }
@@ -65,7 +80,7 @@ static void generate_ioport_fdt_node(void *fdt,
 
 int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, int count, void *param)
 {
-	struct ioport *entry;
+	struct ioport_data *entry;
 	int r;
 
 	br_write_lock(kvm);
@@ -74,14 +89,16 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
 	if (entry) {
 		pr_warning("ioport re-registered: %x", port);
 		rb_int_erase(&ioport_tree, &entry->node);
+		ref_put(&entry->ref_cnt, ioport_release);
 	}
 
 	entry = malloc(sizeof(*entry));
 	if (entry == NULL)
 		return -ENOMEM;
 
-	*entry = (struct ioport) {
-		.node		= RB_INT_INIT(port, port + count),
+	ref_cnt_init(&entry->ref_cnt);
+	entry->node = RB_INT_INIT(port, port + count);
+	entry->ioport = (struct ioport) {
 		.ops		= ops,
 		.priv		= param,
 		.dev_hdr	= (struct device_header) {
@@ -90,14 +107,15 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
 		},
 	};
 
+	/* Give the ref to the tree */
 	r = ioport_insert(&ioport_tree, entry);
 	if (r < 0) {
-		free(entry);
+		ref_put(&entry->ref_cnt, ioport_release);
 		br_write_unlock(kvm);
 		return r;
 	}
 
-	device__register(&entry->dev_hdr);
+	device__register(&entry->ioport.dev_hdr);
 	br_write_unlock(kvm);
 
 	return port;
@@ -105,7 +123,7 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i
 
 int ioport__unregister(struct kvm *kvm, u16 port)
 {
-	struct ioport *entry;
+	struct ioport_data *entry;
 	int r;
 
 	br_write_lock(kvm);
@@ -115,10 +133,9 @@ int ioport__unregister(struct kvm *kvm, u16 port)
 	if (!entry)
 		goto done;
 
-	device__unregister(&entry->dev_hdr);
+	device__unregister(&entry->ioport.dev_hdr);
 	ioport_remove(&ioport_tree, entry);
-
-	free(entry);
+	ref_put(&entry->ref_cnt, ioport_release);
 
 	r = 0;
 
@@ -130,7 +147,7 @@ done:
 
 static void ioport__unregister_all(void)
 {
-	struct ioport *entry;
+	struct ioport_data *entry;
 	struct rb_node *rb;
 	struct rb_int_node *rb_node;
 
@@ -138,9 +155,9 @@ static void ioport__unregister_all(void)
 	while (rb) {
 		rb_node = rb_int(rb);
 		entry = ioport_node(rb_node);
-		device__unregister(&entry->dev_hdr);
+		device__unregister(&entry->ioport.dev_hdr);
 		ioport_remove(&ioport_tree, entry);
-		free(entry);
+		ref_put(&entry->ref_cnt, ioport_release);
 		rb = rb_first(&ioport_tree);
 	}
 }
@@ -162,29 +179,34 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
 {
 	struct ioport_operations *ops;
 	bool ret = false;
-	struct ioport *entry;
+	struct ioport_data *entry;
 	void *ptr = data;
 	struct kvm *kvm = vcpu->kvm;
 
 	br_read_lock(kvm);
 	entry = ioport_search(&ioport_tree, port);
-	if (!entry)
+	if (!entry) {
+		br_read_unlock(kvm);
 		goto out;
+	}
+
+	ref_get(&entry->ref_cnt);
+	br_read_unlock(kvm);
 
-	ops	= entry->ops;
+	ops	= entry->ioport.ops;
 
 	while (count--) {
 		if (direction == KVM_EXIT_IO_IN && ops->io_in)
-				ret = ops->io_in(entry, vcpu, port, ptr, size);
+				ret = ops->io_in(&entry->ioport, vcpu, port, ptr, size);
 		else if (direction == KVM_EXIT_IO_OUT && ops->io_out)
-				ret = ops->io_out(entry, vcpu, port, ptr, size);
+				ret = ops->io_out(&entry->ioport, vcpu, port, ptr, size);
 
 		ptr += size;
 	}
 
-out:
-	br_read_unlock(kvm);
+	ref_put(&entry->ref_cnt, ioport_release);
 
+out:
 	if (ret)
 		return true;
 
-- 
1.9.1

[PATCH kvmtool 13/16] vfio: Add support for BAR configuration

[Julien Thierry]

When a guest can reassign BARs, kvmtool needs to maintain the vfio_region
consistent with their corresponding BARs. Take the new updated addresses
from the PCI header read back from the vfio driver.

Also, to modify the BARs, it is expected that guests will disable
IO/Memory response in the PCI command. Support this by mapping/unmapping
regions when the corresponding response gets enabled/disabled.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 vfio/core.c |  8 +++---
 vfio/pci.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/vfio/core.c b/vfio/core.c
index 0ed1e6f..b554897 100644
--- a/vfio/core.c
+++ b/vfio/core.c
@@ -202,14 +202,13 @@ static int vfio_setup_trap_region(struct kvm *kvm, struct vfio_device *vdev,
 				  struct vfio_region *region)
 {
 	if (region->is_ioport) {
-		int port = pci_get_io_port_block(region->info.size);
+		int port = ioport__register(kvm, region->port_base,
+					    &vfio_ioport_ops,
+					    region->info.size, region);
 
-		port = ioport__register(kvm, port, &vfio_ioport_ops,
-					region->info.size, region);
 		if (port < 0)
 			return port;
 
-		region->port_base = port;
 		return 0;
 	}
 
@@ -258,6 +257,7 @@ void vfio_unmap_region(struct kvm *kvm, struct vfio_region *region)
 {
 	if (region->host_addr) {
 		munmap(region->host_addr, region->info.size);
+		region->host_addr = NULL;
 	} else if (region->is_ioport) {
 		ioport__unregister(kvm, region->port_base);
 	} else {
diff --git a/vfio/pci.c b/vfio/pci.c
index 03de3c1..474f1c1 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -1,3 +1,4 @@
+#include "kvm/ioport.h"
 #include "kvm/irq.h"
 #include "kvm/kvm.h"
 #include "kvm/kvm-cpu.h"
@@ -452,6 +453,64 @@ static void vfio_pci_cfg_read(struct kvm *kvm, struct pci_device_header *pci_hdr
 			      sz, offset);
 }
 
+static void vfio_pci_cfg_handle_command(struct kvm *kvm, struct vfio_device *vdev,
+					void *data, int sz)
+{
+	struct pci_device_header *hdr = &vdev->pci.hdr;
+	bool switch_io;
+	bool switch_mem;
+	u16 cmd;
+	int i;
+
+	cmd = ioport__read16(data);
+	switch_io = !!((cmd ^ hdr->command) & PCI_COMMAND_IO);
+	switch_mem = !!((cmd ^ hdr->command) & PCI_COMMAND_MEMORY);
+
+	for (i = VFIO_PCI_BAR0_REGION_INDEX; i <= VFIO_PCI_BAR5_REGION_INDEX; ++i) {
+		struct vfio_region *region = &vdev->regions[i];
+
+		if (region->is_ioport && switch_io) {
+			if (cmd & PCI_COMMAND_IO)
+				vfio_map_region(kvm, vdev, region);
+			else
+				vfio_unmap_region(kvm, region);
+		}
+
+		if (!region->is_ioport && switch_mem) {
+			if (cmd & PCI_COMMAND_MEMORY)
+				vfio_map_region(kvm, vdev, region);
+			else
+				vfio_unmap_region(kvm, region);
+		}
+	}
+}
+
+static void vfio_pci_cfg_update_bar(struct kvm *kvm, struct vfio_device *vdev, int bar)
+{
+	struct pci_device_header *hdr = &vdev->pci.hdr;
+	struct vfio_region *bar_region;
+
+	bar_region = &vdev->regions[bar + VFIO_PCI_BAR0_REGION_INDEX];
+
+	if (bar_region->is_ioport) {
+		if (hdr->command & PCI_COMMAND_IO)
+			vfio_unmap_region(kvm, bar_region);
+
+		bar_region->port_base = hdr->bar[bar] & PCI_BASE_ADDRESS_IO_MASK;
+
+		if (hdr->command & PCI_COMMAND_IO)
+			vfio_map_region(kvm, vdev, bar_region);
+	} else {
+		if (hdr->command & PCI_COMMAND_MEMORY)
+			vfio_unmap_region(kvm, bar_region);
+
+		bar_region->guest_phys_addr = hdr->bar[bar] & PCI_BASE_ADDRESS_MEM_MASK;
+
+		if (hdr->command & PCI_COMMAND_MEMORY)
+			vfio_map_region(kvm, vdev, bar_region);
+	}
+}
+
 static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hdr,
 			       u8 offset, void *data, int sz)
 {
@@ -475,9 +534,15 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
 	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI)
 		vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz);
 
+	if (offset == PCI_COMMAND)
+		vfio_pci_cfg_handle_command(kvm, vdev, data, sz);
+
 	if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz)
 		vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x",
 			      sz, offset);
+
+	if (offset >= PCI_BASE_ADDRESS_0 && offset <= PCI_BASE_ADDRESS_5)
+		vfio_pci_cfg_update_bar(kvm, vdev, offset - PCI_BASE_ADDRESS_0);
 }
 
 static ssize_t vfio_pci_msi_cap_size(struct msi_cap_64 *cap_hdr)
@@ -801,6 +866,7 @@ static int vfio_pci_configure_bar(struct kvm *kvm, struct vfio_device *vdev,
 	size_t map_size;
 	struct vfio_pci_device *pdev = &vdev->pci;
 	struct vfio_region *region = &vdev->regions[nr];
+	bool map_now;
 
 	if (nr >= vdev->info.num_regions)
 		return 0;
@@ -840,12 +906,20 @@ static int vfio_pci_configure_bar(struct kvm *kvm, struct vfio_device *vdev,
 		/* Grab some MMIO space in the guest */
 		map_size = ALIGN(region->info.size, PAGE_SIZE);
 		region->guest_phys_addr = pci_get_io_space_block(map_size);
+
+		map_now = pdev->hdr.command & PCI_COMMAND_MEMORY;
+	} else {
+		region->port_base = pci_get_io_port_block(region->info.size);
+
+		map_now = pdev->hdr.command & PCI_COMMAND_IO;
 	}
 
-	/* Map the BARs into the guest or setup a trap region. */
-	ret = vfio_map_region(kvm, vdev, region);
-	if (ret)
-		return ret;
+	if (map_now) {
+		/* Map the BARs into the guest or setup a trap region. */
+		ret = vfio_map_region(kvm, vdev, region);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
1.9.1

[PATCH kvmtool 14/16] virtio/pci: Make memory and IO BARs independent

[Julien Thierry]

Currently, callbacks for memory BAR (BAR[1]) sits on the IO BAR, calling
the IO port emulation. This means that BAR[1] needs COMMAND_IO to be
enabled whenever COMMAND_MEMORY is enabled.

Refactor the code so both BARs are indenpendent. Also, unify ioport/mmio
callback arguments so that they all receive a virtio_device.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 virtio/pci.c | 69 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/virtio/pci.c b/virtio/pci.c
index 5a6c0d0..32f9824 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -77,7 +77,7 @@ static inline bool virtio_pci__msix_enabled(struct virtio_pci *vpci)
 	return vpci->pci_hdr.msix.ctrl & cpu_to_le16(PCI_MSIX_FLAGS_ENABLE);
 }
 
-static bool virtio_pci__specific_io_in(struct kvm *kvm, struct virtio_device *vdev, u16 port,
+static bool virtio_pci__specific_io_in(struct kvm *kvm, struct virtio_device *vdev,
 					void *data, int size, int offset)
 {
 	u32 config_offset;
@@ -107,21 +107,18 @@ static bool virtio_pci__specific_io_in(struct kvm *kvm, struct virtio_device *vd
 	return false;
 }
 
-static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev,
+				unsigned long bar_offset, void *data, int size)
 {
-	unsigned long offset;
 	bool ret = true;
-	struct virtio_device *vdev;
 	struct virtio_pci *vpci;
 	struct kvm *kvm;
 	u32 val;
 
 	kvm = vcpu->kvm;
-	vdev = ioport->priv;
 	vpci = vdev->virtio;
-	offset = port - vpci->port_addr;
 
-	switch (offset) {
+	switch (bar_offset) {
 	case VIRTIO_PCI_HOST_FEATURES:
 		val = vdev->ops->get_host_features(kvm, vpci->dev);
 		ioport__write32(data, val);
@@ -143,13 +140,24 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
 		vpci->isr = VIRTIO_IRQ_LOW;
 		break;
 	default:
-		ret = virtio_pci__specific_io_in(kvm, vdev, port, data, size, offset);
+		ret = virtio_pci__specific_io_in(kvm, vdev, data, size, bar_offset);
 		break;
 	};
 
 	return ret;
 }
 
+static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+{
+	struct virtio_device *vdev;
+	struct virtio_pci *vpci;
+
+	vdev = ioport->priv;
+	vpci = vdev->virtio;
+
+	return virtio_pci__data_in(vcpu, vdev, port - vpci->port_addr, data, size);
+}
+
 static void update_msix_map(struct virtio_pci *vpci,
 			    struct msix_table *msix_entry, u32 vecnum)
 {
@@ -174,7 +182,7 @@ static void update_msix_map(struct virtio_pci *vpci,
 	irq__update_msix_route(vpci->kvm, gsi, &msix_entry->msg);
 }
 
-static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *vdev, u16 port,
+static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *vdev,
 					void *data, int size, int offset)
 {
 	struct virtio_pci *vpci = vdev->virtio;
@@ -248,21 +256,18 @@ static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *v
 	return false;
 }
 
-static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vdev,
+				 unsigned long bar_offset, void *data, int size)
 {
-	unsigned long offset;
 	bool ret = true;
-	struct virtio_device *vdev;
 	struct virtio_pci *vpci;
 	struct kvm *kvm;
 	u32 val;
 
 	kvm = vcpu->kvm;
-	vdev = ioport->priv;
 	vpci = vdev->virtio;
-	offset = port - vpci->port_addr;
 
-	switch (offset) {
+	switch (bar_offset) {
 	case VIRTIO_PCI_GUEST_FEATURES:
 		val = ioport__read32(data);
 		virtio_set_guest_features(kvm, vdev, vpci->dev, val);
@@ -289,13 +294,26 @@ static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16
 			vdev->ops->notify_status(kvm, vpci->dev, vpci->status);
 		break;
 	default:
-		ret = virtio_pci__specific_io_out(kvm, vdev, port, data, size, offset);
+		ret = virtio_pci__specific_io_out(kvm, vdev, data, size, bar_offset);
 		break;
 	};
 
 	return ret;
 }
 
+static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+{
+	unsigned long offset;
+	struct virtio_device *vdev;
+	struct virtio_pci *vpci;
+
+	vdev = ioport->priv;
+	vpci = vdev->virtio;
+	offset = port - vpci->port_addr;
+
+	return virtio_pci__data_out(vcpu, vdev, offset, data, size);
+}
+
 static struct ioport_operations virtio_pci__io_ops = {
 	.io_in	= virtio_pci__io_in,
 	.io_out	= virtio_pci__io_out,
@@ -305,7 +323,8 @@ static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu,
 					   u64 addr, u8 *data, u32 len,
 					   u8 is_write, void *ptr)
 {
-	struct virtio_pci *vpci = ptr;
+	struct virtio_device *vdev = ptr;
+	struct virtio_pci *vpci = vdev->virtio;
 	struct msix_table *table;
 	int vecnum;
 	size_t offset;
@@ -404,11 +423,15 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
 					 u64 addr, u8 *data, u32 len,
 					 u8 is_write, void *ptr)
 {
-	struct virtio_pci *vpci = ptr;
-	int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-	u16 port = vpci->port_addr + (addr & (PCI_IO_SIZE - 1));
+	struct virtio_device *vdev = ptr;
+	struct virtio_pci *vpci = vdev->virtio;
 
-	kvm__emulate_io(vcpu, port, data, direction, len, 1);
+	if (!is_write)
+		virtio_pci__data_in(vcpu, vdev, addr - vpci->mmio_addr,
+				    data, len);
+	else
+		virtio_pci__data_out(vcpu, vdev, addr - vpci->mmio_addr,
+				     data, len);
 }
 
 int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
@@ -428,13 +451,13 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 
 	vpci->mmio_addr = pci_get_io_space_block(PCI_IO_SIZE);
 	r = kvm__register_mmio(kvm, vpci->mmio_addr, PCI_IO_SIZE, false,
-			       virtio_pci__io_mmio_callback, vpci);
+			       virtio_pci__io_mmio_callback, vdev);
 	if (r < 0)
 		goto free_ioport;
 
 	vpci->msix_io_block = pci_get_io_space_block(PCI_IO_SIZE * 2);
 	r = kvm__register_mmio(kvm, vpci->msix_io_block, PCI_IO_SIZE * 2, false,
-			       virtio_pci__msix_mmio_callback, vpci);
+			       virtio_pci__msix_mmio_callback, vdev);
 	if (r < 0)
 		goto free_mmio;
 
-- 
1.9.1

[PATCH kvmtool 15/16] virtio/pci: update virtio mapping when PCI BARs are reconfigured

[Julien Thierry]

Software can change the addresses of PCI BARs. In the case of virtio, the
BARs are associated with some IO ports or mmio regions. These are not
updated when the guest modifies PCI BARs, leading to some surprises.

Re-register the ports and mmio regions related to PCI BARs when they are
updated.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 include/kvm/virtio-pci.h |   1 +
 virtio/pci.c             | 153 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 144 insertions(+), 10 deletions(-)

diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
index b70cadd..37ffe02 100644
--- a/include/kvm/virtio-pci.h
+++ b/include/kvm/virtio-pci.h
@@ -23,6 +23,7 @@ struct virtio_pci {
 	struct device_header	dev_hdr;
 	void			*dev;
 	struct kvm		*kvm;
+	struct virtio_device	*vdev;
 
 	u16			port_addr;
 	u32			mmio_addr;
diff --git a/virtio/pci.c b/virtio/pci.c
index 32f9824..1275d82 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -434,6 +434,132 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
 				     data, len);
 }
 
+static inline int virtio_pci__register_io(struct kvm *kvm,
+					  struct virtio_pci *vpci)
+{
+	int r;
+
+	r = ioport__register(kvm, vpci->port_addr, &virtio_pci__io_ops,
+			     PCI_IO_SIZE, vpci->vdev);
+	if (r < 0)
+		pr_warning("failed to register io port virtio_pci bar[0]: 0x%x, err: %d\n",
+			   (u32) vpci->port_addr, r);
+
+	return r;
+}
+
+static inline int virtio_pci__register_mmio(struct kvm *kvm,
+					    struct virtio_pci *vpci)
+{
+	int r;
+
+	r = kvm__register_mmio(kvm, vpci->mmio_addr, PCI_IO_SIZE, false,
+			       virtio_pci__io_mmio_callback, vpci->vdev);
+	if (r < 0)
+		pr_warning("failed to register mmio virtio_pci bar[1]: 0x%x, err: %d\n",
+			   vpci->mmio_addr, r);
+
+	return r;
+}
+
+static inline int virtio_pci__register_msix(struct kvm *kvm,
+					    struct virtio_pci *vpci)
+{
+	int r;
+
+	r = kvm__register_mmio(kvm, vpci->msix_io_block,
+			       PCI_IO_SIZE * 2, false,
+			       virtio_pci__msix_mmio_callback,
+			       vpci->vdev);
+	if (r < 0)
+		pr_warning("failed to register mmio virtio_pci bar[2]: 0x%x, err: %d\n",
+			   vpci->msix_io_block, r);
+
+	return r;
+}
+
+static void virtio_pci__config_write(struct kvm *kvm,
+				     struct pci_device_header *pci_hdr,
+				     u8 offset, void *data, int sz)
+{
+	struct virtio_pci *vpci;
+
+	vpci = container_of(pci_hdr, struct virtio_pci, pci_hdr);
+
+	switch (offset) {
+	case PCI_COMMAND:
+	{
+		u16 cmd;
+
+		if (sz != 2)
+			die("unsupported size for pci command access");
+
+		cmd = ioport__read16(data);
+
+		/* Enable I/O response? */
+		if (cmd & PCI_COMMAND_IO
+		    && !(pci_hdr->command & PCI_COMMAND_IO))
+			virtio_pci__register_io(kvm, vpci);
+
+		/* Enable mmio response? */
+		if (cmd & PCI_COMMAND_MEMORY
+		    && !(pci_hdr->command & PCI_COMMAND_MEMORY)) {
+			virtio_pci__register_mmio(kvm, vpci);
+			virtio_pci__register_msix(kvm, vpci);
+		}
+
+		/* Disable mmio response? */
+		if (!(cmd & PCI_COMMAND_MEMORY)
+			   && pci_hdr->command & PCI_COMMAND_MEMORY) {
+			kvm__deregister_mmio(kvm, vpci->msix_io_block);
+			kvm__deregister_mmio(kvm, vpci->mmio_addr);
+		}
+
+		/* Disable I/O response? */
+		if (!(cmd & PCI_COMMAND_IO)
+		    && pci_hdr->command & PCI_COMMAND_IO)
+			ioport__unregister(kvm, vpci->port_addr);
+
+		break;
+	}
+	case PCI_BASE_ADDRESS_0:
+		if (sz != 4)
+			die("unsupported size for pci bar[0] access");
+
+		if (pci_hdr->command & PCI_COMMAND_IO)
+			ioport__unregister(kvm, vpci->port_addr);
+
+		vpci->port_addr = ioport__read32(data) & 0xFFFF;
+
+		vpci->port_addr &= PCI_BASE_ADDRESS_IO_MASK;
+
+		if (pci_hdr->command & PCI_COMMAND_IO)
+			virtio_pci__register_io(kvm, vpci);
+		break;
+	case PCI_BASE_ADDRESS_1:
+		if (pci_hdr->command & PCI_COMMAND_MEMORY)
+			kvm__deregister_mmio(kvm, vpci->mmio_addr);
+
+		vpci->mmio_addr = ioport__read32(data) & PCI_BASE_ADDRESS_MEM_MASK;
+
+		if (pci_hdr->command & PCI_COMMAND_MEMORY)
+			virtio_pci__register_mmio(kvm, vpci);
+		break;
+	case PCI_BASE_ADDRESS_2:
+		if (pci_hdr->command & PCI_COMMAND_MEMORY)
+			kvm__deregister_mmio(kvm, vpci->msix_io_block);
+
+		vpci->msix_io_block = ioport__read32(data) & PCI_BASE_ADDRESS_MEM_MASK;
+
+		if (pci_hdr->command & PCI_COMMAND_MEMORY)
+			virtio_pci__register_msix(kvm, vpci);
+		break;
+	default:
+		/* Default PCI config code is enough */
+		break;
+	}
+}
+
 int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 		     int device_id, int subsys_id, int class)
 {
@@ -442,22 +568,20 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 
 	vpci->kvm = kvm;
 	vpci->dev = dev;
+	vpci->vdev = vdev;
 
-	r = pci_get_io_port_block(PCI_IO_SIZE);
-	r = ioport__register(kvm, r, &virtio_pci__io_ops, PCI_IO_SIZE, vdev);
+	vpci->port_addr = pci_get_io_port_block(PCI_IO_SIZE);
+	r = virtio_pci__register_io(kvm, vpci);
 	if (r < 0)
 		return r;
-	vpci->port_addr = (u16)r;
 
 	vpci->mmio_addr = pci_get_io_space_block(PCI_IO_SIZE);
-	r = kvm__register_mmio(kvm, vpci->mmio_addr, PCI_IO_SIZE, false,
-			       virtio_pci__io_mmio_callback, vdev);
+	r = virtio_pci__register_mmio(kvm, vpci);
 	if (r < 0)
 		goto free_ioport;
 
 	vpci->msix_io_block = pci_get_io_space_block(PCI_IO_SIZE * 2);
-	r = kvm__register_mmio(kvm, vpci->msix_io_block, PCI_IO_SIZE * 2, false,
-			       virtio_pci__msix_mmio_callback, vdev);
+	r = virtio_pci__register_msix(kvm, vpci);
 	if (r < 0)
 		goto free_mmio;
 
@@ -485,6 +609,10 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 		.bar_size[2]		= cpu_to_le32(PCI_IO_SIZE*2),
 	};
 
+	vpci->pci_hdr.cfg_ops = (struct pci_config_operations) {
+		.write	= virtio_pci__config_write,
+	};
+
 	vpci->dev_hdr = (struct device_header) {
 		.bus_type		= DEVICE_BUS_PCI,
 		.data			= &vpci->pci_hdr,
@@ -534,11 +662,16 @@ free_ioport:
 int virtio_pci__exit(struct kvm *kvm, struct virtio_device *vdev)
 {
 	struct virtio_pci *vpci = vdev->virtio;
+	struct pci_device_header *pci_hdr = &vpci->pci_hdr;
 	int i;
 
-	kvm__deregister_mmio(kvm, vpci->mmio_addr);
-	kvm__deregister_mmio(kvm, vpci->msix_io_block);
-	ioport__unregister(kvm, vpci->port_addr);
+	if (pci_hdr->command & PCI_COMMAND_MEMORY) {
+		kvm__deregister_mmio(kvm, vpci->mmio_addr);
+		kvm__deregister_mmio(kvm, vpci->msix_io_block);
+	}
+
+	if (pci_hdr->command & PCI_COMMAND_IO)
+		ioport__unregister(kvm, vpci->port_addr);
 
 	for (i = 0; i < VIRTIO_PCI_MAX_VQ; i++) {
 		ioeventfd__del_event(vpci->port_addr + VIRTIO_PCI_QUEUE_NOTIFY, i);
-- 
1.9.1

[PATCH kvmtool 16/16] arm/fdt: Remove PCI probe only property

[Julien Thierry]

PCI devices support BAR reassignment. Get rid of the no longer needed
linux property.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arm/fdt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 980015b..219248e 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -140,7 +140,6 @@ static int setup_fdt(struct kvm *kvm)
 
 	/* /chosen */
 	_FDT(fdt_begin_node(fdt, "chosen"));
-	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
 	_FDT(fdt_property_string(fdt, "bootargs", kvm->cfg.real_cmdline));
 	_FDT(fdt_property_u64(fdt, "kaslr-seed", kvm->cfg.arch.kaslr_seed));
 
-- 
1.9.1

Re: [PATCH kvmtool 00/16] Support PCI BAR configuration

[Will Deacon]

On Thu, Mar 07, 2019 at 08:36:01AM +0000, Julien Thierry wrote:
> This series add support for guests writting to PCI BARs. Edk2 does this
> and is not aware of the "linux,pci-probe-only" property in the chosen node.
> 
> - Patches 1 to 3 do miscelaneous fixes

I've picked up these three fixes, but it looks like there are some
outstanding comments from Andre on some of the later patches.

Will

[PATCH kvmtool 00/16] Add writable BARs and PCIE 1.1 support

[Alexandru Elisei]

kvmtool uses the Linux-only dt property 'linux,pci-probe-only' to prevent
it from trying to reprogram the BARs. Let's make the BARs writable so we
can get rid of this band-aid.

Let's also extend the legacy PCI emulation, which came out in 1992, so we
can properly emulate the PCI Express version 1.1 protocol, which is
relatively newer, being published in 2005.

With these two changes, we are very close to running EDK2 as the firmware
for the virtual machine; the only thing that is missing is flash emulation
for storing firmware variables.

Summary of the patches:
* Patches 1-12 are fixes or enhancements needed to support reprogramable
  BARs.
* Patches 13-15 add support for reprogramable BARs and remove the dt
  property.
* Patch 16 adds support for PCIE 1.1.

Based on the series at [1].

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2019-March/034964.html

Alexandru Elisei (8):
  Makefile: Use correct objcopy binary when cross-compiling for x86_64
  Remove pci-shmem device
  Check that a PCI device's memory size is power of two
  arm: pci.c: Advertise only PCI bus 0 in the DT
  virtio/pci: Ignore MMIO and I/O accesses when they are disabled
  Use independent read/write locks for ioport and mmio
  virtio/pci: Add support for BAR configuration
  Add PCI Express 1.1 support

Julien Thierry (7):
  ioport: pci: Move port allocations to PCI devices
  pci: Fix ioport allocation size
  arm/pci: Fix PCI IO region
  arm/pci: Do not use first PCI IO space bytes for devices
  virtio/pci: Make memory and IO BARs independent
  vfio: Add support for BAR configuration
  arm/fdt: Remove 'linux,pci-probe-only' property

Sami Mujawar (1):
  pci: Fix BAR resource sizing arbitration

 Makefile                          |   4 +-
 arm/fdt.c                         |   1 -
 arm/include/arm-common/kvm-arch.h |   2 +-
 arm/include/arm-common/pci.h      |   1 +
 arm/kvm.c                         |   3 +
 arm/pci.c                         |  28 ++-
 builtin-run.c                     |   5 -
 hw/pci-shmem.c                    | 400 ------------------------------
 hw/vesa.c                         |  21 +-
 include/kvm/ioport.h              |   4 -
 include/kvm/pci-shmem.h           |  32 ---
 include/kvm/pci.h                 |  61 ++++-
 include/kvm/util.h                |   2 +
 include/kvm/vesa.h                |   6 +-
 include/kvm/virtio-pci.h          |   1 +
 ioport.c                          |  36 +--
 mmio.c                            |  26 +-
 pci.c                             |  64 ++++-
 powerpc/include/kvm/kvm-arch.h    |   2 +-
 vfio/core.c                       |   6 +-
 vfio/pci.c                        |  97 +++++++-
 virtio/pci.c                      | 355 ++++++++++++++++++++------
 x86/include/kvm/kvm-arch.h        |   2 +-
 23 files changed, 562 insertions(+), 597 deletions(-)
 delete mode 100644 hw/pci-shmem.c
 delete mode 100644 include/kvm/pci-shmem.h

-- 
2.20.1

[PATCH kvmtool 13/16] vfio: Add support for BAR configuration

[Alexandru Elisei]

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

When a guest can reassign BARs, kvmtool needs to maintain the vfio_region
consistent with their corresponding BARs. Take the new updated addresses
from the PCI header read back from the vfio driver.

Also, to modify the BARs, it is expected that guests will disable
IO/Memory response in the PCI command. Support this by mapping/unmapping
regions when the corresponding response gets enabled/disabled.

Cc: julien.thierry.kdev@gmail.com
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
[Fixed BAR selection]
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 vfio/core.c |  8 ++---
 vfio/pci.c  | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/vfio/core.c b/vfio/core.c
index 0ed1e6fee6bf..b554897fc8c1 100644
--- a/vfio/core.c
+++ b/vfio/core.c
@@ -202,14 +202,13 @@ static int vfio_setup_trap_region(struct kvm *kvm, struct vfio_device *vdev,
 				  struct vfio_region *region)
 {
 	if (region->is_ioport) {
-		int port = pci_get_io_port_block(region->info.size);
+		int port = ioport__register(kvm, region->port_base,
+					    &vfio_ioport_ops,
+					    region->info.size, region);
 
-		port = ioport__register(kvm, port, &vfio_ioport_ops,
-					region->info.size, region);
 		if (port < 0)
 			return port;
 
-		region->port_base = port;
 		return 0;
 	}
 
@@ -258,6 +257,7 @@ void vfio_unmap_region(struct kvm *kvm, struct vfio_region *region)
 {
 	if (region->host_addr) {
 		munmap(region->host_addr, region->info.size);
+		region->host_addr = NULL;
 	} else if (region->is_ioport) {
 		ioport__unregister(kvm, region->port_base);
 	} else {
diff --git a/vfio/pci.c b/vfio/pci.c
index bc5a6d452f7a..28f895c06b27 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -1,3 +1,4 @@
+#include "kvm/ioport.h"
 #include "kvm/irq.h"
 #include "kvm/kvm.h"
 #include "kvm/kvm-cpu.h"
@@ -464,6 +465,67 @@ static void vfio_pci_cfg_read(struct kvm *kvm, struct pci_device_header *pci_hdr
 			      sz, offset);
 }
 
+static void vfio_pci_cfg_handle_command(struct kvm *kvm, struct vfio_device *vdev,
+					void *data, int sz)
+{
+	struct pci_device_header *hdr = &vdev->pci.hdr;
+	bool toggle_io;
+	bool toggle_mem;
+	u16 cmd;
+	int i;
+
+	cmd = ioport__read16(data);
+	toggle_io = !!((cmd ^ hdr->command) & PCI_COMMAND_IO);
+	toggle_mem = !!((cmd ^ hdr->command) & PCI_COMMAND_MEMORY);
+
+	for (i = VFIO_PCI_BAR0_REGION_INDEX; i <= VFIO_PCI_BAR5_REGION_INDEX; ++i) {
+		struct vfio_region *region = &vdev->regions[i];
+
+		if (region->is_ioport && toggle_io) {
+			if (cmd & PCI_COMMAND_IO)
+				vfio_map_region(kvm, vdev, region);
+			else
+				vfio_unmap_region(kvm, region);
+		}
+
+		if (!region->is_ioport && toggle_mem) {
+			if (cmd & PCI_COMMAND_MEMORY)
+				vfio_map_region(kvm, vdev, region);
+			else
+				vfio_unmap_region(kvm, region);
+		}
+	}
+}
+
+static void vfio_pci_cfg_update_bar(struct kvm *kvm, struct vfio_device *vdev,
+				    int bar_num, void *data, int sz)
+{
+	struct pci_device_header *hdr = &vdev->pci.hdr;
+	struct vfio_region *region;
+	uint32_t bar;
+
+	region = &vdev->regions[bar_num + VFIO_PCI_BAR0_REGION_INDEX];
+	bar = ioport__read32(data);
+
+	if (region->is_ioport) {
+		if (hdr->command & PCI_COMMAND_IO)
+			vfio_unmap_region(kvm, region);
+
+		region->port_base = bar & PCI_BASE_ADDRESS_IO_MASK;
+
+		if (hdr->command & PCI_COMMAND_IO)
+			vfio_map_region(kvm, vdev, region);
+	} else {
+		if (hdr->command & PCI_COMMAND_MEMORY)
+			vfio_unmap_region(kvm, region);
+
+		region->guest_phys_addr = bar & PCI_BASE_ADDRESS_MEM_MASK;
+
+		if (hdr->command & PCI_COMMAND_MEMORY)
+			vfio_map_region(kvm, vdev, region);
+	}
+}
+
 static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hdr,
 			       u8 offset, void *data, int sz)
 {
@@ -471,6 +533,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
 	struct vfio_pci_device *pdev;
 	struct vfio_device *vdev;
 	void *base = pci_hdr;
+	int bar_num;
 
 	pdev = container_of(pci_hdr, struct vfio_pci_device, hdr);
 	vdev = container_of(pdev, struct vfio_device, pci);
@@ -487,9 +550,17 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
 	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI)
 		vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz);
 
+	if (offset == PCI_COMMAND)
+		vfio_pci_cfg_handle_command(kvm, vdev, data, sz);
+
 	if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz)
 		vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x",
 			      sz, offset);
+
+	if (offset >= PCI_BASE_ADDRESS_0 && offset <= PCI_BASE_ADDRESS_5) {
+		bar_num = (offset - PCI_BASE_ADDRESS_0) / sizeof(u32);
+		vfio_pci_cfg_update_bar(kvm, vdev, bar_num, data, sz);
+	}
 }
 
 static ssize_t vfio_pci_msi_cap_size(struct msi_cap_64 *cap_hdr)
@@ -808,6 +879,7 @@ static int vfio_pci_configure_bar(struct kvm *kvm, struct vfio_device *vdev,
 	size_t map_size;
 	struct vfio_pci_device *pdev = &vdev->pci;
 	struct vfio_region *region = &vdev->regions[nr];
+	bool map_now;
 
 	if (nr >= vdev->info.num_regions)
 		return 0;
@@ -848,16 +920,22 @@ static int vfio_pci_configure_bar(struct kvm *kvm, struct vfio_device *vdev,
 		}
 	}
 
-	if (!region->is_ioport) {
+	if (region->is_ioport) {
+		region->port_base = pci_get_io_port_block(region->info.size);
+		map_now = !!(pdev->hdr.command & PCI_COMMAND_IO);
+	} else {
 		/* Grab some MMIO space in the guest */
 		map_size = ALIGN(region->info.size, PAGE_SIZE);
 		region->guest_phys_addr = pci_get_mmio_block(map_size);
+		map_now = !!(pdev->hdr.command & PCI_COMMAND_MEMORY);
 	}
 
-	/* Map the BARs into the guest or setup a trap region. */
-	ret = vfio_map_region(kvm, vdev, region);
-	if (ret)
-		return ret;
+	if (map_now) {
+		/* Map the BARs into the guest or setup a trap region. */
+		ret = vfio_map_region(kvm, vdev, region);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH kvmtool 13/16] vfio: Add support for BAR configuration

[Alexandru Elisei]

Hi,

On 11/25/19 10:30 AM, Alexandru Elisei wrote:
> From: Julien Thierry <julien.thierry@arm.com>
>
> When a guest can reassign BARs, kvmtool needs to maintain the vfio_region
> consistent with their corresponding BARs. Take the new updated addresses
> from the PCI header read back from the vfio driver.
>
> Also, to modify the BARs, it is expected that guests will disable
> IO/Memory response in the PCI command. Support this by mapping/unmapping
> regions when the corresponding response gets enabled/disabled.
>
> Cc: julien.thierry.kdev@gmail.com
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> [Fixed BAR selection]
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  vfio/core.c |  8 ++---
>  vfio/pci.c  | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 87 insertions(+), 9 deletions(-)

I've finally had the chance to do more testing for PCI passthrough, and this patch
is pretty broken, so far I've found that: kvmtool does trap-and-emulate for the
BAR(s) dedicated to the MSI-X table and MSI-X PBA structure, and we don't take
that into account; vfio_unmap_region doesn't destroy the memslot; when the guest
enables memory or I/O accesses, we call vfio_map_region for all 6 BARs, even
though some of them might be unimplemented (their value is 0).

Thanks,
Alex
> diff --git a/vfio/core.c b/vfio/core.c
> index 0ed1e6fee6bf..b554897fc8c1 100644
> --- a/vfio/core.c
> +++ b/vfio/core.c
> @@ -202,14 +202,13 @@ static int vfio_setup_trap_region(struct kvm *kvm, struct vfio_device *vdev,
>  				  struct vfio_region *region)
>  {
>  	if (region->is_ioport) {
> -		int port = pci_get_io_port_block(region->info.size);
> +		int port = ioport__register(kvm, region->port_base,
> +					    &vfio_ioport_ops,
> +					    region->info.size, region);
>  
> -		port = ioport__register(kvm, port, &vfio_ioport_ops,
> -					region->info.size, region);
>  		if (port < 0)
>  			return port;
>  
> -		region->port_base = port;
>  		return 0;
>  	}
>  
> @@ -258,6 +257,7 @@ void vfio_unmap_region(struct kvm *kvm, struct vfio_region *region)
>  {
>  	if (region->host_addr) {
>  		munmap(region->host_addr, region->info.size);
> +		region->host_addr = NULL;
>  	} else if (region->is_ioport) {
>  		ioport__unregister(kvm, region->port_base);
>  	} else {
> diff --git a/vfio/pci.c b/vfio/pci.c
> index bc5a6d452f7a..28f895c06b27 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -1,3 +1,4 @@
> +#include "kvm/ioport.h"
>  #include "kvm/irq.h"
>  #include "kvm/kvm.h"
>  #include "kvm/kvm-cpu.h"
> @@ -464,6 +465,67 @@ static void vfio_pci_cfg_read(struct kvm *kvm, struct pci_device_header *pci_hdr
>  			      sz, offset);
>  }
>  
> +static void vfio_pci_cfg_handle_command(struct kvm *kvm, struct vfio_device *vdev,
> +					void *data, int sz)
> +{
> +	struct pci_device_header *hdr = &vdev->pci.hdr;
> +	bool toggle_io;
> +	bool toggle_mem;
> +	u16 cmd;
> +	int i;
> +
> +	cmd = ioport__read16(data);
> +	toggle_io = !!((cmd ^ hdr->command) & PCI_COMMAND_IO);
> +	toggle_mem = !!((cmd ^ hdr->command) & PCI_COMMAND_MEMORY);
> +
> +	for (i = VFIO_PCI_BAR0_REGION_INDEX; i <= VFIO_PCI_BAR5_REGION_INDEX; ++i) {
> +		struct vfio_region *region = &vdev->regions[i];
> +
> +		if (region->is_ioport && toggle_io) {
> +			if (cmd & PCI_COMMAND_IO)
> +				vfio_map_region(kvm, vdev, region);
> +			else
> +				vfio_unmap_region(kvm, region);
> +		}
> +
> +		if (!region->is_ioport && toggle_mem) {
> +			if (cmd & PCI_COMMAND_MEMORY)
> +				vfio_map_region(kvm, vdev, region);
> +			else
> +				vfio_unmap_region(kvm, region);
> +		}
> +	}
> +}
> +
> +static void vfio_pci_cfg_update_bar(struct kvm *kvm, struct vfio_device *vdev,
> +				    int bar_num, void *data, int sz)
> +{
> +	struct pci_device_header *hdr = &vdev->pci.hdr;
> +	struct vfio_region *region;
> +	uint32_t bar;
> +
> +	region = &vdev->regions[bar_num + VFIO_PCI_BAR0_REGION_INDEX];
> +	bar = ioport__read32(data);
> +
> +	if (region->is_ioport) {
> +		if (hdr->command & PCI_COMMAND_IO)
> +			vfio_unmap_region(kvm, region);
> +
> +		region->port_base = bar & PCI_BASE_ADDRESS_IO_MASK;
> +
> +		if (hdr->command & PCI_COMMAND_IO)
> +			vfio_map_region(kvm, vdev, region);
> +	} else {
> +		if (hdr->command & PCI_COMMAND_MEMORY)
> +			vfio_unmap_region(kvm, region);
> +
> +		region->guest_phys_addr = bar & PCI_BASE_ADDRESS_MEM_MASK;
> +
> +		if (hdr->command & PCI_COMMAND_MEMORY)
> +			vfio_map_region(kvm, vdev, region);
> +	}
> +}
> +
>  static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hdr,
>  			       u8 offset, void *data, int sz)
>  {
> @@ -471,6 +533,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
>  	struct vfio_pci_device *pdev;
>  	struct vfio_device *vdev;
>  	void *base = pci_hdr;
> +	int bar_num;
>  
>  	pdev = container_of(pci_hdr, struct vfio_pci_device, hdr);
>  	vdev = container_of(pdev, struct vfio_device, pci);
> @@ -487,9 +550,17 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
>  	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI)
>  		vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz);
>  
> +	if (offset == PCI_COMMAND)
> +		vfio_pci_cfg_handle_command(kvm, vdev, data, sz);
> +
>  	if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz)
>  		vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x",
>  			      sz, offset);
> +
> +	if (offset >= PCI_BASE_ADDRESS_0 && offset <= PCI_BASE_ADDRESS_5) {
> +		bar_num = (offset - PCI_BASE_ADDRESS_0) / sizeof(u32);
> +		vfio_pci_cfg_update_bar(kvm, vdev, bar_num, data, sz);
> +	}
>  }
>  
>  static ssize_t vfio_pci_msi_cap_size(struct msi_cap_64 *cap_hdr)
> @@ -808,6 +879,7 @@ static int vfio_pci_configure_bar(struct kvm *kvm, struct vfio_device *vdev,
>  	size_t map_size;
>  	struct vfio_pci_device *pdev = &vdev->pci;
>  	struct vfio_region *region = &vdev->regions[nr];
> +	bool map_now;
>  
>  	if (nr >= vdev->info.num_regions)
>  		return 0;
> @@ -848,16 +920,22 @@ static int vfio_pci_configure_bar(struct kvm *kvm, struct vfio_device *vdev,
>  		}
>  	}
>  
> -	if (!region->is_ioport) {
> +	if (region->is_ioport) {
> +		region->port_base = pci_get_io_port_block(region->info.size);
> +		map_now = !!(pdev->hdr.command & PCI_COMMAND_IO);
> +	} else {
>  		/* Grab some MMIO space in the guest */
>  		map_size = ALIGN(region->info.size, PAGE_SIZE);
>  		region->guest_phys_addr = pci_get_mmio_block(map_size);
> +		map_now = !!(pdev->hdr.command & PCI_COMMAND_MEMORY);
>  	}
>  
> -	/* Map the BARs into the guest or setup a trap region. */
> -	ret = vfio_map_region(kvm, vdev, region);
> -	if (ret)
> -		return ret;
> +	if (map_now) {
> +		/* Map the BARs into the guest or setup a trap region. */
> +		ret = vfio_map_region(kvm, vdev, region);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }