On 2017-04-07 18:40, Henning Schild wrote: > Am Fri, 7 Apr 2017 18:30:53 +0200 > schrieb Jan Kiszka <[email protected]>: > >> 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? > > Well the init will not be in the general init because we do not know > the current size. And the exit is required because the second "enable" > could load another hypervisor with a different size. > > And i want the file there from the beginning since we might change the > policy about reading it at runtime.
OK. 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.
