* [PATCH v2] x86/svm: retry after unhandled NPT fault if gfn was marked for recalculation
@ 2020-05-27 1:01 Igor Druzhinin
2020-05-27 18:14 ` Roger Pau Monné
0 siblings, 1 reply; 2+ messages in thread
From: Igor Druzhinin @ 2020-05-27 1:01 UTC (permalink / raw)
To: xen-devel
Cc: Igor Druzhinin, kevin.tian, jbeulich, wl, andrew.cooper3,
jun.nakajima, roger.pau
If a recalculation NPT fault hasn't been handled explicitly in
hvm_hap_nested_page_fault() then it's potentially safe to retry -
US bit has been re-instated in PTE and any real fault would be correctly
re-raised next time. Do it by allowing hvm_hap_nested_page_fault to
fall through in that case.
This covers a specific case of migration with vGPU assigned on AMD:
global log-dirty is enabled and causes immediate recalculation NPT
fault in MMIO area upon access. This type of fault isn't described
explicitly in hvm_hap_nested_page_fault (this isn't called on
EPT misconfig exit on Intel) which results in domain crash.
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v2:
- don't gamble with retrying every recal fault and instead let
hvm_hap_nested_page_fault know it's allowed to fall through in default case
---
xen/arch/x86/hvm/hvm.c | 6 +++---
xen/arch/x86/hvm/svm/svm.c | 7 ++++++-
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
xen/include/asm-x86/hvm/hvm.h | 2 +-
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 74c9f84..42bd720 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1731,7 +1731,7 @@ void hvm_inject_event(const struct x86_event *event)
}
int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
- struct npfec npfec)
+ struct npfec npfec, bool fall_through)
{
unsigned long gfn = gpa >> PAGE_SHIFT;
p2m_type_t p2mt;
@@ -1740,7 +1740,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
struct vcpu *curr = current;
struct domain *currd = curr->domain;
struct p2m_domain *p2m, *hostp2m;
- int rc, fall_through = 0, paged = 0;
+ int rc, paged = 0;
bool sharing_enomem = false;
vm_event_request_t *req_ptr = NULL;
bool sync = false;
@@ -1905,7 +1905,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr);
if ( !sync )
- fall_through = 1;
+ fall_through = true;
else
{
/* Rights not promoted (aka. sync event), work here is done */
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 46a1aac..8ef3fed 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1695,7 +1695,12 @@ static void svm_do_nested_pgfault(struct vcpu *v,
else if ( pfec & NPT_PFEC_in_gpt )
npfec.kind = npfec_kind_in_gpt;
- ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
+ /*
+ * US bit being set in error code indicates P2M type recalculation has
+ * just been done meaning that it's possible there is nothing else to handle
+ * and we can just fall through and retry.
+ */
+ ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec, !!(pfec & PFEC_user_mode));
if ( tb_init_done )
{
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 11a4dd9..10f1eeb 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3398,7 +3398,7 @@ static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
else
gla = ~0ull;
- ret = hvm_hap_nested_page_fault(gpa, gla, npfec);
+ ret = hvm_hap_nested_page_fault(gpa, gla, npfec, false);
switch ( ret )
{
case 0: // Unhandled L1 EPT violation
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 1eb377d..03e5f1d 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -329,7 +329,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx);
struct npfec;
int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
- struct npfec npfec);
+ struct npfec npfec, bool fall_through);
/* Check CR4/EFER values */
const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] x86/svm: retry after unhandled NPT fault if gfn was marked for recalculation
2020-05-27 1:01 [PATCH v2] x86/svm: retry after unhandled NPT fault if gfn was marked for recalculation Igor Druzhinin
@ 2020-05-27 18:14 ` Roger Pau Monné
0 siblings, 0 replies; 2+ messages in thread
From: Roger Pau Monné @ 2020-05-27 18:14 UTC (permalink / raw)
To: Igor Druzhinin
Cc: kevin.tian, jbeulich, wl, andrew.cooper3, jun.nakajima, xen-devel
On Wed, May 27, 2020 at 02:01:48AM +0100, Igor Druzhinin wrote:
> If a recalculation NPT fault hasn't been handled explicitly in
> hvm_hap_nested_page_fault() then it's potentially safe to retry -
> US bit has been re-instated in PTE and any real fault would be correctly
> re-raised next time. Do it by allowing hvm_hap_nested_page_fault to
> fall through in that case.
>
> This covers a specific case of migration with vGPU assigned on AMD:
> global log-dirty is enabled and causes immediate recalculation NPT
> fault in MMIO area upon access. This type of fault isn't described
> explicitly in hvm_hap_nested_page_fault (this isn't called on
> EPT misconfig exit on Intel) which results in domain crash.
>
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> Changes in v2:
> - don't gamble with retrying every recal fault and instead let
> hvm_hap_nested_page_fault know it's allowed to fall through in default case
> ---
> xen/arch/x86/hvm/hvm.c | 6 +++---
> xen/arch/x86/hvm/svm/svm.c | 7 ++++++-
> xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> xen/include/asm-x86/hvm/hvm.h | 2 +-
> 4 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 74c9f84..42bd720 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1731,7 +1731,7 @@ void hvm_inject_event(const struct x86_event *event)
> }
>
> int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> - struct npfec npfec)
> + struct npfec npfec, bool fall_through)
> {
> unsigned long gfn = gpa >> PAGE_SHIFT;
> p2m_type_t p2mt;
> @@ -1740,7 +1740,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> struct vcpu *curr = current;
> struct domain *currd = curr->domain;
> struct p2m_domain *p2m, *hostp2m;
> - int rc, fall_through = 0, paged = 0;
> + int rc, paged = 0;
> bool sharing_enomem = false;
> vm_event_request_t *req_ptr = NULL;
> bool sync = false;
I would assert that the parameter is never set when running on Intel,
since those code path is not supposed to use it.
I also wonder whether it would be possible to avoid passing a
parameter, and instead check whether the guest is in logdirty mode on
AMD and the fault is actually a logdirty triggered one. That would IMO
make it more robust since the caller doesn't need to care about
whether it's a recalc fault or not.
> @@ -1905,7 +1905,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr);
>
> if ( !sync )
> - fall_through = 1;
> + fall_through = true;
> else
> {
> /* Rights not promoted (aka. sync event), work here is done */
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 46a1aac..8ef3fed 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1695,7 +1695,12 @@ static void svm_do_nested_pgfault(struct vcpu *v,
> else if ( pfec & NPT_PFEC_in_gpt )
> npfec.kind = npfec_kind_in_gpt;
>
> - ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
> + /*
> + * US bit being set in error code indicates P2M type recalculation has
> + * just been done meaning that it's possible there is nothing else to handle
> + * and we can just fall through and retry.
> + */
> + ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec, !!(pfec & PFEC_user_mode));
>
> if ( tb_init_done )
> {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 11a4dd9..10f1eeb 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3398,7 +3398,7 @@ static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
> else
> gla = ~0ull;
>
> - ret = hvm_hap_nested_page_fault(gpa, gla, npfec);
> + ret = hvm_hap_nested_page_fault(gpa, gla, npfec, false);
> switch ( ret )
> {
> case 0: // Unhandled L1 EPT violation
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 1eb377d..03e5f1d 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -329,7 +329,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx);
>
> struct npfec;
> int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> - struct npfec npfec);
> + struct npfec npfec, bool fall_through);
I would rename fall_through to recalc, recalculate or misconfig. It's
not easy to understand the meaning of the parameter when looking at
the function prototype.
Thanks, Roger.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-27 18:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 1:01 [PATCH v2] x86/svm: retry after unhandled NPT fault if gfn was marked for recalculation Igor Druzhinin
2020-05-27 18:14 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.