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

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

2016-08-18 Thread Jan Beulich

>>> On 18.08.16 at 13:41,  wrote:
> On Thu, Aug 18, 2016 at 03:43:54AM -0600, Jan Beulich wrote:
>> >>> On 18.08.16 at 11:23,  wrote:
>> > On Wed, Aug 17, 2016 at 09:39:58AM -0600, Jan Beulich wrote:
>> >> >>> On 06.08.16 at 01:04,  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@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-08-18 Thread Daniel Kiper

On Thu, Aug 18, 2016 at 03:43:54AM -0600, Jan Beulich wrote:
> >>> On 18.08.16 at 11:23,  wrote:
> > On Wed, Aug 17, 2016 at 09:39:58AM -0600, Jan Beulich wrote:
> >> >>> On 06.08.16 at 01:04,  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...
>
> 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?

Daniel

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

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

2016-08-18 Thread Jan Beulich

>>> On 18.08.16 at 11:23,  wrote:
> On Wed, Aug 17, 2016 at 09:39:58AM -0600, Jan Beulich wrote:
>> >>> On 06.08.16 at 01:04,  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...

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
of interest really is entry_size > sizeof(*mmap_src), and that's
what mmap_src[i] above doesn't handle correctly.

Jan


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

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

2016-08-18 Thread Daniel Kiper

On Wed, Aug 17, 2016 at 09:39:58AM -0600, Jan Beulich wrote:
> >>> On 06.08.16 at 01:04,  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@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-08-17 Thread Jan Beulich

>>> On 06.08.16 at 01:04,  wrote:
> @@ -106,3 +121,119 @@ multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 
> trampoline)
>  
>  return mbi_out;
>  }
> +
> +static multiboot_info_t *mbi2_mbi(u32 mbi_in)
> +{
> +const multiboot2_memory_map_t *mmap_src;
> +const multiboot2_tag_t *tag;
> +/* Do not complain that mbi_out_mods is not initialized. */
> +module_t *mbi_out_mods = NULL;

Please drop the comment.

> +memory_map_t *mmap_dst;
> +multiboot_info_t *mbi_out;
> +u32 ptr;
> +unsigned int i, mod_idx = 0;
> +
> +ptr = alloc_mem(sizeof(*mbi_out));
> +mbi_out = (multiboot_info_t *)ptr;
> +zero_mem(ptr, sizeof(*mbi_out));
> +
> +/* Skip Multiboot2 information fixed part. */
> +ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), 
> MULTIBOOT2_TAG_ALIGN);
> +
> +/* Get the number of modules. */
> +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) )

There's still a lot of casting here, but I agree it's not straightforward
to improve the situation.

> +if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +++mbi_out->mods_count;
> +else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +break;
> +
> +if ( mbi_out->mods_count )
> +{
> +mbi_out->flags = MBI_MODULES;
> +mbi_out->mods_addr = alloc_mem(mbi_out->mods_count * 
> sizeof(module_t));
> +mbi_out_mods = (module_t *)mbi_out->mods_addr;
> +}
> +
> +/* Skip Multiboot2 information fixed part. */
> +ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), 
> MULTIBOOT2_TAG_ALIGN);
> +
> +/* Put all needed data into mbi_out. */
> +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) )
> +switch ( tag->type )
> +{
> +case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
> +mbi_out->flags |= MBI_LOADERNAME;
> +ptr = get_mb2_string(tag, string, string);
> +mbi_out->boot_loader_name = copy_string(ptr);
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_CMDLINE:
> +mbi_out->flags |= MBI_CMDLINE;
> +ptr = get_mb2_string(tag, string, string);
> +mbi_out->cmdline = copy_string(ptr);
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO:
> +mbi_out->flags |= MBI_MEMLIMITS;
> +mbi_out->mem_lower = get_mb2_data(tag, basic_meminfo, mem_lower);
> +mbi_out->mem_upper = get_mb2_data(tag, basic_meminfo, mem_upper);
> +break;
> +
> +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.

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, ==).

> +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.

Jan

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

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

2016-08-05 Thread Daniel Kiper

Add multiboot2 protocol support. Alter min memory limit handling as we
now may not find it from either multiboot (v1) or multiboot2.

This way we are laying the foundation for EFI + GRUB2 + Xen development.

