On 2017-01-24 17:35, Ralf Ramsauer wrote:
> 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()
One is inner/lower the other is outer/higher level. It's not clear from
the name which is which, but maybe describing the functions can help.
>>
>>> +{
>>> + 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.
Perfect. The simpler, the better.
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.
