Re: [Xen-devel] [PATCH v6 09/24] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.16 at 19:08, <konrad.wilk@xxxxxxxxxx> wrote:
> If the system admin continously tried to unload and load the patchset
> then we certainly would spam.
> 
> But the 'loading' is (or ought to) be a single event. The applying
> or reverting may be done more often.
> 
> As such I would say that the operations that are tied to apply/reverting
> should go through printk - to at least leave breadcrumbs if things
> fall apart. I would say:
> 
>         printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
>         printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
>             printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps 
> lock!\n",
>         printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
>         printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
>     dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
>     dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
>     dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
>             dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u 
> CPUs\n",
> 
> Should be come printk. And make them INFO (except on errors - they should be 
> ERR).

Especially for the last one I don't see what use this has outside of
debugging activities. For the others a primary question is: Can any
of these occur more than once for a single operation (hypercall)?

> Then comes the question of payloads loading. In the fields all
> the dprintk are gone - and that is exactly where the payloads would
> be used. And that is the only _way_ to actually test the payload. But
> if you don't have dprintk and something goes wrong you only get -EINVAL.
> 
> As such I would think that all of the dprintk that deal with the payload
> should be made printk. So these:
> 
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported ELF Machine type!\n",
> +    dprintk(XENLOG_ERR, XSPLICE "%s: SHT_REL relocation unsupported\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section relative header is 
> corrupted!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative entry %u in %s is past 
> end!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative symbol wants symbol@%u 
> which is past end!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: No WX sections!\n", elf->name);
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong version (%u). Expected  
> %d!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Address or size fields are  
> zero!\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => 
> %p\n",
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of 
> .bug_frames.%u!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .alt_instr 
> (exp:%lu vs %lu)!\n",
> +                dprintk(XENLOG_ERR, XSPLICE "%s Alt patching outside 
> payload: 0x%lx!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .ex_table 
> (exp:%lu vs %lu)!\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
> +        dprintk(XENLOG_ERR, "%s%s: check against %s build-id failed!\n",
> +                dprintk(XENLOG_ERR, "%s%s: can't unload. Top is %s!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Section table is past end of 
> payload!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Section [%u] data is past end 
> of payload!\n",
> +                dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported multiple symbol 
> tables!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: No symbol table found!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Symbol table header is 
> corrupted!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: String table section is 
> corrupted\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section string table is 
> corrupted\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end 
> of payload!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Symbol [%u] data is past end of 
> payload!\n",
> +                dprintk(XENLOG_ERR, XSPLICE "%s: Unknown type=%#"PRIx16"\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative link of %s is 
> incorrect (%d, expected=%d)\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header is bigger than 
> payload!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Not an ELF payload!\n", elf->name);
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Invalid ELF payload!\n", elf->name);
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx is undefined!?\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx (%u) is past end 
> of sections (%u)!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header size is %u! Expected 
> %zu!?\n",
> 
> Should be printk.

I disagree - issues with the payload image should be diagnosed using
user space tools. I.e. I'd rather question whether many of the above
shouldn't go away altogether.

> Which would leave almost no dprintks in the code.
> 
> but some of them are chatty.
> 
>  +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
>  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => 
> %p\n",
>  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
>  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
>  +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => 
> %#"PRIxElfAddr"(%s)\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s 
> => %#"PRIxElfAddr"\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => 
> %#"PRIxElfAddr"\n",
> 
> and those could certainly be printk(XENLOG_DEBUG' perhaps? Or leave them as 
> dprintk?

Afaic - leave as many dprintk() as possible.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel