From 728c2edfcf14b3b61bd0ff82894f03455ca0e7d7 Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Mon, 29 Aug 2022 11:15:36 -0400 Subject: [PATCH 01/15] xen-pcifront: Handle missed Connected state An HVM guest with linux stubdomain and 2 PCI devices failed to start as libxl timed out waiting for the PCI devices to be added. It happens intermittently but with some regularity. libxl wrote the two xenstore entries for the devices, but then timed out waiting for backend state 4 (Connected) - the state stayed at 7 (Reconfiguring). (PCI passthrough to an HVM with stubdomain is PV passthrough to the stubdomain and then HVM passthrough with the QEMU inside the stubdomain.) The stubdomain kernel never printed "pcifront pci-0: Installing PCI frontend", so it seems to have missed state 4 which would have called pcifront_try_connect() -> pcifront_connect_and_init_dma() Have pcifront_detach_devices() special-case state Initialised and call pcifront_connect_and_init_dma(). Don't use pcifront_try_connect() because that sets the xenbus state which may throw off the backend. After connecting, skip the remainder of detach_devices since none have been initialized yet. When the backend switches to Reconfigured, pcifront_attach_devices() will pick them up again. Signed-off-by: Jason Andryuk Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20220829151536.8578-1-jandryuk@gmail.com Signed-off-by: Juergen Gross --- drivers/pci/xen-pcifront.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index 689271c4245c..77e61b470121 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -981,13 +981,26 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) { int err = 0; int i, num_devs; + enum xenbus_state state; unsigned int domain, bus, slot, func; struct pci_dev *pci_dev; char str[64]; - if (xenbus_read_driver_state(pdev->xdev->nodename) != - XenbusStateConnected) + state = xenbus_read_driver_state(pdev->xdev->nodename); + if (state == XenbusStateInitialised) { + dev_dbg(&pdev->xdev->dev, "Handle skipped connect.\n"); + /* We missed Connected and need to initialize. */ + err = pcifront_connect_and_init_dma(pdev); + if (err && err != -EEXIST) { + xenbus_dev_fatal(pdev->xdev, err, + "Error setting up PCI Frontend"); + goto out; + } + + goto out_switch_state; + } else if (state != XenbusStateConnected) { goto out; + } err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, "num_devs", "%d", &num_devs); @@ -1048,6 +1061,7 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) domain, bus, slot, func); } + out_switch_state: err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); out: From 06c62f8cbb1f660a4147b0d8cbe65cf2cfc1aa5a Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Tue, 4 Oct 2022 17:06:39 +0100 Subject: [PATCH 02/15] xen/xenbus: Fix spelling mistake "hardward" -> "hardware" There is a spelling mistake in the module description. Fix it. Signed-off-by: Colin Ian King Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20221004160639.154421-1-colin.i.king@gmail.com Signed-off-by: Juergen Gross --- drivers/xen/xen-pciback/xenbus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c index bde63ef677b8..d171091eec12 100644 --- a/drivers/xen/xen-pciback/xenbus.c +++ b/drivers/xen/xen-pciback/xenbus.c @@ -31,7 +31,7 @@ MODULE_PARM_DESC(passthrough, " frontend (for example, a device at 06:01.b will still appear at\n"\ " 06:01.b to the frontend). This is similar to how Xen 2.0.x\n"\ " exposed PCI devices to its driver domains. This may be required\n"\ - " for drivers which depend on finding their hardward in certain\n"\ + " for drivers which depend on finding their hardware in certain\n"\ " bus/slot locations."); static struct xen_pcibk_device *alloc_pdev(struct xenbus_device *xdev) From e433715b116553892ecad8796018ae4b64304252 Mon Sep 17 00:00:00 2001 From: Oleksandr Tyshchenko Date: Wed, 5 Oct 2022 20:48:22 +0300 Subject: [PATCH 03/15] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Take page offset into the account when calculating the number of pages to be granted. Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen") Signed-off-by: Oleksandr Tyshchenko Reviewed-by: Stefano Stabellini Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20221005174823.1800761-2-olekstysh@gmail.com Signed-off-by: Juergen Gross --- drivers/xen/grant-dma-ops.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c index 8973fc1e9ccc..1998d0e8ce82 100644 --- a/drivers/xen/grant-dma-ops.c +++ b/drivers/xen/grant-dma-ops.c @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page, unsigned long attrs) { struct xen_grant_dma_data *data; - unsigned int i, n_pages = PFN_UP(size); + unsigned int i, n_pages = PFN_UP(offset + size); grant_ref_t grant; dma_addr_t dma_handle; @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle, unsigned long attrs) { struct xen_grant_dma_data *data; - unsigned int i, n_pages = PFN_UP(size); + unsigned long offset = dma_handle & (PAGE_SIZE - 1); + unsigned int i, n_pages = PFN_UP(offset + size); grant_ref_t grant; if (WARN_ON(dir == DMA_NONE)) From 77be00f194b6e1647cddb644b7023b352c2c6ee8 Mon Sep 17 00:00:00 2001 From: Oleksandr Tyshchenko Date: Wed, 5 Oct 2022 20:48:23 +0300 Subject: [PATCH 04/15] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices As find_xen_grant_dma_data() is called from both interrupt and process contexts, the access to xen_grant_dma_devices XArray must be protected by xa_lock_irqsave to avoid deadlock scenario. As XArray API doesn't provide xa_store_irqsave helper, call lockless __xa_store directly and guard it externally. Also move the storage of the XArray's entry to a separate helper. Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen") Signed-off-by: Oleksandr Tyshchenko Reviewed-by: Stefano Stabellini Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20221005174823.1800761-3-olekstysh@gmail.com Signed-off-by: Juergen Gross --- drivers/xen/grant-dma-ops.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c index 1998d0e8ce82..c66f56d24013 100644 --- a/drivers/xen/grant-dma-ops.c +++ b/drivers/xen/grant-dma-ops.c @@ -25,7 +25,7 @@ struct xen_grant_dma_data { bool broken; }; -static DEFINE_XARRAY(xen_grant_dma_devices); +static DEFINE_XARRAY_FLAGS(xen_grant_dma_devices, XA_FLAGS_LOCK_IRQ); #define XEN_GRANT_DMA_ADDR_OFF (1ULL << 63) @@ -42,14 +42,29 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma) static struct xen_grant_dma_data *find_xen_grant_dma_data(struct device *dev) { struct xen_grant_dma_data *data; + unsigned long flags; - xa_lock(&xen_grant_dma_devices); + xa_lock_irqsave(&xen_grant_dma_devices, flags); data = xa_load(&xen_grant_dma_devices, (unsigned long)dev); - xa_unlock(&xen_grant_dma_devices); + xa_unlock_irqrestore(&xen_grant_dma_devices, flags); return data; } +static int store_xen_grant_dma_data(struct device *dev, + struct xen_grant_dma_data *data) +{ + unsigned long flags; + int ret; + + xa_lock_irqsave(&xen_grant_dma_devices, flags); + ret = xa_err(__xa_store(&xen_grant_dma_devices, (unsigned long)dev, data, + GFP_ATOMIC)); + xa_unlock_irqrestore(&xen_grant_dma_devices, flags); + + return ret; +} + /* * DMA ops for Xen frontends (e.g. virtio). * @@ -338,8 +353,7 @@ void xen_grant_setup_dma_ops(struct device *dev) */ data->backend_domid = iommu_spec.args[0]; - if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data, - GFP_KERNEL))) { + if (store_xen_grant_dma_data(dev, data)) { dev_err(dev, "Cannot store Xen grant DMA data\n"); goto err; } From 0991028cd49567d7016d1b224fe0117c35059f86 Mon Sep 17 00:00:00 2001 From: "M. Vefa Bicakci" Date: Sun, 2 Oct 2022 18:20:05 -0400 Subject: [PATCH 05/15] xen/gntdev: Prevent leaking grants Prior to this commit, if a grant mapping operation failed partially, some of the entries in the map_ops array would be invalid, whereas all of the entries in the kmap_ops array would be valid. This in turn would cause the following logic in gntdev_map_grant_pages to become invalid: for (i = 0; i < map->count; i++) { if (map->map_ops[i].status == GNTST_okay) { map->unmap_ops[i].handle = map->map_ops[i].handle; if (!use_ptemod) alloced++; } if (use_ptemod) { if (map->kmap_ops[i].status == GNTST_okay) { if (map->map_ops[i].status == GNTST_okay) alloced++; map->kunmap_ops[i].handle = map->kmap_ops[i].handle; } } } ... atomic_add(alloced, &map->live_grants); Assume that use_ptemod is true (i.e., the domain mapping the granted pages is a paravirtualized domain). In the code excerpt above, note that the "alloced" variable is only incremented when both kmap_ops[i].status and map_ops[i].status are set to GNTST_okay (i.e., both mapping operations are successful). However, as also noted above, there are cases where a grant mapping operation fails partially, breaking the assumption of the code excerpt above. The aforementioned causes map->live_grants to be incorrectly set. In some cases, all of the map_ops mappings fail, but all of the kmap_ops mappings succeed, meaning that live_grants may remain zero. This in turn makes it impossible to unmap the successfully grant-mapped pages pointed to by kmap_ops, because unmap_grant_pages has the following snippet of code at its beginning: if (atomic_read(&map->live_grants) == 0) return; /* Nothing to do */ In other cases where only some of the map_ops mappings fail but all kmap_ops mappings succeed, live_grants is made positive, but when the user requests unmapping the grant-mapped pages, __unmap_grant_pages_done will then make map->live_grants negative, because the latter function does not check if all of the pages that were requested to be unmapped were actually unmapped, and the same function unconditionally subtracts "data->count" (i.e., a value that can be greater than map->live_grants) from map->live_grants. The side effects of a negative live_grants value have not been studied. The net effect of all of this is that grant references are leaked in one of the above conditions. In Qubes OS v4.1 (which uses Xen's grant mechanism extensively for X11 GUI isolation), this issue manifests itself with warning messages like the following to be printed out by the Linux kernel in the VM that had granted pages (that contain X11 GUI window data) to dom0: "g.e. 0x1234 still pending", especially after the user rapidly resizes GUI VM windows (causing some grant-mapping operations to partially or completely fail, due to the fact that the VM unshares some of the pages as part of the window resizing, making the pages impossible to grant-map from dom0). The fix for this issue involves counting all successful map_ops and kmap_ops mappings separately, and then adding the sum to live_grants. During unmapping, only the number of successfully unmapped grants is subtracted from live_grants. The code is also modified to check for negative live_grants values after the subtraction and warn the user. Link: https://github.com/QubesOS/qubes-issues/issues/7631 Fixes: dbe97cff7dd9 ("xen/gntdev: Avoid blocking in unmap_grant_pages()") Cc: stable@vger.kernel.org Signed-off-by: M. Vefa Bicakci Acked-by: Demi Marie Obenour Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20221002222006.2077-2-m.v.b@runbox.com Signed-off-by: Juergen Gross --- drivers/xen/gntdev.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 84b143eef395..eb0586b9767d 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -367,8 +367,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) for (i = 0; i < map->count; i++) { if (map->map_ops[i].status == GNTST_okay) { map->unmap_ops[i].handle = map->map_ops[i].handle; - if (!use_ptemod) - alloced++; + alloced++; } else if (!err) err = -EINVAL; @@ -377,8 +376,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) if (use_ptemod) { if (map->kmap_ops[i].status == GNTST_okay) { - if (map->map_ops[i].status == GNTST_okay) - alloced++; + alloced++; map->kunmap_ops[i].handle = map->kmap_ops[i].handle; } else if (!err) err = -EINVAL; @@ -394,8 +392,14 @@ static void __unmap_grant_pages_done(int result, unsigned int i; struct gntdev_grant_map *map = data->data; unsigned int offset = data->unmap_ops - map->unmap_ops; + int successful_unmaps = 0; + int live_grants; for (i = 0; i < data->count; i++) { + if (map->unmap_ops[offset + i].status == GNTST_okay && + map->unmap_ops[offset + i].handle != INVALID_GRANT_HANDLE) + successful_unmaps++; + WARN_ON(map->unmap_ops[offset + i].status != GNTST_okay && map->unmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); pr_debug("unmap handle=%d st=%d\n", @@ -403,6 +407,10 @@ static void __unmap_grant_pages_done(int result, map->unmap_ops[offset+i].status); map->unmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; if (use_ptemod) { + if (map->kunmap_ops[offset + i].status == GNTST_okay && + map->kunmap_ops[offset + i].handle != INVALID_GRANT_HANDLE) + successful_unmaps++; + WARN_ON(map->kunmap_ops[offset + i].status != GNTST_okay && map->kunmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); pr_debug("kunmap handle=%u st=%d\n", @@ -411,11 +419,15 @@ static void __unmap_grant_pages_done(int result, map->kunmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; } } + /* * Decrease the live-grant counter. This must happen after the loop to * prevent premature reuse of the grants by gnttab_mmap(). */ - atomic_sub(data->count, &map->live_grants); + live_grants = atomic_sub_return(successful_unmaps, &map->live_grants); + if (WARN_ON(live_grants < 0)) + pr_err("%s: live_grants became negative (%d) after unmapping %d pages!\n", + __func__, live_grants, successful_unmaps); /* Release reference taken by __unmap_grant_pages */ gntdev_put_map(NULL, map); From 5c13a4a0291b30191eff9ead8d010e1ca43a4d0c Mon Sep 17 00:00:00 2001 From: "M. Vefa Bicakci" Date: Sun, 2 Oct 2022 18:20:06 -0400 Subject: [PATCH 06/15] xen/gntdev: Accommodate VMA splitting Prior to this commit, the gntdev driver code did not handle the following scenario correctly with paravirtualized (PV) Xen domains: * User process sets up a gntdev mapping composed of two grant mappings (i.e., two pages shared by another Xen domain). * User process munmap()s one of the pages. * User process munmap()s the remaining page. * User process exits. In the scenario above, the user process would cause the kernel to log the following messages in dmesg for the first munmap(), and the second munmap() call would result in similar log messages: BUG: Bad page map in process doublemap.test pte:... pmd:... page:0000000057c97bff refcount:1 mapcount:-1 \ mapping:0000000000000000 index:0x0 pfn:... ... page dumped because: bad pte ... file:gntdev fault:0x0 mmap:gntdev_mmap [xen_gntdev] readpage:0x0 ... Call Trace: dump_stack_lvl+0x46/0x5e print_bad_pte.cold+0x66/0xb6 unmap_page_range+0x7e5/0xdc0 unmap_vmas+0x78/0xf0 unmap_region+0xa8/0x110 __do_munmap+0x1ea/0x4e0 __vm_munmap+0x75/0x120 __x64_sys_munmap+0x28/0x40 do_syscall_64+0x38/0x90 entry_SYSCALL_64_after_hwframe+0x61/0xcb ... For each munmap() call, the Xen hypervisor (if built with CONFIG_DEBUG) would print out the following and trigger a general protection fault in the affected Xen PV domain: (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ... (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ... As of this writing, gntdev_grant_map structure's vma field (referred to as map->vma below) is mainly used for checking the start and end addresses of mappings. However, with split VMAs, these may change, and there could be more than one VMA associated with a gntdev mapping. Hence, remove the use of map->vma and rely on map->pages_vm_start for the original start address and on (map->count << PAGE_SHIFT) for the original mapping size. Let the invalidate() and find_special_page() hooks use these. Also, given that there can be multiple VMAs associated with a gntdev mapping, move the "mmu_interval_notifier_remove(&map->notifier)" call to the end of gntdev_put_map, so that the MMU notifier is only removed after the closing of the last remaining VMA. Finally, use an atomic to prevent inadvertent gntdev mapping re-use, instead of using the map->live_grants atomic counter and/or the map->vma pointer (the latter of which is now removed). This prevents the userspace from mmap()'ing (with MAP_FIXED) a gntdev mapping over the same address range as a previously set up gntdev mapping. This scenario can be summarized with the following call-trace, which was valid prior to this commit: mmap gntdev_mmap mmap (repeat mmap with MAP_FIXED over the same address range) gntdev_invalidate unmap_grant_pages (sets 'being_removed' entries to true) gnttab_unmap_refs_async unmap_single_vma gntdev_mmap (maps the shared pages again) munmap gntdev_invalidate unmap_grant_pages (no-op because 'being_removed' entries are true) unmap_single_vma (For PV domains, Xen reports that a granted page is being unmapped and triggers a general protection fault in the affected domain, if Xen was built with CONFIG_DEBUG) The fix for this last scenario could be worth its own commit, but we opted for a single commit, because removing the gntdev_grant_map structure's vma field requires guarding the entry to gntdev_mmap(), and the live_grants atomic counter is not sufficient on its own to prevent the mmap() over a pre-existing mapping. Link: https://github.com/QubesOS/qubes-issues/issues/7631 Fixes: ab31523c2fca ("xen/gntdev: allow usermode to map granted pages") Cc: stable@vger.kernel.org Signed-off-by: M. Vefa Bicakci Reviewed-by: Juergen Gross Link: https://lore.kernel.org/r/20221002222006.2077-3-m.v.b@runbox.com Signed-off-by: Juergen Gross --- drivers/xen/gntdev-common.h | 3 +- drivers/xen/gntdev.c | 58 ++++++++++++++++--------------------- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h index 40ef379c28ab..9c286b2a1900 100644 --- a/drivers/xen/gntdev-common.h +++ b/drivers/xen/gntdev-common.h @@ -44,9 +44,10 @@ struct gntdev_unmap_notify { }; struct gntdev_grant_map { + atomic_t in_use; struct mmu_interval_notifier notifier; + bool notifier_init; struct list_head next; - struct vm_area_struct *vma; int index; int count; int flags; diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index eb0586b9767d..4d9a3050de6a 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -286,6 +286,9 @@ void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map *map) */ } + if (use_ptemod && map->notifier_init) + mmu_interval_notifier_remove(&map->notifier); + if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) { notify_remote_via_evtchn(map->notify.event); evtchn_put(map->notify.event); @@ -298,7 +301,7 @@ void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map *map) static int find_grant_ptes(pte_t *pte, unsigned long addr, void *data) { struct gntdev_grant_map *map = data; - unsigned int pgnr = (addr - map->vma->vm_start) >> PAGE_SHIFT; + unsigned int pgnr = (addr - map->pages_vm_start) >> PAGE_SHIFT; int flags = map->flags | GNTMAP_application_map | GNTMAP_contains_pte | (1 << _GNTMAP_guest_avail0); u64 pte_maddr; @@ -508,11 +511,7 @@ static void gntdev_vma_close(struct vm_area_struct *vma) struct gntdev_priv *priv = file->private_data; pr_debug("gntdev_vma_close %p\n", vma); - if (use_ptemod) { - WARN_ON(map->vma != vma); - mmu_interval_notifier_remove(&map->notifier); - map->vma = NULL; - } + vma->vm_private_data = NULL; gntdev_put_map(priv, map); } @@ -540,29 +539,30 @@ static bool gntdev_invalidate(struct mmu_interval_notifier *mn, struct gntdev_grant_map *map = container_of(mn, struct gntdev_grant_map, notifier); unsigned long mstart, mend; + unsigned long map_start, map_end; if (!mmu_notifier_range_blockable(range)) return false; + map_start = map->pages_vm_start; + map_end = map->pages_vm_start + (map->count << PAGE_SHIFT); + /* * If the VMA is split or otherwise changed the notifier is not * updated, but we don't want to process VA's outside the modified * VMA. FIXME: It would be much more understandable to just prevent * modifying the VMA in the first place. */ - if (map->vma->vm_start >= range->end || - map->vma->vm_end <= range->start) + if (map_start >= range->end || map_end <= range->start) return true; - mstart = max(range->start, map->vma->vm_start); - mend = min(range->end, map->vma->vm_end); + mstart = max(range->start, map_start); + mend = min(range->end, map_end); pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n", - map->index, map->count, - map->vma->vm_start, map->vma->vm_end, - range->start, range->end, mstart, mend); - unmap_grant_pages(map, - (mstart - map->vma->vm_start) >> PAGE_SHIFT, - (mend - mstart) >> PAGE_SHIFT); + map->index, map->count, map_start, map_end, + range->start, range->end, mstart, mend); + unmap_grant_pages(map, (mstart - map_start) >> PAGE_SHIFT, + (mend - mstart) >> PAGE_SHIFT); return true; } @@ -1042,18 +1042,15 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) return -EINVAL; pr_debug("map %d+%d at %lx (pgoff %lx)\n", - index, count, vma->vm_start, vma->vm_pgoff); + index, count, vma->vm_start, vma->vm_pgoff); mutex_lock(&priv->lock); map = gntdev_find_map_index(priv, index, count); if (!map) goto unlock_out; - if (use_ptemod && map->vma) + if (!atomic_add_unless(&map->in_use, 1, 1)) goto unlock_out; - if (atomic_read(&map->live_grants)) { - err = -EAGAIN; - goto unlock_out; - } + refcount_inc(&map->users); vma->vm_ops = &gntdev_vmops; @@ -1074,15 +1071,16 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) map->flags |= GNTMAP_readonly; } + map->pages_vm_start = vma->vm_start; + if (use_ptemod) { - map->vma = vma; err = mmu_interval_notifier_insert_locked( &map->notifier, vma->vm_mm, vma->vm_start, vma->vm_end - vma->vm_start, &gntdev_mmu_ops); - if (err) { - map->vma = NULL; + if (err) goto out_unlock_put; - } + + map->notifier_init = true; } mutex_unlock(&priv->lock); @@ -1099,7 +1097,6 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) */ mmu_interval_read_begin(&map->notifier); - map->pages_vm_start = vma->vm_start; err = apply_to_page_range(vma->vm_mm, vma->vm_start, vma->vm_end - vma->vm_start, find_grant_ptes, map); @@ -1128,13 +1125,8 @@ unlock_out: out_unlock_put: mutex_unlock(&priv->lock); out_put_map: - if (use_ptemod) { + if (use_ptemod) unmap_grant_pages(map, 0, map->count); - if (map->vma) { - mmu_interval_notifier_remove(&map->notifier); - map->vma = NULL; - } - } gntdev_put_map(priv, map); return err; } From 2849752f36848359034616eb70dfc7fb14eb3cd4 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Thu, 6 Oct 2022 10:50:28 +0200 Subject: [PATCH 07/15] xen/pcifront: move xenstore config scanning into sub-function pcifront_try_connect() and pcifront_attach_devices() share a large chunk of duplicated code for reading the config information from Xenstore, which only differs regarding calling pcifront_rescan_root() or pcifront_scan_root(). Put that code into a new sub-function. It is fine to always call pcifront_rescan_root() from that common function, as it will fallback to pcifront_scan_root() if the domain/bus combination isn't known yet (and pcifront_scan_root() should never be called for an already known domain/bus combination anyway). In order to avoid duplicate messages for the fallback case move the check for domain/bus not known to the beginning of pcifront_rescan_root(). While at it fix the error reporting in case the root-xx node had the wrong format. As the return value of pcifront_try_connect() and pcifront_attach_devices() are not used anywhere make those functions return void. As an additional bonus this removes the dubious return of -EFAULT in case of an unexpected driver state. Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk Signed-off-by: Juergen Gross --- drivers/pci/xen-pcifront.c | 143 ++++++++++--------------------------- 1 file changed, 37 insertions(+), 106 deletions(-) diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index 77e61b470121..7378e2f3e525 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -521,24 +521,14 @@ static int pcifront_rescan_root(struct pcifront_device *pdev, int err; struct pci_bus *b; -#ifndef CONFIG_PCI_DOMAINS - if (domain != 0) { - dev_err(&pdev->xdev->dev, - "PCI Root in non-zero PCI Domain! domain=%d\n", domain); - dev_err(&pdev->xdev->dev, - "Please compile with CONFIG_PCI_DOMAINS\n"); - return -EINVAL; - } -#endif - - dev_info(&pdev->xdev->dev, "Rescanning PCI Frontend Bus %04x:%02x\n", - domain, bus); - b = pci_find_bus(domain, bus); if (!b) /* If the bus is unknown, create it. */ return pcifront_scan_root(pdev, domain, bus); + dev_info(&pdev->xdev->dev, "Rescanning PCI Frontend Bus %04x:%02x\n", + domain, bus); + err = pcifront_scan_bus(pdev, domain, bus, b); /* Claim resources before going "live" with our devices */ @@ -819,76 +809,73 @@ out: return err; } -static int pcifront_try_connect(struct pcifront_device *pdev) +static void pcifront_connect(struct pcifront_device *pdev) { - int err = -EFAULT; + int err; int i, num_roots, len; char str[64]; unsigned int domain, bus; - - /* Only connect once */ - if (xenbus_read_driver_state(pdev->xdev->nodename) != - XenbusStateInitialised) - goto out; - - err = pcifront_connect_and_init_dma(pdev); - if (err && err != -EEXIST) { - xenbus_dev_fatal(pdev->xdev, err, - "Error setting up PCI Frontend"); - goto out; - } - err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, "root_num", "%d", &num_roots); if (err == -ENOENT) { xenbus_dev_error(pdev->xdev, err, "No PCI Roots found, trying 0000:00"); - err = pcifront_scan_root(pdev, 0, 0); + err = pcifront_rescan_root(pdev, 0, 0); if (err) { xenbus_dev_fatal(pdev->xdev, err, "Error scanning PCI root 0000:00"); - goto out; + return; } num_roots = 0; } else if (err != 1) { - if (err == 0) - err = -EINVAL; - xenbus_dev_fatal(pdev->xdev, err, + xenbus_dev_fatal(pdev->xdev, err >= 0 ? -EINVAL : err, "Error reading number of PCI roots"); - goto out; + return; } for (i = 0; i < num_roots; i++) { len = snprintf(str, sizeof(str), "root-%d", i); - if (unlikely(len >= (sizeof(str) - 1))) { - err = -ENOMEM; - goto out; - } + if (unlikely(len >= (sizeof(str) - 1))) + return; err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, str, "%x:%x", &domain, &bus); if (err != 2) { - if (err >= 0) - err = -EINVAL; - xenbus_dev_fatal(pdev->xdev, err, + xenbus_dev_fatal(pdev->xdev, err >= 0 ? -EINVAL : err, "Error reading PCI root %d", i); - goto out; + return; } - err = pcifront_scan_root(pdev, domain, bus); + err = pcifront_rescan_root(pdev, domain, bus); if (err) { xenbus_dev_fatal(pdev->xdev, err, "Error scanning PCI root %04x:%02x", domain, bus); - goto out; + return; } } - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); + xenbus_switch_state(pdev->xdev, XenbusStateConnected); +} -out: - return err; +static void pcifront_try_connect(struct pcifront_device *pdev) +{ + int err; + + /* Only connect once */ + if (xenbus_read_driver_state(pdev->xdev->nodename) != + XenbusStateInitialised) + return; + + err = pcifront_connect_and_init_dma(pdev); + if (err && err != -EEXIST) { + xenbus_dev_fatal(pdev->xdev, err, + "Error setting up PCI Frontend"); + return; + } + + pcifront_connect(pdev); } static int pcifront_try_disconnect(struct pcifront_device *pdev) @@ -914,67 +901,11 @@ out: return err; } -static int pcifront_attach_devices(struct pcifront_device *pdev) +static void pcifront_attach_devices(struct pcifront_device *pdev) { - int err = -EFAULT; - int i, num_roots, len; - unsigned int domain, bus; - char str[64]; - - if (xenbus_read_driver_state(pdev->xdev->nodename) != + if (xenbus_read_driver_state(pdev->xdev->nodename) == XenbusStateReconfiguring) - goto out; - - err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, - "root_num", "%d", &num_roots); - if (err == -ENOENT) { - xenbus_dev_error(pdev->xdev, err, - "No PCI Roots found, trying 0000:00"); - err = pcifront_rescan_root(pdev, 0, 0); - if (err) { - xenbus_dev_fatal(pdev->xdev, err, - "Error scanning PCI root 0000:00"); - goto out; - } - num_roots = 0; - } else if (err != 1) { - if (err == 0) - err = -EINVAL; - xenbus_dev_fatal(pdev->xdev, err, - "Error reading number of PCI roots"); - goto out; - } - - for (i = 0; i < num_roots; i++) { - len = snprintf(str, sizeof(str), "root-%d", i); - if (unlikely(len >= (sizeof(str) - 1))) { - err = -ENOMEM; - goto out; - } - - err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, str, - "%x:%x", &domain, &bus); - if (err != 2) { - if (err >= 0) - err = -EINVAL; - xenbus_dev_fatal(pdev->xdev, err, - "Error reading PCI root %d", i); - goto out; - } - - err = pcifront_rescan_root(pdev, domain, bus); - if (err) { - xenbus_dev_fatal(pdev->xdev, err, - "Error scanning PCI root %04x:%02x", - domain, bus); - goto out; - } - } - - xenbus_switch_state(pdev->xdev, XenbusStateConnected); - -out: - return err; + pcifront_connect(pdev); } static int pcifront_detach_devices(struct pcifront_device *pdev) From c9133112f347907774055bbf73179a7ff8504689 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 29 Aug 2022 13:26:07 +0200 Subject: [PATCH 08/15] xen/virtio: restructure xen grant dma setup In order to prepare supporting other means than device tree for setting up virtio devices under Xen, restructure the functions xen_is_grant_dma_device() and xen_grant_setup_dma_ops() a little bit. Signed-off-by: Juergen Gross Reviewed-by: Oleksandr Tyshchenko Tested-by: Oleksandr Tyshchenko # Arm64 only Acked-by: Stefano Stabellini Signed-off-by: Juergen Gross --- drivers/xen/grant-dma-ops.c | 80 +++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 31 deletions(-) diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c index c66f56d24013..7133272918f0 100644 --- a/drivers/xen/grant-dma-ops.c +++ b/drivers/xen/grant-dma-ops.c @@ -289,22 +289,28 @@ static const struct dma_map_ops xen_grant_dma_ops = { .dma_supported = xen_grant_dma_supported, }; -bool xen_is_grant_dma_device(struct device *dev) +static bool xen_is_dt_grant_dma_device(struct device *dev) { struct device_node *iommu_np; bool has_iommu; - /* XXX Handle only DT devices for now */ - if (!dev->of_node) - return false; - iommu_np = of_parse_phandle(dev->of_node, "iommus", 0); - has_iommu = iommu_np && of_device_is_compatible(iommu_np, "xen,grant-dma"); + has_iommu = iommu_np && + of_device_is_compatible(iommu_np, "xen,grant-dma"); of_node_put(iommu_np); return has_iommu; } +bool xen_is_grant_dma_device(struct device *dev) +{ + /* XXX Handle only DT devices for now */ + if (dev->of_node) + return xen_is_dt_grant_dma_device(dev); + + return false; +} + bool xen_virtio_mem_acc(struct virtio_device *dev) { if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) @@ -313,10 +319,38 @@ bool xen_virtio_mem_acc(struct virtio_device *dev) return xen_is_grant_dma_device(dev->dev.parent); } +static int xen_dt_grant_init_backend_domid(struct device *dev, + struct xen_grant_dma_data *data) +{ + struct of_phandle_args iommu_spec; + + if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells", + 0, &iommu_spec)) { + dev_err(dev, "Cannot parse iommus property\n"); + return -ESRCH; + } + + if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") || + iommu_spec.args_count != 1) { + dev_err(dev, "Incompatible IOMMU node\n"); + of_node_put(iommu_spec.np); + return -ESRCH; + } + + of_node_put(iommu_spec.np); + + /* + * The endpoint ID here means the ID of the domain where the + * corresponding backend is running + */ + data->backend_domid = iommu_spec.args[0]; + + return 0; +} + void xen_grant_setup_dma_ops(struct device *dev) { struct xen_grant_dma_data *data; - struct of_phandle_args iommu_spec; data = find_xen_grant_dma_data(dev); if (data) { @@ -324,34 +358,17 @@ void xen_grant_setup_dma_ops(struct device *dev) return; } - /* XXX ACPI device unsupported for now */ - if (!dev->of_node) - goto err; - - if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells", - 0, &iommu_spec)) { - dev_err(dev, "Cannot parse iommus property\n"); - goto err; - } - - if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") || - iommu_spec.args_count != 1) { - dev_err(dev, "Incompatible IOMMU node\n"); - of_node_put(iommu_spec.np); - goto err; - } - - of_node_put(iommu_spec.np); - data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) goto err; - /* - * The endpoint ID here means the ID of the domain where the corresponding - * backend is running - */ - data->backend_domid = iommu_spec.args[0]; + if (dev->of_node) { + if (xen_dt_grant_init_backend_domid(dev, data)) + goto err; + } else { + /* XXX ACPI device unsupported for now */ + goto err; + } if (store_xen_grant_dma_data(dev, data)) { dev_err(dev, "Cannot store Xen grant DMA data\n"); @@ -363,6 +380,7 @@ void xen_grant_setup_dma_ops(struct device *dev) return; err: + devm_kfree(dev, data); dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n"); } From 7228113d1fa0107a377aef71094d610eb8824aa2 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 29 Aug 2022 13:26:08 +0200 Subject: [PATCH 09/15] xen/virtio: use dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT With CONFIG_XEN_VIRTIO_FORCE_GRANT set the default backend domid to 0, enabling to use xen_grant_dma_ops for those devices. Signed-off-by: Juergen Gross Reviewed-by: Oleksandr Tyshchenko Acked-by: Stefano Stabellini Signed-off-by: Juergen Gross --- drivers/xen/grant-dma-ops.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c index 7133272918f0..3e4c590896d0 100644 --- a/drivers/xen/grant-dma-ops.c +++ b/drivers/xen/grant-dma-ops.c @@ -365,6 +365,9 @@ void xen_grant_setup_dma_ops(struct device *dev) if (dev->of_node) { if (xen_dt_grant_init_backend_domid(dev, data)) goto err; + } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) { + dev_info(dev, "Using dom0 as backend\n"); + data->backend_domid = 0; } else { /* XXX ACPI device unsupported for now */ goto err; From 61367688f1fb07678b1d865a0ce9364f5267a896 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 29 Aug 2022 13:26:08 +0200 Subject: [PATCH 10/15] xen/virtio: enable grant based virtio on x86 Use an x86-specific virtio_check_mem_acc_cb() for Xen in order to setup the correct DMA ops. Signed-off-by: Juergen Gross Reviewed-by: Oleksandr Tyshchenko # common code Reviewed-by: Boris Ostrovsky Signed-off-by: Juergen Gross --- arch/x86/xen/enlighten_hvm.c | 2 +- arch/x86/xen/enlighten_pv.c | 2 +- drivers/xen/grant-dma-ops.c | 12 +++++++++++- include/xen/xen-ops.h | 6 ++++++ 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c index 1c1ac418484b..c1cd28e915a3 100644 --- a/arch/x86/xen/enlighten_hvm.c +++ b/arch/x86/xen/enlighten_hvm.c @@ -212,7 +212,7 @@ static void __init xen_hvm_guest_init(void) return; if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc); + virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc); init_hvm_pv_info(); diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 0ed2e487a693..0a5dcadf23b9 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -112,7 +112,7 @@ static void __init xen_pv_init_platform(void) { /* PV guests can't operate virtio devices without grants. */ if (IS_ENABLED(CONFIG_XEN_VIRTIO)) - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc); + virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc); populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP)); diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c index 3e4c590896d0..860f37c93af4 100644 --- a/drivers/xen/grant-dma-ops.c +++ b/drivers/xen/grant-dma-ops.c @@ -313,7 +313,7 @@ bool xen_is_grant_dma_device(struct device *dev) bool xen_virtio_mem_acc(struct virtio_device *dev) { - if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) + if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) return true; return xen_is_grant_dma_device(dev->dev.parent); @@ -387,6 +387,16 @@ err: dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n"); } +bool xen_virtio_restricted_mem_acc(struct virtio_device *dev) +{ + bool ret = xen_virtio_mem_acc(dev); + + if (ret) + xen_grant_setup_dma_ops(dev->dev.parent); + + return ret; +} + MODULE_DESCRIPTION("Xen grant DMA-mapping layer"); MODULE_AUTHOR("Juergen Gross "); MODULE_LICENSE("GPL"); diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h index dae0f350c678..a34f4271a2e9 100644 --- a/include/xen/xen-ops.h +++ b/include/xen/xen-ops.h @@ -219,6 +219,7 @@ static inline void xen_preemptible_hcall_end(void) { } void xen_grant_setup_dma_ops(struct device *dev); bool xen_is_grant_dma_device(struct device *dev); bool xen_virtio_mem_acc(struct virtio_device *dev); +bool xen_virtio_restricted_mem_acc(struct virtio_device *dev); #else static inline void xen_grant_setup_dma_ops(struct device *dev) { @@ -234,6 +235,11 @@ static inline bool xen_virtio_mem_acc(struct virtio_device *dev) { return false; } + +static inline bool xen_virtio_restricted_mem_acc(struct virtio_device *dev) +{ + return false; +} #endif /* CONFIG_XEN_GRANT_DMA_OPS */ #endif /* INCLUDE_XEN_OPS_H */ From 8714f7bcd3c20d36890f43cc6a8e0c3c17b843aa Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 26 Sep 2022 08:23:51 +0200 Subject: [PATCH 11/15] xen/pv: add fault recovery control to pmu msr accesses Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants of read/write MSR in case the MSR access isn't emulated via Xen. Allow the caller to select that faults should not be recovered from by passing NULL for the error pointer. Restructure the code to make it more readable. Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Signed-off-by: Juergen Gross --- arch/x86/xen/pmu.c | 66 ++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c index 21ecbe754cb2..0f98cb1077e3 100644 --- a/arch/x86/xen/pmu.c +++ b/arch/x86/xen/pmu.c @@ -131,6 +131,9 @@ static inline uint32_t get_fam15h_addr(u32 addr) static inline bool is_amd_pmu_msr(unsigned int msr) { + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) + return false; + if ((msr >= MSR_F15H_PERF_CTL && msr < MSR_F15H_PERF_CTR + (amd_num_counters * 2)) || (msr >= MSR_K7_EVNTSEL0 && @@ -144,6 +147,9 @@ static int is_intel_pmu_msr(u32 msr_index, int *type, int *index) { u32 msr_index_pmc; + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) + return false; + switch (msr_index) { case MSR_CORE_PERF_FIXED_CTR_CTRL: case MSR_IA32_DS_AREA: @@ -290,48 +296,52 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read) return false; } +static bool pmu_msr_chk_emulated(unsigned int msr, uint64_t *val, bool is_read, + bool *emul) +{ + int type, index; + + if (is_amd_pmu_msr(msr)) + *emul = xen_amd_pmu_emulate(msr, val, is_read); + else if (is_intel_pmu_msr(msr, &type, &index)) + *emul = xen_intel_pmu_emulate(msr, val, type, index, is_read); + else + return false; + + return true; +} + bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err) { - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) { - if (is_amd_pmu_msr(msr)) { - if (!xen_amd_pmu_emulate(msr, val, 1)) - *val = native_read_msr_safe(msr, err); - return true; - } - } else { - int type, index; + bool emulated; - if (is_intel_pmu_msr(msr, &type, &index)) { - if (!xen_intel_pmu_emulate(msr, val, type, index, 1)) - *val = native_read_msr_safe(msr, err); - return true; - } + if (!pmu_msr_chk_emulated(msr, val, true, &emulated)) + return false; + + if (!emulated) { + *val = err ? native_read_msr_safe(msr, err) + : native_read_msr(msr); } - return false; + return true; } bool pmu_msr_write(unsigned int msr, uint32_t low, uint32_t high, int *err) { uint64_t val = ((uint64_t)high << 32) | low; + bool emulated; - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) { - if (is_amd_pmu_msr(msr)) { - if (!xen_amd_pmu_emulate(msr, &val, 0)) - *err = native_write_msr_safe(msr, low, high); - return true; - } - } else { - int type, index; + if (!pmu_msr_chk_emulated(msr, &val, false, &emulated)) + return false; - if (is_intel_pmu_msr(msr, &type, &index)) { - if (!xen_intel_pmu_emulate(msr, &val, type, index, 0)) - *err = native_write_msr_safe(msr, low, high); - return true; - } + if (!emulated) { + if (err) + *err = native_write_msr_safe(msr, low, high); + else + native_write_msr(msr, low, high); } - return false; + return true; } static unsigned long long xen_amd_read_pmc(int counter) From f90d98bdd06c0f3d1a60462c85324bd61f2a7142 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Wed, 5 Oct 2022 09:42:33 +0200 Subject: [PATCH 12/15] xen/pv: fix vendor checks for pmu emulation The CPU vendor checks for pmu emulation are rather limited today, as the assumption seems to be that only Intel and AMD are existing and/or supported vendors. Fix that by handling Centaur and Zhaoxin CPUs the same way as Intel, and Hygon the same way as AMD. While at it fix the return type of is_intel_pmu_msr(). Suggested-by: Jan Beulich Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Signed-off-by: Juergen Gross --- arch/x86/xen/pmu.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c index 0f98cb1077e3..68aff1382872 100644 --- a/arch/x86/xen/pmu.c +++ b/arch/x86/xen/pmu.c @@ -131,7 +131,8 @@ static inline uint32_t get_fam15h_addr(u32 addr) static inline bool is_amd_pmu_msr(unsigned int msr) { - if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD && + boot_cpu_data.x86_vendor != X86_VENDOR_HYGON) return false; if ((msr >= MSR_F15H_PERF_CTL && @@ -143,11 +144,13 @@ static inline bool is_amd_pmu_msr(unsigned int msr) return false; } -static int is_intel_pmu_msr(u32 msr_index, int *type, int *index) +static bool is_intel_pmu_msr(u32 msr_index, int *type, int *index) { u32 msr_index_pmc; - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL && + boot_cpu_data.x86_vendor != X86_VENDOR_CENTAUR && + boot_cpu_data.x86_vendor != X86_VENDOR_ZHAOXIN) return false; switch (msr_index) { From a1886b915e81439ba045b1431f3319d37ac1b906 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 26 Sep 2022 12:33:03 +0200 Subject: [PATCH 13/15] xen/pv: refactor msr access functions to support safe and unsafe accesses Refactor and rename xen_read_msr_safe() and xen_write_msr_safe() to support both cases of MSR accesses, safe ones and potentially GP-fault generating ones. This will prepare to no longer swallow GPs silently in xen_read_msr() and xen_write_msr(). Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich Signed-off-by: Juergen Gross --- arch/x86/xen/enlighten_pv.c | 73 ++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 0a5dcadf23b9..8c2acccebfe1 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -916,14 +916,18 @@ static void xen_write_cr4(unsigned long cr4) native_write_cr4(cr4); } -static u64 xen_read_msr_safe(unsigned int msr, int *err) +static u64 xen_do_read_msr(unsigned int msr, int *err) { - u64 val; + u64 val = 0; /* Avoid uninitialized value for safe variant. */ if (pmu_msr_read(msr, &val, err)) return val; - val = native_read_msr_safe(msr, err); + if (err) + val = native_read_msr_safe(msr, err); + else + val = native_read_msr(msr); + switch (msr) { case MSR_IA32_APICBASE: val &= ~X2APIC_ENABLE; @@ -932,23 +936,39 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err) return val; } -static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high) +static void set_seg(unsigned int which, unsigned int low, unsigned int high, + int *err) { - int ret; - unsigned int which; - u64 base; + u64 base = ((u64)high << 32) | low; - ret = 0; + if (HYPERVISOR_set_segment_base(which, base) == 0) + return; + if (err) + *err = -EIO; + else + WARN(1, "Xen set_segment_base(%u, %llx) failed\n", which, base); +} + +/* + * Support write_msr_safe() and write_msr() semantics. + * With err == NULL write_msr() semantics are selected. + * Supplying an err pointer requires err to be pre-initialized with 0. + */ +static void xen_do_write_msr(unsigned int msr, unsigned int low, + unsigned int high, int *err) +{ switch (msr) { - case MSR_FS_BASE: which = SEGBASE_FS; goto set; - case MSR_KERNEL_GS_BASE: which = SEGBASE_GS_USER; goto set; - case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set; + case MSR_FS_BASE: + set_seg(SEGBASE_FS, low, high, err); + break; - set: - base = ((u64)high << 32) | low; - if (HYPERVISOR_set_segment_base(which, base) != 0) - ret = -EIO; + case MSR_KERNEL_GS_BASE: + set_seg(SEGBASE_GS_USER, low, high, err); + break; + + case MSR_GS_BASE: + set_seg(SEGBASE_GS_KERNEL, low, high, err); break; case MSR_STAR: @@ -964,11 +984,28 @@ static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high) break; default: - if (!pmu_msr_write(msr, low, high, &ret)) - ret = native_write_msr_safe(msr, low, high); + if (!pmu_msr_write(msr, low, high, err)) { + if (err) + *err = native_write_msr_safe(msr, low, high); + else + native_write_msr(msr, low, high); + } } +} - return ret; +static u64 xen_read_msr_safe(unsigned int msr, int *err) +{ + return xen_do_read_msr(msr, err); +} + +static int xen_write_msr_safe(unsigned int msr, unsigned int low, + unsigned int high) +{ + int err = 0; + + xen_do_write_msr(msr, low, high, &err); + + return err; } static u64 xen_read_msr(unsigned int msr) From 3fac3734c43a2e21fefeb72124d8bd31dff3956f Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Mon, 26 Sep 2022 13:16:56 +0200 Subject: [PATCH 14/15] xen/pv: support selecting safe/unsafe msr accesses Instead of always doing the safe variants for reading and writing MSRs in Xen PV guests, make the behavior controllable via Kconfig option and a boot parameter. The default will be the current behavior, which is to always use the safe variant. Signed-off-by: Juergen Gross --- .../admin-guide/kernel-parameters.txt | 6 +++++ arch/x86/xen/Kconfig | 9 +++++++ arch/x86/xen/enlighten_pv.c | 24 +++++++++++-------- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 426fa892d311..1bda9cf18fae 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -6836,6 +6836,12 @@ Crash from Xen panic notifier, without executing late panic() code such as dumping handler. + xen_msr_safe= [X86,XEN] + Format: + Select whether to always use non-faulting (safe) MSR + access functions when running as Xen PV guest. The + default value is controlled by CONFIG_XEN_PV_MSR_SAFE. + xen_nopvspin [X86,XEN] Disables the qspinlock slowpath using Xen PV optimizations. This parameter is obsoleted by "nopvspin" parameter, which diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig index 85246dd9faa1..9b1ec5d8c99c 100644 --- a/arch/x86/xen/Kconfig +++ b/arch/x86/xen/Kconfig @@ -92,3 +92,12 @@ config XEN_DOM0 select X86_X2APIC if XEN_PVH && X86_64 help Support running as a Xen Dom0 guest. + +config XEN_PV_MSR_SAFE + bool "Always use safe MSR accesses in PV guests" + default y + depends on XEN_PV + help + Use safe (not faulting) MSR access functions even if the MSR access + should not fault anyway. + The default can be changed by using the "xen_msr_safe" boot parameter. diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 8c2acccebfe1..0ad3d4bf52b3 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -108,6 +108,16 @@ struct tls_descs { */ static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc); +static __read_mostly bool xen_msr_safe = IS_ENABLED(CONFIG_XEN_PV_MSR_SAFE); + +static int __init parse_xen_msr_safe(char *str) +{ + if (str) + return strtobool(str, &xen_msr_safe); + return -EINVAL; +} +early_param("xen_msr_safe", parse_xen_msr_safe); + static void __init xen_pv_init_platform(void) { /* PV guests can't operate virtio devices without grants. */ @@ -1010,22 +1020,16 @@ static int xen_write_msr_safe(unsigned int msr, unsigned int low, static u64 xen_read_msr(unsigned int msr) { - /* - * This will silently swallow a #GP from RDMSR. It may be worth - * changing that. - */ int err; - return xen_read_msr_safe(msr, &err); + return xen_do_read_msr(msr, xen_msr_safe ? &err : NULL); } static void xen_write_msr(unsigned int msr, unsigned low, unsigned high) { - /* - * This will silently swallow a #GP from WRMSR. It may be worth - * changing that. - */ - xen_write_msr_safe(msr, low, high); + int err; + + xen_do_write_msr(msr, low, high, xen_msr_safe ? &err : NULL); } /* This is called once we have the cpu_possible_mask */ From 7880672bdc975daa586e8256714d9906d30c615e Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Fri, 7 Oct 2022 21:35:00 +0100 Subject: [PATCH 15/15] xen: Kconfig: Fix spelling mistake "Maxmium" -> "Maximum" There is a spelling mistake in a Kconfig description. Fix it. Signed-off-by: Colin Ian King Acked-by: Stefano Stabellini Link: https://lore.kernel.org/r/20221007203500.2756787-1-colin.i.king@gmail.com Signed-off-by: Juergen Gross --- drivers/xen/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig index a65bd92121a5..d5d7c402b651 100644 --- a/drivers/xen/Kconfig +++ b/drivers/xen/Kconfig @@ -56,7 +56,7 @@ config XEN_MEMORY_HOTPLUG_LIMIT depends on XEN_HAVE_PVMMU depends on MEMORY_HOTPLUG help - Maxmium amount of memory (in GiB) that a PV guest can be + Maximum amount of memory (in GiB) that a PV guest can be expanded to when using memory hotplug. A PV guest can have more memory than this limit if is