On 18-08-13 16:48:33, Wu, Xiaoguang wrote:

The variable native_assign_ports in struct pci_xhci_vdev is used
to record wether certain root hub port in SOS is assgned to UOS.
The logic is zero reserve and nonzero assignement.

In this patch, the meaning of native_assign_ports is expand to the
following logic:
native_assign_ports[x][y] = 0: native port x-y is not assigned.
native_assign_ports[x][y] < 0: native port x-y is assigned but
currently no virtual device attached.
native_assign_ports[x][y] > 0: native paort x-y is assigned and
virtual device attached to virtual port whose number
is stored in native_assign_ports[x][y].

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index f12126b..2df7ce3 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,16 @@ struct pci_xhci_vdev {
int (*excap_write)(struct pci_xhci_vdev *, uint64_t, uint64_t);
int usb2_port_start;
int usb3_port_start;
- uint8_t *native_assign_ports[USB_NATIVE_NUM_BUS];
+
+ /* native_assign_ports is used to record the assigment information:
+ * native_assign_ports[x][y] = 0: native port x-y is not assigned.
+ * native_assign_ports[x][y] < 0: native port x-y is assigned but
+ * currently no virtual device attached.
+ * native_assign_ports[x][y] > 0: native paort x-y is assigned and

For "= 0" and "< 0", please define some macros...

+ * virtual device attached to virtual port whose number
+ * is stored in native_assign_ports[x][y].
+ */
+ int8_t *native_assign_ports[USB_NATIVE_NUM_BUS];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -3365,14 +3374,14 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)

if (!xdev->native_assign_ports[bus]) {
xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT,
- sizeof(uint8_t));
+ sizeof(int8_t));
if (!xdev->native_assign_ports[bus]) {
rc = -3;
goto errout;
}
}
+ xdev->native_assign_ports[bus][port] = -1;

- xdev->native_assign_ports[bus][port] = 1;
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4

On 18-08-13 16:48:32, Wu, Xiaoguang wrote:

Current design cannot get physical USB device information without
the creation of pci_xhci_dev_emu. This brings some dificulties in

dificulties?

certain situations, hence struct usb_native_devinfo is introduced
to describe neccessary information to solve this trouble.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 31 ++++++++++++-------------------
devicemodel/hw/platform/usb_pmapper.c | 14 +++++++++++++-
devicemodel/include/usb_core.h | 10 ++++++++++
3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 490219a..f12126b 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -469,23 +469,22 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
struct pci_xhci_dev_emu *de;
struct pci_xhci_vdev *xdev;
struct usb_devemu *ue;
+ struct usb_native_devinfo *di;
int port_start, port_end;
int slot_start, slot_end;
int port, slot;
void *ud;
- uint8_t native_bus, native_pid, native_port;
- uint16_t native_vid;
- int native_speed;
int intr = 1;

xdev = hci_data;
+ di = dev_data;

assert(xdev);
assert(dev_data);
assert(xdev->devices);
assert(xdev->slots);

- de = pci_xhci_dev_create(xdev, dev_data);
+ de = pci_xhci_dev_create(xdev, di->priv_data);
if (!de) {
UPRINTF(LFTL, "fail to create device\r\n");
return -1;
@@ -499,26 +498,20 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
assert(ue);

/* print physical information about new device */
- ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(native_bus));
- ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(native_port));
- ue->ue_info(ud, USB_INFO_VID, &native_vid, sizeof(native_vid));
- ue->ue_info(ud, USB_INFO_PID, &native_pid, sizeof(native_pid));
- ue->ue_info(ud, USB_INFO_SPEED, &native_speed, sizeof(native_speed));

Then usb_dev_info is not useful now? Should we clear it?

UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
- native_vid, native_pid, native_bus, native_port);
+ di->vid, di->pid, di->bus, di->port);

- if (!xdev->native_assign_ports[native_bus] ||
- !xdev->native_assign_ports[native_bus][native_port]) {
+ if (!xdev->native_assign_ports[di->bus] ||
+ !xdev->native_assign_ports[di->bus][di->port]) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
- "\r\n", native_vid, native_pid, native_bus,
- native_port);
+ "\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
}

- UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", native_vid,
- native_pid, native_bus, native_port);
+ UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid,
+ di->pid, di->bus, di->port);

- if (ue->ue_usbver == 2)
+ if (di->bcd < 0x300)
port_start = xdev->usb2_port_start;
else
port_start = xdev->usb3_port_start;
@@ -550,11 +543,11 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)

pci_xhci_reset_slot(xdev, slot);
UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
- native_vid, native_pid, native_bus, native_port,
+ di->vid, di->pid, di->bus, di->port,
slot, port);

/* Trigger port change event for the arriving device */
- if (pci_xhci_port_connect(xdev, port, native_speed, intr))
+ if (pci_xhci_port_connect(xdev, port, di->speed, intr))
UPRINTF(LFTL, "fail to report port event\n");

