Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-31 Thread Daniel Kiper 
On Wed, Aug 31, 2016 at 09:18:39PM +0100, Andrew Cooper wrote:
> On 19/08/2016 23:43, Daniel Kiper wrote:
> > @@ -106,3 +121,122 @@ 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;
> > +module_t *mbi_out_mods = NULL;
> > +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;
>
> It occurs to me that a lot of this code would be more simple if you also
> pulled the
>
> #define _p(val) ((void *)(unsigned long)val)
>
> in from the usual header files.  It would shorten a lot of the
> (multiboot_info_t *) casting, and reduce most multi-line statements to
> single lines.
>
> Can I also suggest using:

[...]

Make sense for me. I will try to get all your comments into account.

Thanks a lot,

Daniel

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

Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-31 Thread Andrew Cooper 
On 19/08/2016 23:43, Daniel Kiper wrote:
> @@ -106,3 +121,122 @@ 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;
> +module_t *mbi_out_mods = NULL;
> +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;

It occurs to me that a lot of this code would be more simple if you also
pulled the

#define _p(val) ((void *)(unsigned long)val)

in from the usual header files.  It would shorten a lot of the
(multiboot_info_t *) casting, and reduce most multi-line statements to
single lines.

Can I also suggest using:

static multiboot_info_t *mbi2_mbi(u32 mbi_in_addr)
{
const multiboot_info_t *mbi_in = _p(mbi_in_addr);
...

This would avoid repeatedly needing to cast the old mbi_in integer to
access fields like ->total_size.

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

Braces here please
{

> +if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +++mbi_out->mods_count;
> +else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +break;

}

Given the complexity of the for statement, the exact code contained
inside the loop could do with this added clarity.

> +
> +if ( mbi_out->mods_count )
> +{

In the case that this condition evaluates false, and...

> +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:
> +if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
> +break;
> +
> +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);
> +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_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, 
> entry_size);
> +mmap_dst[i].base_addr_low = (u32)mmap_src->addr;
> +mmap_dst[i].base_addr_high = (u32)(mmap_src->addr >> 32);
> +mmap_dst[i].length_low = (u32)mmap_src->len;
> +mmap_dst[i].length_high = (u32)(mmap_src->len >> 32);
> +mmap_dst[i].type = mmap_src->type;
> +}
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_MODULE:

... this tag gets found, then a NULL mbi_out_mods gets followed.

I realise that this calculation is a

Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-31 Thread Daniel Kiper 
On Tue, Aug 30, 2016 at 09:14:28AM -0600, Jan Beulich wrote:
> >>> On 30.08.16 at 16:41,  wrote:
> > On Thu, Aug 25, 2016 at 05:50:04AM -0600, Jan Beulich wrote:
> >> >>> On 20.08.16 at 00:43,  wrote:
> >> > +case MULTIBOOT2_TAG_TYPE_MMAP:
> >> > +if ( get_mb2_data(tag, mmap, entry_size) < 
> >> > sizeof(*mmap_src) )
> >> > +break;
> >> > +
> >> > +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);
> >> > +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_src = (void *)mmap_src + i * get_mb2_data(tag, 
> >> > mmap, entry_size);
> >>
> >> Why do you multiply by i here? The way you've written it you want to
> >> increment mmap_src by entry_size _at the end_ of each iteration. Or
> >
> > This is another option. If you wish I can do that.
>
> This is not just another option - afaict the code as is won't work for
> more than one map entry.

Ugh... Sorry, you are right. Stupid mistake. I will change this.

Daniel

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

Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-30 Thread Jan Beulich 
>>> On 30.08.16 at 16:41,  wrote:
> On Thu, Aug 25, 2016 at 05:50:04AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43,  wrote:
>> > +case MULTIBOOT2_TAG_TYPE_MMAP:
>> > +if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
>> > +break;
>> > +
>> > +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);
>> > +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_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, 
>> > entry_size);
>>
>> Why do you multiply by i here? The way you've written it you want to
>> increment mmap_src by entry_size _at the end_ of each iteration. Or
> 
> This is another option. If you wish I can do that.

This is not just another option - afaict the code as is won't work for
more than one map entry.

Jan


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

Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-30 Thread Daniel Kiper 
On Thu, Aug 25, 2016 at 05:50:04AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43,  wrote:
> > +case MULTIBOOT2_TAG_TYPE_MMAP:
> > +if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
> > +break;
> > +
> > +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);
> > +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_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, 
> > entry_size);
>
> Why do you multiply by i here? The way you've written it you want to
> increment mmap_src by entry_size _at the end_ of each iteration. Or

This is another option. If you wish I can do that.

> else you need a second variable.

I think that former is better.

Daniel

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

Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-25 Thread Jan Beulich 
>>> On 20.08.16 at 00:43,  wrote:
> +case MULTIBOOT2_TAG_TYPE_MMAP:
> +if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
> +break;
> +
> +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);
> +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_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, 
> entry_size);

Why do you multiply by i here? The way you've written it you want to
increment mmap_src by entry_size _at the end_ of each iteration. Or
else you need a second variable.

Jan


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

[Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

2016-08-19 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 
---
v5 - suggestions/fixes:
   - check multiboot2_tag_mmap_t.entry_size before
 multiboot2_tag_mmap_t.entries[] use
 (suggested by Jan Beulich),
   - properly index multiboot2_tag_mmap_t.entries[]
 (suggested by Jan Beulich),
   - use "type name[]" instad of "type name[0]"
 in xen/include/xen/multiboot2.h
 (suggested by Jan Beulich),
   - remove unneeded comment
 (suggested by Jan Beulich).

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 |  144 +--
 xen/arch/x86/x86_64/asm-offsets.c |9 ++
 xen/include/xen/multiboot2.h  |  169 +
 5 files changed, 422 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\@ -