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

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

On Mon, Apr 11, 2016 at 11:26:05AM -0600, Jan Beulich wrote:
> >>> On 11.04.16 at 19:08, <konrad.wilk@oracle.com> 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

True, last one is very much debug.

> of these occur more than once for a single operation (hypercall)?

The apply/replace hypercall can 

     dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
     dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
         printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",

The replace can trigger a lot of "Reverting".. And one "Applying"

The uploading can trigger tons of them if payload is buggy.

The 'get' and 'list' are silent.
> 
> > 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.

Lets wait with the deletion part. They will be useful when doing
the ARM part.

So all related to 'upload' should be dprintk.
> 
> > 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.

OK, dprintk it is.

> 
> Jan

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