* [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
@ 2019-05-20 12:55 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-20 12:55 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
paul.durrant, jbeulich, Alexandru Stefan ISAILA, roger.pau
Thiis is done so hvmemul_linear_to_phys() can be called from
hvmemul_send_vm_event().
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
xen/arch/x86/hvm/emulate.c | 181 ++++++++++++++++++-------------------
1 file changed, 90 insertions(+), 91 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 8659c89862..254ff6515d 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -530,6 +530,95 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
}
+/*
+ * Convert addr from linear to physical form, valid over the range
+ * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
+ * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
+ * @pfec indicates the access checks to be performed during page-table walks.
+ */
+static int hvmemul_linear_to_phys(
+ unsigned long addr,
+ paddr_t *paddr,
+ unsigned int bytes_per_rep,
+ unsigned long *reps,
+ uint32_t pfec,
+ struct hvm_emulate_ctxt *hvmemul_ctxt)
+{
+ struct vcpu *curr = current;
+ unsigned long pfn, npfn, done, todo, i, offset = addr & ~PAGE_MASK;
+ int reverse;
+
+ /*
+ * Clip repetitions to a sensible maximum. This avoids extensive looping in
+ * this function while still amortising the cost of I/O trap-and-emulate.
+ */
+ *reps = min_t(unsigned long, *reps, 4096);
+
+ /* With no paging it's easy: linear == physical. */
+ if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PG) )
+ {
+ *paddr = addr;
+ return X86EMUL_OKAY;
+ }
+
+ /* Reverse mode if this is a backwards multi-iteration string operation. */
+ reverse = (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1);
+
+ if ( reverse && ((PAGE_SIZE - offset) < bytes_per_rep) )
+ {
+ /* Do page-straddling first iteration forwards via recursion. */
+ paddr_t _paddr;
+ unsigned long one_rep = 1;
+ int rc = hvmemul_linear_to_phys(
+ addr, &_paddr, bytes_per_rep, &one_rep, pfec, hvmemul_ctxt);
+ if ( rc != X86EMUL_OKAY )
+ return rc;
+ pfn = _paddr >> PAGE_SHIFT;
+ }
+ else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == gfn_x(INVALID_GFN) )
+ {
+ if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
+ return X86EMUL_RETRY;
+ *reps = 0;
+ x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt);
+ return X86EMUL_EXCEPTION;
+ }
+
+ done = reverse ? bytes_per_rep + offset : PAGE_SIZE - offset;
+ todo = *reps * bytes_per_rep;
+ for ( i = 1; done < todo; i++ )
+ {
+ /* Get the next PFN in the range. */
+ addr += reverse ? -PAGE_SIZE : PAGE_SIZE;
+ npfn = paging_gva_to_gfn(curr, addr, &pfec);
+
+ /* Is it contiguous with the preceding PFNs? If not then we're done. */
+ if ( (npfn == gfn_x(INVALID_GFN)) ||
+ (npfn != (pfn + (reverse ? -i : i))) )
+ {
+ if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
+ return X86EMUL_RETRY;
+ done /= bytes_per_rep;
+ if ( done == 0 )
+ {
+ ASSERT(!reverse);
+ if ( npfn != gfn_x(INVALID_GFN) )
+ return X86EMUL_UNHANDLEABLE;
+ *reps = 0;
+ x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
+ return X86EMUL_EXCEPTION;
+ }
+ *reps = done;
+ break;
+ }
+
+ done += PAGE_SIZE;
+ }
+
+ *paddr = ((paddr_t)pfn << PAGE_SHIFT) | offset;
+ return X86EMUL_OKAY;
+}
+
/*
* Map the frame(s) covering an individual linear access, for writeable
* access. May return NULL for MMIO, or ERR_PTR(~X86EMUL_*) for other errors
@@ -692,97 +781,7 @@ static void hvmemul_unmap_linear_addr(
*mfn++ = _mfn(0);
}
#endif
-}
-
-/*
- * Convert addr from linear to physical form, valid over the range
- * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
- * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
- * @pfec indicates the access checks to be performed during page-table walks.
- */
-static int hvmemul_linear_to_phys(
- unsigned long addr,
- paddr_t *paddr,
- unsigned int bytes_per_rep,
- unsigned long *reps,
- uint32_t pfec,
- struct hvm_emulate_ctxt *hvmemul_ctxt)
-{
- struct vcpu *curr = current;
- unsigned long pfn, npfn, done, todo, i, offset = addr & ~PAGE_MASK;
- int reverse;
-
- /*
- * Clip repetitions to a sensible maximum. This avoids extensive looping in
- * this function while still amortising the cost of I/O trap-and-emulate.
- */
- *reps = min_t(unsigned long, *reps, 4096);
-
- /* With no paging it's easy: linear == physical. */
- if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PG) )
- {
- *paddr = addr;
- return X86EMUL_OKAY;
- }
-
- /* Reverse mode if this is a backwards multi-iteration string operation. */
- reverse = (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1);
-
- if ( reverse && ((PAGE_SIZE - offset) < bytes_per_rep) )
- {
- /* Do page-straddling first iteration forwards via recursion. */
- paddr_t _paddr;
- unsigned long one_rep = 1;
- int rc = hvmemul_linear_to_phys(
- addr, &_paddr, bytes_per_rep, &one_rep, pfec, hvmemul_ctxt);
- if ( rc != X86EMUL_OKAY )
- return rc;
- pfn = _paddr >> PAGE_SHIFT;
- }
- else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == gfn_x(INVALID_GFN) )
- {
- if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
- return X86EMUL_RETRY;
- *reps = 0;
- x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt);
- return X86EMUL_EXCEPTION;
- }
-
- done = reverse ? bytes_per_rep + offset : PAGE_SIZE - offset;
- todo = *reps * bytes_per_rep;
- for ( i = 1; done < todo; i++ )
- {
- /* Get the next PFN in the range. */
- addr += reverse ? -PAGE_SIZE : PAGE_SIZE;
- npfn = paging_gva_to_gfn(curr, addr, &pfec);
-
- /* Is it contiguous with the preceding PFNs? If not then we're done. */
- if ( (npfn == gfn_x(INVALID_GFN)) ||
- (npfn != (pfn + (reverse ? -i : i))) )
- {
- if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
- return X86EMUL_RETRY;
- done /= bytes_per_rep;
- if ( done == 0 )
- {
- ASSERT(!reverse);
- if ( npfn != gfn_x(INVALID_GFN) )
- return X86EMUL_UNHANDLEABLE;
- *reps = 0;
- x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
- return X86EMUL_EXCEPTION;
- }
- *reps = done;
- break;
- }
-
- done += PAGE_SIZE;
- }
-
- *paddr = ((paddr_t)pfn << PAGE_SHIFT) | offset;
- return X86EMUL_OKAY;
-}
-
+}
static int hvmemul_virtual_to_linear(
enum x86_segment seg,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Xen-devel] [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
@ 2019-05-20 12:55 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-20 12:55 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
paul.durrant, jbeulich, Alexandru Stefan ISAILA, roger.pau
Thiis is done so hvmemul_linear_to_phys() can be called from
hvmemul_send_vm_event().
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
xen/arch/x86/hvm/emulate.c | 181 ++++++++++++++++++-------------------
1 file changed, 90 insertions(+), 91 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 8659c89862..254ff6515d 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -530,6 +530,95 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
}
+/*
+ * Convert addr from linear to physical form, valid over the range
+ * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
+ * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
+ * @pfec indicates the access checks to be performed during page-table walks.
+ */
+static int hvmemul_linear_to_phys(
+ unsigned long addr,
+ paddr_t *paddr,
+ unsigned int bytes_per_rep,
+ unsigned long *reps,
+ uint32_t pfec,
+ struct hvm_emulate_ctxt *hvmemul_ctxt)
+{
+ struct vcpu *curr = current;
+ unsigned long pfn, npfn, done, todo, i, offset = addr & ~PAGE_MASK;
+ int reverse;
+
+ /*
+ * Clip repetitions to a sensible maximum. This avoids extensive looping in
+ * this function while still amortising the cost of I/O trap-and-emulate.
+ */
+ *reps = min_t(unsigned long, *reps, 4096);
+
+ /* With no paging it's easy: linear == physical. */
+ if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PG) )
+ {
+ *paddr = addr;
+ return X86EMUL_OKAY;
+ }
+
+ /* Reverse mode if this is a backwards multi-iteration string operation. */
+ reverse = (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1);
+
+ if ( reverse && ((PAGE_SIZE - offset) < bytes_per_rep) )
+ {
+ /* Do page-straddling first iteration forwards via recursion. */
+ paddr_t _paddr;
+ unsigned long one_rep = 1;
+ int rc = hvmemul_linear_to_phys(
+ addr, &_paddr, bytes_per_rep, &one_rep, pfec, hvmemul_ctxt);
+ if ( rc != X86EMUL_OKAY )
+ return rc;
+ pfn = _paddr >> PAGE_SHIFT;
+ }
+ else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == gfn_x(INVALID_GFN) )
+ {
+ if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
+ return X86EMUL_RETRY;
+ *reps = 0;
+ x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt);
+ return X86EMUL_EXCEPTION;
+ }
+
+ done = reverse ? bytes_per_rep + offset : PAGE_SIZE - offset;
+ todo = *reps * bytes_per_rep;
+ for ( i = 1; done < todo; i++ )
+ {
+ /* Get the next PFN in the range. */
+ addr += reverse ? -PAGE_SIZE : PAGE_SIZE;
+ npfn = paging_gva_to_gfn(curr, addr, &pfec);
+
+ /* Is it contiguous with the preceding PFNs? If not then we're done. */
+ if ( (npfn == gfn_x(INVALID_GFN)) ||
+ (npfn != (pfn + (reverse ? -i : i))) )
+ {
+ if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
+ return X86EMUL_RETRY;
+ done /= bytes_per_rep;
+ if ( done == 0 )
+ {
+ ASSERT(!reverse);
+ if ( npfn != gfn_x(INVALID_GFN) )
+ return X86EMUL_UNHANDLEABLE;
+ *reps = 0;
+ x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
+ return X86EMUL_EXCEPTION;
+ }
+ *reps = done;
+ break;
+ }
+
+ done += PAGE_SIZE;
+ }
+
+ *paddr = ((paddr_t)pfn << PAGE_SHIFT) | offset;
+ return X86EMUL_OKAY;
+}
+
/*
* Map the frame(s) covering an individual linear access, for writeable
* access. May return NULL for MMIO, or ERR_PTR(~X86EMUL_*) for other errors
@@ -692,97 +781,7 @@ static void hvmemul_unmap_linear_addr(
*mfn++ = _mfn(0);
}
#endif
-}
-
-/*
- * Convert addr from linear to physical form, valid over the range
- * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
- * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
- * @pfec indicates the access checks to be performed during page-table walks.
- */
-static int hvmemul_linear_to_phys(
- unsigned long addr,
- paddr_t *paddr,
- unsigned int bytes_per_rep,
- unsigned long *reps,
- uint32_t pfec,
- struct hvm_emulate_ctxt *hvmemul_ctxt)
-{
- struct vcpu *curr = current;
- unsigned long pfn, npfn, done, todo, i, offset = addr & ~PAGE_MASK;
- int reverse;
-
- /*
- * Clip repetitions to a sensible maximum. This avoids extensive looping in
- * this function while still amortising the cost of I/O trap-and-emulate.
- */
- *reps = min_t(unsigned long, *reps, 4096);
-
- /* With no paging it's easy: linear == physical. */
- if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PG) )
- {
- *paddr = addr;
- return X86EMUL_OKAY;
- }
-
- /* Reverse mode if this is a backwards multi-iteration string operation. */
- reverse = (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1);
-
- if ( reverse && ((PAGE_SIZE - offset) < bytes_per_rep) )
- {
- /* Do page-straddling first iteration forwards via recursion. */
- paddr_t _paddr;
- unsigned long one_rep = 1;
- int rc = hvmemul_linear_to_phys(
- addr, &_paddr, bytes_per_rep, &one_rep, pfec, hvmemul_ctxt);
- if ( rc != X86EMUL_OKAY )
- return rc;
- pfn = _paddr >> PAGE_SHIFT;
- }
- else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == gfn_x(INVALID_GFN) )
- {
- if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
- return X86EMUL_RETRY;
- *reps = 0;
- x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt);
- return X86EMUL_EXCEPTION;
- }
-
- done = reverse ? bytes_per_rep + offset : PAGE_SIZE - offset;
- todo = *reps * bytes_per_rep;
- for ( i = 1; done < todo; i++ )
- {
- /* Get the next PFN in the range. */
- addr += reverse ? -PAGE_SIZE : PAGE_SIZE;
- npfn = paging_gva_to_gfn(curr, addr, &pfec);
-
- /* Is it contiguous with the preceding PFNs? If not then we're done. */
- if ( (npfn == gfn_x(INVALID_GFN)) ||
- (npfn != (pfn + (reverse ? -i : i))) )
- {
- if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
- return X86EMUL_RETRY;
- done /= bytes_per_rep;
- if ( done == 0 )
- {
- ASSERT(!reverse);
- if ( npfn != gfn_x(INVALID_GFN) )
- return X86EMUL_UNHANDLEABLE;
- *reps = 0;
- x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
- return X86EMUL_EXCEPTION;
- }
- *reps = done;
- break;
- }
-
- done += PAGE_SIZE;
- }
-
- *paddr = ((paddr_t)pfn << PAGE_SHIFT) | offset;
- return X86EMUL_OKAY;
-}
-
+}
static int hvmemul_virtual_to_linear(
enum x86_segment seg,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-20 12:55 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-20 12:55 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
paul.durrant, jbeulich, Alexandru Stefan ISAILA, roger.pau
This patch aims to have mem access vm events sent from the emulator.
This is useful in the case of emulated instructions that cause
page-walks on access protected pages.
We use hvmemul_map_linear_addr() ro intercept r/w access and
hvmemul_insn_fetch() to intercept exec access.
First we try to send a vm event and if the event is sent then emulation
returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
emulation goes on as expected.
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
Changes since V3:
- Calculate gpa in hvmemul_send_vm_event()
- Move hvmemul_linear_to_phys() call inside
hvmemul_send_vm_event()
- Check only if hvmemul_virtual_to_linear() returns X86EMUL_OKAY
- Add commnet for X86EMUL_ACCESS_EXCEPTION.
---
xen/arch/x86/hvm/emulate.c | 89 +++++++++++++++++++++++++-
xen/arch/x86/hvm/vm_event.c | 2 +-
xen/arch/x86/mm/mem_access.c | 3 +-
xen/arch/x86/x86_emulate/x86_emulate.h | 2 +
xen/include/asm-x86/hvm/emulate.h | 4 +-
5 files changed, 95 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 254ff6515d..75403ebc9b 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -15,6 +15,7 @@
#include <xen/paging.h>
#include <xen/trace.h>
#include <xen/vm_event.h>
+#include <xen/monitor.h>
#include <asm/event.h>
#include <asm/i387.h>
#include <asm/xstate.h>
@@ -26,6 +27,7 @@
#include <asm/hvm/support.h>
#include <asm/hvm/svm/svm.h>
#include <asm/vm_event.h>
+#include <asm/altp2m.h>
static void hvmtrace_io_assist(const ioreq_t *p)
{
@@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
return X86EMUL_OKAY;
}
+static bool hvmemul_send_vm_event(unsigned long gla,
+ uint32_t pfec, unsigned int bytes,
+ struct hvm_emulate_ctxt ctxt)
+{
+ xenmem_access_t access;
+ vm_event_request_t req = {};
+ gfn_t gfn;
+ paddr_t gpa;
+ unsigned long reps = 1;
+ int rc;
+
+ if ( !ctxt.send_event || !pfec )
+ return false;
+
+ rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
+
+ if ( rc != X86EMUL_OKAY )
+ return false;
+
+ gfn = gaddr_to_gfn(gpa);
+
+ if ( p2m_get_mem_access(current->domain, gfn, &access,
+ altp2m_vcpu_idx(current)) != 0 )
+ return false;
+
+ switch ( access ) {
+ case XENMEM_access_x:
+ case XENMEM_access_rx:
+ if ( pfec & PFEC_write_access )
+ req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
+ break;
+
+ case XENMEM_access_w:
+ case XENMEM_access_rw:
+ if ( pfec & PFEC_insn_fetch )
+ req.u.mem_access.flags = MEM_ACCESS_X;
+ break;
+
+ case XENMEM_access_r:
+ case XENMEM_access_n:
+ if ( pfec & PFEC_write_access )
+ req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
+ if ( pfec & PFEC_insn_fetch )
+ req.u.mem_access.flags |= MEM_ACCESS_X;
+ break;
+
+ default:
+ return false;
+ }
+
+ if ( !req.u.mem_access.flags )
+ return false; /* no violation */
+
+ req.reason = VM_EVENT_REASON_MEM_ACCESS;
+ req.u.mem_access.gfn = gfn_x(gfn);
+ req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA | MEM_ACCESS_GLA_VALID;
+ req.u.mem_access.gla = gla;
+ req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
+
+ return monitor_traps(current, true, &req) >= 0;
+}
+
/*
* Map the frame(s) covering an individual linear access, for writeable
* access. May return NULL for MMIO, or ERR_PTR(~X86EMUL_*) for other errors
@@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr(
unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
(linear >> PAGE_SHIFT) + 1;
unsigned int i;
+ gfn_t gfn;
/*
* mfn points to the next free slot. All used slots have a page reference
@@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr(
ASSERT(mfn_x(*mfn) == 0);
res = hvm_translate_get_page(curr, addr, true, pfec,
- &pfinfo, &page, NULL, &p2mt);
+ &pfinfo, &page, &gfn, &p2mt);
switch ( res )
{
@@ -704,6 +769,11 @@ static void *hvmemul_map_linear_addr(
if ( pfec & PFEC_write_access )
{
+ if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
+ {
+ err = ERR_PTR(~X86EMUL_ACCESS_EXCEPTION);
+ goto out;
+ }
if ( p2m_is_discard_write(p2mt) )
{
err = ERR_PTR(~X86EMUL_OKAY);
@@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
/* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
+ uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+ unsigned long addr, reps = 1;
+ int rc = 0;
+
+ rc = hvmemul_virtual_to_linear(
+ seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
+
+ if ( rc != X86EMUL_OKAY || !bytes )
+ return rc;
+
+ if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
+ pfec |= PFEC_user_mode;
+ if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
+ return X86EMUL_ACCESS_EXCEPTION;
/*
* Fall back if requested bytes are not in the prefetch cache.
* But always perform the (fake) read when bytes == 0.
@@ -2508,12 +2592,13 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
}
void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
- unsigned int errcode)
+ unsigned int errcode, bool send_event)
{
struct hvm_emulate_ctxt ctx = {{ 0 }};
int rc;
hvm_emulate_init_once(&ctx, NULL, guest_cpu_user_regs());
+ ctx.send_event = send_event;
switch ( kind )
{
diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
index 121de23071..6d203e8db5 100644
--- a/xen/arch/x86/hvm/vm_event.c
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -87,7 +87,7 @@ void hvm_vm_event_do_resume(struct vcpu *v)
kind = EMUL_KIND_SET_CONTEXT_INSN;
hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
- X86_EVENT_NO_EC);
+ X86_EVENT_NO_EC, false);
v->arch.vm_event->emulate_flags = 0;
}
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 0144f92b98..c9972bab8c 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -214,7 +214,8 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
d->arch.monitor.inguest_pagefault_disabled &&
npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
{
- hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, X86_EVENT_NO_EC);
+ hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
+ X86_EVENT_NO_EC, true);
return true;
}
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 08645762cc..8a20e733fa 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
/* (cmpxchg accessor): CMPXCHG failed. */
#define X86EMUL_CMPXCHG_FAILED 7
+/* Emulator tried to access a protected page. */
+#define X86EMUL_ACCESS_EXCEPTION 8
/* FPU sub-types which may be requested via ->get_fpu(). */
enum x86_emulate_fpu_type {
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index b39a1a0331..ed22ed0baf 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -47,6 +47,7 @@ struct hvm_emulate_ctxt {
uint32_t intr_shadow;
bool_t set_context;
+ bool send_event;
};
enum emul_kind {
@@ -63,7 +64,8 @@ int hvm_emulate_one(
struct hvm_emulate_ctxt *hvmemul_ctxt);
void hvm_emulate_one_vm_event(enum emul_kind kind,
unsigned int trapnr,
- unsigned int errcode);
+ unsigned int errcode,
+ bool send_event);
/* Must be called once to set up hvmemul state. */
void hvm_emulate_init_once(
struct hvm_emulate_ctxt *hvmemul_ctxt,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-20 12:55 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-20 12:55 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, rcojocaru, george.dunlap, andrew.cooper3,
paul.durrant, jbeulich, Alexandru Stefan ISAILA, roger.pau
This patch aims to have mem access vm events sent from the emulator.
This is useful in the case of emulated instructions that cause
page-walks on access protected pages.
We use hvmemul_map_linear_addr() ro intercept r/w access and
hvmemul_insn_fetch() to intercept exec access.
First we try to send a vm event and if the event is sent then emulation
returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
emulation goes on as expected.
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
Changes since V3:
- Calculate gpa in hvmemul_send_vm_event()
- Move hvmemul_linear_to_phys() call inside
hvmemul_send_vm_event()
- Check only if hvmemul_virtual_to_linear() returns X86EMUL_OKAY
- Add commnet for X86EMUL_ACCESS_EXCEPTION.
---
xen/arch/x86/hvm/emulate.c | 89 +++++++++++++++++++++++++-
xen/arch/x86/hvm/vm_event.c | 2 +-
xen/arch/x86/mm/mem_access.c | 3 +-
xen/arch/x86/x86_emulate/x86_emulate.h | 2 +
xen/include/asm-x86/hvm/emulate.h | 4 +-
5 files changed, 95 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 254ff6515d..75403ebc9b 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -15,6 +15,7 @@
#include <xen/paging.h>
#include <xen/trace.h>
#include <xen/vm_event.h>
+#include <xen/monitor.h>
#include <asm/event.h>
#include <asm/i387.h>
#include <asm/xstate.h>
@@ -26,6 +27,7 @@
#include <asm/hvm/support.h>
#include <asm/hvm/svm/svm.h>
#include <asm/vm_event.h>
+#include <asm/altp2m.h>
static void hvmtrace_io_assist(const ioreq_t *p)
{
@@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
return X86EMUL_OKAY;
}
+static bool hvmemul_send_vm_event(unsigned long gla,
+ uint32_t pfec, unsigned int bytes,
+ struct hvm_emulate_ctxt ctxt)
+{
+ xenmem_access_t access;
+ vm_event_request_t req = {};
+ gfn_t gfn;
+ paddr_t gpa;
+ unsigned long reps = 1;
+ int rc;
+
+ if ( !ctxt.send_event || !pfec )
+ return false;
+
+ rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
+
+ if ( rc != X86EMUL_OKAY )
+ return false;
+
+ gfn = gaddr_to_gfn(gpa);
+
+ if ( p2m_get_mem_access(current->domain, gfn, &access,
+ altp2m_vcpu_idx(current)) != 0 )
+ return false;
+
+ switch ( access ) {
+ case XENMEM_access_x:
+ case XENMEM_access_rx:
+ if ( pfec & PFEC_write_access )
+ req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
+ break;
+
+ case XENMEM_access_w:
+ case XENMEM_access_rw:
+ if ( pfec & PFEC_insn_fetch )
+ req.u.mem_access.flags = MEM_ACCESS_X;
+ break;
+
+ case XENMEM_access_r:
+ case XENMEM_access_n:
+ if ( pfec & PFEC_write_access )
+ req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
+ if ( pfec & PFEC_insn_fetch )
+ req.u.mem_access.flags |= MEM_ACCESS_X;
+ break;
+
+ default:
+ return false;
+ }
+
+ if ( !req.u.mem_access.flags )
+ return false; /* no violation */
+
+ req.reason = VM_EVENT_REASON_MEM_ACCESS;
+ req.u.mem_access.gfn = gfn_x(gfn);
+ req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA | MEM_ACCESS_GLA_VALID;
+ req.u.mem_access.gla = gla;
+ req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
+
+ return monitor_traps(current, true, &req) >= 0;
+}
+
/*
* Map the frame(s) covering an individual linear access, for writeable
* access. May return NULL for MMIO, or ERR_PTR(~X86EMUL_*) for other errors
@@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr(
unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
(linear >> PAGE_SHIFT) + 1;
unsigned int i;
+ gfn_t gfn;
/*
* mfn points to the next free slot. All used slots have a page reference
@@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr(
ASSERT(mfn_x(*mfn) == 0);
res = hvm_translate_get_page(curr, addr, true, pfec,
- &pfinfo, &page, NULL, &p2mt);
+ &pfinfo, &page, &gfn, &p2mt);
switch ( res )
{
@@ -704,6 +769,11 @@ static void *hvmemul_map_linear_addr(
if ( pfec & PFEC_write_access )
{
+ if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
+ {
+ err = ERR_PTR(~X86EMUL_ACCESS_EXCEPTION);
+ goto out;
+ }
if ( p2m_is_discard_write(p2mt) )
{
err = ERR_PTR(~X86EMUL_OKAY);
@@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
/* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
+ uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+ unsigned long addr, reps = 1;
+ int rc = 0;
+
+ rc = hvmemul_virtual_to_linear(
+ seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
+
+ if ( rc != X86EMUL_OKAY || !bytes )
+ return rc;
+
+ if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
+ pfec |= PFEC_user_mode;
+ if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
+ return X86EMUL_ACCESS_EXCEPTION;
/*
* Fall back if requested bytes are not in the prefetch cache.
* But always perform the (fake) read when bytes == 0.
@@ -2508,12 +2592,13 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
}
void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
- unsigned int errcode)
+ unsigned int errcode, bool send_event)
{
struct hvm_emulate_ctxt ctx = {{ 0 }};
int rc;
hvm_emulate_init_once(&ctx, NULL, guest_cpu_user_regs());
+ ctx.send_event = send_event;
switch ( kind )
{
diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
index 121de23071..6d203e8db5 100644
--- a/xen/arch/x86/hvm/vm_event.c
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -87,7 +87,7 @@ void hvm_vm_event_do_resume(struct vcpu *v)
kind = EMUL_KIND_SET_CONTEXT_INSN;
hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
- X86_EVENT_NO_EC);
+ X86_EVENT_NO_EC, false);
v->arch.vm_event->emulate_flags = 0;
}
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 0144f92b98..c9972bab8c 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -214,7 +214,8 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
d->arch.monitor.inguest_pagefault_disabled &&
npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
{
- hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, X86_EVENT_NO_EC);
+ hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
+ X86_EVENT_NO_EC, true);
return true;
}
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 08645762cc..8a20e733fa 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
#define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
/* (cmpxchg accessor): CMPXCHG failed. */
#define X86EMUL_CMPXCHG_FAILED 7
+/* Emulator tried to access a protected page. */
+#define X86EMUL_ACCESS_EXCEPTION 8
/* FPU sub-types which may be requested via ->get_fpu(). */
enum x86_emulate_fpu_type {
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index b39a1a0331..ed22ed0baf 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -47,6 +47,7 @@ struct hvm_emulate_ctxt {
uint32_t intr_shadow;
bool_t set_context;
+ bool send_event;
};
enum emul_kind {
@@ -63,7 +64,8 @@ int hvm_emulate_one(
struct hvm_emulate_ctxt *hvmemul_ctxt);
void hvm_emulate_one_vm_event(enum emul_kind kind,
unsigned int trapnr,
- unsigned int errcode);
+ unsigned int errcode,
+ bool send_event);
/* Must be called once to set up hvmemul state. */
void hvm_emulate_init_once(
struct hvm_emulate_ctxt *hvmemul_ctxt,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 9:56 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-22 9:56 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 20.05.19 at 14:55, <aisaila@bitdefender.com> wrote:
> This patch aims to have mem access vm events sent from the emulator.
> This is useful in the case of emulated instructions that cause
> page-walks on access protected pages.
>
> We use hvmemul_map_linear_addr() ro intercept r/w access and
> hvmemul_insn_fetch() to intercept exec access.
I'm afraid I don't understand this sentence. Or wait - is this a
simple typo, and you mean "to" instead of "ro"?
> First we try to send a vm event and if the event is sent then emulation
> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
> emulation goes on as expected.
Perhaps it's obvious for a vm-event person why successful sending
of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not
to me, despite having looked at prior versions. Can this (odd at the
first glance) behavior please be briefly explained here?
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -15,6 +15,7 @@
> #include <xen/paging.h>
> #include <xen/trace.h>
> #include <xen/vm_event.h>
> +#include <xen/monitor.h>
> #include <asm/event.h>
> #include <asm/i387.h>
> #include <asm/xstate.h>
> @@ -26,6 +27,7 @@
> #include <asm/hvm/support.h>
> #include <asm/hvm/svm/svm.h>
> #include <asm/vm_event.h>
> +#include <asm/altp2m.h>
In both cases please try to insert at least half way alphabetically
(I didn't check if the directives are fully sorted already), rather
than blindly at the end.
> @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
> return X86EMUL_OKAY;
> }
>
> +static bool hvmemul_send_vm_event(unsigned long gla,
> + uint32_t pfec, unsigned int bytes,
> + struct hvm_emulate_ctxt ctxt)
> +{
> + xenmem_access_t access;
> + vm_event_request_t req = {};
> + gfn_t gfn;
> + paddr_t gpa;
> + unsigned long reps = 1;
> + int rc;
> +
> + if ( !ctxt.send_event || !pfec )
Why the !pfec part of the condition?
> + return false;
> +
> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
As said before - I don't think it's a good idea to do the page walk
twice: This and the pre-existing one can easily return different
results.
Additionally, as also said before (I think), the function may raise
#PF, which you don't seem to deal with despite discarding the
X86EMUL_EXCEPTION return value ...
> + if ( rc != X86EMUL_OKAY )
> + return false;
... here.
> + gfn = gaddr_to_gfn(gpa);
> +
> + if ( p2m_get_mem_access(current->domain, gfn, &access,
> + altp2m_vcpu_idx(current)) != 0 )
> + return false;
> +
> + switch ( access ) {
> + case XENMEM_access_x:
> + case XENMEM_access_rx:
> + if ( pfec & PFEC_write_access )
> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
> + break;
> +
> + case XENMEM_access_w:
> + case XENMEM_access_rw:
> + if ( pfec & PFEC_insn_fetch )
> + req.u.mem_access.flags = MEM_ACCESS_X;
> + break;
> +
> + case XENMEM_access_r:
> + case XENMEM_access_n:
> + if ( pfec & PFEC_write_access )
> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
> + if ( pfec & PFEC_insn_fetch )
> + req.u.mem_access.flags |= MEM_ACCESS_X;
> + break;
> +
> + default:
> + return false;
> + }
Aren't you looking at the leaf page here, rather than at any of the
involved page tables? Or am I misunderstanding the description
saying "page-walks on access protected pages"?
> @@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr(
> unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
> (linear >> PAGE_SHIFT) + 1;
> unsigned int i;
> + gfn_t gfn;
>
> /*
> * mfn points to the next free slot. All used slots have a page reference
> @@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr(
> ASSERT(mfn_x(*mfn) == 0);
>
> res = hvm_translate_get_page(curr, addr, true, pfec,
> - &pfinfo, &page, NULL, &p2mt);
> + &pfinfo, &page, &gfn, &p2mt);
>
> switch ( res )
> {
Are these two hunks leftovers? You don't use "gfn" anywhere.
> @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
> uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
> + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
> + unsigned long addr, reps = 1;
> + int rc = 0;
> +
> + rc = hvmemul_virtual_to_linear(
> + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
> +
> + if ( rc != X86EMUL_OKAY || !bytes )
> + return rc;
> +
> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
> + pfec |= PFEC_user_mode;
>
> + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
> + return X86EMUL_ACCESS_EXCEPTION;
> /*
> * Fall back if requested bytes are not in the prefetch cache.
> * But always perform the (fake) read when bytes == 0.
Despite what was said before you're still doing things a 2nd time
here just because of hvmemul_send_vm_event()'s needs, even
if that function ends up bailing right away.
Also please don't lose the blank line ahead of the comment you
add code ahead of.
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
> #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
> /* (cmpxchg accessor): CMPXCHG failed. */
> #define X86EMUL_CMPXCHG_FAILED 7
> +/* Emulator tried to access a protected page. */
> +#define X86EMUL_ACCESS_EXCEPTION 8
This still doesn't make clear what the difference is to
X86EMUL_EXCEPTION.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 9:56 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-22 9:56 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 20.05.19 at 14:55, <aisaila@bitdefender.com> wrote:
> This patch aims to have mem access vm events sent from the emulator.
> This is useful in the case of emulated instructions that cause
> page-walks on access protected pages.
>
> We use hvmemul_map_linear_addr() ro intercept r/w access and
> hvmemul_insn_fetch() to intercept exec access.
I'm afraid I don't understand this sentence. Or wait - is this a
simple typo, and you mean "to" instead of "ro"?
> First we try to send a vm event and if the event is sent then emulation
> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
> emulation goes on as expected.
Perhaps it's obvious for a vm-event person why successful sending
of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not
to me, despite having looked at prior versions. Can this (odd at the
first glance) behavior please be briefly explained here?
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -15,6 +15,7 @@
> #include <xen/paging.h>
> #include <xen/trace.h>
> #include <xen/vm_event.h>
> +#include <xen/monitor.h>
> #include <asm/event.h>
> #include <asm/i387.h>
> #include <asm/xstate.h>
> @@ -26,6 +27,7 @@
> #include <asm/hvm/support.h>
> #include <asm/hvm/svm/svm.h>
> #include <asm/vm_event.h>
> +#include <asm/altp2m.h>
In both cases please try to insert at least half way alphabetically
(I didn't check if the directives are fully sorted already), rather
than blindly at the end.
> @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
> return X86EMUL_OKAY;
> }
>
> +static bool hvmemul_send_vm_event(unsigned long gla,
> + uint32_t pfec, unsigned int bytes,
> + struct hvm_emulate_ctxt ctxt)
> +{
> + xenmem_access_t access;
> + vm_event_request_t req = {};
> + gfn_t gfn;
> + paddr_t gpa;
> + unsigned long reps = 1;
> + int rc;
> +
> + if ( !ctxt.send_event || !pfec )
Why the !pfec part of the condition?
> + return false;
> +
> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
As said before - I don't think it's a good idea to do the page walk
twice: This and the pre-existing one can easily return different
results.
Additionally, as also said before (I think), the function may raise
#PF, which you don't seem to deal with despite discarding the
X86EMUL_EXCEPTION return value ...
> + if ( rc != X86EMUL_OKAY )
> + return false;
... here.
> + gfn = gaddr_to_gfn(gpa);
> +
> + if ( p2m_get_mem_access(current->domain, gfn, &access,
> + altp2m_vcpu_idx(current)) != 0 )
> + return false;
> +
> + switch ( access ) {
> + case XENMEM_access_x:
> + case XENMEM_access_rx:
> + if ( pfec & PFEC_write_access )
> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
> + break;
> +
> + case XENMEM_access_w:
> + case XENMEM_access_rw:
> + if ( pfec & PFEC_insn_fetch )
> + req.u.mem_access.flags = MEM_ACCESS_X;
> + break;
> +
> + case XENMEM_access_r:
> + case XENMEM_access_n:
> + if ( pfec & PFEC_write_access )
> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
> + if ( pfec & PFEC_insn_fetch )
> + req.u.mem_access.flags |= MEM_ACCESS_X;
> + break;
> +
> + default:
> + return false;
> + }
Aren't you looking at the leaf page here, rather than at any of the
involved page tables? Or am I misunderstanding the description
saying "page-walks on access protected pages"?
> @@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr(
> unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
> (linear >> PAGE_SHIFT) + 1;
> unsigned int i;
> + gfn_t gfn;
>
> /*
> * mfn points to the next free slot. All used slots have a page reference
> @@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr(
> ASSERT(mfn_x(*mfn) == 0);
>
> res = hvm_translate_get_page(curr, addr, true, pfec,
> - &pfinfo, &page, NULL, &p2mt);
> + &pfinfo, &page, &gfn, &p2mt);
>
> switch ( res )
> {
Are these two hunks leftovers? You don't use "gfn" anywhere.
> @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
> uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
> + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
> + unsigned long addr, reps = 1;
> + int rc = 0;
> +
> + rc = hvmemul_virtual_to_linear(
> + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
> +
> + if ( rc != X86EMUL_OKAY || !bytes )
> + return rc;
> +
> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
> + pfec |= PFEC_user_mode;
>
> + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
> + return X86EMUL_ACCESS_EXCEPTION;
> /*
> * Fall back if requested bytes are not in the prefetch cache.
> * But always perform the (fake) read when bytes == 0.
Despite what was said before you're still doing things a 2nd time
here just because of hvmemul_send_vm_event()'s needs, even
if that function ends up bailing right away.
Also please don't lose the blank line ahead of the comment you
add code ahead of.
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
> #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
> /* (cmpxchg accessor): CMPXCHG failed. */
> #define X86EMUL_CMPXCHG_FAILED 7
> +/* Emulator tried to access a protected page. */
> +#define X86EMUL_ACCESS_EXCEPTION 8
This still doesn't make clear what the difference is to
X86EMUL_EXCEPTION.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 12:59 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-22 12:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
On 22.05.2019 12:56, Jan Beulich wrote:
>>>> On 20.05.19 at 14:55, <aisaila@bitdefender.com> wrote:
>> This patch aims to have mem access vm events sent from the emulator.
>> This is useful in the case of emulated instructions that cause
>> page-walks on access protected pages.
>>
>> We use hvmemul_map_linear_addr() ro intercept r/w access and
>> hvmemul_insn_fetch() to intercept exec access.
>
> I'm afraid I don't understand this sentence. Or wait - is this a
> simple typo, and you mean "to" instead of "ro"?
Yes that is a typo it was meant to be a "to".
>
>> First we try to send a vm event and if the event is sent then emulation
>> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
>> emulation goes on as expected.
>
> Perhaps it's obvious for a vm-event person why successful sending
> of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not
> to me, despite having looked at prior versions. Can this (odd at the
> first glance) behavior please be briefly explained here?
If the event was successfully sent then the emulation has to stop and
return.
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -15,6 +15,7 @@
>> #include <xen/paging.h>
>> #include <xen/trace.h>
>> #include <xen/vm_event.h>
>> +#include <xen/monitor.h>
>> #include <asm/event.h>
>> #include <asm/i387.h>
>> #include <asm/xstate.h>
>> @@ -26,6 +27,7 @@
>> #include <asm/hvm/support.h>
>> #include <asm/hvm/svm/svm.h>
>> #include <asm/vm_event.h>
>> +#include <asm/altp2m.h>
>
> In both cases please try to insert at least half way alphabetically
> (I didn't check if the directives are fully sorted already), rather
> than blindly at the end.
Ok, I will correct that.
>
>> @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
>> return X86EMUL_OKAY;
>> }
>>
>> +static bool hvmemul_send_vm_event(unsigned long gla,
>> + uint32_t pfec, unsigned int bytes,
>> + struct hvm_emulate_ctxt ctxt)
>> +{
>> + xenmem_access_t access;
>> + vm_event_request_t req = {};
>> + gfn_t gfn;
>> + paddr_t gpa;
>> + unsigned long reps = 1;
>> + int rc;
>> +
>> + if ( !ctxt.send_event || !pfec )
>
> Why the !pfec part of the condition?
Because it is used to check the type of access violation and if it is 0
then we do not want to call get_mem_access or get the gpa, it is clear
that there will be no violation.
>
>> + return false;
>> +
>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>
> As said before - I don't think it's a good idea to do the page walk
> twice: This and the pre-existing one can easily return different
> results.
I do this just to get the gpa. If there is another way I will gladly use it.
>
> Additionally, as also said before (I think), the function may raise
> #PF, which you don't seem to deal with despite discarding the
> X86EMUL_EXCEPTION return value ... >
>> + if ( rc != X86EMUL_OKAY )
>> + return false;
>
> ... here.
>
>> + gfn = gaddr_to_gfn(gpa);
>> +
>> + if ( p2m_get_mem_access(current->domain, gfn, &access,
>> + altp2m_vcpu_idx(current)) != 0 )
>> + return false;
>> +
>> + switch ( access ) {
>> + case XENMEM_access_x:
>> + case XENMEM_access_rx:
>> + if ( pfec & PFEC_write_access )
>> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
>> + break;
>> +
>> + case XENMEM_access_w:
>> + case XENMEM_access_rw:
>> + if ( pfec & PFEC_insn_fetch )
>> + req.u.mem_access.flags = MEM_ACCESS_X;
>> + break;
>> +
>> + case XENMEM_access_r:
>> + case XENMEM_access_n:
>> + if ( pfec & PFEC_write_access )
>> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
>> + if ( pfec & PFEC_insn_fetch )
>> + req.u.mem_access.flags |= MEM_ACCESS_X;
>> + break;
>> +
>> + default:
>> + return false;
>> + }
>
> Aren't you looking at the leaf page here, rather than at any of the
> involved page tables? Or am I misunderstanding the description
> saying "page-walks on access protected pages"?
We want to ignore access write for the page tables and only fire a
vm_event for "regular" pages possibly hit by the actual instruction that
has also happened to trigger the A/D write(s). So we don't want to send
out vm_events for written-to page tables at all.
>
>> @@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr(
>> unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
>> (linear >> PAGE_SHIFT) + 1;
>> unsigned int i;
>> + gfn_t gfn;
>>
>> /*
>> * mfn points to the next free slot. All used slots have a page reference
>> @@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr(
>> ASSERT(mfn_x(*mfn) == 0);
>>
>> res = hvm_translate_get_page(curr, addr, true, pfec,
>> - &pfinfo, &page, NULL, &p2mt);
>> + &pfinfo, &page, &gfn, &p2mt);
>>
>> switch ( res )
>> {
>
> Are these two hunks leftovers? You don't use "gfn" anywhere.
Yes, there is no need for the gfn any more.
>
>> @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
>> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>> /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
>> uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
>> + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
>> + unsigned long addr, reps = 1;
>> + int rc = 0;
>> +
>> + rc = hvmemul_virtual_to_linear(
>> + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
>> +
>> + if ( rc != X86EMUL_OKAY || !bytes )
>> + return rc;
>> +
>> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
>> + pfec |= PFEC_user_mode;
>>
>> + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
>> + return X86EMUL_ACCESS_EXCEPTION;
>> /*
>> * Fall back if requested bytes are not in the prefetch cache.
>> * But always perform the (fake) read when bytes == 0.
>
> Despite what was said before you're still doing things a 2nd time
> here just because of hvmemul_send_vm_event()'s needs, even
> if that function ends up bailing right away.
I don't understand what things are done 2 times. Can you please explain?
>
> Also please don't lose the blank line ahead of the comment you
> add code ahead of.
>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
>> #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
>> /* (cmpxchg accessor): CMPXCHG failed. */
>> #define X86EMUL_CMPXCHG_FAILED 7
>> +/* Emulator tried to access a protected page. */
>> +#define X86EMUL_ACCESS_EXCEPTION 8
>
> This still doesn't make clear what the difference is to
> X86EMUL_EXCEPTION.
We need a return that has no side effects.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 12:59 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-22 12:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
On 22.05.2019 12:56, Jan Beulich wrote:
>>>> On 20.05.19 at 14:55, <aisaila@bitdefender.com> wrote:
>> This patch aims to have mem access vm events sent from the emulator.
>> This is useful in the case of emulated instructions that cause
>> page-walks on access protected pages.
>>
>> We use hvmemul_map_linear_addr() ro intercept r/w access and
>> hvmemul_insn_fetch() to intercept exec access.
>
> I'm afraid I don't understand this sentence. Or wait - is this a
> simple typo, and you mean "to" instead of "ro"?
Yes that is a typo it was meant to be a "to".
>
>> First we try to send a vm event and if the event is sent then emulation
>> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
>> emulation goes on as expected.
>
> Perhaps it's obvious for a vm-event person why successful sending
> of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not
> to me, despite having looked at prior versions. Can this (odd at the
> first glance) behavior please be briefly explained here?
If the event was successfully sent then the emulation has to stop and
return.
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -15,6 +15,7 @@
>> #include <xen/paging.h>
>> #include <xen/trace.h>
>> #include <xen/vm_event.h>
>> +#include <xen/monitor.h>
>> #include <asm/event.h>
>> #include <asm/i387.h>
>> #include <asm/xstate.h>
>> @@ -26,6 +27,7 @@
>> #include <asm/hvm/support.h>
>> #include <asm/hvm/svm/svm.h>
>> #include <asm/vm_event.h>
>> +#include <asm/altp2m.h>
>
> In both cases please try to insert at least half way alphabetically
> (I didn't check if the directives are fully sorted already), rather
> than blindly at the end.
Ok, I will correct that.
>
>> @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
>> return X86EMUL_OKAY;
>> }
>>
>> +static bool hvmemul_send_vm_event(unsigned long gla,
>> + uint32_t pfec, unsigned int bytes,
>> + struct hvm_emulate_ctxt ctxt)
>> +{
>> + xenmem_access_t access;
>> + vm_event_request_t req = {};
>> + gfn_t gfn;
>> + paddr_t gpa;
>> + unsigned long reps = 1;
>> + int rc;
>> +
>> + if ( !ctxt.send_event || !pfec )
>
> Why the !pfec part of the condition?
Because it is used to check the type of access violation and if it is 0
then we do not want to call get_mem_access or get the gpa, it is clear
that there will be no violation.
>
>> + return false;
>> +
>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>
> As said before - I don't think it's a good idea to do the page walk
> twice: This and the pre-existing one can easily return different
> results.
I do this just to get the gpa. If there is another way I will gladly use it.
>
> Additionally, as also said before (I think), the function may raise
> #PF, which you don't seem to deal with despite discarding the
> X86EMUL_EXCEPTION return value ... >
>> + if ( rc != X86EMUL_OKAY )
>> + return false;
>
> ... here.
>
>> + gfn = gaddr_to_gfn(gpa);
>> +
>> + if ( p2m_get_mem_access(current->domain, gfn, &access,
>> + altp2m_vcpu_idx(current)) != 0 )
>> + return false;
>> +
>> + switch ( access ) {
>> + case XENMEM_access_x:
>> + case XENMEM_access_rx:
>> + if ( pfec & PFEC_write_access )
>> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
>> + break;
>> +
>> + case XENMEM_access_w:
>> + case XENMEM_access_rw:
>> + if ( pfec & PFEC_insn_fetch )
>> + req.u.mem_access.flags = MEM_ACCESS_X;
>> + break;
>> +
>> + case XENMEM_access_r:
>> + case XENMEM_access_n:
>> + if ( pfec & PFEC_write_access )
>> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
>> + if ( pfec & PFEC_insn_fetch )
>> + req.u.mem_access.flags |= MEM_ACCESS_X;
>> + break;
>> +
>> + default:
>> + return false;
>> + }
>
> Aren't you looking at the leaf page here, rather than at any of the
> involved page tables? Or am I misunderstanding the description
> saying "page-walks on access protected pages"?
We want to ignore access write for the page tables and only fire a
vm_event for "regular" pages possibly hit by the actual instruction that
has also happened to trigger the A/D write(s). So we don't want to send
out vm_events for written-to page tables at all.
>
>> @@ -636,6 +700,7 @@ static void *hvmemul_map_linear_addr(
>> unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
>> (linear >> PAGE_SHIFT) + 1;
>> unsigned int i;
>> + gfn_t gfn;
>>
>> /*
>> * mfn points to the next free slot. All used slots have a page reference
>> @@ -674,7 +739,7 @@ static void *hvmemul_map_linear_addr(
>> ASSERT(mfn_x(*mfn) == 0);
>>
>> res = hvm_translate_get_page(curr, addr, true, pfec,
>> - &pfinfo, &page, NULL, &p2mt);
>> + &pfinfo, &page, &gfn, &p2mt);
>>
>> switch ( res )
>> {
>
> Are these two hunks leftovers? You don't use "gfn" anywhere.
Yes, there is no need for the gfn any more.
>
>> @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
>> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>> /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
>> uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
>> + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
>> + unsigned long addr, reps = 1;
>> + int rc = 0;
>> +
>> + rc = hvmemul_virtual_to_linear(
>> + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
>> +
>> + if ( rc != X86EMUL_OKAY || !bytes )
>> + return rc;
>> +
>> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
>> + pfec |= PFEC_user_mode;
>>
>> + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
>> + return X86EMUL_ACCESS_EXCEPTION;
>> /*
>> * Fall back if requested bytes are not in the prefetch cache.
>> * But always perform the (fake) read when bytes == 0.
>
> Despite what was said before you're still doing things a 2nd time
> here just because of hvmemul_send_vm_event()'s needs, even
> if that function ends up bailing right away.
I don't understand what things are done 2 times. Can you please explain?
>
> Also please don't lose the blank line ahead of the comment you
> add code ahead of.
>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
>> #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
>> /* (cmpxchg accessor): CMPXCHG failed. */
>> #define X86EMUL_CMPXCHG_FAILED 7
>> +/* Emulator tried to access a protected page. */
>> +#define X86EMUL_ACCESS_EXCEPTION 8
>
> This still doesn't make clear what the difference is to
> X86EMUL_EXCEPTION.
We need a return that has no side effects.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
@ 2019-05-22 13:13 ` Paul Durrant
0 siblings, 0 replies; 22+ messages in thread
From: Paul Durrant @ 2019-05-22 13:13 UTC (permalink / raw)
To: 'Alexandru Stefan ISAILA', xen-devel
Cc: tamas, Wei Liu, rcojocaru, Andrew Cooper, George Dunlap,
jbeulich, Roger Pau Monne
> -----Original Message-----
> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com]
> Sent: 20 May 2019 13:55
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> rcojocaru@bitdefender.com; tamas@tklengyel.com; George Dunlap <George.Dunlap@citrix.com>; Alexandru
> Stefan ISAILA <aisaila@bitdefender.com>
> Subject: [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
>
> Thiis is done so hvmemul_linear_to_phys() can be called from
> hvmemul_send_vm_event().
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
> xen/arch/x86/hvm/emulate.c | 181 ++++++++++++++++++-------------------
> 1 file changed, 90 insertions(+), 91 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 8659c89862..254ff6515d 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -530,6 +530,95 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
> return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
> }
>
I know this is code movement, but it would probably good to a do a bit of tidying...
> +/*
> + * Convert addr from linear to physical form, valid over the range
> + * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
> + * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
> + * @pfec indicates the access checks to be performed during page-table walks.
> + */
> +static int hvmemul_linear_to_phys(
> + unsigned long addr,
> + paddr_t *paddr,
> + unsigned int bytes_per_rep,
> + unsigned long *reps,
> + uint32_t pfec,
> + struct hvm_emulate_ctxt *hvmemul_ctxt)
This can all be re-flowed since arg-per-line is not really canonical style.
> +{
> + struct vcpu *curr = current;
> + unsigned long pfn, npfn, done, todo, i, offset = addr & ~PAGE_MASK;
> + int reverse;
Looks like this should be bool.
> +
> + /*
> + * Clip repetitions to a sensible maximum. This avoids extensive looping in
> + * this function while still amortising the cost of I/O trap-and-emulate.
> + */
> + *reps = min_t(unsigned long, *reps, 4096);
> +
> + /* With no paging it's easy: linear == physical. */
> + if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PG) )
> + {
> + *paddr = addr;
> + return X86EMUL_OKAY;
> + }
> +
> + /* Reverse mode if this is a backwards multi-iteration string operation. */
> + reverse = (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1);
> +
> + if ( reverse && ((PAGE_SIZE - offset) < bytes_per_rep) )
> + {
> + /* Do page-straddling first iteration forwards via recursion. */
> + paddr_t _paddr;
> + unsigned long one_rep = 1;
> + int rc = hvmemul_linear_to_phys(
> + addr, &_paddr, bytes_per_rep, &one_rep, pfec, hvmemul_ctxt);
Blank line here.
> + if ( rc != X86EMUL_OKAY )
> + return rc;
> + pfn = _paddr >> PAGE_SHIFT;
paddr_to_pfn()
> + }
> + else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == gfn_x(INVALID_GFN) )
> + {
> + if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
> + return X86EMUL_RETRY;
> + *reps = 0;
> + x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt);
> + return X86EMUL_EXCEPTION;
> + }
> +
> + done = reverse ? bytes_per_rep + offset : PAGE_SIZE - offset;
> + todo = *reps * bytes_per_rep;
> + for ( i = 1; done < todo; i++ )
> + {
> + /* Get the next PFN in the range. */
> + addr += reverse ? -PAGE_SIZE : PAGE_SIZE;
> + npfn = paging_gva_to_gfn(curr, addr, &pfec);
> +
> + /* Is it contiguous with the preceding PFNs? If not then we're done. */
> + if ( (npfn == gfn_x(INVALID_GFN)) ||
> + (npfn != (pfn + (reverse ? -i : i))) )
> + {
> + if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
> + return X86EMUL_RETRY;
> + done /= bytes_per_rep;
> + if ( done == 0 )
> + {
> + ASSERT(!reverse);
> + if ( npfn != gfn_x(INVALID_GFN) )
> + return X86EMUL_UNHANDLEABLE;
> + *reps = 0;
> + x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
> + return X86EMUL_EXCEPTION;
> + }
> + *reps = done;
> + break;
> + }
> +
> + done += PAGE_SIZE;
> + }
> +
> + *paddr = ((paddr_t)pfn << PAGE_SHIFT) | offset;
pfn_to_paddr() and a blank line before the return.
> + return X86EMUL_OKAY;
> +}
> +
Thanks,
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
@ 2019-05-22 13:13 ` Paul Durrant
0 siblings, 0 replies; 22+ messages in thread
From: Paul Durrant @ 2019-05-22 13:13 UTC (permalink / raw)
To: 'Alexandru Stefan ISAILA', xen-devel
Cc: tamas, Wei Liu, rcojocaru, Andrew Cooper, George Dunlap,
jbeulich, Roger Pau Monne
> -----Original Message-----
> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com]
> Sent: 20 May 2019 13:55
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> rcojocaru@bitdefender.com; tamas@tklengyel.com; George Dunlap <George.Dunlap@citrix.com>; Alexandru
> Stefan ISAILA <aisaila@bitdefender.com>
> Subject: [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
>
> Thiis is done so hvmemul_linear_to_phys() can be called from
> hvmemul_send_vm_event().
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
> xen/arch/x86/hvm/emulate.c | 181 ++++++++++++++++++-------------------
> 1 file changed, 90 insertions(+), 91 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 8659c89862..254ff6515d 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -530,6 +530,95 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
> return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
> }
>
I know this is code movement, but it would probably good to a do a bit of tidying...
> +/*
> + * Convert addr from linear to physical form, valid over the range
> + * [addr, addr + *reps * bytes_per_rep]. *reps is adjusted according to
> + * the valid computed range. It is always >0 when X86EMUL_OKAY is returned.
> + * @pfec indicates the access checks to be performed during page-table walks.
> + */
> +static int hvmemul_linear_to_phys(
> + unsigned long addr,
> + paddr_t *paddr,
> + unsigned int bytes_per_rep,
> + unsigned long *reps,
> + uint32_t pfec,
> + struct hvm_emulate_ctxt *hvmemul_ctxt)
This can all be re-flowed since arg-per-line is not really canonical style.
> +{
> + struct vcpu *curr = current;
> + unsigned long pfn, npfn, done, todo, i, offset = addr & ~PAGE_MASK;
> + int reverse;
Looks like this should be bool.
> +
> + /*
> + * Clip repetitions to a sensible maximum. This avoids extensive looping in
> + * this function while still amortising the cost of I/O trap-and-emulate.
> + */
> + *reps = min_t(unsigned long, *reps, 4096);
> +
> + /* With no paging it's easy: linear == physical. */
> + if ( !(curr->arch.hvm.guest_cr[0] & X86_CR0_PG) )
> + {
> + *paddr = addr;
> + return X86EMUL_OKAY;
> + }
> +
> + /* Reverse mode if this is a backwards multi-iteration string operation. */
> + reverse = (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1);
> +
> + if ( reverse && ((PAGE_SIZE - offset) < bytes_per_rep) )
> + {
> + /* Do page-straddling first iteration forwards via recursion. */
> + paddr_t _paddr;
> + unsigned long one_rep = 1;
> + int rc = hvmemul_linear_to_phys(
> + addr, &_paddr, bytes_per_rep, &one_rep, pfec, hvmemul_ctxt);
Blank line here.
> + if ( rc != X86EMUL_OKAY )
> + return rc;
> + pfn = _paddr >> PAGE_SHIFT;
paddr_to_pfn()
> + }
> + else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == gfn_x(INVALID_GFN) )
> + {
> + if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
> + return X86EMUL_RETRY;
> + *reps = 0;
> + x86_emul_pagefault(pfec, addr, &hvmemul_ctxt->ctxt);
> + return X86EMUL_EXCEPTION;
> + }
> +
> + done = reverse ? bytes_per_rep + offset : PAGE_SIZE - offset;
> + todo = *reps * bytes_per_rep;
> + for ( i = 1; done < todo; i++ )
> + {
> + /* Get the next PFN in the range. */
> + addr += reverse ? -PAGE_SIZE : PAGE_SIZE;
> + npfn = paging_gva_to_gfn(curr, addr, &pfec);
> +
> + /* Is it contiguous with the preceding PFNs? If not then we're done. */
> + if ( (npfn == gfn_x(INVALID_GFN)) ||
> + (npfn != (pfn + (reverse ? -i : i))) )
> + {
> + if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
> + return X86EMUL_RETRY;
> + done /= bytes_per_rep;
> + if ( done == 0 )
> + {
> + ASSERT(!reverse);
> + if ( npfn != gfn_x(INVALID_GFN) )
> + return X86EMUL_UNHANDLEABLE;
> + *reps = 0;
> + x86_emul_pagefault(pfec, addr & PAGE_MASK, &hvmemul_ctxt->ctxt);
> + return X86EMUL_EXCEPTION;
> + }
> + *reps = done;
> + break;
> + }
> +
> + done += PAGE_SIZE;
> + }
> +
> + *paddr = ((paddr_t)pfn << PAGE_SHIFT) | offset;
pfn_to_paddr() and a blank line before the return.
> + return X86EMUL_OKAY;
> +}
> +
Thanks,
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 13:34 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-22 13:34 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 22.05.19 at 14:59, <aisaila@bitdefender.com> wrote:
> On 22.05.2019 12:56, Jan Beulich wrote:
>>>>> On 20.05.19 at 14:55, <aisaila@bitdefender.com> wrote:
>>> First we try to send a vm event and if the event is sent then emulation
>>> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
>>> emulation goes on as expected.
>>
>> Perhaps it's obvious for a vm-event person why successful sending
>> of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not
>> to me, despite having looked at prior versions. Can this (odd at the
>> first glance) behavior please be briefly explained here?
>
> If the event was successfully sent then the emulation has to stop and
> return.
Which is where we commonly use X86EMUL_RETRY. I've explained to
you before that introduction of new return values needs careful
inspection that it'll work for all involved pieces of code (in particular
ones specially treating some of the values).
>>> @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
>>> return X86EMUL_OKAY;
>>> }
>>>
>>> +static bool hvmemul_send_vm_event(unsigned long gla,
>>> + uint32_t pfec, unsigned int bytes,
>>> + struct hvm_emulate_ctxt ctxt)
>>> +{
>>> + xenmem_access_t access;
>>> + vm_event_request_t req = {};
>>> + gfn_t gfn;
>>> + paddr_t gpa;
>>> + unsigned long reps = 1;
>>> + int rc;
>>> +
>>> + if ( !ctxt.send_event || !pfec )
>>
>> Why the !pfec part of the condition?
>
> Because it is used to check the type of access violation and if it is 0
> then we do not want to call get_mem_access or get the gpa, it is clear
> that there will be no violation.
So what about, as an example, the case of just PFEC_implicit set?
And do you really care about accesses with PFEC_page_present
clear?
>>> + return false;
>>> +
>>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>>
>> As said before - I don't think it's a good idea to do the page walk
>> twice: This and the pre-existing one can easily return different
>> results.
>
> I do this just to get the gpa. If there is another way I will gladly use it.
To get the gpa you need to do a page walk. But you shouldn't do
two page walks. Which as a result tells me that the question is
not about "another way", but that things need to be structured
differently.
>>> + switch ( access ) {
Btw, I'm noticing this style issue only now.
>>> + case XENMEM_access_x:
>>> + case XENMEM_access_rx:
>>> + if ( pfec & PFEC_write_access )
>>> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
>>> + break;
>>> +
>>> + case XENMEM_access_w:
>>> + case XENMEM_access_rw:
>>> + if ( pfec & PFEC_insn_fetch )
>>> + req.u.mem_access.flags = MEM_ACCESS_X;
>>> + break;
>>> +
>>> + case XENMEM_access_r:
>>> + case XENMEM_access_n:
>>> + if ( pfec & PFEC_write_access )
>>> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
>>> + if ( pfec & PFEC_insn_fetch )
>>> + req.u.mem_access.flags |= MEM_ACCESS_X;
>>> + break;
>>> +
>>> + default:
>>> + return false;
>>> + }
>>
>> Aren't you looking at the leaf page here, rather than at any of the
>> involved page tables? Or am I misunderstanding the description
>> saying "page-walks on access protected pages"?
>
> We want to ignore access write for the page tables and only fire a
> vm_event for "regular" pages possibly hit by the actual instruction that
> has also happened to trigger the A/D write(s). So we don't want to send
> out vm_events for written-to page tables at all.
In which case may I ask for the description to be worded to make
this unambiguous?
>>> @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
>>> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>> /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
>>> uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
>>> + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
>>> + unsigned long addr, reps = 1;
>>> + int rc = 0;
>>> +
>>> + rc = hvmemul_virtual_to_linear(
>>> + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
>>> +
>>> + if ( rc != X86EMUL_OKAY || !bytes )
>>> + return rc;
>>> +
>>> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
>>> + pfec |= PFEC_user_mode;
>>>
>>> + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
>>> + return X86EMUL_ACCESS_EXCEPTION;
>>> /*
>>> * Fall back if requested bytes are not in the prefetch cache.
>>> * But always perform the (fake) read when bytes == 0.
>>
>> Despite what was said before you're still doing things a 2nd time
>> here just because of hvmemul_send_vm_event()'s needs, even
>> if that function ends up bailing right away.
>
> I don't understand what things are done 2 times. Can you please explain?
You add code above that exists already in __hvmemul_read().
Even worse, __hvmemul_read() may not need calling at all, in
which case there's no (emulated) memory access and hence
imo there shouldn't be any event. Plus, just like in the
hvmemul_linear_to_phys() case there may be an exception
raised by the function, yet just like there you also discard the
return value saying so without also zapping the exception.
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>>> @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
>>> #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
>>> /* (cmpxchg accessor): CMPXCHG failed. */
>>> #define X86EMUL_CMPXCHG_FAILED 7
>>> +/* Emulator tried to access a protected page. */
>>> +#define X86EMUL_ACCESS_EXCEPTION 8
>>
>> This still doesn't make clear what the difference is to
>> X86EMUL_EXCEPTION.
>
> We need a return that has no side effects.
So besides saying so you also need to make sure there actually
are no side effects.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 13:34 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-22 13:34 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 22.05.19 at 14:59, <aisaila@bitdefender.com> wrote:
> On 22.05.2019 12:56, Jan Beulich wrote:
>>>>> On 20.05.19 at 14:55, <aisaila@bitdefender.com> wrote:
>>> First we try to send a vm event and if the event is sent then emulation
>>> returns X86EMUL_ACCESS_EXCEPTION. If the event is not sent then the
>>> emulation goes on as expected.
>>
>> Perhaps it's obvious for a vm-event person why successful sending
>> of an event is to result in X86EMUL_ACCESS_EXCEPTION, but it's not
>> to me, despite having looked at prior versions. Can this (odd at the
>> first glance) behavior please be briefly explained here?
>
> If the event was successfully sent then the emulation has to stop and
> return.
Which is where we commonly use X86EMUL_RETRY. I've explained to
you before that introduction of new return values needs careful
inspection that it'll work for all involved pieces of code (in particular
ones specially treating some of the values).
>>> @@ -619,6 +621,68 @@ static int hvmemul_linear_to_phys(
>>> return X86EMUL_OKAY;
>>> }
>>>
>>> +static bool hvmemul_send_vm_event(unsigned long gla,
>>> + uint32_t pfec, unsigned int bytes,
>>> + struct hvm_emulate_ctxt ctxt)
>>> +{
>>> + xenmem_access_t access;
>>> + vm_event_request_t req = {};
>>> + gfn_t gfn;
>>> + paddr_t gpa;
>>> + unsigned long reps = 1;
>>> + int rc;
>>> +
>>> + if ( !ctxt.send_event || !pfec )
>>
>> Why the !pfec part of the condition?
>
> Because it is used to check the type of access violation and if it is 0
> then we do not want to call get_mem_access or get the gpa, it is clear
> that there will be no violation.
So what about, as an example, the case of just PFEC_implicit set?
And do you really care about accesses with PFEC_page_present
clear?
>>> + return false;
>>> +
>>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>>
>> As said before - I don't think it's a good idea to do the page walk
>> twice: This and the pre-existing one can easily return different
>> results.
>
> I do this just to get the gpa. If there is another way I will gladly use it.
To get the gpa you need to do a page walk. But you shouldn't do
two page walks. Which as a result tells me that the question is
not about "another way", but that things need to be structured
differently.
>>> + switch ( access ) {
Btw, I'm noticing this style issue only now.
>>> + case XENMEM_access_x:
>>> + case XENMEM_access_rx:
>>> + if ( pfec & PFEC_write_access )
>>> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
>>> + break;
>>> +
>>> + case XENMEM_access_w:
>>> + case XENMEM_access_rw:
>>> + if ( pfec & PFEC_insn_fetch )
>>> + req.u.mem_access.flags = MEM_ACCESS_X;
>>> + break;
>>> +
>>> + case XENMEM_access_r:
>>> + case XENMEM_access_n:
>>> + if ( pfec & PFEC_write_access )
>>> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
>>> + if ( pfec & PFEC_insn_fetch )
>>> + req.u.mem_access.flags |= MEM_ACCESS_X;
>>> + break;
>>> +
>>> + default:
>>> + return false;
>>> + }
>>
>> Aren't you looking at the leaf page here, rather than at any of the
>> involved page tables? Or am I misunderstanding the description
>> saying "page-walks on access protected pages"?
>
> We want to ignore access write for the page tables and only fire a
> vm_event for "regular" pages possibly hit by the actual instruction that
> has also happened to trigger the A/D write(s). So we don't want to send
> out vm_events for written-to page tables at all.
In which case may I ask for the description to be worded to make
this unambiguous?
>>> @@ -1248,7 +1318,21 @@ int hvmemul_insn_fetch(
>>> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>> /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */
>>> uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip;
>>> + uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
>>> + unsigned long addr, reps = 1;
>>> + int rc = 0;
>>> +
>>> + rc = hvmemul_virtual_to_linear(
>>> + seg, offset, bytes, &reps, hvm_access_insn_fetch, hvmemul_ctxt, &addr);
>>> +
>>> + if ( rc != X86EMUL_OKAY || !bytes )
>>> + return rc;
>>> +
>>> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
>>> + pfec |= PFEC_user_mode;
>>>
>>> + if ( hvmemul_send_vm_event(addr, pfec, bytes, *hvmemul_ctxt) )
>>> + return X86EMUL_ACCESS_EXCEPTION;
>>> /*
>>> * Fall back if requested bytes are not in the prefetch cache.
>>> * But always perform the (fake) read when bytes == 0.
>>
>> Despite what was said before you're still doing things a 2nd time
>> here just because of hvmemul_send_vm_event()'s needs, even
>> if that function ends up bailing right away.
>
> I don't understand what things are done 2 times. Can you please explain?
You add code above that exists already in __hvmemul_read().
Even worse, __hvmemul_read() may not need calling at all, in
which case there's no (emulated) memory access and hence
imo there shouldn't be any event. Plus, just like in the
hvmemul_linear_to_phys() case there may be an exception
raised by the function, yet just like there you also discard the
return value saying so without also zapping the exception.
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>>> @@ -162,6 +162,8 @@ struct x86_emul_fpu_aux {
>>> #define X86EMUL_UNRECOGNIZED X86EMUL_UNIMPLEMENTED
>>> /* (cmpxchg accessor): CMPXCHG failed. */
>>> #define X86EMUL_CMPXCHG_FAILED 7
>>> +/* Emulator tried to access a protected page. */
>>> +#define X86EMUL_ACCESS_EXCEPTION 8
>>
>> This still doesn't make clear what the difference is to
>> X86EMUL_EXCEPTION.
>
> We need a return that has no side effects.
So besides saying so you also need to make sure there actually
are no side effects.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 13:50 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-22 13:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> Despite what was said before you're still doing things a 2nd time
>>> here just because of hvmemul_send_vm_event()'s needs, even
>>> if that function ends up bailing right away.
>>
>> I don't understand what things are done 2 times. Can you please explain?
>
> You add code above that exists already in __hvmemul_read().
> Even worse, __hvmemul_read() may not need calling at all, in
> which case there's no (emulated) memory access and hence
> imo there shouldn't be any event. Plus, just like in the
> hvmemul_linear_to_phys() case there may be an exception
> raised by the function, yet just like there you also discard the
> return value saying so without also zapping the exception.
>
Isn't it safer to move the hvmemul_send_vm_event() form
hvmemul_insn_fetch() to __hvmemul_read()?
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 13:50 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-22 13:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> Despite what was said before you're still doing things a 2nd time
>>> here just because of hvmemul_send_vm_event()'s needs, even
>>> if that function ends up bailing right away.
>>
>> I don't understand what things are done 2 times. Can you please explain?
>
> You add code above that exists already in __hvmemul_read().
> Even worse, __hvmemul_read() may not need calling at all, in
> which case there's no (emulated) memory access and hence
> imo there shouldn't be any event. Plus, just like in the
> hvmemul_linear_to_phys() case there may be an exception
> raised by the function, yet just like there you also discard the
> return value saying so without also zapping the exception.
>
Isn't it safer to move the hvmemul_send_vm_event() form
hvmemul_insn_fetch() to __hvmemul_read()?
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
@ 2019-05-22 13:55 ` George Dunlap
0 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2019-05-22 13:55 UTC (permalink / raw)
To: Paul Durrant, 'Alexandru Stefan ISAILA', xen-devel
Cc: tamas, Wei Liu, rcojocaru, Andrew Cooper, jbeulich, Roger Pau Monne
On 5/22/19 2:13 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com]
>> Sent: 20 May 2019 13:55
>> To: xen-devel@lists.xenproject.org
>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>> rcojocaru@bitdefender.com; tamas@tklengyel.com; George Dunlap <George.Dunlap@citrix.com>; Alexandru
>> Stefan ISAILA <aisaila@bitdefender.com>
>> Subject: [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
>>
>> Thiis is done so hvmemul_linear_to_phys() can be called from
>> hvmemul_send_vm_event().
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> ---
>> xen/arch/x86/hvm/emulate.c | 181 ++++++++++++++++++-------------------
>> 1 file changed, 90 insertions(+), 91 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index 8659c89862..254ff6515d 100644
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -530,6 +530,95 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
>> return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
>> }
>>
>
> I know this is code movement, but it would probably good to a do a bit of tidying...
I think there are different minds on this; I *generally* prefer pure
code movement to be with as few changes as possible, to make sure actual
changes are easy to call out.
The changes you've asked for are pretty minor (and you're the maintainer
of the file it's being moved to), so I won't argue about it in this
particular case. Just want to counter the idea that move + change is
the norm. :-)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
@ 2019-05-22 13:55 ` George Dunlap
0 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2019-05-22 13:55 UTC (permalink / raw)
To: Paul Durrant, 'Alexandru Stefan ISAILA', xen-devel
Cc: tamas, Wei Liu, rcojocaru, Andrew Cooper, jbeulich, Roger Pau Monne
On 5/22/19 2:13 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com]
>> Sent: 20 May 2019 13:55
>> To: xen-devel@lists.xenproject.org
>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
>> rcojocaru@bitdefender.com; tamas@tklengyel.com; George Dunlap <George.Dunlap@citrix.com>; Alexandru
>> Stefan ISAILA <aisaila@bitdefender.com>
>> Subject: [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys
>>
>> Thiis is done so hvmemul_linear_to_phys() can be called from
>> hvmemul_send_vm_event().
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> ---
>> xen/arch/x86/hvm/emulate.c | 181 ++++++++++++++++++-------------------
>> 1 file changed, 90 insertions(+), 91 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index 8659c89862..254ff6515d 100644
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -530,6 +530,95 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
>> return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
>> }
>>
>
> I know this is code movement, but it would probably good to a do a bit of tidying...
I think there are different minds on this; I *generally* prefer pure
code movement to be with as few changes as possible, to make sure actual
changes are easy to call out.
The changes you've asked for are pretty minor (and you're the maintainer
of the file it's being moved to), so I won't argue about it in this
particular case. Just want to counter the idea that move + change is
the norm. :-)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 13:57 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-22 13:57 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 22.05.19 at 15:50, <aisaila@bitdefender.com> wrote:
> Isn't it safer to move the hvmemul_send_vm_event() form
> hvmemul_insn_fetch() to __hvmemul_read()?
Possibly - I can't tell whether that'll fit all your needs. I also
don't recall whether this was proposed before and there
were reasons speaking against doing so.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-22 13:57 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-22 13:57 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 22.05.19 at 15:50, <aisaila@bitdefender.com> wrote:
> Isn't it safer to move the hvmemul_send_vm_event() form
> hvmemul_insn_fetch() to __hvmemul_read()?
Possibly - I can't tell whether that'll fit all your needs. I also
don't recall whether this was proposed before and there
were reasons speaking against doing so.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-30 8:59 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-30 8:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>
>> + return false;
>> +
>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>
> As said before - I don't think it's a good idea to do the page walk
> twice: This and the pre-existing one can easily return different
> results.
What preexisting page walk are you talking about here? I don't think
there is a way to get the gpa by passing it from somewhere.
Alex
>
> Additionally, as also said before (I think), the function may raise
> #PF, which you don't seem to deal with despite discarding the
> X86EMUL_EXCEPTION return value ...
>
>> + if ( rc != X86EMUL_OKAY )
>> + return false;
>
> ... here.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-30 8:59 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-05-30 8:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>
>> + return false;
>> +
>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>
> As said before - I don't think it's a good idea to do the page walk
> twice: This and the pre-existing one can easily return different
> results.
What preexisting page walk are you talking about here? I don't think
there is a way to get the gpa by passing it from somewhere.
Alex
>
> Additionally, as also said before (I think), the function may raise
> #PF, which you don't seem to deal with despite discarding the
> X86EMUL_EXCEPTION return value ...
>
>> + if ( rc != X86EMUL_OKAY )
>> + return false;
>
> ... here.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-31 9:16 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-31 9:16 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 30.05.19 at 10:59, <aisaila@bitdefender.com> wrote:
>>
>>> + return false;
>>> +
>>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>>
>> As said before - I don't think it's a good idea to do the page walk
>> twice: This and the pre-existing one can easily return different
>> results.
>
> What preexisting page walk are you talking about here?
Well, I'm afraid I don't know what to say (in a polite way). I'm sure
you're understanding the code you try to add to, so it would seem
natural to me that you can answer this question all by yourself:
Since you don't remove any linear->phys translations in your patch,
and since there necessarily is one prior to your patch, you adding
(another) one means there are now two of them.
So to answer your question directly: hvmemul_map_linear_addr()
calls hvm_translate_get_page(), and hvmemul_insn_fetch() ->
__hvmemul_read() -> linear_read() -> hvm_copy_from_guest_linear()
-> __hvm_copy() calls hvm_translate_get_page() as well.
As an aside - while I'm advocating the stripping of reply quoting that's
not needed as context, you surely went too far here: You've left in
place neither an indication when the mails were sent context of which
is still present above, nor enough context to re-construct what
function your additions go into. IOW I had to search the earlier parts
of this thread to gather enough context to actually be able to reply.
> I don't think
> there is a way to get the gpa by passing it from somewhere.
Possibly, but that's also not what I did suggest. Instead what I
think you need to look into is how to restructure things, perhaps
just as to the place where you insert your new code.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH v4 2/2] x86/emulate: Send vm_event from emulate
@ 2019-05-31 9:16 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2019-05-31 9:16 UTC (permalink / raw)
To: aisaila
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Paul Durrant, xen-devel, Roger Pau Monne
>>> On 30.05.19 at 10:59, <aisaila@bitdefender.com> wrote:
>>
>>> + return false;
>>> +
>>> + rc = hvmemul_linear_to_phys(gla, &gpa, bytes, &reps, pfec, &ctxt);
>>
>> As said before - I don't think it's a good idea to do the page walk
>> twice: This and the pre-existing one can easily return different
>> results.
>
> What preexisting page walk are you talking about here?
Well, I'm afraid I don't know what to say (in a polite way). I'm sure
you're understanding the code you try to add to, so it would seem
natural to me that you can answer this question all by yourself:
Since you don't remove any linear->phys translations in your patch,
and since there necessarily is one prior to your patch, you adding
(another) one means there are now two of them.
So to answer your question directly: hvmemul_map_linear_addr()
calls hvm_translate_get_page(), and hvmemul_insn_fetch() ->
__hvmemul_read() -> linear_read() -> hvm_copy_from_guest_linear()
-> __hvm_copy() calls hvm_translate_get_page() as well.
As an aside - while I'm advocating the stripping of reply quoting that's
not needed as context, you surely went too far here: You've left in
place neither an indication when the mails were sent context of which
is still present above, nor enough context to re-construct what
function your additions go into. IOW I had to search the earlier parts
of this thread to gather enough context to actually be able to reply.
> I don't think
> there is a way to get the gpa by passing it from somewhere.
Possibly, but that's also not what I did suggest. Instead what I
think you need to look into is how to restructure things, perhaps
just as to the place where you insert your new code.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-05-31 9:16 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-20 12:55 [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys Alexandru Stefan ISAILA
2019-05-20 12:55 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-05-20 12:55 ` [PATCH v4 2/2] x86/emulate: Send vm_event from emulate Alexandru Stefan ISAILA
2019-05-20 12:55 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-05-22 9:56 ` Jan Beulich
2019-05-22 9:56 ` [Xen-devel] " Jan Beulich
2019-05-22 12:59 ` Alexandru Stefan ISAILA
2019-05-22 12:59 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-05-22 13:34 ` Jan Beulich
2019-05-22 13:34 ` [Xen-devel] " Jan Beulich
2019-05-22 13:50 ` Alexandru Stefan ISAILA
2019-05-22 13:50 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-05-22 13:57 ` Jan Beulich
2019-05-22 13:57 ` [Xen-devel] " Jan Beulich
2019-05-30 8:59 ` Alexandru Stefan ISAILA
2019-05-30 8:59 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-05-31 9:16 ` Jan Beulich
2019-05-31 9:16 ` [Xen-devel] " Jan Beulich
2019-05-22 13:13 ` [PATCH v4 1/2] x86/emulate: Move hvmemul_linear_to_phys Paul Durrant
2019-05-22 13:13 ` [Xen-devel] " Paul Durrant
2019-05-22 13:55 ` George Dunlap
2019-05-22 13:55 ` [Xen-devel] " George Dunlap
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.