Re: [RFC PATCH 07/10] Add new vmport code.

[Don Slutz <dslutz@verizon.com>]

On 12/12/13 19:06, Andrew Cooper wrote:
> On 12/12/2013 19:15, Don Slutz wrote:
>> From: Don Slutz <dslutz@verizon.com>
>>
>> enable vmport_flush call.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   xen/arch/x86/hvm/Makefile                |   1 +
>>   xen/arch/x86/hvm/hvm.c                   |   2 -
>>   xen/arch/x86/hvm/vmport/Makefile         |   1 +
>>   xen/arch/x86/hvm/vmport/includeCheck.h   |  17 +
>>   xen/arch/x86/hvm/vmport/vmport.c         | 719 +++++++++++++++++++++++++++++++
>>   xen/arch/x86/hvm/vmport/xen_vmport_def.h |  36 ++
>>   xen/include/asm-x86/hvm/trace.h          |   3 +
>>   7 files changed, 777 insertions(+), 2 deletions(-)
>>   create mode 100644 xen/arch/x86/hvm/vmport/Makefile
>>   create mode 100644 xen/arch/x86/hvm/vmport/includeCheck.h
>>   create mode 100644 xen/arch/x86/hvm/vmport/vmport.c
>>   create mode 100644 xen/arch/x86/hvm/vmport/xen_vmport_def.h
>>
>> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
>> index eea5555..954a81c 100644
>> --- a/xen/arch/x86/hvm/Makefile
>> +++ b/xen/arch/x86/hvm/Makefile
>> @@ -1,5 +1,6 @@
>>   subdir-y += svm
>>   subdir-y += vmx
>> +subdir-y += vmport
>>   
>>   obj-y += asid.o
>>   obj-y += emulate.o
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index fa5d382..a557272 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -614,9 +614,7 @@ int hvm_domain_initialise(struct domain *d)
>>                (d->max_pages + (1 << 10) - 1) >> 10,
>>                (d->max_pages + (1 << 20) - 1) >> 20);
>>   
>> -#if 0
>>       vmport_flush(&d->arch.hvm_domain);
>> -#endif
>>   
>>       if ( is_pvh_domain(d) )
>>       {
>> diff --git a/xen/arch/x86/hvm/vmport/Makefile b/xen/arch/x86/hvm/vmport/Makefile
>> new file mode 100644
>> index 0000000..2648fae
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmport/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += vmport.o
>> diff --git a/xen/arch/x86/hvm/vmport/includeCheck.h b/xen/arch/x86/hvm/vmport/includeCheck.h
>> new file mode 100644
>> index 0000000..26e0d59
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmport/includeCheck.h
>> @@ -0,0 +1,17 @@
>> +/*
>> + * includeCheck.h
>> + *
>> + * Copyright (C) 2012 Verizon Corporation
>> + *
>> + * This file is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License Version 2 (GPLv2)
>> + * as published by the Free Software Foundation.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details. <http://www.gnu.org/licenses/>.
>> + */
>> +/*
>> + * Nothing here.  Just to use backdoor_def.h without change.
> What do you mean by this?

In VMware it looks like this file that is included by backdoor_def.h 
does a lot of defines that I have no use for.  So to be able to use 
backdoor_def.h without any changes I needed an include file with the 
name includeCheck.h;  Not sure which is the better way to go.


>
>> + */
>> diff --git a/xen/arch/x86/hvm/vmport/vmport.c b/xen/arch/x86/hvm/vmport/vmport.c
>> new file mode 100644
>> index 0000000..43bdf7b
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmport/vmport.c
>> @@ -0,0 +1,719 @@
>> +/*
>> + * HVM VMPORT emulation
>> + *
>> + * Copyright (C) 2012 Verizon Corporation
>> + *
>> + * This file is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License Version 2 (GPLv2)
>> + * as published by the Free Software Foundation.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details. <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "xen_vmport_def.h"
>> +#include "backdoor_def.h"
>> +#include "guest_msg_def.h"
>> +#include "asm-x86/hvm/support.h"
> #include <asm/hvm/support.h>

Will fix. Thanks.

>
>> +
>> +#define LOG_RPC             0x0000001
>> +#define LOG_RECV_STATUS     0x0000002
>> +#define LOG_SKIP_SEND       0x0000004
>> +#define LOG_SEND            0x0000008
>> +#define LOG_SEND_SIZE_ALL   0x0000010
>> +#define LOG_SEND_SIZE       0x0000020
>> +#define LOG_RECV_SIZE_ALL   0x0000040
>> +#define LOG_RECV_SIZE       0x0000080
>> +#define LOG_CLOSE           0x0000100
>> +#define LOG_OPEN            0x0000200
>> +#define LOG_FLUSH           0x0000400
>> +#define LOG_TRACE           0x0000800
>> +#define LOG_PING            0x0001000
>> +#define LOG_SWEEP           0x0002000
>> +#define LOG_BUILD           0x0004000
>> +#define LOG_STATUS          0x0008000
>> +
>> +#define LOG_ERROR           0x0010000
>> +
>> +#define LOG_INFO_GET        0x0020000
>> +#define LOG_INFO_SET        0x0040000
>> +
>> +#define LOG_GP_UNKNOWN      0x0100000
>> +#define LOG_GP_NOT_VMWARE   0x0200000
>> +#define LOG_GP_FAIL_RD_INST 0x0400000
>> +#define LOG_GP_VMWARE_AFTER 0x0800000
>> +
>> +#define LOG_VGP_UNKNOWN     0x1000000
>> +#define LOG_REALMODE_GP     0x8000000
>> +
>> +extern unsigned long get_sec(void);
> Please include the appropriate header files.

Will do (assuming you mean for get_sec).

>> +
>> +/* Note: VMPORT_PORT and VMPORT_MAGIC is also defined as BDOOR_PORT
>> + * and BDOOR_MAGIC in backdoor_def.h Defined in vmport.h also.
>> + */
>> +
>> +inline uint16_t getLowBits(uint32_t bits)
>> +{
>> +    return bits & 0xffff;
>> +}
>> +
>> +inline uint16_t getHighBits(uint32_t bits)
>> +{
>> +    return bits >> 16;
>> +}
>> +
>> +inline uint32_t setHighBits(uint32_t b, uint32_t val)
>> +{
>> +    return (val << 16) | getLowBits(b);
>> +}
> What is the point of these functions being separated out?

It was attempt to may the code more readable.  Just noticed that these 
are in camelCase (getHighBits vs get_high_bits).  Which way should I go?

> setHighBits only seems to be used with regs->ecx (which is actually a
> 64bit value not a 32bit value) to set a status value.
> Why not set_status(cpu_user_regs *, uint16_t val) ?


Will change to this.


>
>> +
>> +static inline long getLogMask(struct hvm_domain *hd)
>> +{
>> +    return hd->params[HVM_PARAM_VMPORT_LOGMASK];
> A param is unsigned long, not long...

Will fix.


>> +}
>> +
>> +static inline char *getStatus(struct hvm_domain *hd)
>> +{
>> +    return (char*)&hd->params[HVM_PARAM_VMPORT_STATUS];
> ... And most certainly not a string.

Current planning on dropping all code that uses this.

>> +}
>> +
>> +void vmport_safe_print(char *prefix, int len, char *msg)
>> +{
>> +    unsigned char c;
>> +    int end = len;
>> +    int i,k;
>> +    char out[4*(VMPORT_MAX_SEND_BUF + 1)*3 + 6];
>> +
>> +    if (end > (sizeof(out)/3 - 6))
>> +        end = sizeof(out)/3 - 6;
>> +    out[0] = '<';
>> +    k = 1;
>> +    for (i = 0; i < end; i++) {
>> +        c = msg[i];
>> +        if ((c == '^') || (c == '\\') || (c == '>')) {
>> +            out[k++] = '\\';
>> +            out[k++] = c;
>> +        } else if ((c >= ' ') && (c <= '~')) {
>> +            out[k++] = c;
>> +        } else if (c < ' ') {
>> +            out[k++] = '^';
>> +            out[k++] = c ^ 0x40;
>> +        } else {
>> +            snprintf(&out[k], sizeof(out) - k, "\\%02x", c);
>> +            k += 3;
>> +        }
>> +    }
>> +    out[k++] = '>';
>> +    if (len > end) {
>> +        out[k++] = '.';
>> +        out[k++] = '.';
>> +        out[k++] = '.';
>> +    }
>> +    out[k++] = 0;
>> +    gdprintk(XENLOG_DEBUG, "%s%d(%d,%d,%ld)%s\n", prefix, end, len, k, sizeof(out), out);
> the correct format for any sizeof is %zu or %zx depending on decimal/hex.

Will fix.

>> +}
>> +
>> +void vmport_send(struct hvm_domain *hd, vmport_channel_t *c, char *msg, int slot)
>> +{
>> +    unsigned int cur_recv_len = strlen(msg) + 1;
>> +    char prefix[30];
>> +    unsigned int my_bkt = c->recv_write;
>> +    unsigned int next_bkt = my_bkt + 1;
>> +    vmport_bucket_t *b;
>> +
>> +    if (next_bkt >= VMPORT_MAX_BKTS)
>> +        next_bkt = 0;
>> +
>> +    if (next_bkt == c->recv_read) {
>> +        if (getLogMask(hd) & LOG_SKIP_SEND) {
>> +            snprintf(prefix, sizeof(prefix),
>> +                     "VMware _send skipped %d (%d, %d) ", c->chan_id, my_bkt, c->recv_read);
>> +            prefix[sizeof(prefix)-1] = 0;
>> +            vmport_safe_print(prefix, cur_recv_len, msg);
>> +        }
>> +        getStatus(hd)[slot] = 200;
> This (and every use of getStatus() below) most certainly needs to
> guarentee that slot is strictly in the bound 0 to 7, and writing the
> integer 200 to a char is undefined I believe.

You are right.  Current planning on dropping all code that uses getStatus.



>
>> +        if (getLogMask(hd) & LOG_STATUS)
>> +            gdprintk(XENLOG_DEBUG, "VMware %d getStatus[%d]=200\n", c->chan_id, slot);
>> +        return;
>> +    }
>> +
>> +    c->recv_write = next_bkt;
>> +    b = &c->recv_bkt[my_bkt];
>> +    if (getLogMask(hd) & LOG_SEND) {
>> +        snprintf(prefix, sizeof(prefix),
>> +                 "VMware _send %d (%d) ", c->chan_id, my_bkt);
>> +        prefix[sizeof(prefix)-1] = 0;
>> +        vmport_safe_print(prefix, cur_recv_len, msg);
>> +    }
>> +
>> +    b->recv_len = cur_recv_len;
>> +    b->recv_slot = slot;
>> +    b->recv_idx = 0;
>> +    memset(b->recv_buf, 0, sizeof(b->recv_buf));
>> +    if (cur_recv_len >= (sizeof(b->recv_buf) - 1)) {
>> +        if (getLogMask(hd) & LOG_ERROR)
>> +            gdprintk(XENLOG_DEBUG, "VMware recv_len=%d >= %ld.\n",
>> +                     cur_recv_len, sizeof(b->recv_buf) - 1);
>> +        cur_recv_len = sizeof(b->recv_buf) - 1;
>> +    }
>> +    memcpy(b->recv_buf, msg, cur_recv_len);
>> +    getStatus(hd)[b->recv_slot] = 1;
>> +    if (getLogMask(hd) & LOG_STATUS)
>> +        gdprintk(XENLOG_DEBUG, "VMware %d,%d getStatus[%d]=1\n",
>> +                 c->chan_id, c->recv_read, b->recv_slot);
>> +}
>> +
>> +void vmport_ctrl_send(struct hvm_domain *hd, char *msg, int slot)
>> +{
>> +    struct vmport_state *vs = hd->vmport_data;
>> +    int i;
>> +
>> +    if (slot < 1 || slot > 7)
>> +        slot = 7;
>> +    hd->vmport_data->ping_time = get_sec();
>> +    spin_lock(&hd->vmport_lock);
>> +    for (i = 0; i < VMPORT_MAX_CHANS; i++) {
>> +        if (vs->chans[i].proto_num == 0x4f4c4354) {
>> +            vmport_send(hd, &vs->chans[i], msg, slot);
>> +        }
>> +    }
>> +    spin_unlock(&hd->vmport_lock);
>> +}
>> +
>> +void vmport_flush(struct hvm_domain *hd)
>> +{
>> +    if (getLogMask(hd) & LOG_FLUSH)
>> +        gdprintk(XENLOG_DEBUG, "VMware flush.\n");
>> +    spin_lock(&hd->vmport_lock);
>> +    memset(&hd->vmport_data->chans, 0, sizeof(hd->vmport_data->chans));
>> +    spin_unlock(&hd->vmport_lock);
>> +}
>> +
>> +void vmport_sweep(struct hvm_domain *hd, unsigned long now_time)
>> +{
>> +    struct vmport_state *vs = hd->vmport_data;
>> +    int i;
>> +
>> +    for (i = 0; i < VMPORT_MAX_CHANS; i++) {
>> +        if (vs->chans[i].proto_num) {
>> +            vmport_channel_t *c = &vs->chans[i];
>> +            long delta = now_time - c->active_time;
>> +
>> +            if ( delta >= 80 ) {
>> +                if (getLogMask(hd) & LOG_SWEEP)
>> +                    gdprintk(XENLOG_DEBUG, "VMware flush %d. delta=%ld\n",
>> +                             c->chan_id, delta);
>> +                // Return channel to free pool
>> +                c->proto_num = 0;
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +vmport_channel_t *vmport_new_chan(struct vmport_state *vs, unsigned long now_time)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < VMPORT_MAX_CHANS; i++) {
>> +        if (!vs->chans[i].proto_num) {
>> +            vmport_channel_t *c = &vs->chans[i];
>> +
>> +            c->chan_id = i;
>> +            c->cookie = vs->open_cookie++;
>> +            c->active_time = now_time;
>> +            c->send_len = 0;
>> +            c->send_idx = 0;
>> +            c->recv_read = 0;
>> +            c->recv_write = 0;
>> +            return c;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +void vmport_process_send_size(struct hvm_domain *hd, vmport_channel_t *c, struct cpu_user_regs *ur)
>> +{
>> +    // vmware tools often send a 0 byte request size.
>> +    c->send_len = ur->ebx;
>> +    c->send_idx = 0;
>> +    ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +    if ((getLogMask(hd) & LOG_SEND_SIZE_ALL) ||
>> +        ((getLogMask(hd) & LOG_SEND_SIZE) && (c->send_len)))
>> +        gdprintk(XENLOG_DEBUG, "VMware SENDSIZE %d is %d.\n",
>> +                 c->chan_id, c->send_len);
>> +}
>> +
>> +void vmport_process_send_payload(struct hvm_domain *hd, vmport_channel_t *c,
>> +                                 struct cpu_user_regs *ur, unsigned long now_time)
>> +{
>> +    char prefix[30];
>> +
>> +    if (c->send_idx < VMPORT_MAX_SEND_BUF) {
>> +        c->send_buf[c->send_idx] = ur->ebx;
>> +    }
>> +    c->send_idx++;
>> +    ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +    if (c->send_idx * 4 >= c->send_len) {
>> +        if (c->send_idx < VMPORT_MAX_SEND_BUF)
>> +            ((char*)c->send_buf)[c->send_len] = 0;
>> +        if (getLogMask(hd) & LOG_RPC) {
>> +            snprintf(prefix, sizeof(prefix),
>> +                     "VMware RPC %d (%d) ", c->chan_id, c->recv_read);
>> +            prefix[sizeof(prefix)-1] = 0;
>> +            vmport_safe_print(prefix, c->send_len, (char*)c->send_buf);
>> +        }
>> +        if (c->proto_num == 0x49435052) {
>> +/* log toolbox: Version: build-341836 */
>> +/* SetGuestInfo  4 build-341836 */
>> +/* info-get guestinfo.ip */
>> +/* info-set guestinfo.ip joe */
>> +            char * build = NULL;
>> +            char * info_key = NULL;
>> +            char * ret_msg = "1 ";
> I cant remember offhand whether Xen is compiled with or without
> read-only strings,

Not sure why it matters.  What ret_msg points to is not changed. ret_msg 
it's self is changed to point to different strings.

0 hvm/vmport/vmport.c vmport_process_send_payload 274 char * ret_msg = "1 ";
1 hvm/vmport/vmport.c vmport_process_send_payload 311 ret_msg = ret_buffer;
2 hvm/vmport/vmport.c vmport_process_send_payload 316 ret_msg = "0 No 
value found";
3 hvm/vmport/vmport.c vmport_process_send_payload 319 ret_msg = "0 Key 
is too long";
4 hvm/vmport/vmport.c vmport_process_send_payload 364 ret_msg = "0 Too 
many keys";
5 hvm/vmport/vmport.c vmport_process_send_payload 373 ret_msg = "0 Value 
too long";
6 hvm/vmport/vmport.c vmport_process_send_payload 376 ret_msg = "0 Key 
is too long";
7 hvm/vmport/vmport.c vmport_process_send_payload 384 ret_msg = "0 Two 
and exactly two arguments expected";
8 hvm/vmport/vmport.c vmport_process_send_payload 388 vmport_send(hd, c, 
ret_msg, 5);

And it looks like I should change:

void vmport_send(struct hvm_domain *hd, vmport_channel_t *c, char *msg, 
int slot)

into:

void vmport_send(struct hvm_domain *hd, vmport_channel_t *c, const char 
*msg, int slot)


>> +            char ret_buffer[2 + VMPORT_MAX_VAL_LEN + 2];
>> +
>> +            if (strncmp((char*)c->send_buf, "log toolbox: Version: build-",
>> +                        strlen("log toolbox: Version: build-")) == 0) {
>> +                build = (char*)c->send_buf + strlen("log toolbox: Version: build-");
>> +            } else if (strncmp((char*)c->send_buf, "SetGuestInfo  4 build-",
>> +                               strlen("SetGuestInfo  4 build-")) == 0) {
>> +                build = (char*)c->send_buf + strlen("SetGuestInfo  4 build-");
>> +            } else if (strncmp((char*)c->send_buf, "info-get guestinfo.",
>> +                               strlen("info-get guestinfo.")) == 0) {
>> +                int keyLen = c->send_len - strlen("info-get guestinfo.");
>> +                int idx;
>> +                struct vmport_state *vs = hd->vmport_data;
>> +
>> +                info_key = (char*)c->send_buf + strlen("info-get guestinfo.");
>> +                if (getLogMask(hd) & LOG_INFO_GET) {
>> +                    snprintf(prefix, sizeof(prefix),
>> +                             "VMware info-get key:");
>> +                    vmport_safe_print(prefix, keyLen, info_key);
>> +                }
>> +                if (keyLen <= VMPORT_MAX_KEY_LEN) {
>> +                    for (idx = 0; idx < vs->used_guestinfo; idx++) {
>> +                        if ((vs->guestinfo[idx]->key_len == keyLen) &&
>> +                            (memcmp(info_key,
>> +                                    vs->guestinfo[idx]->key_data,
>> +                                    vs->guestinfo[idx]->key_len) == 0)) {
>> +                            if (getLogMask(hd) & LOG_INFO_GET) {
>> +                                snprintf(prefix, sizeof(prefix),
>> +                                         "VMware info-get val:");
>> +                                vmport_safe_print(prefix,
>> +                                                  vs->guestinfo[idx]->val_len,
>> +                                                  vs->guestinfo[idx]->val_data);
>> +                            }
>> +                            snprintf(ret_buffer, sizeof(ret_buffer) - 1, "1 %.*s",
>> +                                     (int)vs->guestinfo[idx]->val_len,
>> +                                     vs->guestinfo[idx]->val_data);
>> +                            ret_msg = ret_buffer;
>> +                            break;
>> +                        }
>> +                    }
>> +                    if (idx >= vs->used_guestinfo) {
>> +                        ret_msg = "0 No value found";
>> +                    }
>> +                } else {
>> +                    ret_msg = "0 Key is too long";
>> +                }
>> +            } else if (strncmp((char*)c->send_buf, "info-set guestinfo.",
>> +                               strlen("info-set guestinfo.")) == 0) {
>> +                char * val;
>> +                int rest_len = c->send_len - strlen("info-set guestinfo.");
>> +
>> +                info_key = (char*)c->send_buf + strlen("info-set guestinfo.");
>> +                val = strstr(info_key, " ");
>> +                if (val) {
>> +                    int keyLen = val - info_key;
>> +                    int valLen = rest_len - keyLen - 1;
>> +                    int free_idx = -1;
>> +                    int idx;
>> +                    struct vmport_state *vs = hd->vmport_data;
>> +
>> +                    val++;
>> +                    if (getLogMask(hd) & LOG_INFO_SET) {
>> +                        snprintf(prefix, sizeof(prefix),
>> +                                 "VMware info-set key:");
>> +                        vmport_safe_print(prefix, keyLen, info_key);
>> +                        snprintf(prefix, sizeof(prefix),
>> +                                 "VMware info-set val:");
>> +                        vmport_safe_print(prefix, valLen, val);
>> +                    }
>> +                    if (keyLen <= VMPORT_MAX_KEY_LEN) {
>> +                        if (valLen <= VMPORT_MAX_VAL_LEN) {
>> +                            for (idx = 0; idx < vs->used_guestinfo; idx++) {
>> +                                if (!vs->guestinfo[idx]) {
>> +                                    gdprintk(XENLOG_WARNING, "idx=%d not allocated, but used_guestinfo=%d\n",
>> +                                             idx, vs->used_guestinfo);
>> +                                } else if ((vs->guestinfo[idx]->key_len == keyLen) &&
>> +                                           (memcmp(info_key,
>> +                                                   vs->guestinfo[idx]->key_data,
>> +                                                   vs->guestinfo[idx]->key_len) == 0)) {
>> +                                    vs->guestinfo[idx]->val_len = valLen;
>> +                                    memcpy(vs->guestinfo[idx]->val_data, val, valLen);
>> +                                    break;
>> +                                } else if ((vs->guestinfo[idx]->key_len == 0) &&
>> +                                           (free_idx == -1)) {
>> +                                    free_idx = idx;
>> +                                }
>> +                            }
>> +                            if (idx >= vs->used_guestinfo) {
>> +                                if (free_idx == -1) {
>> +                                    ret_msg = "0 Too many keys";
>> +                                } else {
>> +                                    vs->guestinfo[free_idx]->key_len = keyLen;
>> +                                    memcpy(vs->guestinfo[free_idx]->key_data, info_key, keyLen);
>> +                                    vs->guestinfo[free_idx]->val_len = valLen;
>> +                                    memcpy(vs->guestinfo[free_idx]->val_data, val, valLen);
>> +                                }
>> +                            }
>> +                        } else {
>> +                            ret_msg = "0 Value too long";
>> +                        }
>> +                    } else {
>> +                        ret_msg = "0 Key is too long";
>> +                    }
>> +                } else {
>> +                    if (getLogMask(hd) & LOG_INFO_SET) {
>> +                        snprintf(prefix, sizeof(prefix),
>> +                                 "VMware info-set missing val; key:");
>> +                        vmport_safe_print(prefix, rest_len, info_key);
>> +                    }
>> +                    ret_msg = "0 Two and exactly two arguments expected";
>> +                }
>> +            }
>> +
>> +            vmport_send(hd, c, ret_msg, 5);
>> +            if (build) {
>> +                long val = 0;
>> +                char *p = build;
>> +
>> +                while (*p) {
>> +                    if (*p < '0' || *p > '9')
>> +                        break;
>> +                    val = val * 10 + *p - '0';
>> +                    p++;
>> +                };
>> +
>> +                hd->params[HVM_PARAM_VMPORT_BUILD_NUMBER_VALUE] = val;
>> +                hd->params[HVM_PARAM_VMPORT_BUILD_NUMBER_TIME] = now_time;
>> +                if (getLogMask(hd) & LOG_BUILD) {
>> +                    snprintf(prefix, sizeof(prefix),
>> +                             "VMware build %ld ", val);
>> +                    vmport_safe_print(prefix, p - build, build);
>> +                }
>> +            }
>> +        } else {
>> +            unsigned int my_bkt = c->recv_read - 1;
>> +            vmport_bucket_t *b;
>> +            int stat = 100;
>> +            int slot;
>> +
>> +            if (my_bkt >= VMPORT_MAX_BKTS)
>> +                my_bkt = VMPORT_MAX_BKTS - 1;
>> +            b = &c->recv_bkt[my_bkt];
>> +            b->recv_len = 0;
>> +            slot = b->recv_slot;
>> +            if (slot < 1 || slot > 7)
>> +                slot = 7;
>> +            if ((c->send_len > 2) && ((c->send_buf[0] & 0xffff) == 0x4b4f))
>> +                stat = 3;
>> +            if (getLogMask(hd) & LOG_STATUS)
>> +                gdprintk(XENLOG_DEBUG, "VMware %d,%d(%d) getStatus[%d(%d)]=%d <== %d hex=0x%x\n",
>> +                         c->chan_id, my_bkt, c->recv_read, slot, b->recv_slot,
>> +                         getStatus(hd)[slot], stat, c->send_buf[0] & 0xffff);
>> +            getStatus(hd)[slot] = stat;
>> +        }
>> +    }
>> +}
>> +
>> +void vmport_process_recv_size(struct hvm_domain *hd, vmport_channel_t *c, struct cpu_user_regs *ur)
>> +{
>> +    vmport_bucket_t *b = &c->recv_bkt[c->recv_read];
>> +
>> +    if ((getLogMask(hd) & LOG_RECV_SIZE_ALL) ||
>> +        ((getLogMask(hd) & LOG_RECV_SIZE) && (b->recv_len)))
>> +        gdprintk(XENLOG_DEBUG, "VMware RECVSIZE %d is %d.\n",
>> +                 c->chan_id, b->recv_len);
>> +
>> +    if (b->recv_len) {
>> +        ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_DORECV | MESSAGE_STATUS_SUCCESS);
>> +        ur->edx = setHighBits(ur->edx, MESSAGE_TYPE_SENDSIZE);
>> +        ur->ebx = b->recv_len;
>> +    } else {
>> +        ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +    }
>> +}
>> +
>> +void vmport_process_recv_payload(struct hvm_domain *hd, vmport_channel_t *c, struct cpu_user_regs *ur)
>> +{
>> +    vmport_bucket_t *b = &c->recv_bkt[c->recv_read];
>> +
>> +    if (b->recv_idx < VMPORT_MAX_RECV_BUF) {
>> +        ur->ebx = b->recv_buf[b->recv_idx++];
>> +    } else {
>> +        ur->ebx = 0;
>> +    }
>> +    ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +    ur->edx = setHighBits(ur->edx, MESSAGE_TYPE_SENDPAYLOAD);
>> +}
>> +
>> +void vmport_process_recv_status(struct hvm_domain *hd, vmport_channel_t *c, struct cpu_user_regs *ur)
>> +{
>> +    vmport_bucket_t *b = &c->recv_bkt[c->recv_read];
>> +    char prefix[30];
>> +
>> +    ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +    if (getLogMask(hd) & LOG_RECV_STATUS) {
>> +        snprintf(prefix, sizeof(prefix),
>> +                 "VMware RECVSTATUS %d (%d) ", c->chan_id, c->recv_read);
>> +        prefix[sizeof(prefix)-1] = 0;
>> +        vmport_safe_print(prefix, b->recv_len, (char*)b->recv_buf);
>> +    }
>> +    getStatus(hd)[b->recv_slot] = 2;
>> +    if (getLogMask(hd) & LOG_STATUS)
>> +        gdprintk(XENLOG_DEBUG, "VMware %d,%d getStatus[%d]=2\n",
>> +                 c->chan_id, c->recv_read, b->recv_slot);
>> +    c->recv_read++;
>> +    if (c->recv_read >= VMPORT_MAX_BKTS)
>> +        c->recv_read = 0;
>> +}
>> +
>> +void vmport_process_close(struct hvm_domain *hd, vmport_channel_t *c, struct cpu_user_regs *ur)
>> +{
>> +    // Return channel to free pool
>> +    c->proto_num = 0;
>> +    ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +    if (getLogMask(hd) & LOG_CLOSE)
>> +        gdprintk(XENLOG_DEBUG, "VMware CLOSE %d.\n",
>> +                 c->chan_id);
>> +    if (getLogMask(hd) & LOG_STATUS)
>> +        gdprintk(XENLOG_DEBUG, "VMware %d getStatus[0]=0x%x & ~0x%x=0x%x\n",
>> +                 c->chan_id, getStatus(hd)[0], 1 << c->chan_id, ~(1 << c->chan_id));
>> +    getStatus(hd)[0] &= ~(1 << c->chan_id);
>> +}
>> +
>> +void vmport_process_packet(struct hvm_domain *hd, vmport_channel_t *c,
>> +                           struct cpu_user_regs *ur, int sub_cmd,
>> +                           unsigned long now_time)
>> +{
>> +    c->active_time = now_time;
>> +    switch (sub_cmd) {
>> +    case MESSAGE_TYPE_SENDSIZE:
>> +        vmport_process_send_size(hd, c, ur);
>> +        break;
>> +    case MESSAGE_TYPE_SENDPAYLOAD:
>> +        vmport_process_send_payload(hd, c, ur, now_time);
>> +        break;
>> +
>> +    case MESSAGE_TYPE_RECVSIZE:
>> +        vmport_process_recv_size(hd, c, ur);
>> +        break;
>> +    case MESSAGE_TYPE_RECVPAYLOAD:
>> +        vmport_process_recv_payload(hd, c, ur);
>> +        break;
>> +    case MESSAGE_TYPE_RECVSTATUS:
>> +        vmport_process_recv_status(hd, c, ur);
>> +        break;
>> +
>> +    case MESSAGE_TYPE_CLOSE:
>> +        vmport_process_close(hd, c, ur);
>> +        break;
>> +
>> +    default:
>> +        ur->ecx = 0;
>> +        break;
>> +    }
>> +}
>> +
>> +void vmport_rpc(struct hvm_domain *hd, struct cpu_user_regs *ur)
>> +{
>> +    int sub_cmd = (ur->ecx >> 16) & 0xffff;
>> +    vmport_channel_t *c = NULL;
>> +    uint16_t msg_id;
>> +    uint32_t msg_cookie;
>> +    unsigned long now_time = get_sec();
>> +    long delta = now_time - hd->vmport_data->ping_time;
>> +
>> +    if ( delta >= hd->params[HVM_PARAM_VMPORT_RESET_TIME] ) {
>> +        if (getLogMask(hd) & LOG_PING)
>> +            gdprintk(XENLOG_DEBUG, "VMware ping. delta=%ld\n",
>> +                     delta);
>> +        vmport_ctrl_send(hd, "reset", 7);
>> +    }
>> +    spin_lock(&hd->vmport_lock);
>> +    vmport_sweep(hd, now_time);
>> +    do {
>> +        // Check to see if a new open request is happening...
>> +        if (MESSAGE_TYPE_OPEN == sub_cmd) {
>> +            c = vmport_new_chan(hd->vmport_data, now_time);
>> +            if (NULL == c) {
>> +                if (getLogMask(hd) & LOG_ERROR)
>> +                    gdprintk(XENLOG_ERR, "VMware failed to find a free channel.\n");
>> +                break;
>> +            }
>> +
>> +            // Attach the apropriate protocol the the channel
>> +            c->proto_num = ur->ebx & ~GUESTMSG_FLAG_COOKIE;
>> +            ur->ecx = setHighBits(ur->ecx, MESSAGE_STATUS_SUCCESS);
>> +            ur->edx = setHighBits(ur->edx, c->chan_id);
>> +            ur->edi = getLowBits(c->cookie);
>> +            ur->esi = getHighBits(c->cookie);
>> +            if (getLogMask(hd) & LOG_OPEN)
>> +                gdprintk(XENLOG_DEBUG, "VMware OPEN %d p=%x.\n",
>> +                         c->chan_id, c->proto_num);
>> +            if (getLogMask(hd) & LOG_STATUS)
>> +                gdprintk(XENLOG_DEBUG, "VMware %d getStatus[0]=0x%x | 0x%x\n",
>> +                         c->chan_id, getStatus(hd)[0], 1 << c->chan_id);
>> +            getStatus(hd)[0] |= 1 << c->chan_id;
>> +            if (c->proto_num == 0x4f4c4354) {
>> +                vmport_send(hd, c, "reset", 6);
>> +            }
>> +            break;
>> +        }
>> +
>> +        msg_id = getHighBits(ur->edx);
>> +        msg_cookie = getLowBits(ur->edi) | (ur->esi << 16);
>> +        if (msg_id >= VMPORT_MAX_CHANS) {
>> +            if (getLogMask(hd) & LOG_ERROR)
>> +                gdprintk(XENLOG_ERR, "VMware chan id err %d >= %d.\n",
>> +                         msg_id, VMPORT_MAX_CHANS);
>> +            break;
>> +        }
>> +        c = &hd->vmport_data->chans[msg_id];
>> +        if (!c->proto_num) {
>> +            if (getLogMask(hd) & LOG_ERROR)
>> +                gdprintk(XENLOG_ERR, "VMware chan %d not open.\n",
>> +                         msg_id);
>> +            break;
>> +        }
>> +
>> +        // We check the cookie here since it's possible that the
>> +        // connection timed out on us and another channel was opened
>> +        // if this happens, return error and the um tool will
>> +        // need to reopen the connection
>> +        if (msg_cookie != c->cookie) {
>> +            if (getLogMask(hd) & LOG_ERROR)
>> +                gdprintk(XENLOG_ERR, "VMware cookie err %x vs %x.\n",
>> +                         msg_cookie, c->cookie);
>> +            break;
>> +        }
>> +        vmport_process_packet(hd, c, ur, sub_cmd, now_time);
>> +    } while( 0 );
>> +
>> +    if( NULL == c )
>> +        ur->ecx = setHighBits(ur->ecx, 0);
>> +
>> +    spin_unlock(&hd->vmport_lock);
>> +}
>> +
>> +int vmport_ioport(int dir, int size, unsigned long data, struct cpu_user_regs *regs)
>> +{
>> +    uint32_t cmd = getLowBits(regs->ecx);
>> +    uint32_t magic = regs->eax;
>> +    struct hvm_domain *hd = &current->domain->arch.hvm_domain;
>> +
>> +    if ( dir != IOREQ_WRITE )
>> +        data = 0;
>> +
>> +    if (magic == BDOOR_MAGIC) {
>> +        const uint32_t apicHz = 1000000000L;
> Again this hard coded constant.

Will be looking into how to fix correctly.

>
>> +        uint64_t value;
>> +
>> +        switch (cmd) {
>> +        case BDOOR_CMD_GETMHZ:
>> +            /* ... */
>> +            regs->ebx = BDOOR_MAGIC;
>> +            regs->eax = (uint32_t)(current->domain->arch.tsc_khz / 1000);
>> +            break;
>> +        case BDOOR_CMD_GETVERSION:
>> +            /* ... */
>> +            regs->ebx = BDOOR_MAGIC;
>> +            /* VERSION_MAGIC */
>> +            regs->eax = 6;
>> +            /* Claim we are an ESX. VMX_TYPE_SCALABLE_SERVER */
>> +            regs->ecx = 2;
>> +            break;
>> +        case BDOOR_CMD_GETHWVERSION:
>> +            /* ... */
>> +            regs->ebx = BDOOR_MAGIC;
>> +            /* ?? */
>> +            regs->eax = 0x4;
>> +            break;
>> +        case BDOOR_CMD_GETHZ:
>> +            value = current->domain->arch.tsc_khz * 1000;
>> +            /* apic-frequency (bus speed) */
>> +            regs->ecx = apicHz;
>> +            /* High part of tsc-frequency */
>> +            regs->ebx = (uint32_t)(value >> 32);
>> +            /* Low part of tsc-frequency */
>> +            regs->eax = (uint32_t)value;
>> +            break;
>> +        case BDOOR_CMD_GETTIME:
>> +            value = get_localtime_us(current->domain);
>> +            /* hostUsecs */
>> +            regs->ebx = (uint32_t)(value % 1000000UL);
>> +            /* hostSecs */
>> +            regs->eax = (uint32_t)(value / 1000000ULL);
>> +            /* maxTimeLag */
>> +            regs->ecx = 0;
>> +            break;
>> +        case BDOOR_CMD_GETTIMEFULL:
>> +            value = get_localtime_us(current->domain);
>> +            /* ... */
>> +            regs->eax = BDOOR_MAGIC;
>> +            /* hostUsecs */
>> +            regs->ebx = (uint32_t)(value % 1000000UL);
>> +            /* High part of hostSecs */
>> +            regs->esi = (uint32_t)((value / 1000000ULL) >> 32);
>> +            /* Low part of hostSecs */
>> +            regs->edx = (uint32_t)(value / 1000000ULL);
>> +            /* maxTimeLag */
>> +            regs->ecx = 0;
>> +            break;
>> +        case BDOOR_CMD_MESSAGE:
>> +            vmport_rpc(hd, regs);
>> +            break;
>> +
>> +        default:
>> +            if (getLogMask(hd) & LOG_ERROR)
>> +                gdprintk(XENLOG_DEBUG, "VMware size=%d dir=%d data=%lx cmd=%d.\n",
>> +                         size, dir, data, cmd);
>> +            break;
>> +        }
>> +        if (getLogMask(hd) & LOG_TRACE)
>> +            gdprintk(XENLOG_DEBUG, "VMware ip=%lx cmd=%d ax=%lx bx=%lx cx=%lx dx=%lx si=%lx di=%lx\n",
>> +                     (unsigned long)regs->eip, cmd,
>> +                     (unsigned long)regs->eax, (unsigned long)regs->ebx,
>> +                     (unsigned long)regs->ecx, (unsigned long)regs->edx,
>> +                     (unsigned long)regs->esi, (unsigned long)regs->edi);
>> +    } else
>> +        if (getLogMask(hd) & LOG_ERROR)
>> +            gdprintk(XENLOG_ERR, "Not VMware %x vs %x vs %x; ip=%lx ax=%lx bx=%lx cx=%lx dx=%lx si=%lx di=%lx\n",
>> +                     magic, BDOOR_MAGIC, VMPORT_MAGIC,
>> +                     (unsigned long)regs->eip,
>> +                     (unsigned long)regs->eax, (unsigned long)regs->ebx,
>> +                     (unsigned long)regs->ecx, (unsigned long)regs->edx,
>> +                     (unsigned long)regs->esi, (unsigned long)regs->edi);
>> +
>> +    if (dir == IOREQ_READ)
>> +        HVMTRACE_ND(IOPORT_READ, 0, 1/*cycles*/, 5, VMPORT_PORT, cmd,
>> +                    regs->eax, regs->ebx, regs->ecx, 0);
>> +    else
>> +        HVMTRACE_ND(IOPORT_WRITE, 0, 1/*cycles*/, 5, VMPORT_PORT, cmd,
>> +                    regs->eax, regs->ebx, regs->ecx, 0);
>> +
>> +    return 1;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-set-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> Frankly, this while file is quite nasty.  There is a huge amount of
> pointer arithmetic and array indexing with guest data, which doesn't
> appear at a glance to have sufficient validation.

Will look into make it clearer.

> The vast majority of the ints should be unsigned as they are used as
> indices.  All the times should probably be s_time_t's

Will fix.

> The idiom
>
> if ( getLogMask(hd) & LOG_$SOMETHING )
>      gdprintk(XENLOG_$SOMETHING, ...);
>
> Can probably be moved into a single macro, which will simply the reading
> of the code quite a bit.

Will do.

>
> Is there a spec for what this code is trying to accomplish?  Some of the
> comments suggest that it has been reverse engineered?

I am not aware of such a spec.  I will attempt to find one.  It looks 
like add what I know as a documnet file would be helpful. Some of it has 
been "reverse" engineered in that it comes from decoding the code 
provided by:

http://packages.vmware.com/tools/esx/3.5latest/rhel4/SRPMS/index.html
  open-vm-tools-kmod-7.4.8-396269.423167.src.rpm

And also seeing the debug output of doing some actions using VMware tools.



>
> I think I have counted 4 coding styles in there.  Given the Xen block on
> the bottom, can it settle on a single style.

Sure, I will working on getting this to 1 style.

>
>> diff --git a/xen/arch/x86/hvm/vmport/xen_vmport_def.h b/xen/arch/x86/hvm/vmport/xen_vmport_def.h
>> new file mode 100644
>> index 0000000..e87845b
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmport/xen_vmport_def.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * xen_vmport_def.h: HVM VMPORT emulation
>> + *
>> + *
>> + * Copyright (C) 2012 Verizon Corporation
>> + *
>> + * This file is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License Version 2 (GPLv2)
>> + * as published by the Free Software Foundation.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details. <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __XEN_VMPORT_DEF_H__
>> +#define __XEN_VMPORT_DEF_H__
>> +
>> +#include <xen/config.h>
>> +#include <xen/init.h>
>> +#include <xen/mm.h>
>> +#include <xen/lib.h>
>> +#include <xen/errno.h>
>> +#include <xen/trace.h>
>> +#include <xen/event.h>
>> +#include <xen/hypercall.h>
>> +#include <asm/current.h>
>> +#include <asm/cpufeature.h>
>> +#include <asm/processor.h>
>> +#include <asm/hvm/hvm.h>
>> +#include <asm/hvm/support.h>
>> +#include <asm/hvm/trace.h>
>> +#include <asm/hvm/vmport.h>
> What is the point of this header file?

At some point in the past there was multiple users.  I see that there is 
only one now, and so will be removed.

>
>> +
>> +#endif
>> diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
>> index 9d7e00b..d5c3a3e 100644
>> --- a/xen/include/asm-x86/hvm/trace.h
>> +++ b/xen/include/asm-x86/hvm/trace.h
>> @@ -52,8 +52,11 @@
>>   #define DO_TRC_HVM_LMSW64      DEFAULT_HVM_MISC
>>   #define DO_TRC_HVM_REALMODE_EMULATE DEFAULT_HVM_MISC
>>   #define DO_TRC_HVM_TRAP             DEFAULT_HVM_MISC
>> +#define DO_TRC_HVM_TRAP64           DEFAULT_HVM_MISC
>>   #define DO_TRC_HVM_TRAP_DEBUG       DEFAULT_HVM_MISC
>>   #define DO_TRC_HVM_VLAPIC           DEFAULT_HVM_MISC
>> +#define DO_TRC_HVM_IOPORT_READ      DEFAULT_HVM_IO
>> +#define DO_TRC_HVM_IOPORT_WRITE     DEFAULT_HVM_IO
> There are already traps for io ports, which I believe are part of the
> generic vmexit traps.
>

Looks like the name is not good.  And normal ports to not use or change 
other cpu registers.  These last 2 are used:

     if (dir == IOREQ_READ)
         HVMTRACE_ND(IOPORT_READ, 0, 1/*cycles*/, 5, VMPORT_PORT, cmd,
                     regs->eax, regs->ebx, regs->ecx, 0);
     else
         HVMTRACE_ND(IOPORT_WRITE, 0, 1/*cycles*/, 5, VMPORT_PORT, cmd,
                     regs->eax, regs->ebx, regs->ecx, 0);

I.E. they are VMware backdoor port only traces.  As such, it would be 
good to drop the fixed and duplicated data, and add the missing data.  
I.E. Change them to:


     if (dir == IOREQ_READ)
         HVMTRACE_ND(IOPORT_READ, 0, 1/*cycles*/, 6,
                     regs->eax, regs->ebx, regs->ecx,
                     regs->edx, regs->esi, regs->edi);
     else
         HVMTRACE_ND(IOPORT_WRITE, 0, 1/*cycles*/, 6,
                     regs->eax, regs->ebx, regs->ecx,
                     regs->edx, regs->esi, regs->edi);

With IOPORT_READ changed also to something like VMWARE_PORT_READ.


   -Don Slutz

> ~Andrew
>
>>   
>>   
>>   #define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)