|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/2] x86/vm_event: Allow returning i-cache for emulation
On Fri, Sep 16, 2016 at 1:21 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 09/15/16 20:08, Razvan Cojocaru wrote:
>> On 09/15/16 19:36, Tamas K Lengyel wrote:
>>> On Wed, Sep 14, 2016 at 1:58 AM, Razvan Cojocaru
>>> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 09/13/2016 09:12 PM, Tamas K Lengyel wrote:
>>>>> When emulating instructions the emulator maintains a small i-cache fetched
>>>>> from the guest memory. This patch extends the vm_event interface to allow
>>>>> returning this i-cache via the vm_event response instead.
>>>>>
>>>>> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor
>>>>> subscriber
>>>>> normally has to remove the INT3 from memory - singlestep - place back INT3
>>>>> to allow the guest to continue execution. This routine however is
>>>>> susceptible
>>>>> to a race-condition on multi-vCPU guests. By allowing the subscriber to
>>>>> return
>>>>> the i-cache to be used for emulation it can side-step the problem by
>>>>> returning
>>>>> a clean buffer without the INT3 present.
>>>>>
>>>>> As part of this patch we rename hvm_mem_access_emulate_one to
>>>>> hvm_emulate_one_vm_event to better reflect that it is used in various
>>>>> vm_event
>>>>> scenarios now, not just in response to mem_access events.
>>>>>
>>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>>>> ---
>>>>> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>>>>> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
>>>>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>>>>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>>>>
>>>>> Note: This patch only has been compile-tested.
>>>>> ---
>>>>> xen/arch/x86/hvm/emulate.c | 44
>>>>> ++++++++++++++++++++++++++-------------
>>>>> xen/arch/x86/hvm/hvm.c | 9 +++++---
>>>>> xen/arch/x86/hvm/vmx/vmx.c | 1 +
>>>>> xen/arch/x86/vm_event.c | 9 +++++++-
>>>>> xen/common/vm_event.c | 1 -
>>>>> xen/include/asm-x86/hvm/emulate.h | 8 ++++---
>>>>> xen/include/asm-x86/vm_event.h | 3 ++-
>>>>> xen/include/public/vm_event.h | 16 +++++++++++++-
>>>>> 8 files changed, 67 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>>> index cc25676..504ed35 100644
>>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>>> @@ -76,9 +76,9 @@ static int set_context_data(void *buffer, unsigned int
>>>>> size)
>>>>> if ( curr->arch.vm_event )
>>>>> {
>>>>> unsigned int safe_size =
>>>>> - min(size, curr->arch.vm_event->emul_read_data.size);
>>>>> + min(size, curr->arch.vm_event->emul_read.size);
>>>>>
>>>>> - memcpy(buffer, curr->arch.vm_event->emul_read_data.data,
>>>>> safe_size);
>>>>> + memcpy(buffer, curr->arch.vm_event->emul_read.data, safe_size);
>>>>> memset(buffer + safe_size, 0, size - safe_size);
>>>>> return X86EMUL_OKAY;
>>>>> }
>>>>> @@ -827,7 +827,7 @@ static int hvmemul_read(
>>>>> struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>>> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>>>
>>>>> - if ( unlikely(hvmemul_ctxt->set_context) )
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>>> return set_context_data(p_data, bytes);
>>>>>
>>>>> return __hvmemul_read(
>>>>> @@ -1029,7 +1029,7 @@ static int hvmemul_cmpxchg(
>>>>> struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>>> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>>>
>>>>> - if ( unlikely(hvmemul_ctxt->set_context) )
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>>> {
>>>>> int rc = set_context_data(p_new, bytes);
>>>>>
>>>>> @@ -1122,7 +1122,7 @@ static int hvmemul_rep_outs(
>>>>> p2m_type_t p2mt;
>>>>> int rc;
>>>>>
>>>>> - if ( unlikely(hvmemul_ctxt->set_context) )
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>>> return hvmemul_rep_outs_set_context(src_seg, src_offset,
>>>>> dst_port,
>>>>> bytes_per_rep, reps, ctxt);
>>>>>
>>>>> @@ -1264,7 +1264,7 @@ static int hvmemul_rep_movs(
>>>>> if ( buf == NULL )
>>>>> return X86EMUL_UNHANDLEABLE;
>>>>>
>>>>> - if ( unlikely(hvmemul_ctxt->set_context) )
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>>> {
>>>>> rc = set_context_data(buf, bytes);
>>>>>
>>>>> @@ -1470,7 +1470,7 @@ static int hvmemul_read_io(
>>>>>
>>>>> *val = 0;
>>>>>
>>>>> - if ( unlikely(hvmemul_ctxt->set_context) )
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_data) )
>>>>> return set_context_data(val, bytes);
>>>>>
>>>>> return hvmemul_do_pio_buffer(port, bytes, IOREQ_READ, val);
>>>>> @@ -1793,7 +1793,14 @@ static int _hvm_emulate_one(struct
>>>>> hvm_emulate_ctxt *hvmemul_ctxt,
>>>>> pfec |= PFEC_user_mode;
>>>>>
>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>> - if ( !vio->mmio_insn_bytes )
>>>>> +
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event
>>>>> )
>>>>> + {
>>>>> + hvmemul_ctxt->insn_buf_bytes =
>>>>> sizeof(curr->arch.vm_event->emul_insn);
>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul_insn,
>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>> + }
>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>> {
>>>>> hvmemul_ctxt->insn_buf_bytes =
>>>>> hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?:
>>>>> @@ -1931,7 +1938,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
>>>>> unsigned long gla)
>>>>> return rc;
>>>>> }
>>>>>
>>>>> -void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>>>>> +void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>>>>> unsigned int errcode)
>>>>> {
>>>>> struct hvm_emulate_ctxt ctx = {{ 0 }};
>>>>> @@ -1944,11 +1951,19 @@ void hvm_mem_access_emulate_one(enum emul_kind
>>>>> kind, unsigned int trapnr,
>>>>> case EMUL_KIND_NOWRITE:
>>>>> rc = hvm_emulate_one_no_write(&ctx);
>>>>> break;
>>>>> - case EMUL_KIND_SET_CONTEXT:
>>>>> - ctx.set_context = 1;
>>>>> - /* Intentional fall-through. */
>>>>> - default:
>>>>> + case EMUL_KIND_SET_CONTEXT_DATA:
>>>>> + ctx.set_context_data = 1;
>>>>> + rc = hvm_emulate_one(&ctx);
>>>>> + break;
>>>>> + case EMUL_KIND_SET_CONTEXT_INSN:
>>>>> + ctx.set_context_insn = 1;
>>>>> rc = hvm_emulate_one(&ctx);
>>>>> + break;
>>>>> + case EMUL_KIND_NORMAL:
>>>>> + rc = hvm_emulate_one(&ctx);
>>>>> + break;
>>>>> + default:
>>>>> + return;
>>>>> }
>>>>>
>>>>> switch ( rc )
>>>>> @@ -1983,7 +1998,8 @@ void hvm_emulate_prepare(
>>>>> hvmemul_ctxt->ctxt.force_writeback = 1;
>>>>> hvmemul_ctxt->seg_reg_accessed = 0;
>>>>> hvmemul_ctxt->seg_reg_dirty = 0;
>>>>> - hvmemul_ctxt->set_context = 0;
>>>>> + hvmemul_ctxt->set_context_data = 0;
>>>>> + hvmemul_ctxt->set_context_insn = 0;
>>>>> hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
>>>>> hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
>>>>> }
>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>> index ca96643..7462794 100644
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>>>>>
>>>>> if ( v->arch.vm_event->emulate_flags &
>>>>> VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>>>>> - kind = EMUL_KIND_SET_CONTEXT;
>>>>> + kind = EMUL_KIND_SET_CONTEXT_DATA;
>>>>> else if ( v->arch.vm_event->emulate_flags &
>>>>> VM_EVENT_FLAG_EMULATE_NOWRITE )
>>>>> kind = EMUL_KIND_NOWRITE;
>>>>> + else if ( v->arch.vm_event->emulate_flags &
>>>>> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>>>>> + kind = EMUL_KIND_SET_CONTEXT_INSN;
>>>>>
>>>>> - hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
>>>>> - HVM_DELIVER_NO_ERROR_CODE);
>>>>> + hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
>>>>> + HVM_DELIVER_NO_ERROR_CODE);
>>>>>
>>>>> v->arch.vm_event->emulate_flags = 0;
>>>>> }
>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>>>> index 2759e6f..d214716 100644
>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>>> @@ -57,6 +57,7 @@
>>>>> #include <asm/altp2m.h>
>>>>> #include <asm/event.h>
>>>>> #include <asm/monitor.h>
>>>>> +#include <asm/vm_event.h>
>>>>> #include <public/arch-x86/cpuid.h>
>>>>>
>>>>> static bool_t __initdata opt_force_ept;
>>>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>>>> index 343b9c8..03beed3 100644
>>>>> --- a/xen/arch/x86/vm_event.c
>>>>> +++ b/xen/arch/x86/vm_event.c
>>>>> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v,
>>>>> vm_event_response_t *rsp)
>>>>> if ( p2m_mem_access_emulate_check(v, rsp) )
>>>>> {
>>>>> if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>>>>> - v->arch.vm_event->emul_read_data =
>>>>> rsp->data.emul_read_data;
>>>>> + v->arch.vm_event->emul_read = rsp->data.emul.read;
>>>>>
>>>>> v->arch.vm_event->emulate_flags = rsp->flags;
>>>>> }
>>>>> break;
>>>>> + case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
>>>>> + if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>>>>> + {
>>>>> + v->arch.vm_event->emul_insn = rsp->data.emul.insn;
>>>>> + v->arch.vm_event->emulate_flags = rsp->flags;
>>>>> + }
>>>>> + break;
>>>>> default:
>>>>> break;
>>>>> };
>>>>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>>>>> index 907ab40..d8ee7f3 100644
>>>>> --- a/xen/common/vm_event.c
>>>>> +++ b/xen/common/vm_event.c
>>>>> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct
>>>>> vm_event_domain *ved)
>>>>> * In some cases the response type needs extra handling, so here
>>>>> * we call the appropriate handlers.
>>>>> */
>>>>> -
>>>>> /* Check flags which apply only when the vCPU is paused */
>>>>> if ( atomic_read(&v->vm_event_pause_count) )
>>>>> {
>>>>> diff --git a/xen/include/asm-x86/hvm/emulate.h
>>>>> b/xen/include/asm-x86/hvm/emulate.h
>>>>> index 3aabcbe..b52f99e 100644
>>>>> --- a/xen/include/asm-x86/hvm/emulate.h
>>>>> +++ b/xen/include/asm-x86/hvm/emulate.h
>>>>> @@ -34,20 +34,22 @@ struct hvm_emulate_ctxt {
>>>>>
>>>>> uint32_t intr_shadow;
>>>>>
>>>>> - bool_t set_context;
>>>>> + bool_t set_context_data;
>>>>> + bool_t set_context_insn;
>>>>> };
>>>>>
>>>>> enum emul_kind {
>>>>> EMUL_KIND_NORMAL,
>>>>> EMUL_KIND_NOWRITE,
>>>>> - EMUL_KIND_SET_CONTEXT
>>>>> + EMUL_KIND_SET_CONTEXT_DATA,
>>>>> + EMUL_KIND_SET_CONTEXT_INSN
>>>>
>>>> Since this breaks compilation of existing clients, I think we should
>>>> increment some macro to alow for compile-time switching (maybe
>>>> VM_EVENT_INTERFACE_VERSION?).
>>>
>>> I'm not sure I follow - this is a Xen internal-only enumeration. What
>>> kind-of clients are you referring to?
>>
>> Nevermind, I thought these changes propagate to the toolstack headers.
>> Sorry for the confusion.
>
> On second thought (and a night's rest), the problem is real. I've made
> it a point to test the patches today before re-reviewing them, and this
> happened:
>
> bdvmixeneventmanager.cpp:359:46: error: ‘union vm_event_st::<anonymous>’
> has no member named ‘emul_read_data’
> uint32_t rspDataSize = sizeof( rsp.data.emul_read_data.data );
> ^
> bdvmixeneventmanager.cpp:386:44: error: ‘union vm_event_st::<anonymous>’
> has no member named ‘emul_read_data’
> action, rsp.data.emul_read_data.data,
> rspDataSize,
> ^
> bdvmixeneventmanager.cpp:389:16: error: ‘union vm_event_st::<anonymous>’
> has no member named ‘emul_read_data’
> rsp.data.emul_read_data.size = rspDataSize;
> ^
> make: *** [bdvmixeneventmanager.o] Error 1
>
> So, as you can see, existing clients really don't compile without
> modification.
>
Hi Razvan,
yes, for Xen 4.8 we already bumped the VM_EVENT_INTERFACE_VERSION, so
any client building on it need to adapt accordingly. That's why we
have the versioning in place. The way we handle this in LibVMI is by
defining a local copy of all supported Xen events ABI versions and
build with that rather then with xen/vm_event.h so we have backwards
compatibility. See
https://github.com/libvmi/libvmi/blob/master/libvmi/driver/xen/xen_events_abi.h.
Cheers,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |