On 01/24/2017 04:41 PM, Jan Kiszka wrote:
> On 2017-01-19 21:11, Ralf Ramsauer wrote:
>> Signed-off-by: Ralf Ramsauer <[email protected]>
>> ---
>> Documentation/sysfs-entries.txt | 3 ++
>> TODO.md | 1 -
>> driver/main.c | 98
>> +++++++++++++++++++++++++++++++++++++++++
>> driver/main.h | 2 +
>> driver/sysfs.c | 26 +++++++++++
>> 5 files changed, 129 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/sysfs-entries.txt
>> b/Documentation/sysfs-entries.txt
>> index 6b483bba20..58d1f5a2df 100644
>> --- a/Documentation/sysfs-entries.txt
>> +++ b/Documentation/sysfs-entries.txt
>> @@ -5,6 +5,7 @@ The following sysfs entries are provided by the Jailhouse
>> Linux driver. These
>> can be used for monitoring the state of the hypervisor and its cells.
>>
>> /sys/devices/jailhouse
>> +|- console - hypervisor console (see [1])
>> |- enabled - 1 if Jailhouse is enabled, 0 otherwise
>> |- mem_pool_size - number of pages in hypervisor memory pool
>> |- mem_pool_used - used pages of hypervisor memory pool
>> @@ -30,3 +31,5 @@ may not reflect a fully consistent state. The existence
>> and semantics of VM
>> exit reason values are architecture-dependent and may change in future
>> versions. In general statistics shall only be considered as a first hint
>> when
>> analyzing cell behavior.
>> +
>> +[1] Documentation/debug-output.md
>> diff --git a/TODO.md b/TODO.md
>> index e452d30630..4e93b1d7af 100644
>> --- a/TODO.md
>> +++ b/TODO.md
>> @@ -89,7 +89,6 @@ Hardware error handling
>>
>> Monitoring
>> - report error-triggering devices behind IOMMUs via sysfs
>> - - hypervisor console via debugfs?
>> - cell software watchdog via comm region messages
>> -> time out pending comm region messages and kill failing cells
>> (includes timeouts of unanswered shutdown requests)
>> diff --git a/driver/main.c b/driver/main.c
>> index 676794bef1..2a86842448 100644
>> --- a/driver/main.c
>> +++ b/driver/main.c
>> @@ -19,11 +19,13 @@
>> #include <linux/miscdevice.h>
>> #include <linux/firmware.h>
>> #include <linux/mm.h>
>> +#include <linux/slab.h>
>> #include <linux/smp.h>
>> #include <linux/uaccess.h>
>> #include <linux/reboot.h>
>> #include <linux/vmalloc.h>
>> #include <linux/io.h>
>> +#include <asm/barrier.h>
>> #include <asm/smp.h>
>> #include <asm/cacheflush.h>
>> #include <asm/tlbflush.h>
>> @@ -77,6 +79,8 @@ static void *hypervisor_mem;
>> static unsigned long hv_core_and_percpu_size;
>> static atomic_t call_done;
>> static int error_code;
>> +static struct jailhouse_console* volatile console_page;
>> +static bool console_available;
>>
>> #ifdef CONFIG_X86
>> bool jailhouse_use_vmcall;
>> @@ -91,6 +95,23 @@ static void init_hypercall(void)
>> }
>> #endif
>>
>> +static void copy_console_page(struct jailhouse_console *dst)
>> +{
>> + unsigned int tail;
>> +
>> + do {
>> + /* spin while hypervisor is writing to console */
>> + while (console_page->lock)
>> + cpu_relax();
>> + tail = console_page->tail;
>> + rmb();
>> +
>> + /* copy console page */
>> + memcpy(dst, console_page, sizeof(struct jailhouse_console));
>> + rmb();
>> + } while (console_page->tail != tail || console_page->lock);
>> +}
>> +
>> static long get_max_cpus(u32 cpu_set_size,
>> const struct jailhouse_system __user *system_config)
>> {
>> @@ -182,6 +203,77 @@ static inline const char * jailhouse_get_fw_name(void)
>> #endif
>> }
>>
>> +static int jailhouse_console_delta(struct jailhouse_console *console,
>> + char *dst, unsigned int head,
>> + unsigned int *miss)
>> +{
>> + int ret;
>> + unsigned int head_mod, tail_mod;
>> + unsigned int delta;
>> +
>> + if (miss)
>> + *miss = 0;
>
> I would use a local var for "missed", check at the end if the passed
> pointer is non-NULL, and only then write back.
>
>> +
>> + /* check if tail overflowed */
>> + if (console->tail < head)
>> + delta = (0 - head) + console->tail;
>> + else
>> + delta = console->tail - head;
>
> Maybe me mind is still frozen, but these two statements should generate
> the same instructions, shouldn't they...?
All-clear, no chilblains. Had to think twice, but it's the same, yes. We
don't even need to check for this case.
This must be an artifact when i was using a content size that was not a
power of two, because then you need to check for this case.
>
>> +
>> + /* check if we have misses */
>> + if (delta > sizeof(console->content)) {
>> + if (miss)
>> + *miss = delta - sizeof(console->content);
>> + head = console->tail - sizeof(console->content);
>> + delta = sizeof(console->content);
>> + }
>> +
>> + head_mod = head % sizeof(console->content);
>> + tail_mod = console->tail % sizeof(console->content);
>> +
>> + if (head_mod + delta > sizeof(console->content)) {
>> + ret = sizeof(console->content) - head_mod;
>> + memcpy(dst, console->content + head_mod, ret);
>> + delta -= ret;
>> + memcpy(dst + ret, console->content, delta);
>> + ret += delta;
>> + } else {
>> + ret = delta;
>> + memcpy(dst, console->content + head_mod, delta);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +int jailhouse_console_page_delta(char *dst, unsigned int head,
>> + unsigned int *miss)
>
> Can you find some more telling names for this one vs.
> "jailhouse_console_delta"? Both are missing some verb, and it's not
> obvious in what they are different?
Mmhm. My intention was:
- jailhouse_console_delta: dumps the delta of an arbitrary struct
jailhouse_console
- jailhouse_console_page_delta: dumps the delta of THE console_page
does locking, checks its availability, copies content, ...
What about:
jailhouse_dump_console_delta()
jailhouse_dump_console_{log/page}_delta()
>
>> +{
>> + int ret;
>> + struct jailhouse_console *console;
>> +
>> + if (!jailhouse_enabled)
>> + return -EAGAIN;
>> +
>> + if (!console_available)
>> + return -EPERM;
>> +
>> + console = kmalloc(sizeof(struct jailhouse_console), GFP_KERNEL);
>> + if (console == NULL)
>> + return -ENOMEM;
>> +
>> + copy_console_page(console);
>> + if (console->tail == head) {
>> + ret = 0;
>> + goto console_free_out;
>> + }
>> +
>> + ret = jailhouse_console_delta(console, dst, head, miss);
>> +
>> +console_free_out:
>> + kfree(console);
>> + return ret;
>> +}
>> +
>> /* See Documentation/bootstrap-interface.txt */
>> static int jailhouse_cmd_enable(struct jailhouse_system __user *arg)
>> {
>> @@ -273,6 +365,9 @@ static int jailhouse_cmd_enable(struct jailhouse_system
>> __user *arg)
>> goto error_release_fw;
>> }
>>
>> + console_page = (struct jailhouse_console*)
>> + (hypervisor_mem + header->console_page);
>> +
>> /* Copy hypervisor's binary image at beginning of the memory region
>> * and clear the rest to zero. */
>> memcpy(hypervisor_mem, hypervisor->data, hypervisor->size);
>> @@ -329,6 +424,9 @@ static int jailhouse_cmd_enable(struct jailhouse_system
>> __user *arg)
>> }
>> #endif
>>
>> + console_available = CON2_TYPE(config->debug_console.flags) ==
>> + JAILHOUSE_CON2_TYPE_ROOTPAGE;
>> +
>> err = jailhouse_cell_prepare_root(&config->root_cell);
>> if (err)
>> goto error_unmap;
>> diff --git a/driver/main.h b/driver/main.h
>> index e01ca5b52f..fe322ead18 100644
>> --- a/driver/main.h
>> +++ b/driver/main.h
>> @@ -22,5 +22,7 @@ extern bool jailhouse_enabled;
>>
>> void *jailhouse_ioremap(phys_addr_t phys, unsigned long virt,
>> unsigned long size);
>> +int jailhouse_console_page_delta(char *dst, unsigned int head,
>> + unsigned int *miss);
>>
>> #endif /* !_JAILHOUSE_DRIVER_MAIN_H */
>> diff --git a/driver/sysfs.c b/driver/sysfs.c
>> index 8da1f766ce..151a5bf46a 100644
>> --- a/driver/sysfs.c
>> +++ b/driver/sysfs.c
>> @@ -309,6 +309,30 @@ void jailhouse_sysfs_cell_delete(struct cell *cell)
>> kobject_put(&cell->kobj);
>> }
>>
>> +static ssize_t console_show(struct device *dev, struct device_attribute
>> *attr,
>> + char *buffer)
>> +{
>> + ssize_t ret;
>> +
>> + if (mutex_lock_interruptible(&jailhouse_lock) != 0)
>> + return -EINTR;
>> +
>> + ret = jailhouse_console_page_delta(buffer, 0, NULL);
>> + if (ret > 0)
>> + /* We can safely fill the whole buffer and append a terminating
>> + * \0, as buffer comes preallocated with PAGE_SIZE and
>> + * the content is smaller than PAGE_SIZE */
>> + buffer[ret++] = 0;
>
> The "why" for terminating the buffer with 0 should be mentioned. It's
> not clear to me at least
... wait -- we don't even need to do any string termination here.
Returning the size of the content inside the buffer should be sufficient.
Ralf
>
>> +
>> + /* don't return error if jailhouse is not enabled */
>> + if (ret == -EAGAIN)
>> + ret = 0;
>> +
>> + mutex_unlock(&jailhouse_lock);
>> +
>> + return ret;
>> +}
>> +
>> static ssize_t enabled_show(struct device *dev, struct device_attribute
>> *attr,
>> char *buffer)
>> {
>> @@ -361,6 +385,7 @@ static ssize_t remap_pool_used_show(struct device *dev,
>> return info_show(dev, buffer, JAILHOUSE_INFO_REMAP_POOL_USED);
>> }
>>
>> +static DEVICE_ATTR_RO(console);
>> static DEVICE_ATTR_RO(enabled);
>> static DEVICE_ATTR_RO(mem_pool_size);
>> static DEVICE_ATTR_RO(mem_pool_used);
>> @@ -368,6 +393,7 @@ static DEVICE_ATTR_RO(remap_pool_size);
>> static DEVICE_ATTR_RO(remap_pool_used);
>>
>> static struct attribute *jailhouse_sysfs_entries[] = {
>> + &dev_attr_console.attr,
>> &dev_attr_enabled.attr,
>> &dev_attr_mem_pool_size.attr,
>> &dev_attr_mem_pool_used.attr,
>>
>
> Jan
>
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
