[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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
> 

Attachment: 0016-Add-IOREQ_TYPE_VMWARE_PORT.patch
Description: Text Data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.