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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Apr 11, 2016 at 10:55:38AM -0600, Jan Beulich wrote:
> >>> On 11.04.16 at 18:34, <konrad.wilk@oracle.com> 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@oracle.com> wrote:
> >> > > On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
> >> > >> >>> On 09.04.16 at 00:45, <konrad.wilk@oracle.com> wrote:
> >> > >> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> >> > >> >> >>> On 08.04.16 at 23:10, <konrad.wilk@oracle.com> 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@lists.xen.org
http://lists.xen.org/xen-devel