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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.08.16 at 13:41, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Aug 18, 2016 at 03:43:54AM -0600, Jan Beulich wrote:
>> >>> On 18.08.16 at 11:23, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Wed, Aug 17, 2016 at 09:39:58AM -0600, Jan Beulich wrote:
>> >> >>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > +            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...
>>
>> Perhaps you misunderstood my comment?
>> entry_size < sizeof(*mmap_src) is a case we simply can't deal with,
>> so you should (as you say) not obtain the memory map, which I
>> assume is equivalent to not failing the boot altogether. The point
> 
> Yep.
> 
>> of interest really is entry_size > sizeof(*mmap_src), and that's
>> what mmap_src[i] above doesn't handle correctly.
> 
> Ahhh... I have missed that. So, we can fix it in that way:
>   mmap_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, entry_size);
>   mmap_dst[i].base_addr_low = (u32)mmap_src.addr;
>   ...
> 
> Does it make sense?

Yes, that's what I had in mind.

Jan


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