On 01/24/2017 05:04 PM, Jan Kiszka wrote:
> On 2017-01-19 21:11, Ralf Ramsauer wrote:
>> Signed-off-by: Ralf Ramsauer <[email protected]>
>> ---
>> Documentation/debug-output.md | 8 +++-
>> driver/main.c | 97
>> +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/debug-output.md b/Documentation/debug-output.md
>> index 1c11ca180a..5d0610a154 100644
>> --- a/Documentation/debug-output.md
>> +++ b/Documentation/debug-output.md
>> @@ -89,7 +89,13 @@ Hypervisor Console via sysfs
>>
>> If the debug console of root cell has the flag JAILHOUSE_CON2_TYPE_ROOTPAGE
>> set, the hypervisor console is available through
>> -/sys/devices/jailhouse/console.
>> +/sys/devices/jailhouse/console. Continuous reading of the hypervisor
>> console
>> +is available through /dev/jailhouse.
>> +
>> +Example
>> +```
>> +cat /dev/jailhouse
>> +```
>>
>> Inmates
>> -------
>> diff --git a/driver/main.c b/driver/main.c
>> index 2a86842448..4eade5724d 100644
>> --- a/driver/main.c
>> +++ b/driver/main.c
>> @@ -71,6 +71,10 @@ MODULE_FIRMWARE(JAILHOUSE_FW_NAME);
>> #endif
>> MODULE_VERSION(JAILHOUSE_VERSION);
>>
>> +struct console_userdata {
>
> How about "console_state"?
>
>> + unsigned int head;
>> +};
>> +
>> DEFINE_MUTEX(jailhouse_lock);
>> bool jailhouse_enabled;
>>
>> @@ -616,11 +620,104 @@ static long jailhouse_ioctl(struct file *file,
>> unsigned int ioctl,
>> return err;
>> }
>>
>> +static int jailhouse_console_open(struct inode *inode, struct file *file)
>> +{
>> + struct console_userdata *user;
>> +
>> + user = kzalloc(sizeof(struct console_userdata), GFP_KERNEL);
>> + if (!user)
>> + return -ENOMEM;
>> +
>> + file->private_data = user;
>> +
>> + return 0;
>> +}
>> +
>> +static int jailhouse_console_release(struct inode *inode, struct file *file)
>> +{
>> + struct console_userdata *user = file->private_data;
>> +
>> + kfree(user);
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t jailhouse_console_read(struct file *file, char __user *out,
>> + size_t size, loff_t *off)
>> +{
>> + struct console_userdata *user = file->private_data;
>> + char *content;
>> + unsigned int miss;
>> + int ret;
>> +
>> + content = kmalloc(sizeof(console_page->content), GFP_KERNEL);
>> + if (content == NULL)
>> + return -ENOMEM;
>> +
>> + /* wait for new data */
>> + while (1) {
>> + if (mutex_lock_interruptible(&jailhouse_lock) != 0) {
>> + ret = -EINTR;
>> + goto console_free_out;
>> + }
>> +
>> + ret = jailhouse_console_page_delta(content, user->head, &miss);
>> +
>> + mutex_unlock(&jailhouse_lock);
>> +
>> + if ((!ret || ret == -EAGAIN) && file->f_flags & O_NONBLOCK)
>> + goto console_free_out;
>> +
>> + if (ret == -EAGAIN)
>> + /* Reset the user head, if jailhouse is not enabled. We
>> + * have to do this, as jailhouse might be reenabled and
>> + * the file handle was kept open in the meanwhile */
>> + user->head = 0;
>> + else if (ret < 0)
>> + goto console_free_out;
>> + else if (ret)
>> + break;
>> +
>> + schedule_timeout_uninterruptible(HZ / 10);
>> + if (signal_pending(current)) {
>> + ret = -EINTR;
>> + goto console_free_out;
>> + }
>> + }
>> +
>> + if (miss) {
>> + /* If we missed anything, warn user. We will dump the actual
>> + * content in the next call. */
>> + ret = snprintf(content, sizeof(console_page->content),
>> + "<NOTE: missing %u byte console log>\n",
>
> "missed %u bytes of console log"
Without brackets and without \n? Didn't you propose brackets?
>
>> + miss);
>> + user->head += miss;
>> + if (size < ret)
>> + ret = size;
>
> Why is user->head only update by "miss" here? ...
In this round we're only going to return 'missed %u byte console log' to
userspace and forward the head to the latest valid position.
Rest of the content is caught in the next round, right afterwards.
Otherwise we have a significant amount of error checking and rewinding
and string composition, if we don't want to loose any content.
>
>> + } else {
>> + if (size < ret)
>> + ret = size;
>> + user->head += ret;
>
> ...And shouldn't the last line be pulled out and executed
> unconditionally? Actually, that would allow to pull the "if size < ret"
> as well.
No, in case of misses, we don't want to increase the head by the length
of the 'missed %u byte console log' string.
"user->head += ret" forwards the head by the amount of bytes that were
successfully read from the ring buffer and fit into userspace buffer.
"user->head += miss" forwards the head to the next valid byte inside the
ringbuffer, that will be caught in the next round. In this case, ret
countains the number of bytes that are returned by the function and
represent the length of the 'missing foo bytes' message.
Ralf
>
>> + }
>> +
>> + if (copy_to_user(out, content, ret))
>> + ret = -EFAULT;
>> +
>> +console_free_out:
>> + set_current_state(TASK_RUNNING);
>> + kfree(content);
>> + return ret;
>> +}
>> +
>> +
>> static const struct file_operations jailhouse_fops = {
>> .owner = THIS_MODULE,
>> .unlocked_ioctl = jailhouse_ioctl,
>> .compat_ioctl = jailhouse_ioctl,
>> .llseek = noop_llseek,
>> + .open = jailhouse_console_open,
>> + .release = jailhouse_console_release,
>> + .read = jailhouse_console_read,
>> };
>>
>> static struct miscdevice jailhouse_misc_dev = {
>>
>
> 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.
