From 62d02fd1f807bf5a259a242c483c9fb98a242630 Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Mon, 26 Jun 2017 15:20:49 +0800 Subject: drm/i915/gvt: Fix possible recursive locking issue vfio_unpin_pages will hold a read semaphore however it is already hold in the same thread by vfio ioctl. It will cause below warning: [ 5102.127454] ============================================ [ 5102.133379] WARNING: possible recursive locking detected [ 5102.139304] 4.12.0-rc4+ #3 Not tainted [ 5102.143483] -------------------------------------------- [ 5102.149407] qemu-system-x86/1620 is trying to acquire lock: [ 5102.155624] (&container->group_lock){++++++}, at: [] vfio_unpin_pages+0x96/0xf0 [ 5102.165626] but task is already holding lock: [ 5102.172134] (&container->group_lock){++++++}, at: [] vfio_fops_unl_ioctl+0x5f/0x280 [ 5102.182522] other info that might help us debug this: [ 5102.189806] Possible unsafe locking scenario: [ 5102.196411] CPU0 [ 5102.199136] ---- [ 5102.201861] lock(&container->group_lock); [ 5102.206527] lock(&container->group_lock); [ 5102.211191] *** DEADLOCK *** [ 5102.217796] May be due to missing lock nesting notation [ 5102.225370] 3 locks held by qemu-system-x86/1620: [ 5102.230618] #0: (&container->group_lock){++++++}, at: [] vfio_fops_unl_ioctl+0x5f/0x280 [ 5102.241482] #1: (&(&iommu->notifier)->rwsem){++++..}, at: [] __blocking_notifier_call_chain+0x35/0x70 [ 5102.253713] #2: (&vgpu->vdev.cache_lock){+.+...}, at: [] intel_vgpu_iommu_notifier+0x77/0x120 [ 5102.265163] stack backtrace: [ 5102.270022] CPU: 5 PID: 1620 Comm: qemu-system-x86 Not tainted 4.12.0-rc4+ #3 [ 5102.277991] Hardware name: Intel Corporation S1200RP/S1200RP, BIOS S1200RP.86B.03.01.APER.061220151418 06/12/2015 [ 5102.289445] Call Trace: [ 5102.292175] dump_stack+0x85/0xc7 [ 5102.295871] validate_chain.isra.21+0x9da/0xaf0 [ 5102.300925] __lock_acquire+0x405/0x820 [ 5102.305202] lock_acquire+0xc7/0x220 [ 5102.309191] ? vfio_unpin_pages+0x96/0xf0 [ 5102.313666] down_read+0x2b/0x50 [ 5102.317259] ? vfio_unpin_pages+0x96/0xf0 [ 5102.321732] vfio_unpin_pages+0x96/0xf0 [ 5102.326024] intel_vgpu_iommu_notifier+0xe5/0x120 [ 5102.331283] notifier_call_chain+0x4a/0x70 [ 5102.335851] __blocking_notifier_call_chain+0x4d/0x70 [ 5102.341490] blocking_notifier_call_chain+0x16/0x20 [ 5102.346935] vfio_iommu_type1_ioctl+0x87b/0x920 [ 5102.351994] vfio_fops_unl_ioctl+0x81/0x280 [ 5102.356660] ? __fget+0xf0/0x210 [ 5102.360261] do_vfs_ioctl+0x93/0x6a0 [ 5102.364247] ? __fget+0x111/0x210 [ 5102.367942] SyS_ioctl+0x41/0x70 [ 5102.371542] entry_SYSCALL_64_fastpath+0x1f/0xbe put the vfio_unpin_pages in a workqueue can fix this. v2: - use for style instead of do{}while(1). (Zhenyu) v3: - rename gvt_cache_mark to gvt_cache_mark_remove. (Zhenyu) Fixes: 659643f7d814 ("drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT") Signed-off-by: Chuanxiao Dong Cc: Zhenyu Wang Cc: stable@vger.kernel.org # v4.10+ Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/gvt.h | 3 +++ drivers/gpu/drm/i915/gvt/kvmgt.c | 55 ++++++++++++++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index 930732e5c780..8b5876d63624 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -183,6 +183,9 @@ struct intel_vgpu { struct kvm *kvm; struct work_struct release_work; atomic_t released; + struct work_struct unpin_work; + spinlock_t unpin_lock; /* To protect unpin_list */ + struct list_head unpin_list; } vdev; #endif }; diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 1ae0b4083ce1..56a8d6a7d9c6 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -78,6 +78,7 @@ struct gvt_dma { struct rb_node node; gfn_t gfn; unsigned long iova; + struct list_head list; }; static inline bool handle_valid(unsigned long handle) @@ -166,6 +167,7 @@ static void gvt_cache_add(struct intel_vgpu *vgpu, gfn_t gfn, new->gfn = gfn; new->iova = iova; + INIT_LIST_HEAD(&new->list); mutex_lock(&vgpu->vdev.cache_lock); while (*link) { @@ -197,26 +199,52 @@ static void __gvt_cache_remove_entry(struct intel_vgpu *vgpu, kfree(entry); } -static void gvt_cache_remove(struct intel_vgpu *vgpu, gfn_t gfn) +static void intel_vgpu_unpin_work(struct work_struct *work) { + struct intel_vgpu *vgpu = container_of(work, struct intel_vgpu, + vdev.unpin_work); struct device *dev = mdev_dev(vgpu->vdev.mdev); struct gvt_dma *this; - unsigned long g1; - int rc; + unsigned long gfn; + + for (;;) { + spin_lock(&vgpu->vdev.unpin_lock); + if (list_empty(&vgpu->vdev.unpin_list)) { + spin_unlock(&vgpu->vdev.unpin_lock); + break; + } + this = list_first_entry(&vgpu->vdev.unpin_list, + struct gvt_dma, list); + list_del(&this->list); + spin_unlock(&vgpu->vdev.unpin_lock); + + gfn = this->gfn; + vfio_unpin_pages(dev, &gfn, 1); + kfree(this); + } +} + +static bool gvt_cache_mark_remove(struct intel_vgpu *vgpu, gfn_t gfn) +{ + struct gvt_dma *this; mutex_lock(&vgpu->vdev.cache_lock); this = __gvt_cache_find(vgpu, gfn); if (!this) { mutex_unlock(&vgpu->vdev.cache_lock); - return; + return false; } - - g1 = gfn; gvt_dma_unmap_iova(vgpu, this->iova); - rc = vfio_unpin_pages(dev, &g1, 1); - WARN_ON(rc != 1); - __gvt_cache_remove_entry(vgpu, this); + /* remove this from rb tree */ + rb_erase(&this->node, &vgpu->vdev.cache); mutex_unlock(&vgpu->vdev.cache_lock); + + /* put this to the unpin_list */ + spin_lock(&vgpu->vdev.unpin_lock); + list_move_tail(&this->list, &vgpu->vdev.unpin_list); + spin_unlock(&vgpu->vdev.unpin_lock); + + return true; } static void gvt_cache_init(struct intel_vgpu *vgpu) @@ -453,6 +481,9 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) } INIT_WORK(&vgpu->vdev.release_work, intel_vgpu_release_work); + INIT_WORK(&vgpu->vdev.unpin_work, intel_vgpu_unpin_work); + spin_lock_init(&vgpu->vdev.unpin_lock); + INIT_LIST_HEAD(&vgpu->vdev.unpin_list); vgpu->vdev.mdev = mdev; mdev_set_drvdata(mdev, vgpu); @@ -482,6 +513,7 @@ static int intel_vgpu_iommu_notifier(struct notifier_block *nb, struct intel_vgpu *vgpu = container_of(nb, struct intel_vgpu, vdev.iommu_notifier); + bool sched_unmap = false; if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) { struct vfio_iommu_type1_dma_unmap *unmap = data; @@ -491,7 +523,10 @@ static int intel_vgpu_iommu_notifier(struct notifier_block *nb, end_gfn = gfn + unmap->size / PAGE_SIZE; while (gfn < end_gfn) - gvt_cache_remove(vgpu, gfn++); + sched_unmap |= gvt_cache_mark_remove(vgpu, gfn++); + + if (sched_unmap) + schedule_work(&vgpu->vdev.unpin_work); } return NOTIFY_OK; -- cgit From f16bd3dda2c8bf6699e808cd9cc540cfab10e60e Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Mon, 26 Jun 2017 15:20:50 +0800 Subject: drm/i915/gvt: Fix inconsistent locks holding sequence There are two kinds of locking sequence. One is in the thread which is started by vfio ioctl to do the iommu unmapping. The locking sequence is: down_read(&group_lock) ----> mutex_lock(&cached_lock) The other is in the vfio release thread which will unpin all the cached pages. The lock sequence is: mutex_lock(&cached_lock) ---> down_read(&group_lock) And, the cache_lock is used to protect the rb tree of the cache node and doing vfio unpin doesn't require this lock. Move the vfio unpin out of the cache_lock protected region. v2: - use for style instead of do{}while(1). (Zhenyu) Fixes: f30437c5e7bf ("drm/i915/gvt: add KVMGT support") Signed-off-by: Chuanxiao Dong Cc: Zhenyu Wang Cc: stable@vger.kernel.org # v4.10+ Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/kvmgt.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 56a8d6a7d9c6..75a6e1d8af0d 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -260,16 +260,20 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu) struct device *dev = mdev_dev(vgpu->vdev.mdev); unsigned long gfn; - mutex_lock(&vgpu->vdev.cache_lock); - while ((node = rb_first(&vgpu->vdev.cache))) { + for (;;) { + mutex_lock(&vgpu->vdev.cache_lock); + node = rb_first(&vgpu->vdev.cache); + if (!node) { + mutex_unlock(&vgpu->vdev.cache_lock); + break; + } dma = rb_entry(node, struct gvt_dma, node); gvt_dma_unmap_iova(vgpu, dma->iova); gfn = dma->gfn; - - vfio_unpin_pages(dev, &gfn, 1); __gvt_cache_remove_entry(vgpu, dma); + mutex_unlock(&vgpu->vdev.cache_lock); + vfio_unpin_pages(dev, &gfn, 1); } - mutex_unlock(&vgpu->vdev.cache_lock); } static struct intel_vgpu_type *intel_gvt_find_vgpu_type(struct intel_gvt *gvt, -- cgit From 295a0d0b55269fce2290a7aebfcc8c2525866c33 Mon Sep 17 00:00:00 2001 From: Xiong Zhang Date: Tue, 20 Jun 2017 11:37:22 +0800 Subject: drm/i915/gvt: Set initial PORT_CLK_SEL vreg for BDW On BDW, when host physical screen and guest virtual screen aren't on the same DDI port, guest i915 driver prints the following error and stop running. [ 6.775873] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068 [ 6.775928] IP: intel_ddi_clock_get+0x81/0x430 [i915] [ 6.776206] Call Trace: [ 6.776233] ? vgpu_read32+0x4f/0x100 [i915] [ 6.776264] intel_ddi_get_config+0x11c/0x230 [i915] [ 6.776298] intel_modeset_setup_hw_state+0x313/0xd40 [i915] [ 6.776334] intel_modeset_init+0xe49/0x18d0 [i915] [ 6.776368] ? vgpu_write32+0x53/0x100 [i915] [ 6.776731] ? intel_i2c_reset+0x42/0x50 [i915] [ 6.777085] ? intel_setup_gmbus+0x32a/0x350 [i915] [ 6.777427] i915_driver_load+0xabc/0x14d0 [i915] [ 6.777768] i915_pci_probe+0x4f/0x70 [i915] The null pointer is guest intel_crtc_state->shared_dpll which is setted in haswell_get_ddi_pll(). When guest and host screen are on different DDI port, host driver won't set PORT_CLK_SET(guest_port), so haswell_get_ddi_pll() will return null and don't set pipe_config->shared_dpll, once the following program refernce this structure, it will print the above error. This patch set the initial val of guest PORT_CLK_SEL(guest_port) to LCPLL_810. And guest i915 driver will reset this value according to guest screen mode. Signed-off-by: Xiong Zhang Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/display.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c index e0261fcc5b50..818e94907c3b 100644 --- a/drivers/gpu/drm/i915/gvt/display.c +++ b/drivers/gpu/drm/i915/gvt/display.c @@ -197,6 +197,12 @@ static void emulate_monitor_status_change(struct intel_vgpu *vgpu) (TRANS_DDI_BPC_8 | TRANS_DDI_MODE_SELECT_DP_SST | (PORT_B << TRANS_DDI_PORT_SHIFT) | TRANS_DDI_FUNC_ENABLE); + if (IS_BROADWELL(dev_priv)) { + vgpu_vreg(vgpu, PORT_CLK_SEL(PORT_B)) &= + ~PORT_CLK_SEL_MASK; + vgpu_vreg(vgpu, PORT_CLK_SEL(PORT_B)) |= + PORT_CLK_SEL_LCPLL_810; + } vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_B)) |= DDI_BUF_CTL_ENABLE; vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_B)) &= ~DDI_BUF_IS_IDLE; vgpu_vreg(vgpu, SDEISR) |= SDE_PORTB_HOTPLUG_CPT; @@ -211,6 +217,12 @@ static void emulate_monitor_status_change(struct intel_vgpu *vgpu) (TRANS_DDI_BPC_8 | TRANS_DDI_MODE_SELECT_DP_SST | (PORT_C << TRANS_DDI_PORT_SHIFT) | TRANS_DDI_FUNC_ENABLE); + if (IS_BROADWELL(dev_priv)) { + vgpu_vreg(vgpu, PORT_CLK_SEL(PORT_C)) &= + ~PORT_CLK_SEL_MASK; + vgpu_vreg(vgpu, PORT_CLK_SEL(PORT_C)) |= + PORT_CLK_SEL_LCPLL_810; + } vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_C)) |= DDI_BUF_CTL_ENABLE; vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_C)) &= ~DDI_BUF_IS_IDLE; vgpu_vreg(vgpu, SFUSE_STRAP) |= SFUSE_STRAP_DDIC_DETECTED; @@ -225,6 +237,12 @@ static void emulate_monitor_status_change(struct intel_vgpu *vgpu) (TRANS_DDI_BPC_8 | TRANS_DDI_MODE_SELECT_DP_SST | (PORT_D << TRANS_DDI_PORT_SHIFT) | TRANS_DDI_FUNC_ENABLE); + if (IS_BROADWELL(dev_priv)) { + vgpu_vreg(vgpu, PORT_CLK_SEL(PORT_D)) &= + ~PORT_CLK_SEL_MASK; + vgpu_vreg(vgpu, PORT_CLK_SEL(PORT_D)) |= + PORT_CLK_SEL_LCPLL_810; + } vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_D)) |= DDI_BUF_CTL_ENABLE; vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_D)) &= ~DDI_BUF_IS_IDLE; vgpu_vreg(vgpu, SFUSE_STRAP) |= SFUSE_STRAP_DDID_DETECTED; -- cgit From 75e64ff2c2f5ce1ae5b47b2f372fe5b9dc99f5a9 Mon Sep 17 00:00:00 2001 From: Xiong Zhang Date: Wed, 28 Jun 2017 02:03:16 +0800 Subject: drm/i915/gvt: Don't read ADPA_CRT_HOTPLUG_MONITOR from host When host connects a crt screen, linux guest will detect two screens: crt and dp. This is wrong as linux guest has only one dp. In order to avoid guest get host crt screen, we should set ADPA_CRT_HOTPLUG_MONITOR to none. But MMIO_RO(PCH_ADPA) prevent from that. So MMIO_DH should be used instead of MMIO_RO. v2: Clear its staus to none at initialize, so guest don't get host crt.(Zhangyu) v3: SKL doesn't have this register, limit it to pre_skl.(xiong) Signed-off-by: Xiong Zhang Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/display.c | 4 ++++ drivers/gpu/drm/i915/gvt/handlers.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c index 818e94907c3b..2deb05f618fb 100644 --- a/drivers/gpu/drm/i915/gvt/display.c +++ b/drivers/gpu/drm/i915/gvt/display.c @@ -262,6 +262,10 @@ static void emulate_monitor_status_change(struct intel_vgpu *vgpu) vgpu_vreg(vgpu, DDI_BUF_CTL(PORT_A)) |= DDI_INIT_DISPLAY_DETECTED; } + + /* Clear host CRT status, so guest couldn't detect this host CRT. */ + if (IS_BROADWELL(dev_priv)) + vgpu_vreg(vgpu, PCH_ADPA) &= ~ADPA_CRT_HOTPLUG_MONITOR_MASK; } static void clean_virtual_dp_monitor(struct intel_vgpu *vgpu, int port_num) diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c index 0ffd69654592..437a35f41b62 100644 --- a/drivers/gpu/drm/i915/gvt/handlers.c +++ b/drivers/gpu/drm/i915/gvt/handlers.c @@ -1905,7 +1905,7 @@ static int init_generic_mmio_info(struct intel_gvt *gvt) MMIO_F(_PCH_DPD_AUX_CH_CTL, 6 * 4, 0, 0, 0, D_PRE_SKL, NULL, dp_aux_ch_ctl_mmio_write); - MMIO_RO(PCH_ADPA, D_ALL, 0, ADPA_CRT_HOTPLUG_MONITOR_MASK, NULL, pch_adpa_mmio_write); + MMIO_DH(PCH_ADPA, D_PRE_SKL, NULL, pch_adpa_mmio_write); MMIO_DH(_PCH_TRANSACONF, D_ALL, NULL, transconf_mmio_write); MMIO_DH(_PCH_TRANSBCONF, D_ALL, NULL, transconf_mmio_write); -- cgit From 5cd82b757795228516bf60a0552d1a40fa8adeb2 Mon Sep 17 00:00:00 2001 From: Changbin Du Date: Tue, 13 Jun 2017 10:15:26 +0800 Subject: drm/i915/gvt: Make function dpy_reg_mmio_readx safe The dpy_reg_mmio_read_x functions directly copy 4 bytes data to the target address with considering the length. If may cause the target memory corrupted if the requested length less than 4 bytes. Fix it for safety even we already have some checking to avoid this happen. And for convince, the 3 functions are merged. Signed-off-by: Changbin Du Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/handlers.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c index 437a35f41b62..a9114aa1eddb 100644 --- a/drivers/gpu/drm/i915/gvt/handlers.c +++ b/drivers/gpu/drm/i915/gvt/handlers.c @@ -366,21 +366,24 @@ static int lcpll_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset, static int dpy_reg_mmio_read(struct intel_vgpu *vgpu, unsigned int offset, void *p_data, unsigned int bytes) { - *(u32 *)p_data = (1 << 17); - return 0; -} - -static int dpy_reg_mmio_read_2(struct intel_vgpu *vgpu, unsigned int offset, - void *p_data, unsigned int bytes) -{ - *(u32 *)p_data = 3; - return 0; -} + switch (offset) { + case 0xe651c: + case 0xe661c: + case 0xe671c: + case 0xe681c: + vgpu_vreg(vgpu, offset) = 1 << 17; + break; + case 0xe6c04: + vgpu_vreg(vgpu, offset) = 0x3; + break; + case 0xe6e1c: + vgpu_vreg(vgpu, offset) = 0x2f << 16; + break; + default: + return -EINVAL; + } -static int dpy_reg_mmio_read_3(struct intel_vgpu *vgpu, unsigned int offset, - void *p_data, unsigned int bytes) -{ - *(u32 *)p_data = (0x2f << 16); + read_vreg(vgpu, offset, p_data, bytes); return 0; } @@ -1991,8 +1994,8 @@ static int init_generic_mmio_info(struct intel_gvt *gvt) MMIO_DH(0xe661c, D_ALL, dpy_reg_mmio_read, NULL); MMIO_DH(0xe671c, D_ALL, dpy_reg_mmio_read, NULL); MMIO_DH(0xe681c, D_ALL, dpy_reg_mmio_read, NULL); - MMIO_DH(0xe6c04, D_ALL, dpy_reg_mmio_read_2, NULL); - MMIO_DH(0xe6e1c, D_ALL, dpy_reg_mmio_read_3, NULL); + MMIO_DH(0xe6c04, D_ALL, dpy_reg_mmio_read, NULL); + MMIO_DH(0xe6e1c, D_ALL, dpy_reg_mmio_read, NULL); MMIO_RO(PCH_PORT_HOTPLUG, D_ALL, 0, PORTA_HOTPLUG_STATUS_MASK -- cgit