On Wed, 24 Feb 2021 22:28:50 -0500
Tony Krowiak <akrowiak(a)linux.ibm.com> wrote:
> >> static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> >> {
> >> - kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> >> - matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> >> - vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> >> - kvm_put_kvm(matrix_mdev->kvm);
> >> - matrix_mdev->kvm = NULL;
> >> + struct kvm *kvm;
> >> +
> >> + if (matrix_mdev->kvm) {
> >> + kvm = matrix_mdev->kvm;
> >> + kvm_get_kvm(kvm);
> >> + matrix_mdev->kvm = NULL;
> > I think if there were two threads dong the unset in parallel, one
> > of them could bail out and carry on before the cleanup is done. But
> > since nothing much happens in release after that, I don't see an
> > immediate problem.
> >
> > Another thing to consider is, that setting ->kvm to NULL arms
> > vfio_ap_mdev_remove()...
>
> I'm not entirely sure what you mean by this, but my
> assumption is that you are talking about the check
> for matrix_mdev->kvm != NULL at the start of
> that function.
Yes I was talking about the check
static int vfio_ap_mdev_remove(struct mdev_device *mdev)
{
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
if (matrix_mdev->kvm)
return -EBUSY;
...
kfree(matrix_mdev);
...
}
As you see, we bail out if kvm is still set, otherwise we clean up the
matrix_mdev which includes kfree-ing it. And vfio_ap_mdev_remove() is
initiated via the sysfs, i.e. can be initiated at any time. If we were
to free matrix_mdev in mdev_remove() and then carry on with kvm_unset()
with mutex_lock(&matrix_dev->lock); that would be bad.
> The reason
> matrix_mdev->kvm is set to NULL before giving up
> the matrix_dev->lock is so that functions that check
> for the presence of the matrix_mdev->kvm pointer,
> such as assign_adapter_store() - will exit if they get
> control while the masks are being cleared.
I disagree!
static ssize_t assign_adapter_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
int ret;
unsigned long apid;
struct mdev_device *mdev = mdev_from_dev(dev);
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
/* If the guest is running, disallow assignment of adapter */
if (matrix_mdev->kvm)
return -EBUSY;
We bail out when kvm != NULL, so having it set to NULL while the
mask are being cleared will make these not bail out.
> So what we have
> here is a catch-22; in other words, we have the case
> you pointed out above and the cases related to
> assigning/unassigning adapters, domains and
> control domains which should exit when a guest
> is running.
See above.
>
> I may have an idea to resolve this. Suppose we add:
>
> struct ap_matrix_mdev {
> ...
> bool kvm_busy;
> ...
> }
>
> This flag will be set to true at the start of both the
> vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm()
> and set to false at the end. The assignment/unassignment
> and remove callback functions can test this flag and
> return -EBUSY if the flag is true. That will preclude assigning
> or unassigning adapters, domains and control domains when
> the KVM pointer is being set/unset. Likewise, removal of the
> mediated device will also be prevented while the KVM pointer
> is being set/unset.
>
> In the case of the PQAP handler function, it can wait for the
> set/unset of the KVM pointer as follows:
>
> /while (matrix_mdev->kvm_busy) {//
> // mutex_unlock(&matrix_dev->lock);//
> // msleep(100);//
> // mutex_lock(&matrix_dev->lock);//
> //}//
> //
> //if (!matrix_mdev->kvm)//
> // goto out_unlock;
>
> /What say you?
> //
I'm not sure. Since I disagree with your analysis above it is difficult
to deal with the conclusion. I'm not against decoupling the tracking of
the state of the mdev_matrix device from the value of the kvm pointer. I
think we should first get a common understanding of the problem, before
we proceed to the solution.
Regards,
Halil
When vbus auto discharge is enabled, TCPM can sometimes be faster than
the TCPC i.e. TCPM can go ahead and move the port to unattached state
(involves disabling vbus auto discharge) before TCPC could effectively
discharge vbus to VSAFE0V. This leaves vbus with residual charge and
increases the decay time which prevents tsafe0v from being met.
This change introduces a new state VBUS_DISCHARGE where the TCPM waits
for a maximum of tSafe0V(max) for vbus to discharge to VSAFE0V before
transitioning to unattached state and re-enable toggling. If vbus
discharges to vsafe0v sooner, then, transition to unattached state
happens right away.
Also, while in SNK_READY, when auto discharge is enabled, drive
disconnect based on vbus turning off instead of Rp disappearing on
CC pins. Rp disappearing on CC pins is almost instanteous compared
to vbus decay.
Fixes: f321a02caebd ("usb: typec: tcpm: Implement enabling Auto
Discharge disconnect support")
Signed-off-by: Badhri Jagan Sridharan <badhri(a)google.com>
---
Changes since V1:
- Add Fixes tag
---
drivers/usb/typec/tcpm/tcpm.c | 60 +++++++++++++++++++++++++++++++----
1 file changed, 53 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index be0b6469dd3d..0ed71725980f 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -62,6 +62,8 @@
S(SNK_TRANSITION_SINK_VBUS), \
S(SNK_READY), \
\
+ S(VBUS_DISCHARGE), \
+ \
S(ACC_UNATTACHED), \
S(DEBUG_ACC_ATTACHED), \
S(AUDIO_ACC_ATTACHED), \
@@ -438,6 +440,9 @@ struct tcpm_port {
enum tcpm_ams next_ams;
bool in_ams;
+ /* Auto vbus discharge state */
+ bool auto_vbus_discharge_enabled;
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
struct mutex logbuffer_lock; /* log buffer access lock */
@@ -3413,6 +3418,8 @@ static int tcpm_src_attach(struct tcpm_port *port)
if (port->tcpc->enable_auto_vbus_discharge) {
ret = port->tcpc->enable_auto_vbus_discharge(port->tcpc, true);
tcpm_log_force(port, "enable vbus discharge ret:%d", ret);
+ if (!ret)
+ port->auto_vbus_discharge_enabled = true;
}
ret = tcpm_set_roles(port, true, TYPEC_SOURCE, tcpm_data_role_for_source(port));
@@ -3495,6 +3502,8 @@ static void tcpm_reset_port(struct tcpm_port *port)
if (port->tcpc->enable_auto_vbus_discharge) {
ret = port->tcpc->enable_auto_vbus_discharge(port->tcpc, false);
tcpm_log_force(port, "Disable vbus discharge ret:%d", ret);
+ if (!ret)
+ port->auto_vbus_discharge_enabled = false;
}
port->in_ams = false;
port->ams = NONE_AMS;
@@ -3568,6 +3577,8 @@ static int tcpm_snk_attach(struct tcpm_port *port)
tcpm_set_auto_vbus_discharge_threshold(port, TYPEC_PWR_MODE_USB, false, VSAFE5V);
ret = port->tcpc->enable_auto_vbus_discharge(port->tcpc, true);
tcpm_log_force(port, "enable vbus discharge ret:%d", ret);
+ if (!ret)
+ port->auto_vbus_discharge_enabled = true;
}
ret = tcpm_set_roles(port, true, TYPEC_SINK, tcpm_data_role_for_sink(port));
@@ -3684,6 +3695,12 @@ static void run_state_machine(struct tcpm_port *port)
switch (port->state) {
case TOGGLING:
break;
+ case VBUS_DISCHARGE:
+ if (port->port_type == TYPEC_PORT_SRC)
+ tcpm_set_state(port, SRC_UNATTACHED, PD_T_SAFE_0V);
+ else
+ tcpm_set_state(port, SNK_UNATTACHED, PD_T_SAFE_0V);
+ break;
/* SRC states */
case SRC_UNATTACHED:
if (!port->non_pd_role_swap)
@@ -4669,7 +4686,9 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
case SRC_READY:
if (tcpm_port_is_disconnected(port) ||
!tcpm_port_is_source(port)) {
- if (port->port_type == TYPEC_PORT_SRC)
+ if (port->auto_vbus_discharge_enabled && !port->vbus_vsafe0v)
+ tcpm_set_state(port, VBUS_DISCHARGE, 0);
+ else if (port->port_type == TYPEC_PORT_SRC)
tcpm_set_state(port, SRC_UNATTACHED, 0);
else
tcpm_set_state(port, SNK_UNATTACHED, 0);
@@ -4703,7 +4722,18 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
tcpm_set_state(port, SNK_DEBOUNCED, 0);
break;
case SNK_READY:
- if (tcpm_port_is_disconnected(port))
+ /*
+ * When set_auto_vbus_discharge_threshold is enabled, CC pins go
+ * away before vbus decays to disconnect threshold. Allow
+ * disconnect to be driven by vbus disconnect when auto vbus
+ * discharge is enabled.
+ *
+ * EXIT condition is based primarily on vbus disconnect and CC is secondary.
+ * "A port that has entered into USB PD communications with the Source and
+ * has seen the CC voltage exceed vRd-USB may monitor the CC pin to detect
+ * cable disconnect in addition to monitoring VBUS.
+ */
+ if (!port->auto_vbus_discharge_enabled && tcpm_port_is_disconnected(port))
tcpm_set_state(port, unattached_state(port), 0);
else if (!port->pd_capable &&
(cc1 != old_cc1 || cc2 != old_cc2))
@@ -4803,9 +4833,16 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
*/
break;
+ case VBUS_DISCHARGE:
+ /* Do nothing. Waiting for vsafe0v signal */
+ break;
default:
- if (tcpm_port_is_disconnected(port))
- tcpm_set_state(port, unattached_state(port), 0);
+ if (tcpm_port_is_disconnected(port)) {
+ if (port->auto_vbus_discharge_enabled && !port->vbus_vsafe0v)
+ tcpm_set_state(port, VBUS_DISCHARGE, 0);
+ else
+ tcpm_set_state(port, unattached_state(port), 0);
+ }
break;
}
}
@@ -4988,9 +5025,12 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
break;
default:
- if (port->pwr_role == TYPEC_SINK &&
- port->attached)
- tcpm_set_state(port, SNK_UNATTACHED, 0);
+ if (port->pwr_role == TYPEC_SINK && port->attached) {
+ if (port->auto_vbus_discharge_enabled && !port->vbus_vsafe0v)
+ tcpm_set_state(port, VBUS_DISCHARGE, 0);
+ else
+ tcpm_set_state(port, SNK_UNATTACHED, 0);
+ }
break;
}
}
@@ -5012,6 +5052,12 @@ static void _tcpm_pd_vbus_vsafe0v(struct tcpm_port *port)
tcpm_set_state(port, tcpm_try_snk(port) ? SNK_TRY : SRC_ATTACHED,
PD_T_CC_DEBOUNCE);
break;
+ case VBUS_DISCHARGE:
+ if (port->port_type == TYPEC_PORT_SRC)
+ tcpm_set_state(port, SRC_UNATTACHED, 0);
+ else
+ tcpm_set_state(port, SNK_UNATTACHED, 0);
+ break;
default:
break;
}
--
2.30.0.617.g56c4b15f3c-goog
From: Thomas Gleixner <tglx(a)linutronix.de>
The handle_exit_race() function is defined in commit c158b461306df82
("futex: Cure exit race"), which never returns -EBUSY. This results
in a small piece of dead code in the attach_to_pi_owner() function:
int ret = handle_exit_race(uaddr, uval, p); /* Never return -EBUSY */
...
if (ret == -EBUSY)
*exiting = p; /* dead code */
The return value -EBUSY is added to handle_exit_race() in upsteam
commit ac31c7ff8624409 ("futex: Provide distinct return value when
owner is exiting"). This commit was incorporated into v4.9.255, before
the function handle_exit_race() was introduced, whitout Modify
handle_exit_race().
To fix dead code, extract the change of handle_exit_race() from
commit ac31c7ff8624409 ("futex: Provide distinct return value when owner
is exiting"), re-incorporated.
Fixes: c158b461306df82 ("futex: Cure exit race")
Cc: stable(a)vger.kernel.org # 4.9.258-rc1
Signed-off-by: Xiaoming Ni <nixiaoming(a)huawei.com>
---
kernel/futex.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index b65dbb5d60bb..0fd785410150 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1207,11 +1207,11 @@ static int handle_exit_race(u32 __user *uaddr, u32 uval,
u32 uval2;
/*
- * If the futex exit state is not yet FUTEX_STATE_DEAD, wait
- * for it to finish.
+ * If the futex exit state is not yet FUTEX_STATE_DEAD, tell the
+ * caller that the alleged owner is busy.
*/
if (tsk && tsk->futex_state != FUTEX_STATE_DEAD)
- return -EAGAIN;
+ return -EBUSY;
/*
* Reread the user space value to handle the following situation:
--
2.27.0
The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable(a)vger.kernel.org>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
>From a9545779ee9e9e103648f6f2552e73cfe808d0f4 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc(a)google.com>
Date: Mon, 8 Feb 2021 12:19:40 -0800
Subject: [PATCH] KVM: Use kvm_pfn_t for local PFN variable in
hva_to_pfn_remapped()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Use kvm_pfn_t, a.k.a. u64, for the local 'pfn' variable when retrieving
a so called "remapped" hva/pfn pair. In theory, the hva could resolve to
a pfn in high memory on a 32-bit kernel.
This bug was inadvertantly exposed by commit bd2fae8da794 ("KVM: do not
assume PTE is writable after follow_pfn"), which added an error PFN value
to the mix, causing gcc to comlain about overflowing the unsigned long.
arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function ‘hva_to_pfn_remapped’:
include/linux/kvm_host.h:89:30: error: conversion from ‘long long unsigned int’
to ‘long unsigned int’ changes value from
‘9218868437227405314’ to ‘2’ [-Werror=overflow]
89 | #define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 2)
| ^
virt/kvm/kvm_main.c:1935:9: note: in expansion of macro ‘KVM_PFN_ERR_RO_FAULT’
Cc: stable(a)vger.kernel.org
Fixes: add6a0cd1c5b ("KVM: MMU: try to fix up page faults before giving up")
Signed-off-by: Sean Christopherson <seanjc(a)google.com>
Message-Id: <20210208201940.1258328-1-seanjc(a)google.com>
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ee4ac2618ec5..001b9de4e727 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1906,7 +1906,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
bool write_fault, bool *writable,
kvm_pfn_t *p_pfn)
{
- unsigned long pfn;
+ kvm_pfn_t pfn;
pte_t *ptep;
spinlock_t *ptl;
int r;
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable(a)vger.kernel.org>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
>From a9545779ee9e9e103648f6f2552e73cfe808d0f4 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc(a)google.com>
Date: Mon, 8 Feb 2021 12:19:40 -0800
Subject: [PATCH] KVM: Use kvm_pfn_t for local PFN variable in
hva_to_pfn_remapped()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Use kvm_pfn_t, a.k.a. u64, for the local 'pfn' variable when retrieving
a so called "remapped" hva/pfn pair. In theory, the hva could resolve to
a pfn in high memory on a 32-bit kernel.
This bug was inadvertantly exposed by commit bd2fae8da794 ("KVM: do not
assume PTE is writable after follow_pfn"), which added an error PFN value
to the mix, causing gcc to comlain about overflowing the unsigned long.
arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function ‘hva_to_pfn_remapped’:
include/linux/kvm_host.h:89:30: error: conversion from ‘long long unsigned int’
to ‘long unsigned int’ changes value from
‘9218868437227405314’ to ‘2’ [-Werror=overflow]
89 | #define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 2)
| ^
virt/kvm/kvm_main.c:1935:9: note: in expansion of macro ‘KVM_PFN_ERR_RO_FAULT’
Cc: stable(a)vger.kernel.org
Fixes: add6a0cd1c5b ("KVM: MMU: try to fix up page faults before giving up")
Signed-off-by: Sean Christopherson <seanjc(a)google.com>
Message-Id: <20210208201940.1258328-1-seanjc(a)google.com>
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ee4ac2618ec5..001b9de4e727 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1906,7 +1906,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
bool write_fault, bool *writable,
kvm_pfn_t *p_pfn)
{
- unsigned long pfn;
+ kvm_pfn_t pfn;
pte_t *ptep;
spinlock_t *ptl;
int r;
The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable(a)vger.kernel.org>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
>From a9545779ee9e9e103648f6f2552e73cfe808d0f4 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc(a)google.com>
Date: Mon, 8 Feb 2021 12:19:40 -0800
Subject: [PATCH] KVM: Use kvm_pfn_t for local PFN variable in
hva_to_pfn_remapped()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Use kvm_pfn_t, a.k.a. u64, for the local 'pfn' variable when retrieving
a so called "remapped" hva/pfn pair. In theory, the hva could resolve to
a pfn in high memory on a 32-bit kernel.
This bug was inadvertantly exposed by commit bd2fae8da794 ("KVM: do not
assume PTE is writable after follow_pfn"), which added an error PFN value
to the mix, causing gcc to comlain about overflowing the unsigned long.
arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function ‘hva_to_pfn_remapped’:
include/linux/kvm_host.h:89:30: error: conversion from ‘long long unsigned int’
to ‘long unsigned int’ changes value from
‘9218868437227405314’ to ‘2’ [-Werror=overflow]
89 | #define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 2)
| ^
virt/kvm/kvm_main.c:1935:9: note: in expansion of macro ‘KVM_PFN_ERR_RO_FAULT’
Cc: stable(a)vger.kernel.org
Fixes: add6a0cd1c5b ("KVM: MMU: try to fix up page faults before giving up")
Signed-off-by: Sean Christopherson <seanjc(a)google.com>
Message-Id: <20210208201940.1258328-1-seanjc(a)google.com>
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ee4ac2618ec5..001b9de4e727 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1906,7 +1906,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
bool write_fault, bool *writable,
kvm_pfn_t *p_pfn)
{
- unsigned long pfn;
+ kvm_pfn_t pfn;
pte_t *ptep;
spinlock_t *ptl;
int r;