1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
|
From 85100ed78ca18f188b1ca495f132db7df705f1a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Roger=20Pau=20Monn=C3=A9?= <roger.pau@citrix.com>
Date: Fri, 31 Mar 2023 08:34:26 +0200
Subject: [PATCH 83/89] vpci/msix: handle accesses adjacent to the MSI-X table
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The handling of the MSI-X table accesses by Xen requires that any
pages part of the MSI-X related tables are not mapped into the domain
physmap. As a result, any device registers in the same pages as the
start or the end of the MSIX or PBA tables is not currently
accessible, as the accesses are just dropped.
Note the spec forbids such placing of registers, as the MSIX and PBA
tables must be 4K isolated from any other registers:
"If a Base Address register that maps address space for the MSI-X
Table or MSI-X PBA also maps other usable address space that is not
associated with MSI-X structures, locations (e.g., for CSRs) used in
the other address space must not share any naturally aligned 4-KB
address range with one where either MSI-X structure resides."
Yet the 'Intel Wi-Fi 6 AX201' device on one of my boxes has registers
in the same page as the MSIX tables, and thus won't work on a PVH dom0
without this fix.
In order to cope with the behavior passthrough any accesses that fall
on the same page as the MSIX tables (but don't fall in between) to the
underlying hardware. Such forwarding also takes care of the PBA
accesses, so it allows to remove the code doing this handling in
msix_{read,write}. Note that as a result accesses to the PBA array
are no longer limited to 4 and 8 byte sizes, there's no access size
restriction for PBA accesses documented in the specification.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
vpci/msix: restore PBA access length and alignment restrictions
Accesses to the PBA array have the same length and alignment
limitations as accesses to the MSI-X table:
"For all accesses to MSI-X Table and MSI-X PBA fields, software must
use aligned full DWORD or aligned full QWORD transactions; otherwise,
the result is undefined."
Introduce such length and alignment checks into the handling of PBA
accesses for vPCI. This was a mistake of mine for not reading the
specification correctly.
Note that accesses must now be aligned, and hence there's no longer a
need to check that the end of the access falls into the PBA region as
both the access and the region addresses must be aligned.
Fixes: b177892d2d ('vpci/msix: handle accesses adjacent to the MSI-X table')
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
master commit: b177892d2d0e8a31122c218989f43130aeba5282
master date: 2023-03-28 14:20:35 +0200
master commit: 7a502b4fbc339e9d3d3d45fb37f09da06bc3081c
master date: 2023-03-29 14:56:33 +0200
---
xen/drivers/vpci/msix.c | 353 +++++++++++++++++++++++++++++-----------
xen/drivers/vpci/vpci.c | 7 +-
xen/include/xen/vpci.h | 8 +-
3 files changed, 273 insertions(+), 95 deletions(-)
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index bea0cc7aed..cafddcf305 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -27,6 +27,11 @@
((addr) >= vmsix_table_addr(vpci, nr) && \
(addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
+#define VMSIX_ADDR_SAME_PAGE(addr, vpci, nr) \
+ (PFN_DOWN(addr) >= PFN_DOWN(vmsix_table_addr(vpci, nr)) && \
+ PFN_DOWN(addr) <= PFN_DOWN(vmsix_table_addr(vpci, nr) + \
+ vmsix_table_size(vpci, nr) - 1))
+
static uint32_t cf_check control_read(
const struct pci_dev *pdev, unsigned int reg, void *data)
{
@@ -149,7 +154,7 @@ static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr)
for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
- VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
+ VMSIX_ADDR_SAME_PAGE(addr, msix->pdev->vpci, i) )
return msix;
}
@@ -182,36 +187,172 @@ static struct vpci_msix_entry *get_entry(struct vpci_msix *msix,
return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
}
-static void __iomem *get_pba(struct vpci *vpci)
+static void __iomem *get_table(struct vpci *vpci, unsigned int slot)
{
struct vpci_msix *msix = vpci->msix;
+ paddr_t addr = 0;
+
+ ASSERT(spin_is_locked(&vpci->lock));
+
+ if ( likely(msix->table[slot]) )
+ return msix->table[slot];
+
+ switch ( slot )
+ {
+ case VPCI_MSIX_TBL_TAIL:
+ addr = vmsix_table_size(vpci, VPCI_MSIX_TABLE);
+ fallthrough;
+ case VPCI_MSIX_TBL_HEAD:
+ addr += vmsix_table_addr(vpci, VPCI_MSIX_TABLE);
+ break;
+
+ case VPCI_MSIX_PBA_TAIL:
+ addr = vmsix_table_size(vpci, VPCI_MSIX_PBA);
+ fallthrough;
+ case VPCI_MSIX_PBA_HEAD:
+ addr += vmsix_table_addr(vpci, VPCI_MSIX_PBA);
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ return NULL;
+ }
+
+ msix->table[slot] = ioremap(round_pgdown(addr), PAGE_SIZE);
+
+ return msix->table[slot];
+}
+
+unsigned int get_slot(const struct vpci *vpci, unsigned long addr)
+{
+ unsigned long pfn = PFN_DOWN(addr);
+
/*
- * PBA will only be unmapped when the device is deassigned, so access it
- * without holding the vpci lock.
+ * The logic below relies on having the tables identity mapped to the guest
+ * address space, or for the `addr` parameter to be translated into its
+ * host physical memory address equivalent.
*/
- void __iomem *pba = read_atomic(&msix->pba);
- if ( likely(pba) )
- return pba;
+ if ( pfn == PFN_DOWN(vmsix_table_addr(vpci, VPCI_MSIX_TABLE)) )
+ return VPCI_MSIX_TBL_HEAD;
+ if ( pfn == PFN_DOWN(vmsix_table_addr(vpci, VPCI_MSIX_TABLE) +
+ vmsix_table_size(vpci, VPCI_MSIX_TABLE) - 1) )
+ return VPCI_MSIX_TBL_TAIL;
+ if ( pfn == PFN_DOWN(vmsix_table_addr(vpci, VPCI_MSIX_PBA)) )
+ return VPCI_MSIX_PBA_HEAD;
+ if ( pfn == PFN_DOWN(vmsix_table_addr(vpci, VPCI_MSIX_PBA) +
+ vmsix_table_size(vpci, VPCI_MSIX_PBA) - 1) )
+ return VPCI_MSIX_PBA_TAIL;
+
+ ASSERT_UNREACHABLE();
+ return -1;
+}
+
+static bool adjacent_handle(const struct vpci_msix *msix, unsigned long addr)
+{
+ unsigned int i;
+
+ if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
+ return true;
+
+ if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_TABLE) )
+ return false;
+
+ for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
+ if ( VMSIX_ADDR_SAME_PAGE(addr, msix->pdev->vpci, i) )
+ return true;
+
+ return false;
+}
- pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
- vmsix_table_size(vpci, VPCI_MSIX_PBA));
- if ( !pba )
- return read_atomic(&msix->pba);
+static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
+ unsigned long addr, unsigned int len,
+ unsigned long *data)
+{
+ const void __iomem *mem;
+ struct vpci *vpci = msix->pdev->vpci;
+ unsigned int slot;
+
+ *data = ~0ul;
+
+ if ( !adjacent_handle(msix, addr + len - 1) )
+ return X86EMUL_OKAY;
+
+ if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
+ !access_allowed(msix->pdev, addr, len) )
+ /* PBA accesses must be aligned and 4 or 8 bytes in size. */
+ return X86EMUL_OKAY;
+
+ slot = get_slot(vpci, addr);
+ if ( slot >= ARRAY_SIZE(msix->table) )
+ return X86EMUL_OKAY;
+
+ if ( unlikely(!IS_ALIGNED(addr, len)) )
+ {
+ unsigned int i;
+
+ gprintk(XENLOG_DEBUG, "%pp: unaligned read to MSI-X related page\n",
+ &msix->pdev->sbdf);
+
+ /*
+ * Split unaligned accesses into byte sized ones. Shouldn't happen in
+ * the first place, but devices shouldn't have registers in the same 4K
+ * page as the MSIX tables either.
+ *
+ * It's unclear whether this could cause issues if a guest expects
+ * registers to be accessed atomically, it better use an aligned access
+ * if it has such expectations.
+ */
+ for ( i = 0; i < len; i++ )
+ {
+ unsigned long partial = ~0ul;
+ int rc = adjacent_read(d, msix, addr + i, 1, &partial);
+
+ if ( rc != X86EMUL_OKAY )
+ return rc;
+
+ *data &= ~(0xfful << (i * 8));
+ *data |= (partial & 0xff) << (i * 8);
+ }
+
+ return X86EMUL_OKAY;
+ }
spin_lock(&vpci->lock);
- if ( !msix->pba )
+ mem = get_table(vpci, slot);
+ if ( !mem )
{
- write_atomic(&msix->pba, pba);
spin_unlock(&vpci->lock);
+ gprintk(XENLOG_WARNING,
+ "%pp: unable to map MSI-X page, returning all bits set\n",
+ &msix->pdev->sbdf);
+ return X86EMUL_OKAY;
}
- else
+
+ switch ( len )
{
- spin_unlock(&vpci->lock);
- iounmap(pba);
+ case 1:
+ *data = readb(mem + PAGE_OFFSET(addr));
+ break;
+
+ case 2:
+ *data = readw(mem + PAGE_OFFSET(addr));
+ break;
+
+ case 4:
+ *data = readl(mem + PAGE_OFFSET(addr));
+ break;
+
+ case 8:
+ *data = readq(mem + PAGE_OFFSET(addr));
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
}
+ spin_unlock(&vpci->lock);
- return read_atomic(&msix->pba);
+ return X86EMUL_OKAY;
}
static int cf_check msix_read(
@@ -227,47 +368,11 @@ static int cf_check msix_read(
if ( !msix )
return X86EMUL_RETRY;
- if ( !access_allowed(msix->pdev, addr, len) )
- return X86EMUL_OKAY;
-
- if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
- {
- struct vpci *vpci = msix->pdev->vpci;
- unsigned int idx = addr - vmsix_table_addr(vpci, VPCI_MSIX_PBA);
- const void __iomem *pba = get_pba(vpci);
-
- /*
- * Access to PBA.
- *
- * TODO: note that this relies on having the PBA identity mapped to the
- * guest address space. If this changes the address will need to be
- * translated.
- */
- if ( !pba )
- {
- gprintk(XENLOG_WARNING,
- "%pp: unable to map MSI-X PBA, report all pending\n",
- &msix->pdev->sbdf);
- return X86EMUL_OKAY;
- }
-
- switch ( len )
- {
- case 4:
- *data = readl(pba + idx);
- break;
-
- case 8:
- *data = readq(pba + idx);
- break;
-
- default:
- ASSERT_UNREACHABLE();
- break;
- }
+ if ( adjacent_handle(msix, addr) )
+ return adjacent_read(d, msix, addr, len, data);
+ if ( !access_allowed(msix->pdev, addr, len) )
return X86EMUL_OKAY;
- }
spin_lock(&msix->pdev->vpci->lock);
entry = get_entry(msix, addr);
@@ -303,56 +408,102 @@ static int cf_check msix_read(
return X86EMUL_OKAY;
}
-static int cf_check msix_write(
- struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
+static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
+ unsigned long addr, unsigned int len,
+ unsigned long data)
{
- const struct domain *d = v->domain;
- struct vpci_msix *msix = msix_find(d, addr);
- struct vpci_msix_entry *entry;
- unsigned int offset;
+ void __iomem *mem;
+ struct vpci *vpci = msix->pdev->vpci;
+ unsigned int slot;
- if ( !msix )
- return X86EMUL_RETRY;
+ if ( !adjacent_handle(msix, addr + len - 1) )
+ return X86EMUL_OKAY;
- if ( !access_allowed(msix->pdev, addr, len) )
+ /*
+ * Only check start and end of the access because the size of the PBA is
+ * assumed to be equal or bigger (8 bytes) than the length of any access
+ * handled here.
+ */
+ if ( VMSIX_ADDR_IN_RANGE(addr, vpci, VPCI_MSIX_PBA) &&
+ (!access_allowed(msix->pdev, addr, len) || !is_hardware_domain(d)) )
+ /* Ignore writes to PBA for DomUs, it's undefined behavior. */
return X86EMUL_OKAY;
- if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
+ slot = get_slot(vpci, addr);
+ if ( slot >= ARRAY_SIZE(msix->table) )
+ return X86EMUL_OKAY;
+
+ if ( unlikely(!IS_ALIGNED(addr, len)) )
{
- struct vpci *vpci = msix->pdev->vpci;
- unsigned int idx = addr - vmsix_table_addr(vpci, VPCI_MSIX_PBA);
- const void __iomem *pba = get_pba(vpci);
+ unsigned int i;
- if ( !is_hardware_domain(d) )
- /* Ignore writes to PBA for DomUs, it's behavior is undefined. */
- return X86EMUL_OKAY;
+ gprintk(XENLOG_DEBUG, "%pp: unaligned write to MSI-X related page\n",
+ &msix->pdev->sbdf);
- if ( !pba )
+ for ( i = 0; i < len; i++ )
{
- /* Unable to map the PBA, ignore write. */
- gprintk(XENLOG_WARNING,
- "%pp: unable to map MSI-X PBA, write ignored\n",
- &msix->pdev->sbdf);
- return X86EMUL_OKAY;
+ int rc = adjacent_write(d, msix, addr + i, 1, data >> (i * 8));
+
+ if ( rc != X86EMUL_OKAY )
+ return rc;
}
- switch ( len )
- {
- case 4:
- writel(data, pba + idx);
- break;
+ return X86EMUL_OKAY;
+ }
- case 8:
- writeq(data, pba + idx);
- break;
+ spin_lock(&vpci->lock);
+ mem = get_table(vpci, slot);
+ if ( !mem )
+ {
+ spin_unlock(&vpci->lock);
+ gprintk(XENLOG_WARNING,
+ "%pp: unable to map MSI-X page, dropping write\n",
+ &msix->pdev->sbdf);
+ return X86EMUL_OKAY;
+ }
- default:
- ASSERT_UNREACHABLE();
- break;
- }
+ switch ( len )
+ {
+ case 1:
+ writeb(data, mem + PAGE_OFFSET(addr));
+ break;
- return X86EMUL_OKAY;
+ case 2:
+ writew(data, mem + PAGE_OFFSET(addr));
+ break;
+
+ case 4:
+ writel(data, mem + PAGE_OFFSET(addr));
+ break;
+
+ case 8:
+ writeq(data, mem + PAGE_OFFSET(addr));
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
}
+ spin_unlock(&vpci->lock);
+
+ return X86EMUL_OKAY;
+}
+
+static int cf_check msix_write(
+ struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
+{
+ const struct domain *d = v->domain;
+ struct vpci_msix *msix = msix_find(d, addr);
+ struct vpci_msix_entry *entry;
+ unsigned int offset;
+
+ if ( !msix )
+ return X86EMUL_RETRY;
+
+ if ( adjacent_handle(msix, addr) )
+ return adjacent_write(d, msix, addr, len, data);
+
+ if ( !access_allowed(msix->pdev, addr, len) )
+ return X86EMUL_OKAY;
spin_lock(&msix->pdev->vpci->lock);
entry = get_entry(msix, addr);
@@ -482,6 +633,26 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
}
}
+ if ( is_hardware_domain(d) )
+ {
+ /*
+ * For dom0 only: remove any hypervisor mappings of the MSIX or PBA
+ * related areas, as dom0 is capable of moving the position of the BARs
+ * in the host address space.
+ *
+ * We rely on being called with the vPCI lock held once the domain is
+ * running, so the maps are not in use.
+ */
+ for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->table); i++ )
+ if ( pdev->vpci->msix->table[i] )
+ {
+ /* If there are any maps, the domain must be running. */
+ ASSERT(spin_is_locked(&pdev->vpci->lock));
+ iounmap(pdev->vpci->msix->table[i]);
+ pdev->vpci->msix->table[i] = NULL;
+ }
+ }
+
return 0;
}
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 6d48d496bb..652807a4a4 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -54,9 +54,12 @@ void vpci_remove_device(struct pci_dev *pdev)
spin_unlock(&pdev->vpci->lock);
if ( pdev->vpci->msix )
{
+ unsigned int i;
+
list_del(&pdev->vpci->msix->next);
- if ( pdev->vpci->msix->pba )
- iounmap(pdev->vpci->msix->pba);
+ for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->table); i++ )
+ if ( pdev->vpci->msix->table[i] )
+ iounmap(pdev->vpci->msix->table[i]);
}
xfree(pdev->vpci->msix);
xfree(pdev->vpci->msi);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index d8acfeba8a..0b8a2a3c74 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -133,8 +133,12 @@ struct vpci {
bool enabled : 1;
/* Masked? */
bool masked : 1;
- /* PBA map */
- void __iomem *pba;
+ /* Partial table map. */
+#define VPCI_MSIX_TBL_HEAD 0
+#define VPCI_MSIX_TBL_TAIL 1
+#define VPCI_MSIX_PBA_HEAD 2
+#define VPCI_MSIX_PBA_TAIL 3
+ void __iomem *table[4];
/* Entries. */
struct vpci_msix_entry {
uint64_t addr;
--
2.40.0
|