Re: [Xen-devel] [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.08.16 at 21:39, <daniel.kiper@xxxxxxxxxx> wrote:
> I understood that you need reloc.c after this patch but it looks
> that I was wrong. So, here it is after applying whole series.

Thanks. Here are my notes:

All callers of alloc_mem() cast the result to a pointer. While most of
them also need the result as an integer, there's one exception:

    ptr = alloc_mem(sizeof(*mbi_out));
    mbi_out = (multiboot_info_t *)ptr;
    zero_mem(ptr, sizeof(*mbi_out));

Since this is also the only caller of zero_mem() it tells me that less
casting would be needed if alloc_mem() returned void *, and if
zero_mem() took void * for its first parameter.

Otoh it looks like copy_{mem,string}() are best left the way they are
now.

The amount of casting in e.g.

    for ( tag = (multiboot2_tag_t *)ptr;
          (u32)tag - mbi_in < ((multiboot2_fixed_t *)mbi_in)->total_size;
          tag = (multiboot2_tag_t *)ALIGN_UP((u32)tag + tag->size, 
MULTIBOOT2_TAG_ALIGN) )

is still ugly, the more that the entire construct exists more than once.
One thing to consider (which would make things at least a little less
fragile) would be to make tag (and maybe then also mbi_in) a union of
u32 and multiboot2_fixed_t *. For parameter passing purposes from
reloc() it may then be desirable for this to actually be a transparent
union.

Jan


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