On 2017-04-07 18:24, Henning Schild wrote: > Am Wed, 5 Apr 2017 15:01:55 +0200 > schrieb Henning Schild <[email protected]>: > >> Am Wed, 5 Apr 2017 08:40:20 +0200 >> schrieb Jan Kiszka <[email protected]>: >> >>> On 2017-04-03 13:59, Henning Schild wrote: >>>> Make the "used, dirty" firmware image visible in sysfs. The data >>>> segment contains data that could be useful for all sorts of >>>> debugging tools. This feature is introduced to extract code >>>> coverage information (gcov). >>>> >>>> Signed-off-by: Henning Schild <[email protected]> >>>> >>>> diff --git a/driver/main.c b/driver/main.c >>>> --- a/driver/main.c >>>> +++ b/driver/main.c >>>> @@ -81,9 +81,9 @@ struct console_state { >>>> >>>> DEFINE_MUTEX(jailhouse_lock); >>>> bool jailhouse_enabled; >>>> +void *hypervisor_mem; >>>> >>>> static struct device *jailhouse_dev; >>>> -static void *hypervisor_mem; >>>> static unsigned long hv_core_and_percpu_size; >>>> static atomic_t call_done; >>>> static int error_code; >>>> @@ -279,6 +279,7 @@ static int __jailhouse_console_dump_delt >>>> >>>> static void jailhouse_firmware_free(void) >>>> { >>>> + jailhouse_sysfs_firmware_exit(jailhouse_dev); >>>> vunmap(hypervisor_mem); >>>> hypervisor_mem = NULL; >>>> } >>>> @@ -419,6 +420,10 @@ static int jailhouse_cmd_enable(struct j >>>> header = (struct jailhouse_header *)hypervisor_mem; >>>> header->max_cpus = max_cpus; >>>> >>>> + err = jailhouse_sysfs_firmware_init(jailhouse_dev, >>>> hypervisor->size); >>>> + if (err) >>>> + goto error_unmap; >>>> + >>>> /* >>>> * ARMv8 requires to clean D-cache and invalidate I-cache >>>> for memory >>>> * containing new instructions. On x86 this is a NOP. On >>>> ARMv7 the diff --git a/driver/main.h b/driver/main.h >>>> --- a/driver/main.h >>>> +++ b/driver/main.h >>>> @@ -19,6 +19,7 @@ >>>> >>>> extern struct mutex jailhouse_lock; >>>> extern bool jailhouse_enabled; >>>> +extern void *hypervisor_mem; >>>> >>>> void *jailhouse_ioremap(phys_addr_t phys, unsigned long virt, >>>> unsigned long size); >>>> diff --git a/driver/sysfs.c b/driver/sysfs.c >>>> --- a/driver/sysfs.c >>>> +++ b/driver/sysfs.c >>>> @@ -379,6 +379,29 @@ static ssize_t remap_pool_used_show(stru >>>> return info_show(dev, buffer, >>>> JAILHOUSE_INFO_REMAP_POOL_USED); } >>>> >>>> +static ssize_t firmware_show(struct file *filp, struct kobject >>>> *kobj, >>>> + struct bin_attribute *a, char *buf, >>>> loff_t off, >>> >>> No single-character variable names please if the variable is as >>> complex as "a". >>> >>>> + size_t count) >>>> +{ >>>> + if (mutex_lock_interruptible(&jailhouse_lock) != 0) >>>> + return -EINTR; >>>> + >>>> + if (jailhouse_enabled || >>>> + hypervisor_mem == NULL || a->size == 0) { >>> >>> Why is that check for a->size == 0 required? Not seeing this in >>> other drivers. >> >> You are right, hypervisor_mem == NULL is good enough. >> >>>> + mutex_unlock(&jailhouse_lock); >>>> + return -EIO; >>> >>> -EBUSY? >> >> Yes, why not. >> >>>> + } >>>> + if (off >= a->size) { >>>> + count = 0; >>>> + } else { >>>> + if (off + count > a->size) >>>> + count = a->size - off; >>>> + memcpy(buf, hypervisor_mem + off, count); >>> >>> Would memory_read_from_buffer be useful here? >> >> Yes, thanks! >> >>>> + } >>>> + mutex_unlock(&jailhouse_lock); >>>> + return count; >>>> +} >>>> + >>>> static DEVICE_ATTR_RO(console); >>> >>> For my understanding: We need a special binary entry because >>> attributes like console are limited to a single page, right? >> >> Yes, the bin_attribute allows more than 4k of payload. >> >>>> static DEVICE_ATTR_RO(enabled); >>>> static DEVICE_ATTR_RO(mem_pool_size); >>>> @@ -401,6 +424,24 @@ static struct attribute_group jailhouse_ >>>> .attrs = jailhouse_sysfs_entries, >>>> }; >>>> >>>> +static struct bin_attribute bin_attr_firmware = { >>>> + .attr.name = "core", >>>> + .attr.mode = S_IRUSR, >>>> + .size = 0, >>>> + .read = firmware_show >>>> +}; >>>> + >>>> +int jailhouse_sysfs_firmware_init(struct device *dev, size_t >>>> hypervisor_size) +{ >>>> + bin_attr_firmware.size = hypervisor_size; >>>> + return(sysfs_create_bin_file(&dev->kobj, >>>> &bin_attr_firmware)); >>> >>> Coding style: return is not a function. >>> >>>> +} >>>> + >>>> +void jailhouse_sysfs_firmware_exit(struct device *dev) >>>> +{ >>>> + sysfs_remove_bin_file(&dev->kobj, &bin_attr_firmware); >>>> +} >>> >>> What speaks against registering via jailhouse_sysfs_init? I mean, >>> you already register earlier than the data is actually available, >>> so there is no strict correlation with the attribute being >>> visible. >> >> We need to pass the hypervisor size. But you are right the file can >> exist independant of that vallue being correct and final. > > We need the size at creation time of the file and the size might differ > between multiple jailhouse runs. Not giving the file the right size > works kind of but the tool would have to play tricks because stat would > be broken. > So i decided to keep it the way it was.
Well, then you could try creating the file only after the first Jailhouse run - or does anything make that complex? Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux -- 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.
