* [PATCH 3/4] iwlwifi: mvm: properly check debugfs dentry before using it
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo
Cc: linux-wireless, Greg Kroah-Hartman, Laura Abbott, Gabriel Ramirez,
Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, stable
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
debugfs can now report an error code if something went wrong instead of
just NULL. So if the return value is to be used as a "real" dentry, it
needs to be checked if it is an error before dereferencing it.
This is now happening because of ff9fb72bc077 ("debugfs: return error
values, not NULL"). If multiple iwlwifi devices are in the system, this
can cause problems when the driver attempts to create the main debugfs
directory again. Later on in the code we fail horribly by trying to
dereference a pointer that is an error value.
Reported-by: Laura Abbott <labbott@redhat.com>
Reported-by: Gabriel Ramirez <gabriello.ramirez@gmail.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: Intel Linux Wireless <linuxwifi@intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: stable <stable@vger.kernel.org> # 5.0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
index 2453ceabf00d..738eddb2e7ac 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
@@ -774,6 +774,11 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
return;
mvmvif->dbgfs_dir = debugfs_create_dir("iwlmvm", dbgfs_dir);
+ if (IS_ERR_OR_NULL(mvmvif->dbgfs_dir)) {
+ IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
+ dbgfs_dir);
+ return;
+ }
if (!mvmvif->dbgfs_dir) {
IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
--
2.20.1
^ permalink raw reply related
* [PATCH 4/4] iwlwifi: cfg: use family 22560 based_params for AX210 family
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Shaul Triebitz, Luca Coelho
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Shaul Triebitz <shaul.triebitz@intel.com>
Specifically, max_tfd_queue_size should be 0x10000 like in
22560 family and not 0x100 like in 22000 family.
Signed-off-by: Shaul Triebitz <shaul.triebitz@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/cfg/22000.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/cfg/22000.c b/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
index eb6defb6d0cd..0a87d87fbb4f 100644
--- a/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
+++ b/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
@@ -201,7 +201,7 @@ static const struct iwl_ht_params iwl_22000_ht_params = {
#define IWL_DEVICE_AX210 \
IWL_DEVICE_AX200_COMMON, \
.device_family = IWL_DEVICE_FAMILY_AX210, \
- .base_params = &iwl_22000_base_params, \
+ .base_params = &iwl_22560_base_params, \
.csr = &iwl_csr_v1, \
.min_txq_size = 128
--
2.20.1
^ permalink raw reply related
* [PATCH 2/4] iwlwifi: dbg_ini: check debug TLV type explicitly
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Shahar S Matityahu, Luca Coelho
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Shahar S Matityahu <shahar.s.matityahu@intel.com>
In ini debug TLVs bit 24 is set. The driver relies on it in the memory
allocation for the debug configuration. This implementation is
problematic in case of a new debug TLV that is not supported yet is added
and uses bit 24. In such a scenario the driver allocate space without
using it which causes errors in the apply point enabling flow.
Solve it by explicitly checking if a given TLV is part of the list of
the supported ini debug TLVs.
Signed-off-by: Shahar S Matityahu <shahar.s.matityahu@intel.com>
Fixes: f14cda6f3b31 ("iwlwifi: trans: parse and store debug ini TLVs")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/fw/file.h | 15 +++++++++------
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 ++-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/file.h b/drivers/net/wireless/intel/iwlwifi/fw/file.h
index 641c95d03b15..e06407dc088b 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/file.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/file.h
@@ -93,7 +93,7 @@ struct iwl_ucode_header {
} u;
};
-#define IWL_UCODE_INI_TLV_GROUP BIT(24)
+#define IWL_UCODE_INI_TLV_GROUP 0x1000000
/*
* new TLV uCode file layout
@@ -148,11 +148,14 @@ enum iwl_ucode_tlv_type {
IWL_UCODE_TLV_UMAC_DEBUG_ADDRS = 54,
IWL_UCODE_TLV_LMAC_DEBUG_ADDRS = 55,
IWL_UCODE_TLV_FW_RECOVERY_INFO = 57,
- IWL_UCODE_TLV_TYPE_BUFFER_ALLOCATION = IWL_UCODE_INI_TLV_GROUP | 0x1,
- IWL_UCODE_TLV_TYPE_HCMD = IWL_UCODE_INI_TLV_GROUP | 0x2,
- IWL_UCODE_TLV_TYPE_REGIONS = IWL_UCODE_INI_TLV_GROUP | 0x3,
- IWL_UCODE_TLV_TYPE_TRIGGERS = IWL_UCODE_INI_TLV_GROUP | 0x4,
- IWL_UCODE_TLV_TYPE_DEBUG_FLOW = IWL_UCODE_INI_TLV_GROUP | 0x5,
+
+ IWL_UCODE_TLV_TYPE_BUFFER_ALLOCATION = IWL_UCODE_INI_TLV_GROUP + 0x1,
+ IWL_UCODE_TLV_DEBUG_BASE = IWL_UCODE_TLV_TYPE_BUFFER_ALLOCATION,
+ IWL_UCODE_TLV_TYPE_HCMD = IWL_UCODE_INI_TLV_GROUP + 0x2,
+ IWL_UCODE_TLV_TYPE_REGIONS = IWL_UCODE_INI_TLV_GROUP + 0x3,
+ IWL_UCODE_TLV_TYPE_TRIGGERS = IWL_UCODE_INI_TLV_GROUP + 0x4,
+ IWL_UCODE_TLV_TYPE_DEBUG_FLOW = IWL_UCODE_INI_TLV_GROUP + 0x5,
+ IWL_UCODE_TLV_DEBUG_MAX = IWL_UCODE_TLV_TYPE_DEBUG_FLOW,
/* TLVs 0x1000-0x2000 are for internal driver usage */
IWL_UCODE_TLV_FW_DBG_DUMP_LST = 0x1000,
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index 5798f434f68f..c7070760a10a 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
@@ -126,7 +126,8 @@ void iwl_alloc_dbg_tlv(struct iwl_trans *trans, size_t len, const u8 *data,
len -= ALIGN(tlv_len, 4);
data += sizeof(*tlv) + ALIGN(tlv_len, 4);
- if (!(tlv_type & IWL_UCODE_INI_TLV_GROUP))
+ if (tlv_type < IWL_UCODE_TLV_DEBUG_BASE ||
+ tlv_type > IWL_UCODE_TLV_DEBUG_MAX)
continue;
hdr = (void *)&tlv->data[0];
--
2.20.1
^ permalink raw reply related
* [PATCH 0/4] iwlwifi: fixes intended for 5.1 2019-04-18
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Luca Coelho
From: Luca Coelho <luciano.coelho@intel.com>
Hi,
This is the third patchset with fixes for 5.1.
The changes are:
* Fix an oops when creating debugfs entries;
* Fix bug when trying to capture debugging info while in rfkill;
* Prevent potential uninitialized memory dumps into debugging logs;
* Fix some initialization parameters for AX210 devices;
Not in this series, but sent before separately:
* Fix an oops with non-MSIX devices;
As usual, I'm pushing this to a pending branch, for kbuild bot, and
will send a pull-request later.
Please review.
Cheers,
Luca.
Greg Kroah-Hartman (1):
iwlwifi: mvm: properly check debugfs dentry before using it
Johannes Berg (1):
iwlwifi: mvm: don't attempt debug collection in rfkill
Shahar S Matityahu (1):
iwlwifi: dbg_ini: check debug TLV type explicitly
Shaul Triebitz (1):
iwlwifi: cfg: use family 22560 based_params for AX210 family
drivers/net/wireless/intel/iwlwifi/cfg/22000.c | 2 +-
drivers/net/wireless/intel/iwlwifi/fw/file.h | 15 +++++++++------
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 ++-
.../net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 5 +++++
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 4 +++-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 2 +-
6 files changed, 21 insertions(+), 10 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH 1/4] iwlwifi: mvm: don't attempt debug collection in rfkill
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Johannes Berg, Luca Coelho
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Johannes Berg <johannes.berg@intel.com>
If we fail to initialize because rfkill is enabled, then trying
to do debug collection currently just fails. Prevent that in the
high-level code, although we should probably also fix the lower
level code to do things more carefully.
It's not 100% clear that it fixes this commit, as the original
dump code at the time might've been more careful. In any case,
we don't really need to dump anything in this expected scenario.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Fixes: 7125648074e8 ("iwlwifi: add fw dump upon RT ucode start failure")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 4 +++-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
index 00a47f6f1d81..ab68b5d53ec9 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
@@ -1121,7 +1121,9 @@ int iwl_mvm_up(struct iwl_mvm *mvm)
ret = iwl_mvm_load_rt_fw(mvm);
if (ret) {
IWL_ERR(mvm, "Failed to start RT ucode: %d\n", ret);
- iwl_fw_dbg_error_collect(&mvm->fwrt, FW_DBG_TRIGGER_DRIVER);
+ if (ret != -ERFKILL)
+ iwl_fw_dbg_error_collect(&mvm->fwrt,
+ FW_DBG_TRIGGER_DRIVER);
goto error;
}
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
index ba27dce4c2bb..13681b03c10e 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
@@ -834,7 +834,7 @@ iwl_op_mode_mvm_start(struct iwl_trans *trans, const struct iwl_cfg *cfg,
mutex_lock(&mvm->mutex);
iwl_mvm_ref(mvm, IWL_MVM_REF_INIT_UCODE);
err = iwl_run_init_mvm_ucode(mvm, true);
- if (err)
+ if (err && err != -ERFKILL)
iwl_fw_dbg_error_collect(&mvm->fwrt, FW_DBG_TRIGGER_DRIVER);
if (!iwlmvm_mod_params.init_dbg || !err)
iwl_mvm_stop_device(mvm);
--
2.20.1
^ permalink raw reply related
* Re: brcmfmac: NULL pointer dereference during brcmf_detach() after firmware crash
From: Rafał Miłecki @ 2019-04-18 11:55 UTC (permalink / raw)
To: Arend Van Spriel
Cc: linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER <brcm80211-dev-list.pdl@broadcom.com>,,
Aaron Blair
In-Reply-To: <CACna6rxR4t1MpniasnUfyiuBhc9tTkJRx=O7GrMnmVPOXCZ-dQ@mail.gmail.com>
On Fri, 15 Feb 2019 at 07:15, Rafał Miłecki <zajec5@gmail.com> wrote:
> On Thu, 14 Feb 2019 at 23:37, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
> > On 2/14/2019 11:30 PM, Rafał Miłecki wrote:
> > > I've just found a well reproducible brcmfmac crash (NULL pointer
> > > dereference).
> > >
> > > Steps:
> > > 1. Wait for or trigger a FullMAC firmware crash
> > > 2. Wait for some skb to get queued on a flowring
> > > 3. Call rmmod brcmfmac
> > >
> > > Problem:
> > > There is a NULL pointer dereference in one of the brcmf_detach() calls.
> > >
> > > Explanation:
> > > brcmf_detach() first frees all "ifp"s and then deletes flowrings. If any
> > > flowring has a skb it results in calling brcmf_txfinalize() which tries
> > > to access "ifp" (struct brcmf_if) which is a NULL.
> >
> > Hi Rafał,
> >
> > Thanks for diving in. That was my suspicion. Does it mean you are
> > working on a patch or shall I take care of it.
>
> It would be nice to have someone more experienced with detaching &
> rings look at it. Is adding a simple
> if (ifp)
> enough? Or should that code get redesigned? Should we e.g. reorder detach order?
Hi Arend, would you find a moment to look at that crash, please?
--
Rafał
^ permalink raw reply
* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Thomas Gleixner @ 2019-04-18 11:49 UTC (permalink / raw)
To: David Laight
Cc: 'Fenghua Yu', 'Ingo Molnar',
'Borislav Petkov', 'H Peter Anvin',
'Paolo Bonzini', 'Dave Hansen',
'Ashok Raj', 'Peter Zijlstra',
'Ravi V Shankar', 'Xiaoyao Li ',
'Christopherson Sean J', 'Kalle Valo',
'Michael Chan', 'linux-kernel', 'x86',
'kvm@vger.kernel.org', 'netdev@vger.kernel.org',
'linux-wireless@vger.kernel.org'
In-Reply-To: <698b9419c81b443fb9c180d2b27fd34a@AcuMS.aculab.com>
On Thu, 18 Apr 2019, David Laight wrote:
> From: David Laight
> > Sent: 18 April 2019 10:21
> > From: Fenghua Yu
> > > Sent: 17 April 2019 22:34
> > >
> > > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > > operate on bitmap defined in x86_capability.
> > >
> > > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > > the location is at:
> > > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> > >
> > > Since base address of x86_capability may not be aligned to unsigned long,
> > > the single unsigned long location may cross two cache lines and
> > > accessing the location by locked BTS/BTR introductions will cause
> > > split lock.
> >
> > Isn't the problem that the type (and definition) of x86_capability[] are wrong.
> > If the 'bitmap' functions are used for it, it should be defined as a bitmap.
> > This would make it 'unsigned long' not __u32.
> >
> > This type munging of bitmaps only works on LE systems.
> >
> > OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
> > ISTR some of the associated functions use byte accesses.
> >
> > Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
> > 32bit accesses.
>
> A quick look shows that this isn't the only __32[] that is being
> cast to (unsigned long) and then to set/test/clear_bit() in those
> files.
>
> I wonder how much other code is applying such casts?
The reason for the cpuid stuff using u32 is that this is actually the width
of the information retrieved from CPUID.
Thanks,
tglx
^ permalink raw reply
* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-18 11:08 UTC (permalink / raw)
To: David Laight, 'Fenghua Yu', 'Thomas Gleixner',
'Ingo Molnar', 'Borislav Petkov',
'H Peter Anvin', 'Paolo Bonzini',
'Dave Hansen', 'Ashok Raj',
'Peter Zijlstra', 'Ravi V Shankar',
'Xiaoyao Li ', 'Christopherson Sean J',
'Kalle Valo', 'Michael Chan'
Cc: 'linux-kernel', 'x86',
'kvm@vger.kernel.org', 'netdev@vger.kernel.org',
'linux-wireless@vger.kernel.org'
In-Reply-To: <c8ca959a4bc549b59baca563660d42d6@AcuMS.aculab.com>
From: David Laight
> Sent: 18 April 2019 10:21
> From: Fenghua Yu
> > Sent: 17 April 2019 22:34
> >
> > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > operate on bitmap defined in x86_capability.
> >
> > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > the location is at:
> > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> >
> > Since base address of x86_capability may not be aligned to unsigned long,
> > the single unsigned long location may cross two cache lines and
> > accessing the location by locked BTS/BTR introductions will cause
> > split lock.
>
> Isn't the problem that the type (and definition) of x86_capability[] are wrong.
> If the 'bitmap' functions are used for it, it should be defined as a bitmap.
> This would make it 'unsigned long' not __u32.
>
> This type munging of bitmaps only works on LE systems.
>
> OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
> ISTR some of the associated functions use byte accesses.
>
> Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
> 32bit accesses.
A quick look shows that this isn't the only __32[] that is being
cast to (unsigned long) and then to set/test/clear_bit() in those
files.
I wonder how much other code is applying such casts?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] NFC: st95hf: fix spelling mistake: "receieve" -> "receive"
From: Mukesh Ojha @ 2019-04-18 10:42 UTC (permalink / raw)
To: Colin King, Samuel Ortiz, Daniel Mack, linux-wireless
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190418095756.9264-1-colin.king@canonical.com>
On 4/18/2019 3:27 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a spelling mistake in a dev_error message. Fix it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cheers,
-Mukesh
> ---
> drivers/nfc/st95hf/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
> index a50a95cfcfd8..d72d48243141 100644
> --- a/drivers/nfc/st95hf/core.c
> +++ b/drivers/nfc/st95hf/core.c
> @@ -327,7 +327,7 @@ static int st95hf_echo_command(struct st95hf_context *st95context)
> &echo_response);
> if (result) {
> dev_err(&st95context->spicontext.spidev->dev,
> - "err: echo response receieve error = 0x%x\n", result);
> + "err: echo response receive error = 0x%x\n", result);
> return result;
> }
>
^ permalink raw reply
* [PATCH] NFC: st95hf: fix spelling mistake: "receieve" -> "receive"
From: Colin King @ 2019-04-18 9:57 UTC (permalink / raw)
To: Samuel Ortiz, Daniel Mack, linux-wireless; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There is a spelling mistake in a dev_error message. Fix it.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/nfc/st95hf/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index a50a95cfcfd8..d72d48243141 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -327,7 +327,7 @@ static int st95hf_echo_command(struct st95hf_context *st95context)
&echo_response);
if (result) {
dev_err(&st95context->spicontext.spidev->dev,
- "err: echo response receieve error = 0x%x\n", result);
+ "err: echo response receive error = 0x%x\n", result);
return result;
}
--
2.20.1
^ permalink raw reply related
* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-18 9:20 UTC (permalink / raw)
To: 'Fenghua Yu', Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H Peter Anvin, Paolo Bonzini, Dave Hansen,
Ashok Raj, Peter Zijlstra, Ravi V Shankar, Xiaoyao Li ,
Christopherson Sean J, Kalle Valo, Michael Chan
Cc: linux-kernel, x86, kvm@vger.kernel.org, netdev@vger.kernel.org,
linux-wireless@vger.kernel.org
In-Reply-To: <1555536851-17462-5-git-send-email-fenghua.yu@intel.com>
From: Fenghua Yu
> Sent: 17 April 2019 22:34
>
> set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> operate on bitmap defined in x86_capability.
>
> Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> the location is at:
> base address of x86_capability + (bit offset in x86_capability / 64) * 8
>
> Since base address of x86_capability may not be aligned to unsigned long,
> the single unsigned long location may cross two cache lines and
> accessing the location by locked BTS/BTR introductions will cause
> split lock.
Isn't the problem that the type (and definition) of x86_capability[] are wrong.
If the 'bitmap' functions are used for it, it should be defined as a bitmap.
This would make it 'unsigned long' not __u32.
This type munging of bitmaps only works on LE systems.
OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
ISTR some of the associated functions use byte accesses.
Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
32bit accesses.
>
> To fix the split lock issue, align x86_capability to size of unsigned long
> so that the location will be always within one cache line.
>
> Changing x86_capability's type to unsigned long may also fix the issue
> because x86_capability will be naturally aligned to size of unsigned long.
> But this needs additional code changes. So choose the simpler solution
> by setting the array's alignment to size of unsigned long.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/include/asm/processor.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 2bb3a648fc12..7c62b9ad6e5a 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -93,7 +93,9 @@ struct cpuinfo_x86 {
> __u32 extended_cpuid_level;
> /* Maximum supported CPUID level, -1=no CPUID: */
> int cpuid_level;
> - __u32 x86_capability[NCAPINTS + NBUGINTS];
> + /* Aligned to size of unsigned long to avoid split lock in atomic ops */
> + __u32 x86_capability[NCAPINTS + NBUGINTS]
> + __aligned(sizeof(unsigned long));
> char x86_vendor_id[16];
> char x86_model_id[64];
> /* in KB - valid for CPUS which support this call: */
> --
> 2.19.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2] ath10k: fix different tx duration output
From: Toke Høiland-Jørgensen @ 2019-04-18 8:07 UTC (permalink / raw)
To: leiwa; +Cc: ath10k, linux-wireless
In-Reply-To: <298a645b17aac0f5c466f011225533b0@codeaurora.org>
leiwa@codeaurora.org writes:
> On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote:
>> Lei Wang <leiwa@codeaurora.org> writes:
>>
>>> TX duration output of tx_stats in debugfs and station dump had big
>>> difference because they got tx duration value from different statistic
>>> data. We should use the same statistic data.
>>
>> So are you sure you picked the most accurate one of the two? :)
>>
>> -Toke
>
> Hi Toke,
>
> Yes.
> Now for ath10k, there are two ways to get tx duration output.
> One is got from tx_stats in debugfs reported by firmware. It is a total
> value including all the frames which created by host and firmware sent
> to the peer.
> And the second is calculated from
> ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here
> the tx duration just includes the data frames sent from host to the
> peer.
So the difference is that the former includes control frames as well? Is
that the only difference? And what exactly is a "big difference" (from
the commit message)?
> So the first value is preferable for station dump.
Hmm, I'm not sure if I agree with this. I specifically added the
tx_duration to the station dump to be able to get the values used by the
airtime scheduler. This breaks with this patch.
-Toke
^ permalink raw reply
* Re: [PATCH v2] ath10k: fix different tx duration output
From: leiwa @ 2019-04-18 7:26 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: ath10k, linux-wireless
In-Reply-To: <87mukprnie.fsf@toke.dk>
On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote:
> Lei Wang <leiwa@codeaurora.org> writes:
>
>> TX duration output of tx_stats in debugfs and station dump had big
>> difference because they got tx duration value from different statistic
>> data. We should use the same statistic data.
>
> So are you sure you picked the most accurate one of the two? :)
>
> -Toke
Hi Toke,
Yes.
Now for ath10k, there are two ways to get tx duration output.
One is got from tx_stats in debugfs reported by firmware. It is a total
value including all the frames which created by host and firmware sent
to the peer.
And the second is calculated from
ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here
the tx duration just includes the data frames sent from host to the
peer.
So the first value is preferable for station dump.
Thanks.
Lei
^ permalink raw reply
* Re: [PATCH v7 15/21] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Thomas Gleixner @ 2019-04-18 6:41 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <20190418005711.GB18776@romley-ivt3.sc.intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 18, 2019 at 12:47:24AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2019, Fenghua Yu wrote:
> >
> > > The interface /sys/device/system/cpu/split_lock_detect is added
> > > to allow user to control split lock detection and show current split
> > > lock detection setting.
> > >
> > > Writing [yY1] or [oO][nN] to the file enables split lock detection and
> > > writing [nN0] or [oO][fF] disables split lock detection. Split lock
> > > detection is enabled or disabled on all CPUs.
> > >
> > > Reading the file returns current global split lock detection setting:
> > > 0: disabled
> > > 1: enabled
> >
> > Again, You explain WHAT this patch does and still there is zero
> > justification why this sysfs knob is needed at all. I still do not see any
> > reason why this knob should exist.
>
> An important application has split lock issues which are already discovered
> and need to be fixed. But before the issues are fixed, sysadmin still wants to
> run the application without rebooting the system, the sysfs knob can be useful
> to turn off split lock detection. After the application is done, split lock
> detection will be enabled again through the sysfs knob.
Are you sure that you are talking about the real world? I might buy the
'off' part somehow, but the 'on' part is beyond theoretical.
Even the 'off' part is dubious on a multi user machine. I personally would
neither think about using the sysfs knob nor about rebooting the machine
simply because I'd consider a lock operation accross a cacheline an malicious
DoS attempt. Why would I allow that?
So in reality the sysadmin will either move the workload to a machine w/o
the #AC magic or just tell the user to fix his crap.
> Without the sysfs knob, sysadmin has to reboot the system with kernel option
> "no_split_lock_detect" to run the application before the split lock issues
> are fixed.
>
> Is this a valid justification why the sysfs knob is needed? If it is, I can
> add the justification in the next version.
Why has this information not been in the changelog right away? I'm really
tired of asking the same questions and pointing you to
Documentation/process over and over.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to cache MSR TEST_CTL
From: Thomas Gleixner @ 2019-04-18 6:31 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless, Xiaoyao Li
In-Reply-To: <20190418012848.GC18776@romley-ivt3.sc.intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 18, 2019 at 12:14:12AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2019, Fenghua Yu wrote:
> > > +DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> > > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> >
> > Contrary to things like cpufeatures or MSR bits, it's pretty useless to
> > have a separate patch for this. Please fold this into the place which
> > actualy uses it.
>
> Can I fold this patch into the KVM patch 0013 which first uses (reads) the
> variable? But the variable will be set in later patches when enabling split
> lock feature (patch 0014) and when enabling/disabling split lock feature
> (patch 0015).
>
> Is this a right sequence to fit the variable in the patch set?
As I said in the other reply, you are assuming that the content of that MSR
is 0. Which might be true now, but is that true in a year from now?
So you really want to at least initialize the variable by reading the MSR
_before_ you make use of it in KVM.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 18/21] x86/clearcpuid: Support feature flag string in kernel option clearcpuid
From: Thomas Gleixner @ 2019-04-18 6:16 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <20190417234728.GA18776@romley-ivt3.sc.intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 18, 2019 at 01:19:41AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2019, Fenghua Yu wrote:
> >
> > > The kernel option clearcpuid currently only takes feature bit which
> > > can be changed from kernel to kernel.
> > >
> > > Extend clearcpuid to use cap flag string, which is defined in
> > > x86_cap_flags[] and won't be changed from kernel to kernel.
> > > And user can easily get the cap flag string from /proc/cpuinfo.
> >
> > If your machine dies because init triggers #AC then please explain how that
> > easily can be read from /proc/cpuinfo and how the sysadmin can figure out
> > what the heck he needs to write on the kernel command line.
> >
> > The whole 'clearcpuid' thing should have never been merged. It's a pure
> > testing/debugging thing. And no, we are not going to proliferate it and
> > extend it for dubious value. Quite the contrary, we should simply rip it
> > out.
>
> So I can remove the four 'clearcpuid' related patches 0018-0021 in the next
> version, right?
Yes please. They are unrelated to this problem and 'noac' is way more admin
friendly than that.
Thanks,
tglx
^ permalink raw reply
* Re: Kali Linux 4.19.13 VM, Ralink RT3072 - Not Finding Wireless Networks
From: Tom Psyborg @ 2019-04-18 2:08 UTC (permalink / raw)
To: Kevan Salva; +Cc: linux-wireless
In-Reply-To: <CAAM6Ncf0=1wBbnomxH=o8r9-edhSk2zr_xzhJeA1uWdzRZ84Eg@mail.gmail.com>
On 18/04/2019, Kevan Salva <kenico15@gmail.com> wrote:
> Hello All,
>
> I am running Kali Linux (4.19.13) in a VM. I have a USB network
> adapter - a Ralink RT3072, passed through to the VM. However, it is
> not finding any wireless networks. This issue occurs on both VMware
> and on Virtualbox, but not on a live install of the same OS. This is
> the output I get from dmesg: pastebin.com/HaXUDNug . What may be the
> issue here?
>
> With much appreciation,
>
> Kevan
>
do you get such issues if running older kali/linux version on the same system?
^ permalink raw reply
* Kali Linux 4.19.13 VM, Ralink RT3072 - Not Finding Wireless Networks
From: Kevan Salva @ 2019-04-18 1:57 UTC (permalink / raw)
To: linux-wireless
Hello All,
I am running Kali Linux (4.19.13) in a VM. I have a USB network
adapter - a Ralink RT3072, passed through to the VM. However, it is
not finding any wireless networks. This issue occurs on both VMware
and on Virtualbox, but not on a live install of the same OS. This is
the output I get from dmesg: pastebin.com/HaXUDNug . What may be the
issue here?
With much appreciation,
Kevan
^ permalink raw reply
* Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to cache MSR TEST_CTL
From: Fenghua Yu @ 2019-04-18 1:28 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless, Xiaoyao Li
In-Reply-To: <alpine.DEB.2.21.1904180009200.3174@nanos.tec.linutronix.de>
On Thu, Apr 18, 2019 at 12:14:12AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Fenghua Yu wrote:
>
> > MSR TEST_CTL (0x33) value is cached in per CPU variable msr_test_ctl_cache.
>
> It _is_ cached? How so?
>
> > The cached value will be used in virutalization to avoid costly MSR read.
> >
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com>
>
> That SOB chain is bogus.
>
> > ---
> > arch/x86/include/asm/cpu.h | 1 +
> > arch/x86/kernel/cpu/intel.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> > index 4e03f53fc079..cd7493f20234 100644
> > --- a/arch/x86/include/asm/cpu.h
> > +++ b/arch/x86/include/asm/cpu.h
> > @@ -40,6 +40,7 @@ int mwait_usable(const struct cpuinfo_x86 *);
> > unsigned int x86_family(unsigned int sig);
> > unsigned int x86_model(unsigned int sig);
> > unsigned int x86_stepping(unsigned int sig);
> > +DECLARE_PER_CPU(u64, msr_test_ctl_cache);
> > #ifdef CONFIG_CPU_SUP_INTEL
> > void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
> > #else
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index 62f61a961eb6..997d683d3c27 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -31,6 +31,9 @@
> > #include <asm/apic.h>
> > #endif
> >
> > +DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
>
> Contrary to things like cpufeatures or MSR bits, it's pretty useless to
> have a separate patch for this. Please fold this into the place which
> actualy uses it.
Can I fold this patch into the KVM patch 0013 which first uses (reads) the
variable? But the variable will be set in later patches when enabling split
lock feature (patch 0014) and when enabling/disabling split lock feature
(patch 0015).
Is this a right sequence to fit the variable in the patch set?
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v7 15/21] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Fenghua Yu @ 2019-04-18 0:57 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <alpine.DEB.2.21.1904180042090.3174@nanos.tec.linutronix.de>
On Thu, Apr 18, 2019 at 12:47:24AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Fenghua Yu wrote:
>
> > The interface /sys/device/system/cpu/split_lock_detect is added
> > to allow user to control split lock detection and show current split
> > lock detection setting.
> >
> > Writing [yY1] or [oO][nN] to the file enables split lock detection and
> > writing [nN0] or [oO][fF] disables split lock detection. Split lock
> > detection is enabled or disabled on all CPUs.
> >
> > Reading the file returns current global split lock detection setting:
> > 0: disabled
> > 1: enabled
>
> Again, You explain WHAT this patch does and still there is zero
> justification why this sysfs knob is needed at all. I still do not see any
> reason why this knob should exist.
An important application has split lock issues which are already discovered
and need to be fixed. But before the issues are fixed, sysadmin still wants to
run the application without rebooting the system, the sysfs knob can be useful
to turn off split lock detection. After the application is done, split lock
detection will be enabled again through the sysfs knob.
Without the sysfs knob, sysadmin has to reboot the system with kernel option
"no_split_lock_detect" to run the application before the split lock issues
are fixed.
Is this a valid justification why the sysfs knob is needed? If it is, I can
add the justification in the next version.
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v7 18/21] x86/clearcpuid: Support feature flag string in kernel option clearcpuid
From: Fenghua Yu @ 2019-04-17 23:47 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <alpine.DEB.2.21.1904180106400.3174@nanos.tec.linutronix.de>
On Thu, Apr 18, 2019 at 01:19:41AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Fenghua Yu wrote:
>
> > The kernel option clearcpuid currently only takes feature bit which
> > can be changed from kernel to kernel.
> >
> > Extend clearcpuid to use cap flag string, which is defined in
> > x86_cap_flags[] and won't be changed from kernel to kernel.
> > And user can easily get the cap flag string from /proc/cpuinfo.
>
> If your machine dies because init triggers #AC then please explain how that
> easily can be read from /proc/cpuinfo and how the sysadmin can figure out
> what the heck he needs to write on the kernel command line.
>
> The whole 'clearcpuid' thing should have never been merged. It's a pure
> testing/debugging thing. And no, we are not going to proliferate it and
> extend it for dubious value. Quite the contrary, we should simply rip it
> out.
So I can remove the four 'clearcpuid' related patches 0018-0021 in the next
version, right?
>
> Add a simple 'noac' or whatever command line option, which is documented
> proper and can easily be mapped to a #AC crash during boot.
OK. I will do this.
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v7 18/21] x86/clearcpuid: Support feature flag string in kernel option clearcpuid
From: Thomas Gleixner @ 2019-04-17 23:19 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1555536851-17462-19-git-send-email-fenghua.yu@intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> The kernel option clearcpuid currently only takes feature bit which
> can be changed from kernel to kernel.
>
> Extend clearcpuid to use cap flag string, which is defined in
> x86_cap_flags[] and won't be changed from kernel to kernel.
> And user can easily get the cap flag string from /proc/cpuinfo.
If your machine dies because init triggers #AC then please explain how that
easily can be read from /proc/cpuinfo and how the sysadmin can figure out
what the heck he needs to write on the kernel command line.
The whole 'clearcpuid' thing should have never been merged. It's a pure
testing/debugging thing. And no, we are not going to proliferate it and
extend it for dubious value. Quite the contrary, we should simply rip it
out.
Add a simple 'noac' or whatever command line option, which is documented
proper and can easily be mapped to a #AC crash during boot.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 17/21] x86/clearcpuid: Support multiple clearcpuid options
From: Thomas Gleixner @ 2019-04-17 23:05 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1555536851-17462-18-git-send-email-fenghua.yu@intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> int cmdline_find_option_bool(const char *cmdline_ptr, const char *option);
> int cmdline_find_option(const char *cmdline_ptr, const char *option,
> char *buffer, int bufsize);
> +int cmdline_find_option_in_range(const char *cmdline_ptr, int cmdline_size,
> + const char *option, char *buffer, int bufsize,
> + char **arg_pos);
>
> #endif /* _ASM_X86_CMDLINE_H */
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 6abd83572b01..88bbba7ee96a 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -243,16 +243,31 @@ static void __init fpu__init_system_ctx_switch(void)
> WARN_ON_FPU(current->thread.fpu.initialized);
> }
>
> +static void __init clear_cpuid(void)
> +{
> + char arg[32], *argptr, *option_pos, clearcpuid_option[] = "clearcpuid";
> + int bit, cmdline_size;
> +
> + /* Find each option in boot_command_line and clear specified cpu cap. */
> + cmdline_size = COMMAND_LINE_SIZE;
> + while (cmdline_find_option_in_range(boot_command_line, cmdline_size,
> + clearcpuid_option, arg,
> + sizeof(arg), &option_pos) >= 0) {
> + /* Chang command line range for next search. */
Chang?
> + cmdline_size = option_pos - boot_command_line + 1;
> + argptr = arg;
> + if (get_option(&argptr, &bit) &&
> + bit >= 0 && bit < NCAPINTS * 32)
> + setup_clear_cpu_cap(bit);
> + }
> +}
> +
> /*
> * We parse fpu parameters early because fpu__init_system() is executed
> * before parse_early_param().
> */
> static void __init fpu__init_parse_early_param(void)
> {
> - char arg[32];
> - char *argptr = arg;
> - int bit;
> -
> if (cmdline_find_option_bool(boot_command_line, "no387"))
> setup_clear_cpu_cap(X86_FEATURE_FPU);
>
> @@ -271,12 +286,7 @@ static void __init fpu__init_parse_early_param(void)
> if (cmdline_find_option_bool(boot_command_line, "noxsaves"))
> setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>
> - if (cmdline_find_option(boot_command_line, "clearcpuid", arg,
> - sizeof(arg)) &&
> - get_option(&argptr, &bit) &&
> - bit >= 0 &&
> - bit < NCAPINTS * 32)
> - setup_clear_cpu_cap(bit);
> + clear_cpuid();
I have no idea how that clearcpuid evaluation ended up in the FPU code, but
it does not belong here at all as it can turn off arbitrary cpuid bits.
Please move this out first into a more sensible place in kernel/cpu/
> }
>
> /*
> diff --git a/arch/x86/lib/cmdline.c b/arch/x86/lib/cmdline.c
> index 3261abb21ef4..9cf1a0773877 100644
> --- a/arch/x86/lib/cmdline.c
> +++ b/arch/x86/lib/cmdline.c
> @@ -114,13 +114,15 @@ __cmdline_find_option_bool(const char *cmdline, int max_cmdline_size,
> * @option: option string to look for
> * @buffer: memory buffer to return the option argument
> * @bufsize: size of the supplied memory buffer
> + * @option_pos: pointer to the option if found
> *
> * Returns the length of the argument (regardless of if it was
> * truncated to fit in the buffer), or -1 on not found.
> */
> static int
> __cmdline_find_option(const char *cmdline, int max_cmdline_size,
> - const char *option, char *buffer, int bufsize)
> + const char *option, char *buffer, int bufsize,
> + char **arg_pos)
Bah. For a silly per cpu variable you add a separate patch, but here you do
2 things in one go:
1) Change the parser
2) Change the call site.
No, you know exactly how this should be done.
> {
> char c;
> int pos = 0, len = -1;
> @@ -164,6 +166,9 @@ __cmdline_find_option(const char *cmdline, int max_cmdline_size,
> len = 0;
> bufptr = buffer;
> state = st_bufcpy;
> + if (arg_pos)
> + *arg_pos = (char *)cmdline -
> + strlen(option) - 1;
https://marc.info/?l=linux-kernel&m=148467980905537&w=2
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 15/21] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Thomas Gleixner @ 2019-04-17 22:47 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1555536851-17462-16-git-send-email-fenghua.yu@intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> The interface /sys/device/system/cpu/split_lock_detect is added
> to allow user to control split lock detection and show current split
> lock detection setting.
>
> Writing [yY1] or [oO][nN] to the file enables split lock detection and
> writing [nN0] or [oO][fF] disables split lock detection. Split lock
> detection is enabled or disabled on all CPUs.
>
> Reading the file returns current global split lock detection setting:
> 0: disabled
> 1: enabled
Again, You explain WHAT this patch does and still there is zero
justification why this sysfs knob is needed at all. I still do not see any
reason why this knob should exist.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 14/21] x86/split_lock: Enable split lock detection by default
From: Thomas Gleixner @ 2019-04-17 22:41 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1555536851-17462-15-git-send-email-fenghua.yu@intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> A split locked access locks bus and degrades overall memory access
> performance. When split lock detection feature is enumerated, enable
> the feature by default to find any split lock issue.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/kernel/cpu/intel.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 997d683d3c27..6a692d215bef 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -34,6 +34,8 @@
> DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
>
> +static bool split_lock_detect_enable;
> +
> /*
> * Just in case our CPU detection goes bad, or you have a weird system,
> * allow a way to override the automatic disabling of MPX.
> @@ -164,6 +166,23 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> return false;
> }
>
> +static void split_lock_update_msr(void *__unused)
> +{
> + if (split_lock_detect_enable) {
> + msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> + this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
> + } else {
> + msr_clear_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> + this_cpu_and(msr_test_ctl_cache, ~TEST_CTL_SPLIT_LOCK_DETECT);
> + }
Nothing in this file initializes msr_test_ctl_cache explicitely. Register
caching always requires to read the register and store it in the cache
before doing anything with it. Nothing guarantees that all bits in that MSR
are 0 by default forever.
And once you do that _before_ calling split_lock_update_msr() then you can
spare the RMW in that function.
> +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> +{
> + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT))
> + split_lock_update_msr(NULL);
> +}
> +
> static void early_init_intel(struct cpuinfo_x86 *c)
> {
> u64 misc_enable;
> @@ -661,6 +680,8 @@ static void init_intel(struct cpuinfo_x86 *c)
> {
> early_init_intel(c);
>
> + init_split_lock_detect(c);
Sigh. Why needs this to be squeezed in the middle of the whole enumeration
stuff? Just because....
init_intel_misc_features() is called at the end and it does also MSR
caching etc. So down there is the right place.
> +
> intel_workarounds(c);
>
> /*
> @@ -1032,9 +1053,22 @@ static const struct cpu_dev intel_cpu_dev = {
>
> cpu_dev_register(intel_cpu_dev);
>
> +#undef pr_fmt
> +#define pr_fmt(fmt) "x86/split lock detection: " fmt
> +
> +static void show_split_lock_detection_info(void)
> +{
> + if (split_lock_detect_enable)
> + pr_info("enabled\n");
> + else
> + pr_info("disabled\n");
This function is truly useful. The else path is never invoked. See the call
site below.
> +}
> +
> static void __init set_split_lock_detect(void)
> {
> setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
> + split_lock_detect_enable = true;
> + show_split_lock_detection_info();
> }
Oh well.
Thanks,
tglx
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox