Hi Burakov,
I'm sorry for the late response.
Thanks a lot for your comments. Please find my response below (marked with
"Tone:"). 😊
Br,
Tone
-----Original Message-----
From: Burakov, Anatoly <anatoly.bura...@intel.com>
Sent: Wednesday, October 24, 2018 5:09 PM
To: Tone Zhang (Arm Technology China) <tone.zh...@arm.com>; dev@dpdk.org
Cc: nd <n...@arm.com>
Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with
vfio-pci driver
On 24-Oct-18 3:20 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 MSI table to the
> PAGE_SIZE, the end offset of the MSI table is out 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.
>
> 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>
> ---
> drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c
> b/drivers/bus/pci/linux/pci_vfio.c
> index b1f0683..1373345 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 this BAR%d\n", bar_index);
> return 0;
I feel like "this" is unnecessary here - just "Skipping BAR%d" should be enough
:)
Tone: Will update code and remove "this" in next version.
> + }
>
> if (msix_table->bar_index == bar_index) {
> /*
> @@ -457,7 +459,12 @@ 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_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
IMO these two additions should be replaced by RTE_ALIGN by page size.
Makes the purpose of the code much clearer.
Tone: Sure, it is better! Will update code in next version. Thanks!
> + /* after rounding to PAGE_SIZE, it is over the bar->size,
> + * fall back to the MSI-X table offset in the bar.
> + */
> + if (table_start >= bar->size)
> + table_start = msix_table->offset;
If i understand things correctly, msix_table->offset value here may be
unaligned, so falling back to this value may cause mapping failure, because we
later use this value as a size of mapping (which needs to be page aligned).
Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR
maybe get 0 if the offset is less than page size in the PCI bar. It will
trigger mmap() error. IIRC the input parameter "size" in mmap() is not required
to be aligned with page size, system will do it. But it is better if we can do
it. If I was wrong, please correct me. Thanks a lot.
>
> if (table_start == 0 && table_end >= bar->size) {
> /* Cannot map this BAR */
> @@ -469,8 +476,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.
> + */
> + 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 "
>
--
Thanks,
Anatoly
