On 16-Nov-18 2:34 AM, Tone Zhang (Arm Technology China) wrote:
Hi Anatoly,
I have some comments.
-----Original Message-----
From: Tone Zhang (Arm Technology China)
Sent: Thursday, November 15, 2018 8:49 AM
To: Burakov, Anatoly <anatoly.bura...@intel.com>; dev@dpdk.org
Cc: Gavin Hu (Arm Technology China) <gavin...@arm.com>; Honnappa
Nagarahalli <honnappa.nagaraha...@arm.com>; Steve Capper
<steve.cap...@arm.com>; nd <n...@arm.com>
Subject: RE: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
driver
Hi Anatoly,
Sorry for the late response.
-----Original Message-----
From: Burakov, Anatoly <anatoly.bura...@intel.com>
Sent: Friday, November 9, 2018 8:15 PM
To: Tone Zhang (Arm Technology China) <tone.zh...@arm.com>;
dev@dpdk.org
Cc: Gavin Hu (Arm Technology China) <gavin...@arm.com>; Honnappa
Nagarahalli <honnappa.nagaraha...@arm.com>; Steve Capper
<steve.cap...@arm.com>; nd <n...@arm.com>
Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with
vfio-pci driver
On 09-Nov-18 5:57 AM, tone.zhang wrote:
With a larger PAGE_SIZE it is possible for the MSI table to very
close to the end of the BAR s.t. when we align the start and end of
the MSI table to the PAGE_SIZE, the end offset of the MSI table is
out of the PCI BAR boundary.
This patch addresses the issue by comparing both the start and the
end offset of the MSI table with the BAR size, and skip the mapping
if it is out of Bar scope.
The patch fixes the debug log as below:
EAL: Skipping BAR0
Signed-off-by: tone.zhang <tone.zh...@arm.com>
Reviewed-by: Gavin Hu <gavin...@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
Reviewed-by: Steve Capper <steve.cap...@arm.com>
Reviewed-by: Burakov Anatoly <anatoly.bura...@intel.com>
In the future, please don't include my Reviewed tag unless i actually
sent one :)
Thanks a lot! Will keep in mind. 😊
---
drivers/bus/pci/linux/pci_vfio.c | 36
+++++++++++++++++++++++++++++++---
--
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/pci/linux/pci_vfio.c
b/drivers/bus/pci/linux/pci_vfio.c
index 305cc06..9a0affe 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
mapped_pci_resource *vfio_res,
struct pci_msix_table *msix_table = &vfio_res->msix_table;
struct pci_map *bar = &vfio_res->maps[bar_index];
- if (bar->size == 0)
+ if (bar->size == 0) {
/* Skip this BAR */
+ RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
return 0;
+ }
if (msix_table->bar_index == bar_index) {
/*
@@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
mapped_pci_resource *vfio_res,
*/
uint32_t table_start = msix_table->offset;
uint32_t table_end = table_start + msix_table->size;
- table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
- table_start &= PAGE_MASK;
+ table_end = RTE_ALIGN(table_end, PAGE_SIZE);
+ table_start = RTE_ALIGN(table_start, PAGE_SIZE);
+ /* after rounding to PAGE_SIZE, it is over the bar->size,
+ * fall back to the MSI-X table offset in the bar and
+ * align with PAGE_SIZE.
+ */
Minor nitpick - wording of comment could be better, for example:
if page-aligned start of MSI-X table is beyond BAR size, shrink the
mapping size to MSI-X table start address.
Also, probably needs newline before comment.
Will update the code in next version. Thanks!
+ if (table_start >= bar->size) {
+ table_start = RTE_ALIGN_FLOOR(msix_table->offset,
+ PAGE_SIZE);
+ /* after aligning with PAGE_SIZE, if it is less than
+ * the MSI-X table offset, continue falling back to
+ * the actual MSI-X table offset in the bar.
+ */
Same here, wording could probably be improved. Suggested rewording:
If MSI-X table address, floor-aligned by page size, is lower than
actual MSI-X table offset, fall back to using MSI-X table offset as table start.
Now that i think of it, this could really be expressed like this:
uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
table_start = RTE_MAX(aligned, msix_table_offset);
I believe this would be much clearer.
Will update the patch.
When enter the judgement, it implies the "msix_table->offset" is NOT page size
aligned, I think we can replace the code in the judgement with one line: table_start =
msix_table->offset;
It looks more simple. What's your opinion? Thanks!
Agree, what was i thinking :D
+ if (table_start < msix_table->offset)
+ table_start = msix_table->offset;
+ }
if (table_start == 0 && table_end >= bar->size) {
/* Cannot map this BAR */
@@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
mapped_pci_resource *vfio_res,
memreg[0].offset = bar->offset;
memreg[0].size = table_start;
- memreg[1].offset = bar->offset + table_end;
- memreg[1].size = bar->size - table_end;
+ if (bar->size < table_end) {
+ /*
+ * after rounding to PAGE_SIZE we don't have any space
+ * left after the MSI table, so don't try and map it.
+ */
Suggested rewording:
If MSI-X table end is beyond BAR end, don't attempt to perform second
mapping.
Thanks a lot. Will update.
+ memreg[1].offset = 0;
+ memreg[1].size = 0;
+ }
+ else {
+ memreg[1].offset = bar->offset + table_end;
+ memreg[1].size = bar->size - table_end;
+ }
RTE_LOG(DEBUG, EAL,
"Trying to map BAR%d that contains the MSI-X "
However, the patch can go in as is if needed, so
Reviewed-by: Anatoly Burakov <anatoly.bura...@intel.com>
Thanks! 😉
--
Thanks,
Anatoly
Br,
Tone
Br,
Tone
--
Thanks,
Anatoly