return 0;
diff --git a/devicemodel/hw/platform/usb_pmapper.c b/devicemodel/hw/platform/usb_pmapper.c
index 9f57f1c..a9c8608 100644
--- a/devicemodel/hw/platform/usb_pmapper.c
+++ b/devicemodel/hw/platform/usb_pmapper.c
@@ -1020,6 +1020,9 @@ static int
usb_dev_native_sys_conn_cb(struct libusb_context *ctx, struct libusb_device
*ldev, libusb_hotplug_event event, void *pdata)
{
+ struct libusb_device_descriptor d;
+ struct usb_native_devinfo di;
+
UPRINTF(LDBG, "connect event\r\n");

if (!ctx || !ldev) {
@@ -1027,8 +1030,17 @@ usb_dev_native_sys_conn_cb(struct libusb_context *ctx, struct libusb_device
return -1;
}

+ libusb_get_device_descriptor(ldev, &d);
+ di.bus = libusb_get_bus_number(ldev);
+ di.speed = libusb_get_device_speed(ldev);
+ di.port = libusb_get_port_number(ldev);
+ di.pid = d.idProduct;
+ di.vid = d.idVendor;
+ di.bcd = d.bcdUSB;
+ di.priv_data = ldev;
+
if (g_ctx.conn_cb)
- g_ctx.conn_cb(g_ctx.hci_data, ldev);
+ g_ctx.conn_cb(g_ctx.hci_data, &di);

return 0;
}
diff --git a/devicemodel/include/usb_core.h b/devicemodel/include/usb_core.h
index 3a9648a..8727b8b 100644
--- a/devicemodel/include/usb_core.h
+++ b/devicemodel/include/usb_core.h
@@ -163,6 +163,16 @@ struct usb_data_xfer {
pthread_mutex_t mtx;
};

+struct usb_native_devinfo {
+ int speed;
+ uint8_t bus;
+ uint8_t port;
+ uint16_t bcd;
+ uint16_t pid;
+ uint16_t vid;
+ void *priv_data;
+};

This new structure is confused to me. There has already one structure
struct usb_dev which included native usb device information. Why define
a new one? Should we refine the struct usb_dev to replace these native
description members?

+
enum USB_ERRCODE {
USB_ACK,
USB_NAK,
--
2.7.4

When running longevity test and capturing acrntrace, generated acrntrace
files sizes are too big.
Sometimes we don't care very old trace. This patch adds ring buffer
mode, fixes acrntrace file size and overwrites the oldest trace with new
trace when the buffer is full.

v2:
- update README.rst

Signed-off-by: Zhipeng Gong <zhipeng.gong@...>
---
tools/acrntrace/README.rst | 4 ++--
tools/acrntrace/acrntrace.c | 44 +++++++++++++++++++++++++++++++++++++++++---
tools/acrntrace/acrntrace.h | 1 +
3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/tools/acrntrace/README.rst b/tools/acrntrace/README.rst
index 433f161..f25c5b0 100644
--- a/tools/acrntrace/README.rst
+++ b/tools/acrntrace/README.rst
@@ -21,8 +21,8 @@ Options:
-i period specify polling interval in milliseconds [1-999]
-t max_time max time to capture trace data (in second)
-c clear the buffered old data
--r free_space amount of free space (in MB) remaining on the disk
- before acrntrace stops
+-r ring_buffer_size ring buffer size (in MB), which enables ring buffer
+ mode and overwrites old trace with new trace when full

The ``acrntrace_format.py`` is a offline tool for parsing trace data (as output
by acrntrace) to human-readable formats based on given format.
diff --git a/tools/acrntrace/acrntrace.c b/tools/acrntrace/acrntrace.c
index 39fda35..e4b3899 100644
--- a/tools/acrntrace/acrntrace.c
+++ b/tools/acrntrace/acrntrace.c
@@ -37,16 +37,19 @@ static char trace_file_dir[TRACE_FILE_DIR_LEN];

static reader_struct *reader;
static int pcpu_num = 0;
+static int ring_buffer_size = 0;

static void display_usage(void)
{
printf("acrntrace - tool to collect ACRN trace data\n"
- "[Usage] acrntrace [-i] [period in msec] [-ch]\n\n"
+ "[Usage] acrntrace [-i] [period in msec] [-r] [ring buffer size] [-ch]\n\n"
"[Options]\n"
"\t-h: print this message\n"
"\t-i: period_in_ms: specify polling interval [1-999]\n"
"\t-t: max time to capture trace data (in second)\n"
- "\t-c: clear the buffered old data\n");
+ "\t-c: clear the buffered old data\n"
+ "\t-r: ring_buffer_size (in MB), which enables ring buffer mode"
+ " and overwrites old trace with new trace when full\n");
}

static void timer_handler(union sigval sv)
@@ -113,6 +116,15 @@ static int parse_opt(int argc, char *argv[])
timeout = ret;
pr_dbg("Capture trace data for at most %ds\n", ret);
break;
+ case 'r':
+ ret = atoi(optarg);
+ if (ret <= 0) {
+ pr_err("'-r' require integer greater than 0\n");
+ return -EINVAL;
+ }
+ ring_buffer_size = ret * 1024 * 1024;
+ pr_dbg("Ring buffer size is %dM\n", ret);
+ break;
case 'c':
flags |= FLAG_CLEAR_BUF;
break;
@@ -206,6 +218,15 @@ static void reader_fn(param_t * param)
trace_ev_t e;
struct statvfs stat;
uint64_t freespace;
+ int pos = 0;
+
+ if (ring_buffer_size != 0) {
+ param->buffer = malloc(ring_buffer_size);
+ if (!param->buffer) {
+ perror("Failed to allocate ring buffer\n");
+ return;
+ }
+ }

pr_dbg("reader thread[%lu] created for FILE*[0x%p]\n",
pthread_self(), fp);
@@ -219,7 +240,13 @@ static void reader_fn(param_t * param)

while (1) {
do {
- ret = sbuf_write(fd, sbuf);
+ if (ring_buffer_size != 0) {
+ ret = sbuf_get(sbuf, param->buffer + pos);
+ pos += sbuf->ele_size;
+ if (pos + sbuf->ele_size > ring_buffer_size)
+ pos = 0;
+ } else
+ ret = sbuf_write(fd, sbuf);
} while (ret > 0);

usleep(period);
@@ -284,6 +311,17 @@ static void destory_reader(reader_struct * reader)
reader->thrd = 0;
}

+ if (ring_buffer_size != 0 && reader->param.buffer) {
+ int ret;
+
+ ret = write(reader->param.trace_fd, reader->param.buffer,
+ ring_buffer_size);
+ if (ret != ring_buffer_size) {
+ perror("Failed to write ring buffer\n");
+ }
+ free(reader->param.buffer);
+ }
+
if (reader->param.sbuf) {
munmap(reader->param.sbuf, MMAP_SIZE);
reader->param.sbuf = NULL;
diff --git a/tools/acrntrace/acrntrace.h b/tools/acrntrace/acrntrace.h
index ea54f4d..6615157 100644
--- a/tools/acrntrace/acrntrace.h
+++ b/tools/acrntrace/acrntrace.h
@@ -76,6 +76,7 @@ typedef struct {
int trace_fd;
shared_buf_t *sbuf;
pthread_mutex_t *sbuf_lock;
+ uint8_t *buffer;
} param_t;

typedef struct {
--
2.7.4

On 18-08-13 16:48:31, Wu, Xiaoguang wrote:

PORTSC (Port Status and Control Register) register play a very
important role in USB sub-system. This patch is used to refine
related manipulation functions.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 76 ++++++++++++++++++++++-------------------------
1 file changed, 35 insertions(+), 41 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 792d930..490219a 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -439,11 +439,14 @@ static void pci_xhci_update_ep_ring(struct pci_xhci_vdev *xdev,
struct xhci_endp_ctx *ep_ctx,
uint32_t streamid, uint64_t ringaddr,
int ccs);
-static void pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn);
+static void pci_xhci_port_init(struct pci_xhci_vdev *xdev, int portn);
+static int pci_xhci_port_connect(struct pci_xhci_vdev *xdev, int port,
+ int usb_speed, int intr);
+static int pci_xhci_port_disconnect(struct pci_xhci_vdev *xdev, int port,
+ int intr);
static struct pci_xhci_dev_emu *pci_xhci_dev_create(struct pci_xhci_vdev *
xdev, void *dev_data);
static void pci_xhci_dev_destroy(struct pci_xhci_dev_emu *de);
-static int pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn);
static void pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port,
uint32_t errcode, uint32_t evtype);
static int pci_xhci_xfer_complete(struct pci_xhci_vdev *xdev,
@@ -472,6 +475,8 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
void *ud;
uint8_t native_bus, native_pid, native_port;
uint16_t native_vid;
+ int native_speed;
+ int intr = 1;

Can we remove intr variable? Is hasn't changed in this function and pass
to pci_xhci_port_disconnect directly..

xdev = hci_data;

@@ -498,6 +503,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(native_port));
ue->ue_info(ud, USB_INFO_VID, &native_vid, sizeof(native_vid));
ue->ue_info(ud, USB_INFO_PID, &native_pid, sizeof(native_pid));
+ ue->ue_info(ud, USB_INFO_SPEED, &native_speed, sizeof(native_speed));
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
native_vid, native_pid, native_bus, native_port);

