From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [PATCH] bus/pci: check if 5-level paging is enabled
 when testing IOMMU address width
Date: Thu, 9 Aug 2018 10:03:44 -0700
Message-ID: <20180809100344.15af7e9c@xeon-e3>
References: <1533494497-16253-1-git-send-email-quzeyao@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: maxime.coquelin@redhat.com, dev@dpdk.org
To: Drocula <quzeyao@gmail.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-pf1-f193.google.com (mail-pf1-f193.google.com
 [209.85.210.193]) by dpdk.org (Postfix) with ESMTP id B4B4B343C
 for <dev@dpdk.org>; Thu,  9 Aug 2018 19:03:51 +0200 (CEST)
Received: by mail-pf1-f193.google.com with SMTP id k21-v6so3122261pff.11
 for <dev@dpdk.org>; Thu, 09 Aug 2018 10:03:51 -0700 (PDT)
In-Reply-To: <1533494497-16253-1-git-send-email-quzeyao@gmail.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Thanks for the patch, there are some minor style/cleanups that
could be done.
 
>  #if defined(RTE_ARCH_X86)

Isn't this going to apply to 64 bit only?

> +/*
> + * Try to detect whether the system uses 5-level page table.
> + */
> +static bool
> +system_uses_PML5(void)
> +{
> +	void *page_4k, *mask = (void *)0xf0000000000000;

Magic constants expressed like this seem wrong. Why not use
shift to make it obvious.

Also, you are assuming a particular layout of memory on
Linux which might be problematic. Plus if there is already
some memory in use there, it won't work.

> +	page_4k = mmap(mask, 4096, PROT_READ | PROT_WRITE,
> +		MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

Since you are probing maybe MAP_FIXED is what you want.

> +
> +	if (page_4k == (void *) -1)
> +		return false;
Use MMAP_FAILED here.

> +	munmap(page_4k, 4096);
> +
> +	if ((unsigned long)page_4k & (unsigned long)mask)
> +		return true;
> +	return false;

Wouldn't this work the same for what you expect?
	return page_4k == mask;

I.e you expect kernel to put page where you want.