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. > Jan > > > + > > int jailhouse_sysfs_init(struct device *dev) > > { > > int err; > > diff --git a/driver/sysfs.h b/driver/sysfs.h > > --- a/driver/sysfs.h > > +++ b/driver/sysfs.h > > @@ -19,6 +19,8 @@ int jailhouse_sysfs_cell_create(struct c > > void jailhouse_sysfs_cell_register(struct cell *cell); > > void jailhouse_sysfs_cell_delete(struct cell *cell); > > > > +int jailhouse_sysfs_firmware_init(struct device *dev, size_t > > hypervisor_size); +void jailhouse_sysfs_firmware_exit(struct device > > *dev); int jailhouse_sysfs_init(struct device *dev); > > void jailhouse_sysfs_exit(struct device *dev); > > > > > -- 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.