@@ -543,14 +549,12 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
xdev->ndevices++;

pci_xhci_reset_slot(xdev, slot);
- pci_xhci_init_port(xdev, port);
-
UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
native_vid, native_pid, native_bus, native_port,
slot, port);

/* Trigger port change event for the arriving device */
- if (pci_xhci_port_chg(xdev, port, 1))
+ if (pci_xhci_port_connect(xdev, port, native_speed, intr))
UPRINTF(LFTL, "fail to report port event\n");

return 0;
@@ -566,6 +570,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
struct pci_xhci_dev_emu *edev;
struct usb_dev *udev;
uint8_t port, native_port;
+ int intr = 1;

Ditto.

assert(hci_data);
assert(dev_data);
@@ -595,7 +600,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
}

UPRINTF(LDBG, "report virtual port %d status\r\n", port);
- if (pci_xhci_port_chg(xdev, port, 0)) {
+ if (pci_xhci_port_disconnect(xdev, port, intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
return -1;
}
@@ -784,32 +789,29 @@ pci_xhci_convert_speed(int lspeed)
}

static int
-pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn)
+pci_xhci_port_change(struct pci_xhci_vdev *xdev, int port, int usb_speed,
+ int conn, int intr)
{
int speed, error;
struct xhci_trb evtrb;
struct pci_xhci_portregs *reg;
- struct pci_xhci_dev_emu *dev;

assert(xdev != NULL);

reg = XHCI_PORTREG_PTR(xdev, port);
- dev = XHCI_DEVINST_PTR(xdev, port);
- if (!dev || !dev->dev_ue || !reg) {
- UPRINTF(LWRN, "find nullptr with port %d\r\n", port);
- return -1;
- }
-
if (conn == 0) {
reg->portsc &= ~XHCI_PS_CCS;
reg->portsc |= (XHCI_PS_CSC |
XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET));
} else {
- speed = pci_xhci_convert_speed(dev->dev_ue->ue_usbspeed);
+ speed = pci_xhci_convert_speed(usb_speed);
reg->portsc = XHCI_PS_CCS | XHCI_PS_PP | XHCI_PS_CSC;
reg->portsc |= XHCI_PS_SPEED_SET(speed);
}

+ if (!intr)
+ return 0;

What is mean of intr? From this patch, it almost useless..

+
/* make an event for the guest OS */
pci_xhci_set_evtrb(&evtrb,
port,
@@ -825,6 +827,20 @@ pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn)
return (error == XHCI_TRB_ERROR_SUCCESS) ? 0 : -1;
}

+static int
+pci_xhci_port_connect(struct pci_xhci_vdev *xdev, int port, int usb_speed,
+ int intr)
+{
+ return pci_xhci_port_change(xdev, port, usb_speed, 1, intr);
+}
+
+static int
+pci_xhci_port_disconnect(struct pci_xhci_vdev *xdev, int port, int intr)
+{
+ /* for disconnect, the speed is useless */
+ return pci_xhci_port_change(xdev, port, 0, 0, intr);
+}
+
static void
pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port, uint32_t errcode,
uint32_t evtype)
@@ -3206,32 +3222,10 @@ pci_xhci_reset_port(struct pci_xhci_vdev *xdev, int portn, int warm)
}

static void
-pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn)
+pci_xhci_port_init(struct pci_xhci_vdev *xdev, int portn)

Let's keep pci_xhci_init_port..

{
- struct pci_xhci_portregs *port;
- struct pci_xhci_dev_emu *dev;
-
- port = XHCI_PORTREG_PTR(xdev, portn);
- dev = XHCI_DEVINST_PTR(xdev, portn);
- if (dev) {
- port->portsc = XHCI_PS_CCS | /* connected */
- XHCI_PS_PP; /* port power */
-
- if (dev->dev_ue->ue_usbver == 2) {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_POLL) |
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- } else {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_U0) |
- XHCI_PS_PED | /* enabled */
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- }
-
- UPRINTF(LDBG, "Init port %d 0x%x\n", portn, port->portsc);
- } else {
- port->portsc = XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
- UPRINTF(LDBG, "Init empty port %d 0x%x\n",
- portn, port->portsc);
- }
+ XHCI_PORTREG_PTR(xdev, portn)->portsc =
+ XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
}

static int
@@ -3567,7 +3561,7 @@ pci_xhci_parse_opts(struct pci_xhci_vdev *xdev, char *opts)

/* do not use the zero index element */
for (i = 1; i <= XHCI_MAX_DEVS; i++)
- pci_xhci_init_port(xdev, i);
+ pci_xhci_port_init(xdev, i);

