Re: [PATCH v8.1 11/27] xsplice: Implement payload loading

From: "Jan Beulich" <JBeulich@suse.com>
To: Konrad Rzeszutek Wilk <konrad@kernel.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: sstabellini@kernel.org, keir@xen.org, andrew.cooper3@citrix.com,
	mpohlack@amazon.com, ross.lagerwall@citrix.com,
	julien.grall@arm.com, sasha.levin@oracle.com,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v8.1 11/27] xsplice: Implement payload loading
Date: Fri, 22 Apr 2016 01:40:20 -0600	[thread overview]
Message-ID: <5719F18402000078000E4896@prv-mh.provo.novell.com> (raw)
In-Reply-To: <AC65B78E-658F-470B-9DC6-C1069223A785@oracle.com>

>>> On 21.04.16 at 18:47, <konrad.wilk@oracle.com> wrote:
> On April 21, 2016 11:36:24 AM EDT, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 21.04.16 at 17:15, <konrad@kernel.org> wrote:
>>> On Wed, Apr 20, 2016 at 11:59:34AM -0400, Konrad Rzeszutek Wilk
>>wrote:
>>>> > >@@ -29,6 +30,13 @@ struct payload {
>>>> >      >uint32_t state;                      /* One of the
>>XSPLICE_STATE_*. */
>>>> >      >int32_t rc;                          /* 0 or -XEN_EXX. */
>>>> >      >struct list_head list;               /* Linked to
>>'payload_list'. */
>>>> > >+    void *text_addr;                     /* Virtual address of
>>.text. */
>>>> > >+    size_t text_size;                    /* .. and its size. */
>>>> > >+    void *rw_addr;                       /* Virtual address of
>>.data. */
>>>> > >+    size_t rw_size;                      /* .. and its size (if
>>any). */
>>>> > >+    void *ro_addr;                       /* Virtual address of
>>.rodata. */
>>>> > >+    size_t ro_size;                      /* .. and its size (if
>>any). */
>>>> > 
>>>> > And again the question: Do these pointers really need to be
>>non-const?
>>>> 
>>>> I know I tried making them const and the compiler was not happy. I
>>will
>>>> follow up with the reasoning.
>>> 
>>> The big problem I ran in was that I had to do casting in
>>> 'modify_payload'. To lower the amount of casting I ended making
>>> 
>>> load_addr be void * (if it was const void * I would have to cast away
>>> the constness in xsplice_elf.c):
>>> 
>>>  elf->sec[i].load_addr = (void *)buf + offset[i];
>>> 
>>> where 'buf' is:
>>>   buf = payload->text_addr;
>>> 
>>> (and buf it also a const void *).
>>
>>Hence (taking all of the above) without any cast and with load_addr
>>being const void *, text_addr could be so, too. Or else the picture
>>above isn't the complete one.
>>
> 
> If I did that I will need to uncast constness when memcpy data in (a couple 
> of lines below in move_payload).

Ehem - no, not really. This is what the patch has:

    uint8_t *buf;
...
    buf = arch_xsplice_alloc_payload(size);
...
            elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize;
...
                memcpy(elf->sec[i].load_addr, elf->sec[i].data,
                       elf->sec[i].sec->sh_size);

So all it would take is to substitute the first argument to memcpy()
by an expression involving buf (i.e. the right side of the earlier
assignment, or some intermediate variable used to avoid having
the same expression twice).

> And also when doing relocation.

There it would then be really unavoidable. That's a place where I
would say casting away constness could be considered acceptable,
but I'd like to come back to your earlier reply first: You refer to
modify_payload() and an expression involving "buf + offset[i]",
neither of which I can spot anywhere in this patch. Oh, looks like
you mean this

            if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) )
                 buf = payload->text_addr;
            else if ( (elf->sec[i].sec->sh_flags & SHF_WRITE) )
                buf = payload->rw_addr;
             else
                buf = payload->ro_addr;

            elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize;

in move_payload(). But that's the same as above then - by using
suitable intermediate variables you can deal with the structure
fields pointing to const quite easily, since the loop the above is in
is preceded by

    payload->text_addr = buf;
    payload->rw_addr = payload->text_addr + PAGE_ALIGN(payload->text_size);
    payload->ro_addr = payload->rw_addr + PAGE_ALIGN(payload->rw_size);

and buf points to non-const.

Bottom line: While doing the loading it is clear that some of the
image needs to be modified, and hence you need pointers to
non-const, or cast away constness under rare circumstances.
But since for the whole rest of the lifetime of the module you
do want to express that the image pieces are not supposed to
change, the goal ought to be to have as many as possible
pointers to const in the control structure.

And I'm really sorry for being this picky, you simply suffer from
me having written more than one image loader as well as a linker
and object file analysis/massaging tools in the past.

Jan

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