Signed-off-by: Daniel Kiper 
---
v4 - suggestions/fixes:
   - avoid assembly usage in xen/arch/x86/boot/reloc.c,
   - fix boundary check issue and optimize
 for() loops in mbi2_mbi(),
   - move to stdcall calling convention,
   - remove unneeded typeof() from ALIGN_UP() macro
 (suggested by Jan Beulich),
   - add and use NULL definition in xen/arch/x86/boot/reloc.c
 (suggested by Jan Beulich),
   - do not read data beyond the end of multiboot2
 information in xen/arch/x86/boot/head.S
 (suggested by Jan Beulich),
   - add :req to some .macro arguments
 (suggested by Jan Beulich),
   - use cmovcc if possible,
   - add .L to multiboot2_header_end label
 (suggested by Jan Beulich),
   - add .L to multiboot2_proto label
 (suggested by Jan Beulich),
   - improve label names
 (suggested by Jan Beulich).

v3 - suggestions/fixes:
   - reorder reloc() arguments
 (suggested by Jan Beulich),
   - remove .L from multiboot2 header labels
 (suggested by Andrew Cooper, Jan Beulich and Konrad Rzeszutek Wilk),
   - take into account alignment when skipping multiboot2 fixed part
 (suggested by Konrad Rzeszutek Wilk),
   - create modules data if modules count != 0
 (suggested by Jan Beulich),
   - improve macros
 (suggested by Jan Beulich),
   - reduce number of casts
 (suggested by Jan Beulich),
   - use const if possible
 (suggested by Jan Beulich),
   - drop static and __used__ attribute from reloc()
 (suggested by Jan Beulich),
   - remove isolated/stray __packed attribute from
 multiboot2_memory_map_t type definition
 (suggested by Jan Beulich),
   - reformat xen/include/xen/multiboot2.h
 (suggested by Konrad Rzeszutek Wilk),
   - improve comments
 (suggested by Konrad Rzeszutek Wilk),
   - remove hard tabs
 (suggested by Jan Beulich and Konrad Rzeszutek Wilk).

v2 - suggestions/fixes:
   - generate multiboot2 header using macros
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - simplify assembly in xen/arch/x86/boot/head.S
 (suggested by Jan Beulich),
   - do not include include/xen/compiler.h
 in xen/arch/x86/boot/reloc.c
 (suggested by Jan Beulich),
   - do not read data beyond the end of multiboot2 information
 (suggested by Jan Beulich).

v2 - not fixed yet:
   - dynamic dependency generation for xen/arch/x86/boot/reloc.S;
 this requires more work; I am not sure that it pays because
 potential patch requires more changes than addition of just
 multiboot2.h to Makefile
 (suggested by Jan Beulich),
   - isolated/stray __packed attribute usage for multiboot2_memory_map_t
 (suggested by Jan Beulich).
---
 xen/arch/x86/boot/Makefile|3 +-
 xen/arch/x86/boot/head.S  |  107 ++-
 xen/arch/x86/boot/reloc.c |  141 +--
 xen/arch/x86/x86_64/asm-offsets.c |9 ++
 xen/include/xen/multiboot2.h  |  169 +
 5 files changed, 419 insertions(+), 10 deletions(-)
 create mode 100644 xen/include/xen/multiboot2.h

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 5fdb5ae..06893d8 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,6 +1,7 @@
 obj-bin-y += head.o
 
-RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
$(BASEDIR)/include/xen/multiboot.h
+RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
$(BASEDIR)/include/xen/multiboot.h \
+$(BASEDIR)/include/xen/multiboot2.h
 
 head.o: reloc.S
 
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ffafcb5..5e61854 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -1,5 +1,6 @@
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -19,6 +20,28 @@
 #define BOOT_PSEUDORM_CS 0x0020
 #define BOOT_PSEUDORM_DS 0x0028
 
+#define MB2_HT(name)  (MULTIBOOT2_HEADER_TAG_##name)
+#define MB2_TT(name)  (MULTIBOOT2_TAG_TYPE_##name)
+
+.macro mb2ht_args arg:req, args:vararg
+.long \arg
+.ifnb \args
+mb2ht_args \args
+.endif
+.endm
+
+.macro mb2ht_init type:req, req:req, args:vararg
+.align MULTIBOOT2_TAG_ALIGN
+.Lmb2ht_init_start\@:
+.short \type
+.short \req
+.long .Lmb2ht_init_end\@ - .Lmb2ht_init_start\@
+.ifnb \args
+mb2ht_args \args
+.endif
+.Lmb2ht_init_end\@:
+.endm
+
 ENTRY(start)
 jmp __start
 
@@ -34,6 +57,42 @@ multiboot1_header_start:   /*** MULTIBOOT1 HEADER /
 .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
 .Lmultiboot1_header_end:
 
+/*** MULTIBOOT2 HEADER /
+/* Some ideas are taken from

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information