On 26/03/2020 15:24, Alexandru Elisei wrote:
Hi Will, Alex,
> kvmtool uses the Linux-only dt property 'linux,pci-probe-only' to prevent
> it from trying to reassign the BARs. Let's make the BARs reassignable 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 new in
> comparison, being published in 2005.
>
> During device configuration, Linux can assign a region resource to a BAR
> that temporarily overlaps with another device. This means that the code
> that emulates BAR reassignment must be aware of all the registered PCI
> devices. Because of this, and to avoid re-implementing the same code for
> each emulated device, the algorithm for activating/deactivating emulation
> as BAR addresses change lives completely inside the PCI code. Each device
> registers two callback functions which are called when device emulation is
> activated (for example, to activate emulation for a newly assigned BAR
> address), respectively, when device emulation is deactivated (a previous
> BAR address is changed, and emulation for that region must be deactivated).
[ ... ]
>
> Patches 1-18 are fixes and cleanups, and can be merged independently. They
> are pretty straightforward, so if the size of the series looks off-putting,
> please review these first. I am aware that the series has grown quite a
> lot, I am willing to split the fixes from the rest of the patches, or
> whatever else can make reviewing easier.
I am done with the review of this series. I had only one small comment
on the first 18 patches, so I would suggest that Alex sends a new
version of only those fix&cleanup patches, so we can fix those bugs and
also make the rest of the series easier to digest.
In general this series looks to be in a really good shape, I don't see
anything major anymore.
Alex, many thanks for the accurate and tedious work on this, surely not
the most grateful of tasks.
Cheers,
Andre.
>
> Changes in v3:
> * Patches 18, 24 and 26 are new.
> * Dropped #9 "arm/pci: Fix PCI IO region" for reasons explained above.
> * Reworded commit message for #12 "vfio/pci: Ignore expansion ROM BAR
> writes" to make it clear that kvmtool's implementation of VFIO doesn't
> support expansion BAR emulation.
> * Moved variable declaration at the start of the function for #13
> "vfio/pci: Don't access unallocated regions".
> * Patch #15 "Don't ignore errors registering a device, ioport or mmio
> emulation" uses ioport__remove consistenly.
> * Reworked error cleanup for #16 "hw/vesa: Don't ignore fatal errors".
> * Reworded commit message for #17 "hw/vesa: Set the size for BAR 0".
> * Reworked #19 "ioport: mmio: Use a mutex and reference counting for
> locking" to also use reference counting to prevent races (and updated the
> commit message in the process).
> * Added the function pci__bar_size to #20 "pci: Add helpers for BAR values
> and memory/IO space access".
> * Protect mem_banks list with a mutex in #22 "vfio: Destroy memslot when
> unmapping the associated VAs"; also moved the munmap after the slot is
> destroyed, as per the KVM api.
> * Dropped the rework of the vesa device in patch #27 "pci: Implement
> callbacks for toggling BAR emulation". Also added a few assert statements
> in some callbacks to make sure that they don't get called with an
> unxpected BAR number (which can only result from a coding error). Also
> return -ENOENT when kvm__deregister_mmio fails, to match ioport behavior
> and for better diagnostics.
> * Dropped the PCI configuration write callback from the vesa device in #28
> "pci: Toggle BAR I/O and memory space emulation". Apparently, if we don't
> allow the guest kernel to disable BAR access, it treats the VESA device
> as a VGA boot device, instead of a regular VGA device, and Xorg cannot
> use it.
> * Droped struct bar_info from #29 "pci: Implement reassignable BARs". Also
> changed pci_dev_err to pci_dev_warn in pci_{activate,deactivate}_bar,
> because the errors can be benign (they can happen because two vcpus race
> against each other to deactivate memory or I/O access, for example).
> * Replaced the PCI device configuration space define with the actual
> number in #32 "arm/arm64: Add PCI Express 1.1 support". On some distros
> the defines weren't present in /usr/include/linux/pci_regs.h.
> * Implemented other minor review comments.
> * Gathered Reviewed-by tags.
>
> Changes in v2:
> * Patches 2, 11-18, 20, 22-27, 29 are new.
> * Patches 11, 13, and 14 have been dropped.
> * Reworked the way BAR reassignment is implemented.
> * The patch "Add PCI Express 1.1 support" has been reworked to apply only
> to arm64. For x86 we would need ACPI support in order to advertise the
> location of the ECAM space.
> * Gathered Reviewed-by tags.
> * Implemented review comments.
>
> [1] https://www.spinics.net/lists/kvm/msg209178.html
> [2] https://www.spinics.net/lists/kvm/msg209178.html
> [3] https://www.spinics.net/lists/arm-kernel/msg778623.html
>
>
> Alexandru Elisei (27):
> Makefile: Use correct objcopy binary when cross-compiling for x86_64
> hw/i8042: Compile only for x86
> Remove pci-shmem device
> Check that a PCI device's memory size is power of two
> arm/pci: Advertise only PCI bus 0 in the DT
> vfio/pci: Allocate correct size for MSIX table and PBA BARs
> vfio/pci: Don't assume that only even numbered BARs are 64bit
> vfio/pci: Ignore expansion ROM BAR writes
> vfio/pci: Don't access unallocated regions
> virtio: Don't ignore initialization failures
> Don't ignore errors registering a device, ioport or mmio emulation
> hw/vesa: Don't ignore fatal errors
> hw/vesa: Set the size for BAR 0
> ioport: Fail when registering overlapping ports
> ioport: mmio: Use a mutex and reference counting for locking
> pci: Add helpers for BAR values and memory/IO space access
> virtio/pci: Get emulated region address from BARs
> vfio: Destroy memslot when unmapping the associated VAs
> vfio: Reserve ioports when configuring the BAR
> pci: Limit configuration transaction size to 32 bits
> vfio/pci: Don't write configuration value twice
> vesa: Create device exactly once
> pci: Implement callbacks for toggling BAR emulation
> pci: Toggle BAR I/O and memory space emulation
> pci: Implement reassignable BARs
> vfio: Trap MMIO access to BAR addresses which aren't page aligned
> arm/arm64: Add PCI Express 1.1 support
>
> Julien Thierry (4):
> ioport: pci: Move port allocations to PCI devices
> pci: Fix ioport allocation size
> virtio/pci: Make memory and IO BARs independent
> arm/fdt: Remove 'linux,pci-probe-only' property
>
> Sami Mujawar (1):
> pci: Fix BAR resource sizing arbitration
>
> Makefile | 6 +-
> arm/fdt.c | 1 -
> arm/include/arm-common/kvm-arch.h | 4 +-
> arm/ioport.c | 3 +-
> arm/pci.c | 4 +-
> builtin-run.c | 6 +-
> hw/i8042.c | 14 +-
> hw/pci-shmem.c | 400 ------------------------------
> hw/vesa.c | 113 ++++++---
> include/kvm/devices.h | 3 +-
> include/kvm/ioport.h | 12 +-
> include/kvm/kvm-config.h | 2 +-
> include/kvm/kvm.h | 11 +-
> include/kvm/pci-shmem.h | 32 ---
> include/kvm/pci.h | 163 +++++++++++-
> include/kvm/rbtree-interval.h | 4 +-
> include/kvm/util.h | 2 +
> include/kvm/vesa.h | 6 +-
> include/kvm/virtio-pci.h | 3 -
> include/kvm/virtio.h | 7 +-
> include/linux/compiler.h | 2 +-
> ioport.c | 108 ++++----
> kvm.c | 101 +++++++-
> mips/kvm.c | 3 +-
> mmio.c | 91 +++++--
> pci.c | 334 +++++++++++++++++++++++--
> powerpc/include/kvm/kvm-arch.h | 2 +-
> powerpc/ioport.c | 3 +-
> powerpc/spapr_pci.c | 2 +-
> vfio/core.c | 22 +-
> vfio/pci.c | 233 +++++++++++++----
> virtio/9p.c | 9 +-
> virtio/balloon.c | 10 +-
> virtio/blk.c | 14 +-
> virtio/console.c | 11 +-
> virtio/core.c | 9 +-
> virtio/mmio.c | 13 +-
> virtio/net.c | 32 +--
> virtio/pci.c | 206 ++++++++++-----
> virtio/scsi.c | 14 +-
> x86/include/kvm/kvm-arch.h | 2 +-
> x86/ioport.c | 66 +++--
> 42 files changed, 1287 insertions(+), 796 deletions(-)
> delete mode 100644 hw/pci-shmem.c
> delete mode 100644 include/kvm/pci-shmem.h
>
