Xen project Mailing List

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

On Mon, Apr 11, 2016 at 10:55:38AM -0600, Jan Beulich wrote: > >>> On 11.04.16 at 18:34, <konrad.wilk@xxxxxxxxxx> wrote: > > On Mon, Apr 11, 2016 at 12:03:49PM -0400, Konrad Rzeszutek Wilk wrote: > >> On Mon, Apr 11, 2016 at 09:53:06AM -0600, Jan Beulich wrote: > >> > >>> On 09.04.16 at 02:37, <konrad.wilk@xxxxxxxxxx> wrote: > >> > > On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote: > >> > >> >>> On 09.04.16 at 00:45, <konrad.wilk@xxxxxxxxxx> wrote: > >> > >> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote: > >> > >> >> >>> On 08.04.16 at 23:10, <konrad.wilk@xxxxxxxxxx> wrote: > >> > >> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf, > >> > >> >> >> > + const struct xsplice_elf_sec > >> > >> >> >> > *base, > >> > >> >> >> > + const struct xsplice_elf_sec > >> > >> >> >> > *rela) > >> > >> >> >> > +{ > >> > >> >> >> > + const Elf_RelA *r; > >> > >> >> >> > + unsigned int symndx, i; > >> > >> >> >> > + uint64_t val; > >> > >> >> >> > + uint8_t *dest; > >> > >> >> >> > + > >> > >> >> >> > + if ( !rela->sec->sh_entsize || !rela->sec->sh_size || > >> > >> >> >> > + rela->sec->sh_entsize != sizeof(Elf_RelA) ) > >> > >> >> >> > + { > >> > >> >> >> > + dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative > >> > >> >> >> > header is > > corrupted!\n", > >> > >> >> >> > + elf->name); > >> > >> >> >> > >> > >> >> >> XENLOG_ERR surely? (and the other examples). > >> > >> >> > > >> > >> >> > Yes! I modified all of those that return an error. One of them I > >> > >> >> > made > >> > >> >> > an printk (the one about more than 64 sections). > >> > >> >> > >> > >> >> Why would that be any worse than other check failures? I think > >> > >> >> these log messages should all be issued consistently. > >> > >> > > >> > >> > OK, so all be printk instead of dprintk? > >> > >> > >> > >> Rather the other way around I would say. > >> > > > >> > > Back to dprintk(XENLOG_DEBUG for all of them then. > >> > > >> > I don't see why dprintk() can't be used with log levels other > >> > than debug, as suggested by (I think) Andrew above. > >> > >> OK, let me make them dprintk(XENLOG_ERROR > > > > I've pretty much modified most of them to that, the exceptions are these: > > > > + 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: Already loaded as > > %s!\n", > > + dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n", > > + dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n", > > + 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", > > + dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: > > %s => %#"PRIxElfAddr"\n", > > + dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => > > %#"PRIxElfAddr"\n", > > + dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => > > %#"PRIxElfAddr"(%s)\n", > > > > And then these are printk variants: > > > > > > + printk(XENLOG_ERR XSPLICE "%s: Overflow in relocation %u > > in %s for %s!\n", > > + printk(XENLOG_ERR XSPLICE "%s: Unhandled relocation %lu\n", > > + printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for > > payload!\n", > > + printk(XENLOG_ERR XSPLICE "%s: %s is missing!\n", > > + printk(XENLOG_ERR XSPLICE "%s: %s is empty!\n", > > + printk(XENLOG_ERR XSPLICE "%s: %s was seen more than once!\n", > > + printk(XENLOG_ERR XSPLICE "%s: Could not resolve old > > address of %s\n", > > + printk(XENLOG_ERR XSPLICE "%s: duplicate new symbol: %s\n", > > + 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); > > + printk(XENLOG_ERR XSPLICE"%s: Could not allocate memory for > > section table!\n", > > + printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for > > symbols\n", > > + printk(XENLOG_ERR XSPLICE "%s: Unexpected common symbol: %s\n", > > + printk(XENLOG_ERR XSPLICE "%s: Unknown symbol: %s\n", > > > > We can change some of those to dprintk if folks want that. > > So as mentioned before I'd again like to ask for consistency: I > cannot really see the criteria by which some of these use dprintk() > vs printk(). The main aspect here is: If things go severely wrong, > will the console be spammed? And the second one: Which of these 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). 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. 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? _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.