DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Tone Zhang (Arm Technology China)" <Tone.Zhang@arm.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver
Date: Sat, 3 Nov 2018 05:46:53 +0000
Message-ID: <AM0PR08MB340919655F35F2C8229728A28FC80@AM0PR08MB3409.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <DB7PR08MB33859242951014EF340C897AE9C80@DB7PR08MB3385.eurprd08.prod.outlook.com>

Hi Burakov,

Thanks!
Please check my feedback below.

Br,
Tone

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
Sent: Thursday, November 1, 2018 6:01 PM
To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
Cc: nd <nd@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.burakov@intel.com>
> Sent: Wednesday, October 24, 2018 5:09 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
> dev@dpdk.org
> Cc: nd <nd@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.zhang@arm.com>
>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Reviewed-by: Steve Capper <Steve.Capper@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

  parent reply	other threads:[~2018-11-03  5:46 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-24  2:20 tone.zhang
2018-10-24  9:09 ` Burakov, Anatoly
2018-11-01  2:33   ` Tone Zhang (Arm Technology China)
2018-11-01 10:01     ` Burakov, Anatoly
     [not found]       ` <DB7PR08MB33859242951014EF340C897AE9C80@DB7PR08MB3385.eurprd08.prod.outlook.com>
2018-11-03  5:46         ` Tone Zhang (Arm Technology China) [this message]
2018-11-06 11:03           ` Burakov, Anatoly
2018-11-07  4:55             ` Tone Zhang (Arm Technology China)
2018-11-07 10:12               ` Burakov, Anatoly
2018-11-08  0:45                 ` Tone Zhang (Arm Technology China)
2018-11-09  5:57 ` [dpdk-dev] [PATCH v2] " tone.zhang
2018-11-09 12:15   ` Burakov, Anatoly
2018-11-15  0:49     ` Tone Zhang (Arm Technology China)
2018-11-16  2:34       ` Tone Zhang (Arm Technology China)
2018-11-16 10:36         ` Burakov, Anatoly
2018-11-19  2:37   ` [dpdk-dev] [PATCH v3] " tone.zhang
2018-12-03  7:25     ` Tone Zhang (Arm Technology China)
2018-12-10 11:40       ` Burakov, Anatoly
2018-12-10 11:45     ` Burakov, Anatoly
2018-12-10 15:55       ` Stephen Hemminger
2018-12-12 10:49         ` Tone Zhang (Arm Technology China)
2018-12-12 10:48       ` Tone Zhang (Arm Technology China)
2018-12-12 11:25     ` [dpdk-dev] [PATCH v4] " tone.zhang
2018-12-12 11:27       ` Burakov, Anatoly
2018-12-19 23:05         ` Thomas Monjalon

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=AM0PR08MB340919655F35F2C8229728A28FC80@AM0PR08MB3409.eurprd08.prod.outlook.com \
    --to=tone.zhang@arm.com \
    --cc=anatoly.burakov@intel.com \
    --cc=dev@dpdk.org \
    --cc=nd@arm.com \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git