Re: [PATCH v9 11/27] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.04.16 at 17:48, <konrad@kernel.org> wrote:
> Here is the inline patch:

At first I'll reply on just the particular issue in move_payload(); I'll
then go through the entire patch to see if anything else needs
commenting.

> +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    void *text_buf, *ro_buf, *rw_buf;
> +    unsigned int i;
> +    size_t size = 0;
> +    unsigned int *offset;
> +    int rc = 0;
> +
> +    offset = xmalloc_array(unsigned int, elf->hdr->e_shnum);
> +    if ( !offset )
> +        return -ENOMEM;
> +
> +    /* Compute size of different regions. */
> +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +             (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            calc_section(&elf->sec[i], &payload->text_size, &offset[i]);
> +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +                  !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +                  (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            calc_section(&elf->sec[i], &payload->rw_size, &offset[i]);
> +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +                  !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +                  !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            calc_section(&elf->sec[i], &payload->ro_size, &offset[i]);
> +        else if ( !elf->sec[i].sec->sh_flags ||
> +                  (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) ||
> +                  (elf->sec[i].sec->sh_flags & SHF_MASKPROC) )
> +            /*
> +             * Do nothing. These are .rel.text, rel.*, .symtab, .strtab,
> +             * and .shstrtab. For the non-relocate we allocate and copy these
> +             * via other means - and the .rel we can ignore as we only use it
> +             * once during loading.
> +             */
> +            offset[i] = UINT_MAX;
> +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +                  (elf->sec[i].sec->sh_type == SHT_NOBITS) )
> +        {
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Not supporting %s section!\n",
> +                    elf->name, elf->sec[i].name);
> +            rc = -EOPNOTSUPP;
> +            goto out;
> +        }
> +        else /* Such as .comment, or .debug_str. */
> +        {
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Ignoring %s section!\n",
> +                    elf->name, elf->sec[i].name);
> +            offset[i] = UINT_MAX;
> +        }

I continue to not understand why SHT_NOBITS, SHF_MASKPROC, or
zero sh_flags need considering here at all. You really only care about
SHF_ALLOC sections here (as I think you confirmed on irc), so why
can't you just start this whole sequence with

    if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
        <ignore-this-section>

then handle RX, RW, and RO just like you do now and finally have
an "else" covering unsupported SHF_ALLOC sections. Less code,
and easier to understand.

Jan


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