Hi Burakov,
Thanks!
Please check my feedback below.
Br,
Tone
-----Original Message-----
From: dev <dev-boun...@dpdk.org> On Behalf Of Burakov, Anatoly
Sent: Thursday, November 1, 2018 6:01 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 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
> 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.
Apologies, you're correct - length can be misaligned (just tested it).
However, i think it's still worth aligning (and putting in an additional
check), because we want to make sure we *don't* attempt to map the MSI-X BAR,
and kernel might do that by adjusting length automatically and return mmap
failure that way.
Tone: Thanks a lot! I agree with you. It worth aligning the size. I will update
code (RTE_ALIGN_FLOOR by page size) in next version.
I'd like to discuss one case with you. In the case, base->size is 16384,
msix_table->offset is 8192, page_size is 65536. After align
"msix_table->offset" with page_size (RTE_ALIGN_FLOOR), the value of
"table_start" is 0, mmap() will report error, and the memory mapping is failed.
For the case (table_start is 0 after the aglinment), may I continue falling
back the "table_start" to " msix_table->offset" (not aligned with page size),
and left system adjust the length automatically? Thanks!
--
Thanks,
Anatoly