errout:
if (rc) {
--
2.7.4

Thanks!

@David, can you take a look at this when you get a chance?

Patch v3 have been confirmed by local doc building system. I put the
"images/dot files" into conf.rst, since the doc building system collects only rst
files to doc directory.

Thanks,
Geoffroy

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Liu xinwu
Sent: Monday, August 13, 2018 4:38 AM
To: VanCutsem, Geoffroy <geoffroy.vancutsem@...>; acrn-
dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di <di.zhang@...>;
Jin, Zhi <zhi.jin@...>; Liu, Xiaojing <xiaojing.liu@...>
Subject: Re: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

I send out the patch V3


On 08/10/2018 03:04 PM, VanCutsem, Geoffroy wrote:

Just a couple of nitpicks below then you can move forward with my ACK.

I'm sure David will have suggestions for you but these are best addressed
directly in the PR phase.

Corrected.

I'm assuming you have built that documentation locally and verified it, can

you just confirm?

Patch v3 have been confirmed by local doc building system. I put the
"images/dot files" into conf.rst, since the doc building system collects only rst
files to doc directory.

Thanks,
Geoffroy

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Liu xinwu
Sent: Thursday, August 2, 2018 8:23 AM
To: acrn-dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di
<di.zhang@...>; Liu, Xinwu <xinwu.liu@...>; Jin, Zhi
<zhi.jin@...>; Liu, Xiaojing <xiaojing.liu@...>;
VanCutsem, Geoffroy <geoffroy.vancutsem@...>
Subject: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

This document explains all feilds in acrnprobe.xml.

s/fields/fields

User could use it to control acrnprobe's behavior and configure their
own events.

Signed-off-by: Liu, Xinwu <xinwu.liu@...>
---

v1->v2:
1. refer conf.rst from README.rst
2. drawing by graphviz instead of raw text.

tools/acrn-crashlog/acrnprobe/README.rst | 2 +
tools/acrn-crashlog/acrnprobe/conf.rst | 337
+++++++++++++++++++++
.../acrn-crashlog/acrnprobe/images/build-crash.dot | 21 ++
.../acrn- crashlog/acrnprobe/images/crash-match.dot | 25 ++
4 files changed, 385 insertions(+)
create mode 100644 tools/acrn-crashlog/acrnprobe/conf.rst
create mode 100644 tools/acrn-crashlog/acrnprobe/images/build-

crash.dot

create mode 100644 tools/acrn-crashlog/acrnprobe/images/crash-
match.dot

diff --git a/tools/acrn-crashlog/acrnprobe/README.rst b/tools/acrn-
crashlog/acrnprobe/README.rst index fa137f3..5f537ae 100644
--- a/tools/acrn-crashlog/acrnprobe/README.rst
+++ b/tools/acrn-crashlog/acrnprobe/README.rst
@@ -175,4 +175,6 @@ Configuration files

Custom configuration file that ``acrnprobe`` reads.

+More details about configuration file, please refer to :ref:`acrnprobe-

conf`.

+
.. _`telemetrics-client`:
https://github.com/clearlinux/telemetrics-client
diff --git a/tools/acrn-crashlog/acrnprobe/conf.rst b/tools/acrn-
crashlog/acrnprobe/conf.rst new file mode 100644 index
0000000..7d8ec40
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/conf.rst
@@ -0,0 +1,337 @@
+.. _acrnprobe-conf:
+
+Configuration Of Acrnprobe
+##########################
+
+Description
+***********
+``Acrnprobe`` uses XML as the format of its configuration file,
+namely ``acrnprobe.xml``, so the configuration file should reach the
+`XML standard`_ at least.
+
+Layout
+******
+
+.. code-block:: xml
+
+ <?xml version="1.0" encoding="UTF-8"?>
+ <conf>
+ Root node of configuration.
+
+ <senders>
+ Configuration section of senders.
+ </senders>
+
+ <triggers>
+ Configuration section of triggers.
+ </triggers>
+
+ <vms>
+ Configuration section of virtual machines.
+ </vms>
+
+ <logs>
+ Configuration section of logs.
+ </logs>
+
+ <crashes>
+ Configuration section of crashes.
+ Note that this section should be configured after triggers and logs, as
+ crashes depend on these two sections.
+ </crashes>
+
+ <infos>
+ Configuration section of infos.
+ Note that this section should be configured after triggers and logs, as
+ infos depend on these two sections.
+ </infos>
+
+ </conf>
+
+As for the definition of ``sender``, ``trigger``, ``crash`` and
+``info`` and information of supported ``vm``, please refer to
:ref:`acrnprobe_doc`.
+
+Properties of modules
+*********************
+
+Common properties
+=================
+
+- ``id``:
+ The index, which grows from 1, in its group.
+- ``enable``:
+ This part of configuration can only take effect when the value is ``true``.
+
+Other properties
+================
+
+- ``inherit``:
+ Specify a parent for a certain crash.
+ The child crash will inherit all configurations from the specified
+(by id)
+ crash. These inherited configurations could be overwritten by new

ones.

+ Also, this property helps build the crash tree in ``acrnprobe``.
+- ``expression``:
+ See `Crash`_.
+
+Crash tree in acrnprobe
+***********************
+
+There could be a parent-child relationship between crashes. Refer to
+the diagrams below, crash B and D are the children of crash A,
+because crash B and D inherit from crash A, and crash C is the child of

crash B.

+
+Build crash tree in configuration
+=================================
+
+.. graphviz:: images/build-crash.dot
+ :name: build-crash
+ :align: center
+ :caption: Build crash tree in configuration
+
+Crash type matches at runtime
+=============================
+
+In order to find a more specific type, if one crash type matches
+successfully ``acrnprobe`` will do match for each child of it (if it
+has) continually, and return the last successful one.
+Supposing these crash trees are like the diagram above at runtime:
+If a crash E is triggered, crash E will be returned immediately.
+If a crash A is triggered, then the candidates are crash A, B, C and D.
+The following diagram describes what ``acrnprobe`` will do if the
+matched result is crash D.
+
+.. graphviz:: images/crash-match.dot
+ :name: crash-match
+ :align: center
+ :caption: Crash type matches at runtime
+
+Sections
+********
+
+Sender
+======
+
+Example:
+
+.. code-block:: xml
+
+ <sender id="1" enable="true">
+ <name>crashlog</name>
+ <outdir>/var/log/crashlog</outdir>
+ <maxcrashdirs>1000</maxcrashdirs>
+ <maxlines>5000</maxlines>
+ <spacequota>90</spacequota>
+ <uptime>
+ <name>UPTIME</name>
+ <frequency>5</frequency>
+ <eventhours>6</eventhours>
+ </uptime>
+ </sender>
+
+* ``name``:
+ Name of sender. ``Acrnprobe`` uses this label to distinguish
+different
+ senders.
+ More information about sender, please refer to :ref:`acrnprobe_doc`.
+* ``outdir``:
+ Directory to store generated files of sender. ``Acrnprobe`` will
+create
+ this directory if it doesn't exist.
+* ``maxcrashdirs``:
+ The maximum serial number of generated ``crash directories``,
+``stat directories``
+ and ``vmevent directories``. The serial number will back to 0 if
+it reaches
+ the maximum. Only used by sender crashlog.
+* ``maxlines``:
+ The maximum lines of file ``history_event``. It will trigger
+action
+ ``"mv history_event history_event.bak"`` that the lines of
+``history_event``
+ reaches the ``maxlines``.
+* ``spacequota``:
+ ``Acrnprobe`` will stop collecting logs if it is true that
+ ``(used space / total space) * 100 > spacequota``. Only used by
+sender
+ crashlog.
+* ``uptime``:
+ Configuration to trigger ``UPTIME`` event.
+ sub-nodes:
+
+ + ``name``:
+ The name of event.
+ + ``frequency``:
+ Time interval in seconds to trigger ``uptime`` event.
+ + ``eventhours``:
+ Time interval in hours to generate a record.
+
+
+Trigger
+=======
+
+Example:
+
+.. code-block:: xml
+
+ <trigger id="1" enable="true">
+ <name>t_pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </trigger>
+
+* ``name``:
+ The name of trigger. It's used by crash and info configuration module.
+* ``type`` and ``path``:
+ These two labels are used to get the content of trigger files.
+ ``Types`` have been implemented:
+
+ + ``node``:
+ ``Path`` can not support ``mmap(2)-like`` operations.
+ ``Acrnprobe`` can

"cannot" in one word.

+ only use ``read(2)`` to get its content.
+ + ``file``:
+ ``Path`` is a regular file which supports ``mmap(2)-like`` operations.
+ + ``dir``:
+ ``Path`` is a directory.
+ + ``rebootreason``:
+ The reboot reason of system. The content of ``rebootreason`` is not
+ obtained in a common way. So, it doesn't work with ``path``.
+ + ``cmd``:
+ ``Path`` is a command which will be launched by ``execvp(3)``.
+
+ Some programs often use format ``string%d`` instead of static file
+ name to generate target file dynamically. So ``path`` supports
+ simple formats for these cases:
+
+ + /.../dir/string[*] --> all files with prefix "string" under dir.
+ + /.../dir/string[0] --> the first file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+ + /.../dir/string[-1] --> the last file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+
+Vm
+==
+
+Example:
+
+.. code-block:: xml
+
+ <vm id="1" enable="true">
+ <name>VM1</name>
+ <channel>polling</channel>
+ <interval>60</interval>
+ <syncevent id="1">CRASH/TOMBSTONE</syncevent>
+ <syncevent id="2">CRASH/UIWDT</syncevent>
+ <syncevent id="3">CRASH/IPANIC</syncevent>
+ <syncevent id="4">REBOOT</syncevent>
+ </vm>
+
+* ``name``:
+ The name of virtual machine.
+* ``channel``:
+ The ``channel`` name to get the virtual machine events.
+* ``interval``:
+ Time interval in seconds of polling vm's image.
+* ``syncevent``:
+ Event type ``acrnprobe`` will synchronize from virtual machine's
``crashlog``.
+ User could specify different types by id. The event type can also
+ be indicated by ``type/subtype``.
+
+Log
+===
+
+Example:
+
+.. code-block:: xml
+
+ <log id="1" enable="true">
+ <name>pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </log>
+
+* ``name``:
+ By default, ``acrnprobe`` will take this ``name`` as generated
+log's name in
+ ``outdir`` of sender crashlog.
+ If ``path`` is specified by simple formats (includes [*], [0] or
+[-1]) the
+ file name of generated logs will be the same as original. More
+details about
+ simple formats, see `Trigger`_.
+* ``type`` and ``path``:
+ Same as `Trigger`_.
+* ``lines``:
+ By default, all contents in the original will be copied to generated log.
+ If this label is configured, only the ``lines`` at the end in the
+original
+ will be copied to the generated log. It takes effect only when the
+``type`` is
+ ``file``.
+
+Crash
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <crash id='1' inherit='0' enable='true'>
+ <name>UNKNOWN</name>
+ <trigger>t_rebootreason</trigger>
+ <channel>oneshot</channel>
+ <content id='1'>WARM</content>
+ <log id='1'>pstore</log>
+ <log id='2'>acrnlog_last</log>
+ </crash>
+ <crash id='2' inherit='1' enable='true'>
+ <name>IPANIC</name>
+ <trigger>t_pstore</trigger>
+ <content id='1'> </content>
+ <mightcontent expression='1' id='1'>Kernel panic - not
syncing:</mightcontent>
+ <mightcontent expression='1' id='2'>BUG: unable to handle
kernel</mightcontent>
+ <data id='1'>kernel BUG at</data>
+ <data id='2'>EIP is at</data>
+ <data id='3'>Comm:</data>
+ </crash>
+
+* ``name``:
+ The type of the ``crash``.
+* ``trigger``:
+ The trigger name of the crash.
+* ``channel``:
+ The name of channel crash use.
+* ``content`` and ``mightcontent``:
+ They're used to match crash type. The match is successful if all
+the
+ following conditions are met:
+
+ a. All ``contents`` with different ``ids`` are included in trigger's
+ content.
+ b. One of ``mightcontents`` with the same ``expression`` is included in
+ trigger's content at least.
+ c. If there are ``mightcontents`` with different ``expressions``, each

group

+ with the same ``expression`` should meet condition b.
+* ``log``:
+ The log to be collected. The value is the configured ``name`` in log

module.

+ User could specify different logs by ``id``.
+* ``data``:
+ It is used to generated ``DATA`` fields in ``crashfile``.
+``Acrnprobe`` will
+ copy the line which starts with configured ``data`` in trigger's
+content
+ to ``DATA`` fields. There are 3 fields in ``crashfile`` and they
+could be
+ specified by ``id`` 1, 2, 3.
+
+Info
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <info id='1' enable='true'>
+ <name>BOOT_LOGS</name>
+ <trigger>t_boot</trigger>
+ <channel>oneshot</channel>
+ <log id='1'>kmsg</log>
+ <log id='2'>cmdline</log>
+ <log id='3'>acrnlog_cur</log>
+ <log id='4'>acrnlog_last</log>
+ </info>
+
+* ``name``:
+ The type of info.
+* ``trigger``:
+ The trigger name of the info.
+* ``channel``:
+ The name of channel info use.
+* ``log``:
+ The log to be collected. The value is the configured name in log
+module. User
+ could specify different logs by id.
+
+.. _`XML standard`: http://www.w3.org/TR/REC-xml
diff --git a/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
b/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
new file mode 100644
index 0000000..08e7628
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
@@ -0,0 +1,21 @@
+digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ c5 [ label="crash E\nid 5\ncrash root\ncrash leaf" ];
+ { rank = same; "level 1"; c1; c5;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;color="transparent";];
+ "None" -> {c1 c5} [ label="inherit 0" ];
+ c1 -> {c2 c4} [ label="inherit 1" ];
+ c2 -> c3 [ label="inherit 2" ];
+}
diff --git a/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
b/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
new file mode 100644
index 0000000..7da5ce0
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
@@ -0,0 +1,25 @@
+digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ { rank = same; "level 1"; c1;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;style="rounded,dashed";];
+ exp1 [ label="crash B matches fail\nmatch for the next child\nof crash

A"];

+ exp2 [ label="crash D matches successfully\nreturn crash D"];
+
+ node [shape=box;style="invis";];
+ "channel" -> c1 [ label="trigger" ]
+ c1 -> {exp1 exp2}
+ exp1 -> c2 -> c3 [ style=dashed dir=none]
+ exp2 -> c4
+}
--
2.7.4

