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. Henning > 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.
