Xen project Mailing List

On 02/26/15 03:07, Jan Beulich wrote: >>>> On 25.02.15 at 21:20, <dslutz@xxxxxxxxxxx> wrote: >> On 02/24/15 10:34, Jan Beulich wrote: >>>>>> On 17.02.15 at 00:05, <dslutz@xxxxxxxxxxx> wrote: >>>> @@ -501,22 +542,50 @@ static void hvm_free_ioreq_gmfn(struct domain *d, >>>> unsigned long gmfn) [snip] >>>> @@ -2429,9 +2552,6 @@ struct hvm_ioreq_server >>>> *hvm_select_ioreq_server(struct >> domain *d, >>>> if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) ) >>>> return NULL; >>>> >>>> - if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO ) >>>> - return d->arch.hvm_domain.default_ioreq_server; >>> >>> Shouldn't this rather be amended than deleted? >>> >> >> The reason is below: >> >>>> @@ -2474,7 +2594,12 @@ struct hvm_ioreq_server >>>> *hvm_select_ioreq_server(struct domain *d, >>>> BUILD_BUG_ON(IOREQ_TYPE_PIO != HVMOP_IO_RANGE_PORT); >>>> BUILD_BUG_ON(IOREQ_TYPE_COPY != HVMOP_IO_RANGE_MEMORY); >>>> BUILD_BUG_ON(IOREQ_TYPE_PCI_CONFIG != HVMOP_IO_RANGE_PCI); >>>> + BUILD_BUG_ON(IOREQ_TYPE_VMWARE_PORT != >>>> HVMOP_IO_RANGE_VMWARE_PORT); >>>> + BUILD_BUG_ON(IOREQ_TYPE_TIMEOFFSET != HVMOP_IO_RANGE_TIMEOFFSET); >>>> + BUILD_BUG_ON(IOREQ_TYPE_INVALIDATE != HVMOP_IO_RANGE_INVALIDATE); >>>> r = s->range[type]; >>>> + if ( !r ) >>>> + continue; >>> >>> Why, all of the sudden? >>> >> >> This is the replacement for the deleted "if" above. Continue will lead >> to the same return that was remove above (it is at the end). They are >> currently the same because all ioreq servers have the same set of >> ranges. But if it would help, I can change "continue" into the "return >> default". > > So further down you talk of the "special range 1" (see there for > further remarks in this regard) - how would r be NULL here in the > first place? Since there is a hole in the #defines 0,1,2,7,8 (currently) range[6] is where r will be NULL for example. However no current code should be able to get here. So if you want me to I can drop the "if". > That said - yes, making this explicitly do what is > intended (perhaps rather using "break" instead of "return") would > seem very desirable. There simply is no point in continuing the > loop. > Will use break if the "if" is not dropped. >>>> @@ -2501,6 +2626,13 @@ struct hvm_ioreq_server >>>> *hvm_select_ioreq_server(struct domain *d, >>>> } >>>> >>>> break; >>>> + case IOREQ_TYPE_VMWARE_PORT: >>>> + case IOREQ_TYPE_TIMEOFFSET: >>>> + case IOREQ_TYPE_INVALIDATE: >>>> + if ( rangeset_contains_singleton(r, 1) ) >>>> + return s; >>> >>> This literal 1 at least needs explanation (i.e. a comment). >>> >> >> The comment is below (copied here). Will duplicate it here (with any >> adjustments needed): >> >> + * NOTE: The 'special' range of 1 is what is checked for outside >> + * of the three types of I/O. >> >> How about /* The 'special' range of 1 is checked for being enabled */? > > Along these lines, yes (fixed for coding style). And then "1" is not > a range of any kind. I suppose writing it as a proper range (e.g. > [1,1]) would already help. > I will adjust to using [1,1]. >>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >>>> @@ -112,6 +112,8 @@ struct __packed segment_register { >>>> #define X86EMUL_RETRY 3 >>>> /* (cmpxchg accessor): CMPXCHG failed. Maps to X86EMUL_RETRY in caller. >>>> */ >>>> #define X86EMUL_CMPXCHG_FAILED 3 >>>> + /* Send part of registers also to DM. */ >>>> +#define X86EMUL_VMPORT_SEND 4 >>> >>> Introducing a new value here seems rather fragile, as various code >>> paths you don't touch would need auditing that they do the right >>> thing upon this value being returned. Plus even conceptually this >>> doesn't belong here - the instruction emulator shouldn't be concerned >>> with things like VMware emulation. >>> >> >> The only place I know of where rc is not checked by name is in >> x86_emulate.c. There are a lot of 0 and != 0 checks. Also in area of >> code there are places that return X86EMUL_OKAY when it looks to me that >> the return value is checked for 0 and ignored otherwise. > > The point aren't the checks against zero, but the ones against the > #define-d values. Code may exist that, after excluding certain > values, assumes that only some specific value can be left. While > we aim at adding ASSERT()s for such cases, I'm nowhere near to > being convinced this is the case everywhere. > >> So I will agree that the use of these defines is complex. However, I >> need a way to pass back X86EMUL_UNHANDLEABLE and send a few registers to >> QEMU. Now since the code path that I need to do this is: >> >> ... >> hvmemul_do_io >> hvm_portio_intercept >> hvm_io_intercept >> process_portio_intercept >> vmport_ioport >> >> >> Since there is only 1 caller to hvm_portio_intercept() -- >> hvmemul_do_io, and hvmemul_do_io does not let X86EMUL_VMPORT_SEND be >> returned. I feel that all code paths currently have been checked. >> Adding a return code to me was the simpler code change. It is possible >> to change process_portio_intercept() by adding special code there to >> adjust the ioreq_t p there, but that looked worse to me. >> >> I can change to using a bit in the return of portio_action_t that would >> be masked in process_portio_intercept() and make the code in >> hvmemul_do_io() less clear since the change of p->type changes in >> process_portio_intercept(), and change to hvmemul_do_io() is much more >> involved. >> >> I am happy to use some other number like 65539 if that would help. Also >> any other name like X86EMUL_UNHANDLEABLE_EXTENDED, SPECIAL_DM_HANDLING, >> etc would be fine with me. I have no issue with defining this in a >> different header file if that would help. > > I understand all this is non-trivial and not necessarily obvious. But > as said before - the x86 instruction emulator should please remain > something acting along _only_ architectural specifications. Any > extensions to support things like what you want to do here should > be added using neutral hooks, responsible for keeping state they > need on their own. > How does (the incorrectly formatted for a smaller diff): diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index f271dfc..9027ab8 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -219,6 +219,7 @@ static int hvmemul_do_io( vio->io_state = HVMIO_handle_mmio_awaiting_completion; break; case X86EMUL_UNHANDLEABLE: + if ( vmport_check_port(p.addr) ) { struct hvm_ioreq_server *s = hvm_select_ioreq_server(curr->domain, &p); @@ -238,8 +239,50 @@ static int hvmemul_do_io( else if ( p_data == NULL ) rc = X86EMUL_OKAY; } - break; } + else + { + struct hvm_ioreq_server *s; + vmware_regs_t *vr; + + BUILD_BUG_ON(sizeof(ioreq_t) < sizeof(vmware_regs_t)); + + p.type = IOREQ_TYPE_VMWARE_PORT; + s = hvm_select_ioreq_server(curr->domain, &p); + vr = get_vmport_regs_any(s, curr); + + /* + * If there is no suitable backing DM, just ignore accesses. If + * we do not have access to registers to pass to QEMU, just + * ignore access. + */ + if ( !s || !vr ) + { + hvm_complete_assist_req(&p); + rc = X86EMUL_OKAY; + vio->io_state = HVMIO_none; + } + else + { + struct cpu_user_regs *regs = guest_cpu_user_regs(); + + p.data = regs->rax; + vr->ebx = regs->_ebx; + vr->ecx = regs->_ecx; + vr->edx = regs->_edx; + vr->esi = regs->_esi; + vr->edi = regs->_edi; + + vio->io_state = HVMIO_handle_pio_awaiting_completion; + if ( !hvm_send_assist_req(s, &p) ) + { + rc = X86EMUL_RETRY; + vio->io_state = HVMIO_none; + } + /* else leave rc as X86EMUL_UNHANDLEABLE for below. */ + } + } + break; default: BUG(); } @@ -248,6 +291,13 @@ static int hvmemul_do_io( { if ( ram_page ) put_page(ram_page); + /* + * If rc is still X86EMUL_UNHANDLEABLE, then were are of + * type IOREQ_TYPE_VMWARE_PORT, so completion in + * hvm_io_assist() with no re-emulation required + */ + if ( rc == X86EMUL_UNHANDLEABLE ) + rc = X86EMUL_OKAY; return rc; } look as the change to emulate.c? Attached is the perspective version of this patch. >>>> --- a/xen/include/public/hvm/hvm_op.h >>>> +++ b/xen/include/public/hvm/hvm_op.h >>>> @@ -314,6 +314,9 @@ >>>> DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_ioreq_server_info_t); >>>> * >>>> * NOTE: unless an emulation request falls entirely within a range mapped >>>> * by a secondary emulator, it will not be passed to that emulator. >>>> + * >>>> + * NOTE: The 'special' range of 1 is what is checked for outside >>>> + * of the three types of I/O. >>>> */ >>>> #define HVMOP_map_io_range_to_ioreq_server 19 >>>> #define HVMOP_unmap_io_range_from_ioreq_server 20 >>>> @@ -324,6 +327,9 @@ struct xen_hvm_io_range { >>>> # define HVMOP_IO_RANGE_PORT 0 /* I/O port range */ >>>> # define HVMOP_IO_RANGE_MEMORY 1 /* MMIO range */ >>>> # define HVMOP_IO_RANGE_PCI 2 /* PCI segment/bus/dev/func range */ >>>> +# define HVMOP_IO_RANGE_VMWARE_PORT 3 /* VMware port special range */ >>>> +# define HVMOP_IO_RANGE_TIMEOFFSET 7 /* TIMEOFFSET special range */ >>>> +# define HVMOP_IO_RANGE_INVALIDATE 8 /* INVALIDATE special range */ >>> >>> I can't seem to connect the comment you add above to the three >>> new #define-s. >>> >> >> Will work on better wording for the comment here and in the code >> that is checking for it. Paul Durrant pointed out that >> HVMOP_IO_RANGE_INVALIDATE is not needed and so the plan is to drop it. >> Does: >> >> + * NOTE: The 'special' range of 1 is what is checked for on >> + * VMWARE_PORT and TIMEOFFSET. >> >> help? > > Calling out the affected ones explicitly is certainly better. Beyond > that see my earlier remark. > Also adjust to [1,1]. Rest is done in a different thread. -Don Slutz > Jan >

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Re: [Xen-devel] [PATCH v9 08/13] Add IOREQ_TYPE_VMWARE_PORT