From: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
To: xen-devel@lists.xenproject.org
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
"Jan Beulich" <jbeulich@suse.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>
Subject: [PATCH v5 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table
Date: Wed, 13 Mar 2024 16:16:08 +0100 [thread overview]
Message-ID: <a040f703a884ff4516314f88b22ee0f9f17329a9.1710342968.git-series.marmarek@invisiblethingslab.com> (raw)
In-Reply-To: <cover.afa2d89161590f5193dd6bfd340c5e9347877aae.1710342968.git-series.marmarek@invisiblethingslab.com>
Some devices (notably Intel Wifi 6 AX210 card) keep auxiliary registers
on the same page as MSI-X table. Device model (especially one in
stubdomain) cannot really handle those, as direct writes to that page is
refused (page is on the mmio_ro_ranges list). Instead, extend
msixtbl_mmio_ops to handle such accesses too.
Doing this, requires correlating read/write location with guest
of MSI-X table address. Since QEMU doesn't map MSI-X table to the guest,
it requires msixtbl_entry->gtable, which is HVM-only. Similar feature
for PV would need to be done separately.
This will be also used to read Pending Bit Array, if it lives on the same
page, making QEMU not needing /dev/mem access at all (especially helpful
with lockdown enabled in dom0). If PBA lives on another page, QEMU will
map it to the guest directly.
If PBA lives on the same page, discard writes and log a message.
Technically, writes outside of PBA could be allowed, but at this moment
the precise location of PBA isn't saved, and also no known device abuses
the spec in this way (at least yet).
To access those registers, msixtbl_mmio_ops need the relevant page
mapped. MSI handling already has infrastructure for that, using fixmap,
so try to map first/last page of the MSI-X table (if necessary) and save
their fixmap indexes. Note that msix_get_fixmap() does reference
counting and reuses existing mapping, so just call it directly, even if
the page was mapped before. Also, it uses a specific range of fixmap
indexes which doesn't include 0, so use 0 as default ("not mapped")
value - which simplifies code a bit.
GCC 12.2.1 gets confused about 'desc' variable:
arch/x86/hvm/vmsi.c: In function ‘msixtbl_range’:
arch/x86/hvm/vmsi.c:553:8: error: ‘desc’ may be used uninitialized [-Werror=maybe-uninitialized]
553 | if ( desc )
| ^
arch/x86/hvm/vmsi.c:537:28: note: ‘desc’ was declared here
537 | const struct msi_desc *desc;
| ^~~~
It's conditional initialization is actually correct (in the case where
it isn't initialized, function returns early), but to avoid
build failure initialize it explicitly to NULL anyway.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v5:
- style fixes
- include GCC version in the commit message
- warn only once (per domain, per device) about failed adjacent access
Changes in v4:
- drop same_page parameter of msixtbl_find_entry(), distinguish two
cases in relevant callers
- rename adj_access_table_idx to adj_access_idx
- code style fixes
- drop alignment check in adjacent_{read,write}() - all callers already
have it earlier
- delay mapping first/last MSI-X pages until preparing device for a
passthrough
v3:
- merge handling into msixtbl_mmio_ops
- extend commit message
v2:
- adjust commit message
- pass struct domain to msixtbl_page_handler_get_hwaddr()
- reduce local variables used only once
- log a warning if write is forbidden if MSI-X and PBA lives on the same
page
- do not passthrough unaligned accesses
- handle accesses both before and after MSI-X table
---
xen/arch/x86/hvm/vmsi.c | 209 ++++++++++++++++++++++++++++++++--
xen/arch/x86/include/asm/msi.h | 7 +-
xen/arch/x86/msi.c | 41 +++++++-
3 files changed, 247 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 999917983789..5ac3d9d9b4cf 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -180,6 +180,10 @@ static bool msixtbl_initialised(const struct domain *d)
return d->arch.hvm.msixtbl_list.next;
}
+/*
+ * Lookup an msixtbl_entry on the same page as given addr. It's up to the
+ * caller to check if address is strictly part of the table - if relevant.
+ */
static struct msixtbl_entry *msixtbl_find_entry(
struct vcpu *v, unsigned long addr)
{
@@ -187,8 +191,8 @@ static struct msixtbl_entry *msixtbl_find_entry(
struct domain *d = v->domain;
list_for_each_entry( entry, &d->arch.hvm.msixtbl_list, list )
- if ( addr >= entry->gtable &&
- addr < entry->gtable + entry->table_len )
+ if ( PFN_DOWN(addr) >= PFN_DOWN(entry->gtable) &&
+ PFN_DOWN(addr) <= PFN_DOWN(entry->gtable + entry->table_len - 1) )
return entry;
return NULL;
@@ -213,6 +217,148 @@ static struct msi_desc *msixtbl_addr_to_desc(
return NULL;
}
+/*
+ * Returns:
+ * - ADJACENT_DONT_HANDLE if no handling should be done
+ * - ADJACENT_DISCARD_WRITE if write should be discarded
+ * - a fixmap idx to use for handling
+ */
+#define ADJACENT_DONT_HANDLE UINT_MAX
+#define ADJACENT_DISCARD_WRITE (UINT_MAX - 1)
+static unsigned int adjacent_handle(
+ const struct msixtbl_entry *entry, unsigned long addr, bool write)
+{
+ unsigned int adj_type;
+ struct arch_msix *msix;
+
+ if ( !entry || !entry->pdev )
+ return ADJACENT_DONT_HANDLE;
+
+ if ( PFN_DOWN(addr) == PFN_DOWN(entry->gtable) && addr < entry->gtable )
+ adj_type = ADJ_IDX_FIRST;
+ else if ( PFN_DOWN(addr) == PFN_DOWN(entry->gtable + entry->table_len - 1) &&
+ addr >= entry->gtable + entry->table_len )
+ adj_type = ADJ_IDX_LAST;
+ else
+ return ADJACENT_DONT_HANDLE;
+
+ msix = entry->pdev->msix;
+ ASSERT(msix);
+
+ if ( !msix->adj_access_idx[adj_type] )
+ {
+ if ( msix->warned_domid != entry->pdev->domain->domain_id )
+ {
+ msix->warned_domid = entry->pdev->domain->domain_id;
+ msix->warned_kind.all = 0;
+ }
+ if ( !msix->warned_kind.u.adjacent_not_initialized )
+ {
+ msix->warned_kind.u.adjacent_not_initialized = true;
+ gprintk(XENLOG_WARNING,
+ "Page for adjacent(%d) MSI-X table access not initialized for %pp (addr %#lx, gtable %#lx\n",
+ adj_type, &entry->pdev->sbdf, addr, entry->gtable);
+ }
+ return ADJACENT_DONT_HANDLE;
+ }
+
+ /* If PBA lives on the same page too, discard writes. */
+ if ( write &&
+ ((adj_type == ADJ_IDX_LAST &&
+ msix->table.last == msix->pba.first) ||
+ (adj_type == ADJ_IDX_FIRST &&
+ msix->table.first == msix->pba.last)) )
+ {
+ if ( msix->warned_domid != entry->pdev->domain->domain_id )
+ {
+ msix->warned_domid = entry->pdev->domain->domain_id;
+ msix->warned_kind.all = 0;
+ }
+ if ( !msix->warned_kind.u.adjacent_pba )
+ {
+ msix->warned_kind.u.adjacent_pba = true;
+ gprintk(XENLOG_WARNING,
+ "MSI-X table and PBA of %pp live on the same page, "
+ "writing to other registers there is not implemented\n",
+ &entry->pdev->sbdf);
+ }
+ return ADJACENT_DISCARD_WRITE;
+ }
+
+ return msix->adj_access_idx[adj_type];
+}
+
+static int adjacent_read(
+ unsigned int fixmap_idx,
+ paddr_t address, unsigned int len, uint64_t *pval)
+{
+ const void __iomem *hwaddr;
+
+ *pval = ~0UL;
+
+ ASSERT(fixmap_idx != ADJACENT_DISCARD_WRITE);
+
+ hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
+
+ switch ( len )
+ {
+ case 1:
+ *pval = readb(hwaddr);
+ break;
+
+ case 2:
+ *pval = readw(hwaddr);
+ break;
+
+ case 4:
+ *pval = readl(hwaddr);
+ break;
+
+ case 8:
+ *pval = readq(hwaddr);
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ }
+ return X86EMUL_OKAY;
+}
+
+static int adjacent_write(
+ unsigned int fixmap_idx,
+ paddr_t address, unsigned int len, uint64_t val)
+{
+ void __iomem *hwaddr;
+
+ if ( fixmap_idx == ADJACENT_DISCARD_WRITE )
+ return X86EMUL_OKAY;
+
+ hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
+
+ switch ( len )
+ {
+ case 1:
+ writeb(val, hwaddr);
+ break;
+
+ case 2:
+ writew(val, hwaddr);
+ break;
+
+ case 4:
+ writel(val, hwaddr);
+ break;
+
+ case 8:
+ writeq(val, hwaddr);
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ }
+ return X86EMUL_OKAY;
+}
+
static int cf_check msixtbl_read(
const struct hvm_io_handler *handler, uint64_t address, uint32_t len,
uint64_t *pval)
@@ -220,9 +366,10 @@ static int cf_check msixtbl_read(
unsigned long offset;
struct msixtbl_entry *entry;
unsigned int nr_entry, index;
+ unsigned int adjacent_fixmap;
int r = X86EMUL_UNHANDLEABLE;
- if ( (len != 4 && len != 8) || (address & (len - 1)) )
+ if ( !IS_ALIGNED(address, len) )
return r;
rcu_read_lock(&msixtbl_rcu_lock);
@@ -230,6 +377,21 @@ static int cf_check msixtbl_read(
entry = msixtbl_find_entry(current, address);
if ( !entry )
goto out;
+
+ adjacent_fixmap = adjacent_handle(entry, address, false);
+ if ( adjacent_fixmap != ADJACENT_DONT_HANDLE )
+ {
+ r = adjacent_read(adjacent_fixmap, address, len, pval);
+ goto out;
+ }
+
+ if ( address < entry->gtable ||
+ address >= entry->gtable + entry->table_len )
+ goto out;
+
+ if ( len != 4 && len != 8 )
+ goto out;
+
offset = address & (PCI_MSIX_ENTRY_SIZE - 1);
if ( offset != PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
@@ -282,6 +444,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
int r = X86EMUL_UNHANDLEABLE;
unsigned long flags;
struct irq_desc *desc;
+ unsigned int adjacent_fixmap;
if ( !IS_ALIGNED(address, len) )
return X86EMUL_OKAY;
@@ -291,6 +454,21 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
entry = msixtbl_find_entry(v, address);
if ( !entry )
goto out;
+
+ adjacent_fixmap = adjacent_handle(entry, address, true);
+ if ( adjacent_fixmap != ADJACENT_DONT_HANDLE )
+ {
+ r = adjacent_write(adjacent_fixmap, address, len, val);
+ goto out;
+ }
+
+ if ( address < entry->gtable ||
+ address >= entry->gtable + entry->table_len )
+ goto out;
+
+ if ( len != 4 && len != 8 )
+ goto out;
+
nr_entry = array_index_nospec(((address - entry->gtable) /
PCI_MSIX_ENTRY_SIZE),
MAX_MSIX_TABLE_ENTRIES);
@@ -356,8 +534,8 @@ static int cf_check _msixtbl_write(
const struct hvm_io_handler *handler, uint64_t address, uint32_t len,
uint64_t val)
{
- /* Ignore invalid length or unaligned writes. */
- if ( (len != 4 && len != 8) || !IS_ALIGNED(address, len) )
+ /* Ignore unaligned writes. */
+ if ( !IS_ALIGNED(address, len) )
return X86EMUL_OKAY;
/*
@@ -374,14 +552,22 @@ static bool cf_check msixtbl_range(
{
struct vcpu *curr = current;
unsigned long addr = r->addr;
- const struct msi_desc *desc;
+ const struct msixtbl_entry *entry;
+ const struct msi_desc *desc = NULL;
+ unsigned int adjacent_fixmap;
ASSERT(r->type == IOREQ_TYPE_COPY);
rcu_read_lock(&msixtbl_rcu_lock);
- desc = msixtbl_addr_to_desc(msixtbl_find_entry(curr, addr), addr);
+ entry = msixtbl_find_entry(curr, addr);
+ adjacent_fixmap = adjacent_handle(entry, addr, false);
+ if ( adjacent_fixmap == ADJACENT_DONT_HANDLE )
+ desc = msixtbl_addr_to_desc(entry, addr);
rcu_read_unlock(&msixtbl_rcu_lock);
+ if ( adjacent_fixmap != ADJACENT_DONT_HANDLE )
+ return 1;
+
if ( desc )
return 1;
@@ -622,12 +808,15 @@ void msix_write_completion(struct vcpu *v)
v->arch.hvm.hvm_io.msix_snoop_gpa )
{
unsigned int token = hvmemul_cache_disable(v);
- const struct msi_desc *desc;
+ const struct msi_desc *desc = NULL;
+ const struct msixtbl_entry *entry;
uint32_t data;
rcu_read_lock(&msixtbl_rcu_lock);
- desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
- snoop_addr);
+ entry = msixtbl_find_entry(v, snoop_addr);
+ if ( entry && snoop_addr >= entry->gtable &&
+ snoop_addr < entry->gtable + entry->table_len )
+ desc = msixtbl_addr_to_desc(entry, snoop_addr);
rcu_read_unlock(&msixtbl_rcu_lock);
if ( desc &&
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 19b1e6631fdf..400944682b17 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -208,6 +208,10 @@ struct msg_address {
PCI_MSIX_ENTRY_SIZE + \
(~PCI_MSIX_BIRMASK & (PAGE_SIZE - 1)))
+/* Indexes in adj_access_idx[] below */
+#define ADJ_IDX_FIRST 0
+#define ADJ_IDX_LAST 1
+
struct arch_msix {
unsigned int nr_entries, used_entries;
struct {
@@ -215,6 +219,7 @@ struct arch_msix {
} table, pba;
int table_refcnt[MAX_MSIX_TABLE_PAGES];
int table_idx[MAX_MSIX_TABLE_PAGES];
+ unsigned int adj_access_idx[2];
spinlock_t table_lock;
bool host_maskall, guest_maskall;
domid_t warned_domid;
@@ -222,6 +227,8 @@ struct arch_msix {
uint8_t all;
struct {
bool maskall : 1;
+ bool adjacent_not_initialized : 1;
+ bool adjacent_pba : 1;
} u;
} warned_kind;
};
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 6433df30bd60..b4aa0e53be2c 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -921,6 +921,36 @@ static int msix_capability_init(struct pci_dev *dev,
list_add_tail(&entry->list, &dev->msi_list);
*desc = entry;
}
+ else
+ {
+ /*
+ * If the MSI-X table doesn't start at the page boundary, map the first page for
+ * passthrough accesses.
+ */
+ if ( PAGE_OFFSET(table_paddr) )
+ {
+ int idx = msix_get_fixmap(msix, table_paddr, table_paddr);
+
+ if ( idx > 0 )
+ msix->adj_access_idx[ADJ_IDX_FIRST] = idx;
+ else
+ gprintk(XENLOG_ERR, "Failed to map first MSI-X table page: %d\n", idx);
+ }
+ /*
+ * If the MSI-X table doesn't end on the page boundary, map the last page
+ * for passthrough accesses.
+ */
+ if ( PAGE_OFFSET(table_paddr + msix->nr_entries * PCI_MSIX_ENTRY_SIZE) )
+ {
+ uint64_t entry_paddr = table_paddr + msix->nr_entries * PCI_MSIX_ENTRY_SIZE;
+ int idx = msix_get_fixmap(msix, table_paddr, entry_paddr);
+
+ if ( idx > 0 )
+ msix->adj_access_idx[ADJ_IDX_LAST] = idx;
+ else
+ gprintk(XENLOG_ERR, "Failed to map last MSI-X table page: %d\n", idx);
+ }
+ }
if ( !msix->used_entries )
{
@@ -1087,6 +1117,17 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
msix->table.first = 0;
msix->table.last = 0;
+ if ( msix->adj_access_idx[ADJ_IDX_FIRST] )
+ {
+ msix_put_fixmap(msix, msix->adj_access_idx[ADJ_IDX_FIRST]);
+ msix->adj_access_idx[ADJ_IDX_FIRST] = 0;
+ }
+ if ( msix->adj_access_idx[ADJ_IDX_LAST] )
+ {
+ msix_put_fixmap(msix, msix->adj_access_idx[ADJ_IDX_LAST]);
+ msix->adj_access_idx[ADJ_IDX_LAST] = 0;
+ }
+
if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
msix->pba.last) )
WARN();
--
git-series 0.9.1
next prev parent reply other threads:[~2024-03-13 15:17 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-13 15:16 [PATCH v5 0/7] MSI-X support with qemu in stubdomain, and other related changes Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 1/7] x86/msi: passthrough all MSI-X vector ctrl writes to device model Marek Marczykowski-Górecki
2024-04-25 10:45 ` Jan Beulich
2024-04-29 8:40 ` Roger Pau Monné
2024-03-13 15:16 ` [PATCH v5 2/7] x86/msi: Extend per-domain/device warning mechanism Marek Marczykowski-Górecki
2024-04-25 11:25 ` Jan Beulich
2024-03-13 15:16 ` Marek Marczykowski-Górecki [this message]
2024-04-25 11:15 ` [PATCH v5 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table Jan Beulich
2024-04-26 15:26 ` Marek Marczykowski-Górecki
2024-04-29 6:37 ` Jan Beulich
2024-03-13 15:16 ` [PATCH v5 4/7] automation: prevent QEMU access to /dev/mem in PCI passthrough tests Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 5/7] automation: switch to a wifi card on ADL system Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 6/7] [DO NOT APPLY] switch to qemu fork Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 7/7] [DO NOT APPLY] switch to alternative artifact repo Marek Marczykowski-Górecki
2024-04-18 14:18 ` [PATCH v5 0/7] MSI-X support with qemu in stubdomain, and other related changes Marek Marczykowski-Górecki
2024-04-18 14:22 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a040f703a884ff4516314f88b22ee0f9f17329a9.1710342968.git-series.marmarek@invisiblethingslab.com \
--to=marmarek@invisiblethingslab.com \
--cc=andrew.cooper3@citrix.com \
--cc=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.