On 08-13 Mon 10:01, Eddie Dong wrote:

In general we should do in different patch for bug fix. But this one is simple for those compile option --- this looks like MISRAC patches, we can do together to save

Got it.

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Kaige Fu
Sent: Monday, August 13, 2018 5:18 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH] HV: hv_main: Add #ifdef HV_DEBUG before
debug specific code

On 08-13 Mon 08:27, Eddie Dong wrote:

On 08-13 Mon 07:59, Eddie Dong wrote:

This is part of SBUF component, what kind of compile option do we

use?

Let us use same one.

shell command "vmexit" is the only one user of vmexit_time/cnt and
we use the get_vmexit_profile, enclosed by HV_DEBUG, to access these

variables.

Ok then.

BTW, hcall side also needs this kind of compile option.

I can't follow here. Can you make it more clear.

The sbuf has a hypercall, we need compile option to cover all sbuf code...

Absolutely, we should do it. This should be done in another patch, right?

junjunshan1 <junjun.shan@...> writes:

MISARC has requirements about Marco redefinition, usage of ++ or -- and assignment operator in boolean expression.
This patch is used to solve these violations.

The modifications are summarized as following:
1.The HC_VM_SET_MEMORY_REGION, HC_VM_GPA2HPA, HC_VM_SET_MEMORY_REGIONS are redefined twice in acrn_hv_des.h, so delete them to solve the macro redefinition violations.
2.The macro BUS_LOCK are redefined in bits.h and atomic.h, then delete the declaration in both two files, add a new declaration in cpu.h and include the header file.
3.modify the code to deprecate the usage of -- operators in string.c.
4.modify the while loop to for loop to avoid assignment operator in
boolean expression in vlapic.c.

