Re: [Xen-devel] [PATCH v4 09/19] x86: add multiboot2 protocol support

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Aug 17, 2016 at 09:39:58AM -0600, Jan Beulich wrote:
> >>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:

[...]

> > +        case MULTIBOOT2_TAG_TYPE_MMAP:
> > +            mbi_out->flags |= MBI_MEMMAP;
> > +            mbi_out->mmap_length = get_mb2_data(tag, mmap, size);
> > +            mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t);
> > +            mbi_out->mmap_length /= get_mb2_data(tag, mmap, entry_size);
>
> Okay, here you use the entry size as provided by grub, allowing
> compatibility even when the boot loader uses a newer layout (with
> extra fields added to the end). However ...
>
> > +            mbi_out->mmap_length *= sizeof(memory_map_t);
> > +
> > +            mbi_out->mmap_addr = alloc_mem(mbi_out->mmap_length);
> > +
> > +            mmap_src = get_mb2_data(tag, mmap, entries);
> > +            mmap_dst = (memory_map_t *)mbi_out->mmap_addr;
> > +
> > +            for ( i = 0; i < mbi_out->mmap_length / sizeof(memory_map_t); 
> > i++ )
> > +            {
> > +                /* Init size member properly. */
> > +                mmap_dst[i].size = sizeof(memory_map_t);
> > +                mmap_dst[i].size -= sizeof(((memory_map_t){0}).size);
> > +                /* Now copy a given region data. */
> > +                mmap_dst[i].base_addr_low = (u32)mmap_src[i].addr;
> > +                mmap_dst[i].base_addr_high = (u32)(mmap_src[i].addr >> 32);
> > +                mmap_dst[i].length_low = (u32)mmap_src[i].len;
> > +                mmap_dst[i].length_high = (u32)(mmap_src[i].len >> 32);
>
> ... here you index an array of type multiboot2_memory_map_t.

All calculations looks correct, so, I am not sure what is wrong here.

> Also note that in any event you should check that
> entry_size >= sizeof(*mmap_src) (or, if you don't want [or need]
> to go with the flexible model, ==).

This make sense to some extent. However, I am not sure what we should do
if entry_size < sizeof(*mmap_src) (I think that we should assume flexible
model). Just do not fill memory map? Probably yes...

> > +typedef struct {
> > +    u32 type;
> > +    u32 size;
> > +    char string[0];
>
> This is not a public header - please omit the 0 here and in a similar
> place further down, as we're using all sorts of gcc extensions anyway.

OK.

Daniel

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