Some general conventions about commit logs:

1. Each line should be no more than 80 characters. Break the
paragraph if it is longer than that.

2. Due to the first rule, there typically is an empty line between
adjacent paragraphs/items.

Signed-off-by: junjunshan1 <junjun.shan@...>

Reviewed-by: Junjie Mao <junjie.mao@...>

With a minor style issue below.

Please also set your name properly by executing 'git config --global
user.name <your name>'.

---
hypervisor/arch/x86/guest/vlapic.c | 3 ++-
hypervisor/include/arch/x86/cpu.h | 2 ++
hypervisor/include/lib/atomic.h | 3 +--
hypervisor/include/lib/bits.h | 3 +--
hypervisor/include/public/acrn_hv_defs.h | 3 ---
hypervisor/lib/string.c | 6 ++++--
6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c b/hypervisor/arch/x86/guest/vlapic.c
index 35be79c..72acb76 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -1135,7 +1135,8 @@ vlapic_icrlo_write_handler(struct acrn_vlapic *vlapic)
break;
}

- while ((vcpu_id = ffs64(dmask)) != INVALID_BIT_INDEX) {
+ for (vcpu_id = ffs64(dmask); vcpu_id != INVALID_BIT_INDEX;
+ vcpu_id = ffs64(dmask)) {
bitmap_clear_lock(vcpu_id, &dmask);
target_vcpu = vcpu_from_vid(vlapic->vm, vcpu_id);
if (target_vcpu == NULL) {
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index e56f443..d24228d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -152,6 +152,8 @@

#ifndef ASSEMBLER

+#define BUS_LOCK "lock ; "
+
/**
*
* Identifiers for architecturally defined registers.
diff --git a/hypervisor/include/lib/atomic.h b/hypervisor/include/lib/atomic.h
index 264917c..6085863 100644
--- a/hypervisor/include/lib/atomic.h
+++ b/hypervisor/include/lib/atomic.h
@@ -29,8 +29,7 @@

#ifndef ATOMIC_H
#define ATOMIC_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>

#define build_atomic_load(name, size, type, ptr) \
static inline type name(const volatile type *ptr) \
diff --git a/hypervisor/include/lib/bits.h b/hypervisor/include/lib/bits.h
index 04a5057..ea0e8a1 100644
--- a/hypervisor/include/lib/bits.h
+++ b/hypervisor/include/lib/bits.h
@@ -29,8 +29,7 @@

#ifndef BITS_H
#define BITS_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>
/**
*
* INVALID_BIT_INDEX means when input paramter is zero,
diff --git a/hypervisor/include/public/acrn_hv_defs.h b/hypervisor/include/public/acrn_hv_defs.h
index cfeb834..700ce92 100644
--- a/hypervisor/include/public/acrn_hv_defs.h
+++ b/hypervisor/include/public/acrn_hv_defs.h
@@ -55,9 +55,6 @@
#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)
#define HC_VM_WRITE_PROTECT_PAGE BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x03UL)
-#define HC_VM_SET_MEMORY_REGION BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x00UL)
-#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
-#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)

/* PCI assignment*/
#define HC_ID_PCI_BASE 0x50UL
diff --git a/hypervisor/lib/string.c b/hypervisor/lib/string.c
index db4f145..7d2a5a5 100644
--- a/hypervisor/lib/string.c
+++ b/hypervisor/lib/string.c
@@ -210,7 +210,8 @@ char *strcpy_s(char *d_arg, size_t dmax, const char *s_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;

A single space is required between a binary operator and either of its
operand per our coding style.

+ *d = '\0';
return NULL;
}

@@ -293,7 +294,8 @@ char *strncpy_s(char *d_arg, size_t dmax, const char *s_arg, size_t slen_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;

Ditto.

--
Best Regards
Junjie Mao

+ *d = '\0';
return NULL;
}

MISARC has requirements about Marco redefinition, usage of ++ or -- and assignment operator in boolean expression.
This patch is used to solve these violations.

The modifications are summarized as following:
1.The HC_VM_SET_MEMORY_REGION, HC_VM_GPA2HPA, HC_VM_SET_MEMORY_REGIONS are redefined twice in acrn_hv_des.h, so delete them to solve the macro redefinition violations.
2.The macro BUS_LOCK are redefined in bits.h and atomic.h, then delete the declaration in both two files, add a new declaration in cpu.h and include the header file.
3.modify the code to deprecate the usage of -- operators in string.c.
4.modify the while loop to for loop to avoid assignment operator in boolean expression in vlapic.c.

Signed-off-by: junjunshan1 <junjun.shan@...>
---
hypervisor/arch/x86/guest/vlapic.c | 3 ++-
hypervisor/include/arch/x86/cpu.h | 2 ++
hypervisor/include/lib/atomic.h | 3 +--
hypervisor/include/lib/bits.h | 3 +--
hypervisor/include/public/acrn_hv_defs.h | 3 ---
hypervisor/lib/string.c | 6 ++++--
6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c b/hypervisor/arch/x86/guest/vlapic.c
index 35be79c..72acb76 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -1135,7 +1135,8 @@ vlapic_icrlo_write_handler(struct acrn_vlapic *vlapic)
break;
}

- while ((vcpu_id = ffs64(dmask)) != INVALID_BIT_INDEX) {
+ for (vcpu_id = ffs64(dmask); vcpu_id != INVALID_BIT_INDEX;
+ vcpu_id = ffs64(dmask)) {
bitmap_clear_lock(vcpu_id, &dmask);
target_vcpu = vcpu_from_vid(vlapic->vm, vcpu_id);
if (target_vcpu == NULL) {
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index e56f443..d24228d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -152,6 +152,8 @@

#ifndef ASSEMBLER

+#define BUS_LOCK "lock ; "
+
/**
*
* Identifiers for architecturally defined registers.
diff --git a/hypervisor/include/lib/atomic.h b/hypervisor/include/lib/atomic.h
index 264917c..6085863 100644
--- a/hypervisor/include/lib/atomic.h
+++ b/hypervisor/include/lib/atomic.h
@@ -29,8 +29,7 @@

#ifndef ATOMIC_H
#define ATOMIC_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>

#define build_atomic_load(name, size, type, ptr) \
static inline type name(const volatile type *ptr) \
diff --git a/hypervisor/include/lib/bits.h b/hypervisor/include/lib/bits.h
index 04a5057..ea0e8a1 100644
--- a/hypervisor/include/lib/bits.h
+++ b/hypervisor/include/lib/bits.h
@@ -29,8 +29,7 @@

#ifndef BITS_H
#define BITS_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>
/**
*
* INVALID_BIT_INDEX means when input paramter is zero,
diff --git a/hypervisor/include/public/acrn_hv_defs.h b/hypervisor/include/public/acrn_hv_defs.h
index cfeb834..700ce92 100644
--- a/hypervisor/include/public/acrn_hv_defs.h
+++ b/hypervisor/include/public/acrn_hv_defs.h
@@ -55,9 +55,6 @@
#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)
#define HC_VM_WRITE_PROTECT_PAGE BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x03UL)
-#define HC_VM_SET_MEMORY_REGION BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x00UL)
-#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
-#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)

/* PCI assignment*/
#define HC_ID_PCI_BASE 0x50UL
diff --git a/hypervisor/lib/string.c b/hypervisor/lib/string.c
index db4f145..7d2a5a5 100644
--- a/hypervisor/lib/string.c
+++ b/hypervisor/lib/string.c
@@ -210,7 +210,8 @@ char *strcpy_s(char *d_arg, size_t dmax, const char *s_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
+ *d = '\0';
return NULL;
}

@@ -293,7 +294,8 @@ char *strncpy_s(char *d_arg, size_t dmax, const char *s_arg, size_t slen_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
+ *d = '\0';
return NULL;
}

--
2.7.4

On 8/13/2018 5:58 PM, Eddie Dong wrote:

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Victor Sun
Sent: Friday, August 10, 2018 11:32 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v3] HV: fix "missing for discarded return value"
for vm related api

- add handler if prepare_vm0() failed;

- remove assert in start_vm() and return -1 if failed;

changelog:
v2 -> v3: replace panic with pr_fatal and return directly
if prepare_vm0() failed;
v1 -> v2: panic if prepare_vm0() failed instead of reboot system;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/cpu.c | 9 ++++++---
hypervisor/arch/x86/guest/vm.c | 7 ++++---
hypervisor/common/hypercall.c | 3 +--
3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index
02cbaec..6944592 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -563,9 +563,12 @@ static void bsp_boot_post(void)

exec_vmxon_instr(BOOT_CPU_ID);

- prepare_vm(BOOT_CPU_ID);
-
- default_idle();
+ if (prepare_vm0() == 0) {

Why you want to change from prepare_vm to prepare_vm0? This is obvious issue.

OK, this is my fault. I am not aware the code here has been changed to prepare_vm() recently.
I will fix it in my next version.

+ default_idle();
+ } else {
+ pr_fatal("Prepare VM0 failed!");
+ return;
+ }

/* Control should not come here */
cpu_dead(BOOT_CPU_ID);
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09ceadb..09d9fc0 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -331,7 +331,9 @@ int start_vm(struct vm *vm)

/* Only start BSP (vid = 0) and let BSP start other APs */
vcpu = vcpu_from_vid(vm, 0U);
- ASSERT(vcpu != NULL, "vm%d, vcpu0", vm->vm_id);
+ if (vcpu == NULL) {
+ return -1;

Why -1 ?

The purpose here is want to return a failure.
Is it better to return a error code of "-EFAULT"?

+ }
schedule_vcpu(vcpu);

return 0;
@@ -358,8 +360,7 @@ int reset_vm(struct vm *vm)

vioapic_reset(vm->arch_vm.virt_ioapic);

- start_vm(vm);
- return 0;
+ return (start_vm(vm));

I remember FuSa wants to split them into 2 lines of instructions.

Anthony also has such concern. I already checked with Junjie on this and Junjie confirmed it is safe.
Please refer his comments on patch V2.

BR,
Victor

}

/**
diff --git a/hypervisor/common/hypercall.c
b/hypervisor/common/hypercall.c index f8efb35..7489b99 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -280,8 +280,7 @@ int32_t hcall_reset_vm(uint16_t vmid)
if ((target_vm == NULL) || is_vm0(target_vm))
return -1;

- reset_vm(target_vm);
- return 0;
+ return (reset_vm(target_vm));
}

int32_t hcall_assert_irqline(struct vm *vm, uint16_t vmid, uint64_t param)
--
2.7.4

In general we should do in different patch for bug fix. But this one is simple for those compile option --- this looks like MISRAC patches, we can do together to save

On 08-13 Mon 08:27, Eddie Dong wrote:

On 08-13 Mon 07:59, Eddie Dong wrote:

This is part of SBUF component, what kind of compile option do we use?

Let us use same one.

shell command "vmexit" is the only one user of vmexit_time/cnt and we use
the get_vmexit_profile, enclosed by HV_DEBUG, to access these variables.

Ok then.

BTW, hcall side also needs this kind of compile option.

I can't follow here. Can you make it more clear.

The sbuf has a hypercall, we need compile option to cover all sbuf code...

Absolutely, we should do it. This should be done in another patch, right?

Acked-by: Yu Wang <yu1.wang@...>

On 18-08-13 00:25:12, Ong Hock Yu wrote:

Under multiple VQ use case, it is possible to have concurrent requests.
Added support to return multiple vq index from all vcpu.

Signed-off-by: Ong Hock Yu <ong.hock.yu@...>
---
drivers/vbs/vbs.c | 14 ++++++++------
drivers/vbs/vbs_rng.c | 2 +-
include/linux/vbs/vbs.h | 8 ++++++--
3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/vbs/vbs.c b/drivers/vbs/vbs.c
index 65b8a0bbe3d3..a2f263cb91c0 100644
--- a/drivers/vbs/vbs.c
+++ b/drivers/vbs/vbs.c
@@ -148,9 +148,11 @@ long virtio_dev_deregister(struct virtio_dev_info *dev)
return 0;
}

-int virtio_vq_index_get(struct virtio_dev_info *dev, unsigned long *ioreqs_map)
+int virtio_vqs_index_get(struct virtio_dev_info *dev,
+ unsigned long *ioreqs_map,
+ int *vqs_index, int max_vqs_index)
{
- int val = -1;
+ int idx = 0;
struct vhm_request *req;
int vcpu;

@@ -159,7 +161,7 @@ int virtio_vq_index_get(struct virtio_dev_info *dev, unsigned long *ioreqs_map)
return -EINVAL;
}

- while (1) {
+ while (idx < max_vqs_index) {
vcpu = find_first_bit(ioreqs_map, dev->_ctx.max_vcpu);
if (vcpu == dev->_ctx.max_vcpu)
break;
@@ -179,9 +181,9 @@ int virtio_vq_index_get(struct virtio_dev_info *dev, unsigned long *ioreqs_map)
pr_debug("%s: write request! type %d\n",
__func__, req->type);
if (dev->io_range_type == PIO_RANGE)
- val = req->reqs.pio_request.value;
+ vqs_index[idx++] = req->reqs.pio_request.value;
else
- val = req->reqs.mmio_request.value;
+ vqs_index[idx++] = req->reqs.mmio_request.value;
}
smp_mb();
atomic_set(&req->processed, REQ_STATE_COMPLETE);
@@ -189,7 +191,7 @@ int virtio_vq_index_get(struct virtio_dev_info *dev, unsigned long *ioreqs_map)
}
}

Haven't fix the v2 comment... Please add one log followed v2 suggestion

- return val;
+ return idx;
}

static long virtio_vqs_info_set(struct virtio_dev_info *dev,
diff --git a/drivers/vbs/vbs_rng.c b/drivers/vbs/vbs_rng.c
index fd2bb27af66e..c5e28cc12c55 100644
--- a/drivers/vbs/vbs_rng.c
+++ b/drivers/vbs/vbs_rng.c
@@ -268,7 +268,7 @@ static int handle_kick(int client_id, unsigned long *ioreqs_map)
return -EINVAL;
}

- val = virtio_vq_index_get(&rng->dev, ioreqs_map);
+ virtio_vqs_index_get(&rng->dev, ioreqs_map, &val, 1);

if (val >= 0)
handle_vq_kick(rng, val);
diff --git a/include/linux/vbs/vbs.h b/include/linux/vbs/vbs.h
index 30df8ebf68a0..964bacac865c 100644
--- a/include/linux/vbs/vbs.h
+++ b/include/linux/vbs/vbs.h
@@ -262,7 +262,7 @@ long virtio_dev_register(struct virtio_dev_info *dev);
long virtio_dev_deregister(struct virtio_dev_info *dev);

/**
- * virtio_vq_index_get - get virtqueue index that frontend kicks
+ * virtio_vqs_index_get - get virtqueue indexes that frontend kicks
*
* This API is normally called in the VBS-K device's callback
* function, to get value write to the "kick" register from
@@ -270,10 +270,14 @@ long virtio_dev_deregister(struct virtio_dev_info *dev);
*
* @dev: Pointer to VBS-K device data struct
* @ioreqs_map: requests bitmap need to handle, provided by VHM
+ * @vqs_index: array to store the vq indexes
+ * @max_vqs_index: size of vqs_index array
*
* Return: >=0 on virtqueue index, <0 on error
*/
-int virtio_vq_index_get(struct virtio_dev_info *dev, unsigned long *ioreqs_map);
+int virtio_vqs_index_get(struct virtio_dev_info *dev,
+ unsigned long *ioreqs_map,
+ int *vqs_index, int max_vqs_index);

/**
* virtio_dev_reset - reset a VBS-K device
--
2.17.0

Can you please also modify the documentation (README.rst [1]) to reflect these changes?

[1] https://github.com/projectacrn/acrn-hypervisor/blob/master/tools/acrntrace/README.rst

Thanks,
Geoffroy

On 2018/8/13 14:40, Shuo Liu wrote:

+
+static struct acrn_vhm_ioeventfd *vhm_ioeventfd_match(
+ struct vhm_ioeventfd_info *info,
+ u64 addr, u64 data, int length, int type)
+{
+ struct acrn_vhm_ioeventfd *p = NULL;
+
+ list_for_each_entry(p, &info->ioeventfds, list) {
+ if (p->type == type &&
+ p->addr == addr &&
+ p->length == length &&
+ (p->data == data || p->wildcard))
+ return p;

Is it better to be "(p->wildcard || p->data == data)" ?

The variable native_assign_ports in struct pci_xhci_vdev is used
to record wether certain root hub port in SOS is assgned to UOS.
The logic is zero reserve and nonzero assignement.

In this patch, the meaning of native_assign_ports is expand to the
following logic:
native_assign_ports[x][y] = 0: native port x-y is not assigned.
native_assign_ports[x][y] < 0: native port x-y is assigned but
currently no virtual device attached.
native_assign_ports[x][y] > 0: native paort x-y is assigned and
virtual device attached to virtual port whose number
is stored in native_assign_ports[x][y].

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index f12126b..2df7ce3 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,16 @@ struct pci_xhci_vdev {
int (*excap_write)(struct pci_xhci_vdev *, uint64_t, uint64_t);
int usb2_port_start;
int usb3_port_start;
- uint8_t *native_assign_ports[USB_NATIVE_NUM_BUS];
+
+ /* native_assign_ports is used to record the assigment information:
+ * native_assign_ports[x][y] = 0: native port x-y is not assigned.
+ * native_assign_ports[x][y] < 0: native port x-y is assigned but
+ * currently no virtual device attached.
+ * native_assign_ports[x][y] > 0: native paort x-y is assigned and
+ * virtual device attached to virtual port whose number
+ * is stored in native_assign_ports[x][y].
+ */
+ int8_t *native_assign_ports[USB_NATIVE_NUM_BUS];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -3365,14 +3374,14 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)

if (!xdev->native_assign_ports[bus]) {
xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT,
- sizeof(uint8_t));
+ sizeof(int8_t));
if (!xdev->native_assign_ports[bus]) {
rc = -3;
goto errout;
}
}
+ xdev->native_assign_ports[bus][port] = -1;

- xdev->native_assign_ports[bus][port] = 1;
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4