[PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: [Linaro-uefi] The GRUB multiboot support patch for aarch64(V3.1))

[PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: [Linaro-uefi] The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
> > I can see why you did this (it's what Xen actually supports today) but I
> > wonder if in the interests of reducing the special cases I should create
> > a Xen patch which causes it to accept both names so that you can just
> > implement the "multiboot,*" stuff in common code without the special
> > cases? (Although that depends on the reason for the other non-compat
> > string special cases too)
> 
> I think the "multiboot,*" stuff will be better, once Xen supports
> this, I will update this code.

I've only compile tested this, but I *think* this does the right thing.

Ian.

8<-----------------

>From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Wed, 4 Jun 2014 18:17:10 +0100
Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings

This causes Xen to accept the more generic names originally proposed by
Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and
incorporated into the proposal in
http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot

This will allow bootloaders to not special case Xen (or at least to reduce
the amount which is required).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/common/device_tree.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f0b17a3..5040097 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node,
     struct dt_mb_module *mod;
     int len;
 
-    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
+    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
+         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
         nr = MOD_KERNEL;
-    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0)
+    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
+              fdt_node_check_compatible(fdt, node, "multiboot,linux-initrd") == 0 )
         nr = MOD_INITRD;
     else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
         nr = MOD_XSM;
@@ -433,7 +435,8 @@ static int __init early_scan_node(const void *fdt,
 {
     if ( device_tree_node_matches(fdt, node, "memory") )
         process_memory_node(fdt, node, name, address_cells, size_cells);
-    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) )
+    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
+              device_tree_node_compatible(fdt, node, "boot,module" ))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
         process_chosen_node(fdt, node, name, address_cells, size_cells);
-- 
1.9.0

Re: [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: [Linaro-uefi] The GRUB multiboot support patch for aarch64(V3.1))

[Julien Grall]

Hi Ian,

On 06/05/2014 12:56 PM, Ian Campbell wrote:
> On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
>>> I can see why you did this (it's what Xen actually supports today) but I
>>> wonder if in the interests of reducing the special cases I should create
>>> a Xen patch which causes it to accept both names so that you can just
>>> implement the "multiboot,*" stuff in common code without the special
>>> cases? (Although that depends on the reason for the other non-compat
>>> string special cases too)
>>
>> I think the "multiboot,*" stuff will be better, once Xen supports
>> this, I will update this code.
> 
> I've only compile tested this, but I *think* this does the right thing.
> 
> Ian.
> 
> 8<-----------------
> 
> From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Wed, 4 Jun 2014 18:17:10 +0100
> Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
> 
> This causes Xen to accept the more generic names originally proposed by
> Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and
> incorporated into the proposal in
> http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
> 
> This will allow bootloaders to not special case Xen (or at least to reduce
> the amount which is required).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f0b17a3..5040097 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node,
>      struct dt_mb_module *mod;
>      int len;
>  
> -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
> +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> +         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )

While we are modifying the protocol, "linux-zImage" is confusing in the
name. Actually we can use it for an ELF, another OS... I don't think Xen
will change his behavior depending of the DOM0 image.

I would rename it to something more generic such as "multiboot,kernel".
This will avoid adding a new compatible string every time we support a
new format/OS.

Regards,

-- 
Julien Grall

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Thu, 2014-06-05 at 17:36 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 06/05/2014 12:56 PM, Ian Campbell wrote:
> > On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
> >>> I can see why you did this (it's what Xen actually supports today) but I
> >>> wonder if in the interests of reducing the special cases I should create
> >>> a Xen patch which causes it to accept both names so that you can just
> >>> implement the "multiboot,*" stuff in common code without the special
> >>> cases? (Although that depends on the reason for the other non-compat
> >>> string special cases too)
> >>
> >> I think the "multiboot,*" stuff will be better, once Xen supports
> >> this, I will update this code.
> > 
> > I've only compile tested this, but I *think* this does the right thing.
> > 
> > Ian.
> > 
> > 8<-----------------
> > 
> > From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Wed, 4 Jun 2014 18:17:10 +0100
> > Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
> > 
> > This causes Xen to accept the more generic names originally proposed by
> > Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and
> > incorporated into the proposal in
> > http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
> > 
> > This will allow bootloaders to not special case Xen (or at least to reduce
> > the amount which is required).
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/common/device_tree.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index f0b17a3..5040097 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node,
> >      struct dt_mb_module *mod;
> >      int len;
> >  
> > -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
> > +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> > +         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
> 
> While we are modifying the protocol, "linux-zImage" is confusing in the
> name. Actually we can use it for an ELF, another OS... I don't think Xen
> will change his behavior depending of the DOM0 image.

zImage defines the boot protocol to use. Since the protocol is defined
by Linux as zImage I think that is the appropriate name, if some other
OS wants to mimic Linux then fine. But if we end up supporting some OS
with its own  boot protocol which doesn't match Linux's then that should
have a distinct name of its own.

The "ELF boot protocol" is as yet undefined. It may or may not end up
resembling zImage. In any case it will be a standard made up entirely by
us and as such xen-elf or something would be the correct name (unless it
happens to match zImage).

> I would rename it to something more generic such as "multiboot,kernel".
> This will avoid adding a new compatible string every time we support a
> new format/OS.

That implies that there is some sort of standard kernel boot interface,
which is not the case, the protocol is defined as linux-zimage.

Perhaps there is scope for a generic compat name which probes based on
magic numbers. You are free to propose and implement such a thing if you
like, but it would be inappropriate to use that with the current
implementation I think.

Ian.

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Julien Grall]

On 06/05/2014 05:55 PM, Ian Campbell wrote:
> On Thu, 2014-06-05 at 17:36 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 06/05/2014 12:56 PM, Ian Campbell wrote:
>>> On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
>>>>> I can see why you did this (it's what Xen actually supports today) but I
>>>>> wonder if in the interests of reducing the special cases I should create
>>>>> a Xen patch which causes it to accept both names so that you can just
>>>>> implement the "multiboot,*" stuff in common code without the special
>>>>> cases? (Although that depends on the reason for the other non-compat
>>>>> string special cases too)
>>>>
>>>> I think the "multiboot,*" stuff will be better, once Xen supports
>>>> this, I will update this code.
>>>
>>> I've only compile tested this, but I *think* this does the right thing.
>>>
>>> Ian.
>>>
>>> 8<-----------------
>>>
>>> From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001
>>> From: Ian Campbell <ian.campbell@citrix.com>
>>> Date: Wed, 4 Jun 2014 18:17:10 +0100
>>> Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
>>>
>>> This causes Xen to accept the more generic names originally proposed by
>>> Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and
>>> incorporated into the proposal in
>>> http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
>>>
>>> This will allow bootloaders to not special case Xen (or at least to reduce
>>> the amount which is required).
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>  xen/common/device_tree.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index f0b17a3..5040097 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node,
>>>      struct dt_mb_module *mod;
>>>      int len;
>>>  
>>> -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
>>> +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
>>> +         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
>>
>> While we are modifying the protocol, "linux-zImage" is confusing in the
>> name. Actually we can use it for an ELF, another OS... I don't think Xen
>> will change his behavior depending of the DOM0 image.
> 
> zImage defines the boot protocol to use. Since the protocol is defined
> by Linux as zImage I think that is the appropriate name, if some other
> OS wants to mimic Linux then fine. But if we end up supporting some OS
> with its own  boot protocol which doesn't match Linux's then that should
> have a distinct name of its own.

zImage is the boot protocol for arm32, this is different for arm64. Here
we are talking about the protocol for dom0, why the bootloader is
involved in it?

How the interface will be exposed by grub to the user? Is it the
"multiboot ..." line like in x86? If so, how grub will choose what is
the boot protocol for the guest?

> The "ELF boot protocol" is as yet undefined. It may or may not end up
> resembling zImage. In any case it will be a standard made up entirely by
> us and as such xen-elf or something would be the correct name (unless it
> happens to match zImage).

IHMO, the bootloader should only give the kernel blob to the hypervisor.
Then it will decide what is the best protocol to use for booting dom0.

Regards,

-- 
Julien Grall

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
> On 06/05/2014 05:55 PM, Ian Campbell wrote:
> > On Thu, 2014-06-05 at 17:36 +0100, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 06/05/2014 12:56 PM, Ian Campbell wrote:
> >>> On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
> >>>>> I can see why you did this (it's what Xen actually supports today) but I
> >>>>> wonder if in the interests of reducing the special cases I should create
> >>>>> a Xen patch which causes it to accept both names so that you can just
> >>>>> implement the "multiboot,*" stuff in common code without the special
> >>>>> cases? (Although that depends on the reason for the other non-compat
> >>>>> string special cases too)
> >>>>
> >>>> I think the "multiboot,*" stuff will be better, once Xen supports
> >>>> this, I will update this code.
> >>>
> >>> I've only compile tested this, but I *think* this does the right thing.
> >>>
> >>> Ian.
> >>>
> >>> 8<-----------------
> >>>
> >>> From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001
> >>> From: Ian Campbell <ian.campbell@citrix.com>
> >>> Date: Wed, 4 Jun 2014 18:17:10 +0100
> >>> Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
> >>>
> >>> This causes Xen to accept the more generic names originally proposed by
> >>> Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and
> >>> incorporated into the proposal in
> >>> http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
> >>>
> >>> This will allow bootloaders to not special case Xen (or at least to reduce
> >>> the amount which is required).
> >>>
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>> ---
> >>>  xen/common/device_tree.c | 9 ++++++---
> >>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >>> index f0b17a3..5040097 100644
> >>> --- a/xen/common/device_tree.c
> >>> +++ b/xen/common/device_tree.c
> >>> @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node,
> >>>      struct dt_mb_module *mod;
> >>>      int len;
> >>>  
> >>> -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
> >>> +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> >>> +         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
> >>
> >> While we are modifying the protocol, "linux-zImage" is confusing in the
> >> name. Actually we can use it for an ELF, another OS... I don't think Xen
> >> will change his behavior depending of the DOM0 image.
> > 
> > zImage defines the boot protocol to use. Since the protocol is defined
> > by Linux as zImage I think that is the appropriate name, if some other
> > OS wants to mimic Linux then fine. But if we end up supporting some OS
> > with its own  boot protocol which doesn't match Linux's then that should
> > have a distinct name of its own.
> 
> zImage is the boot protocol for arm32, this is different for arm64.

They are essentially the same thing, modulo arch differences. It doesn't
seem worth defining linux-image as well as linux-zimage.

> Here
> we are talking about the protocol for dom0, why the bootloader is
> involved in it?

Ultimately the dom0 kernel is specified, by the user, in the bootloader
configuration file.

> How the interface will be exposed by grub to the user? Is it the
> "multiboot ..." line like in x86? If so, how grub will choose what is
> the boot protocol for the guest?

This is documented in the wiki. The grub syntax has defaults, but it
also supports explicit specification of the type.

> > The "ELF boot protocol" is as yet undefined. It may or may not end up
> > resembling zImage. In any case it will be a standard made up entirely by
> > us and as such xen-elf or something would be the correct name (unless it
> > happens to match zImage).
> 
> IHMO, the bootloader should only give the kernel blob to the hypervisor.
> Then it will decide what is the best protocol to use for booting dom0.

That might be nice, but is not how things are defined to work right now.

The fact is that this patch is a minor change which makes the existing
scheme less Xen specific and allows the grub implementation of multiboot
to not be to Xen specific.

If you want to make larger changes to the boot protocol then nothing
being proposed here stops you doing that in the future, we aren't
painting ourselves into any more corners than we are in without it.

Ian.

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
> >> While we are modifying the protocol, "linux-zImage" is confusing in the
> >> name. Actually we can use it for an ELF, another OS... I don't think Xen
> >> will change his behavior depending of the DOM0 image.

Actually thinking about this some more I think you are right. Xen
already probes the kernel it gets so we can safely implement this as
multiboot,kernel, since we don't really need the more specific type. If
in the future some non-probable kernel comes along which we want to
support we still have the option of adding more specific compatibility
strings.

Fu Wei -- if this is OK with you I will modify the wiki page to
s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.

Can we do something similar with linux-ramdisk? I'm not sure since we
cannot easily probe the ramdisk contents. We could base the ramdisk
behaviour on the probed behaviour of the kernel. Anyone got any
thoughts?

Ian.

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Julien Grall]

On 06/05/2014 07:31 PM, Ian Campbell wrote:
> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
>>>> will change his behavior depending of the DOM0 image.
> 
> Actually thinking about this some more I think you are right. Xen
> already probes the kernel it gets so we can safely implement this as
> multiboot,kernel, since we don't really need the more specific type. If
> in the future some non-probable kernel comes along which we want to
> support we still have the option of adding more specific compatibility
> strings.
> 
> Fu Wei -- if this is OK with you I will modify the wiki page to
> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
> 
> Can we do something similar with linux-ramdisk? I'm not sure since we
> cannot easily probe the ramdisk contents. We could base the ramdisk
> behaviour on the probed behaviour of the kernel. Anyone got any
> thoughts?

I have only check FreeBSD, and they don't have any bindings for the
ramdisk for now. It seems they use the command line for this purpose.

Probing the ramdisk won't help here because the magic and the underlying
filesystem might be the same.

I was about to say, we should do add a "multiboot,ramdisk" (or another
name) but we have to add the linux,initrd-* foo in the device tree.

In another hand keeping the actual properties with properties from
another ramdisk protocol won't harm here. Each kernel will deal with the
property it would like to use.

Regards,

-- 
Julien Grall

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Thu, 2014-06-05 at 22:00 +0100, Julien Grall wrote:
> On 06/05/2014 07:31 PM, Ian Campbell wrote:
> > On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
> >>>> While we are modifying the protocol, "linux-zImage" is confusing in the
> >>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
> >>>> will change his behavior depending of the DOM0 image.
> > 
> > Actually thinking about this some more I think you are right. Xen
> > already probes the kernel it gets so we can safely implement this as
> > multiboot,kernel, since we don't really need the more specific type. If
> > in the future some non-probable kernel comes along which we want to
> > support we still have the option of adding more specific compatibility
> > strings.
> > 
> > Fu Wei -- if this is OK with you I will modify the wiki page to
> > s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
> > 
> > Can we do something similar with linux-ramdisk? I'm not sure since we
> > cannot easily probe the ramdisk contents. We could base the ramdisk
> > behaviour on the probed behaviour of the kernel. Anyone got any
> > thoughts?
> 
> I have only check FreeBSD, and they don't have any bindings for the
> ramdisk for now. It seems they use the command line for this purpose.
> 
> Probing the ramdisk won't help here because the magic and the underlying
> filesystem might be the same.
> 
> I was about to say, we should do add a "multiboot,ramdisk" (or another
> name) but we have to add the linux,initrd-* foo in the device tree.
> 
> In another hand keeping the actual properties with properties from
> another ramdisk protocol won't harm here. Each kernel will deal with the
> property it would like to use.

Having thought about this I think the way I see it is that the module
contains a ramdisk and that is what should be described by the
compatibility string. The method by which this thing is then passed down
to the kernel is actually a function of the kernel in question, which we
have decided can be probed for. Something which is mimicking the Linux
arm/zImage or arm64/Image format can be expected to be mimicking the
Linux initrd protocol as well IMHO.

So I therefore intend to update the wiki with "multiboot,kernel" and
"multiboot,ramdisk" in place of "multiboot,linux-zimage" and
"multiboot,linux-ramdisk" respectively.

I think "boot,module" should also be "multiboot,module" for consistency.
I intend to make that substitution as well.

I will send out an updated Xen patch with those renamings in place.

Fu Wei, I'm very aware that we are redrafting this under your feet. I'm
sorry about this. I think what is being proposed here amounts to
changing a few string constants on your end, but if you have any
concerns at all please shout at me.

I was also planning to clarify the introduction to the wiki page to make
it clearer that the spec is intended to be generic in nature. I don't
think this should make any difference to the implementation, but again
do shout at me.

Ian.

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Fu Wei]

On 06/06/2014 07:47 PM, Ian Campbell wrote:
> On Thu, 2014-06-05 at 22:00 +0100, Julien Grall wrote:
>> On 06/05/2014 07:31 PM, Ian Campbell wrote:
>>> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
>>>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
>>>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
>>>>>> will change his behavior depending of the DOM0 image.
>>>
>>> Actually thinking about this some more I think you are right. Xen
>>> already probes the kernel it gets so we can safely implement this as
>>> multiboot,kernel, since we don't really need the more specific type. If
>>> in the future some non-probable kernel comes along which we want to
>>> support we still have the option of adding more specific compatibility
>>> strings.
>>>
>>> Fu Wei -- if this is OK with you I will modify the wiki page to
>>> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
>>>
>>> Can we do something similar with linux-ramdisk? I'm not sure since we
>>> cannot easily probe the ramdisk contents. We could base the ramdisk
>>> behaviour on the probed behaviour of the kernel. Anyone got any
>>> thoughts?
>>
>> I have only check FreeBSD, and they don't have any bindings for the
>> ramdisk for now. It seems they use the command line for this purpose.
>>
>> Probing the ramdisk won't help here because the magic and the underlying
>> filesystem might be the same.
>>
>> I was about to say, we should do add a "multiboot,ramdisk" (or another
>> name) but we have to add the linux,initrd-* foo in the device tree.
>>
>> In another hand keeping the actual properties with properties from
>> another ramdisk protocol won't harm here. Each kernel will deal with the
>> property it would like to use.
> 
> Having thought about this I think the way I see it is that the module
> contains a ramdisk and that is what should be described by the
> compatibility string. The method by which this thing is then passed down
> to the kernel is actually a function of the kernel in question, which we
> have decided can be probed for. Something which is mimicking the Linux
> arm/zImage or arm64/Image format can be expected to be mimicking the
> Linux initrd protocol as well IMHO.
> 
> So I therefore intend to update the wiki with "multiboot,kernel" and
> "multiboot,ramdisk" in place of "multiboot,linux-zimage" and
> "multiboot,linux-ramdisk" respectively.
> 
> I think "boot,module" should also be "multiboot,module" for consistency.
> I intend to make that substitution as well.
> 
> I will send out an updated Xen patch with those renamings in place.
> 
> Fu Wei, I'm very aware that we are redrafting this under your feet. I'm
> sorry about this. I think what is being proposed here amounts to
> changing a few string constants on your end, but if you have any
> concerns at all please shout at me.
> 
> I was also planning to clarify the introduction to the wiki page to make
> it clearer that the spec is intended to be generic in nature. I don't
> think this should make any difference to the implementation, but again
> do shout at me.

Sorry for late response, happy to see the code become more generic. 

And your modification for the wiki page  is good for me , will follow it to update my patch! :-)

Thanks 
> 
> Ian.
> 


-- 
Best regards,

Fu Wei
Enterprise Server Engineer From Red Hat
LEG Team
Linaro.org | Open source software for ARM SoCs
Ph: +86 186 2020 4684 (mobile)
IRC: fuwei
Skype: tekkamanninja
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021 

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Fu Wei]

On 06/06/2014 02:31 AM, Ian Campbell wrote:
> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
>>>> will change his behavior depending of the DOM0 image.
> 
> Actually thinking about this some more I think you are right. Xen
> already probes the kernel it gets so we can safely implement this as
> multiboot,kernel, since we don't really need the more specific type. If
> in the future some non-probable kernel comes along which we want to
> support we still have the option of adding more specific compatibility
> strings.
> 
> Fu Wei -- if this is OK with you I will modify the wiki page to
> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.

This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)

> 
> Can we do something similar with linux-ramdisk? I'm not sure since we
> cannot easily probe the ramdisk contents. We could base the ramdisk
> behaviour on the probed behaviour of the kernel. Anyone got any
> thoughts?

My thought looks exactly the same as yours :
The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.

> 
> Ian.
> 


-- 
Best regards,

Fu Wei
Enterprise Server Engineer From Red Hat
LEG Team
Linaro.org | Open source software for ARM SoCs
Ph: +86 186 2020 4684 (mobile)
IRC: fuwei
Skype: tekkamanninja
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021 

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Julien Grall]

On 06/06/2014 01:24 PM, Fu Wei wrote:
> On 06/06/2014 02:31 AM, Ian Campbell wrote:
>> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
>>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
>>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
>>>>> will change his behavior depending of the DOM0 image.
>>
>> Actually thinking about this some more I think you are right. Xen
>> already probes the kernel it gets so we can safely implement this as
>> multiboot,kernel, since we don't really need the more specific type. If
>> in the future some non-probable kernel comes along which we want to
>> support we still have the option of adding more specific compatibility
>> strings.
>>
>> Fu Wei -- if this is OK with you I will modify the wiki page to
>> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
> 
> This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
> 
>>
>> Can we do something similar with linux-ramdisk? I'm not sure since we
>> cannot easily probe the ramdisk contents. We could base the ramdisk
>> behaviour on the probed behaviour of the kernel. Anyone got any
>> thoughts?
> 
> My thought looks exactly the same as yours :
> The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.

cpio is not Linux specific. Probing just the file won't help here to to
determine if we have to add the properties linux,initrd-* or another set.

Regards,

-- 
Julien Grall

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Fri, 2014-06-06 at 13:28 +0100, Julien Grall wrote:
> On 06/06/2014 01:24 PM, Fu Wei wrote:
> > On 06/06/2014 02:31 AM, Ian Campbell wrote:
> >> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
> >>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
> >>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
> >>>>> will change his behavior depending of the DOM0 image.
> >>
> >> Actually thinking about this some more I think you are right. Xen
> >> already probes the kernel it gets so we can safely implement this as
> >> multiboot,kernel, since we don't really need the more specific type. If
> >> in the future some non-probable kernel comes along which we want to
> >> support we still have the option of adding more specific compatibility
> >> strings.
> >>
> >> Fu Wei -- if this is OK with you I will modify the wiki page to
> >> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
> > 
> > This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
> > 
> >>
> >> Can we do something similar with linux-ramdisk? I'm not sure since we
> >> cannot easily probe the ramdisk contents. We could base the ramdisk
> >> behaviour on the probed behaviour of the kernel. Anyone got any
> >> thoughts?
> > 
> > My thought looks exactly the same as yours :
> > The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
> 
> cpio is not Linux specific. Probing just the file won't help here to to
> determine if we have to add the properties linux,initrd-* or another set.

This can/should be determined by probing the type of kernel which the
initrd is being passed to, it is not a property of the initrd.

Ian.

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Julien Grall]

On 06/06/2014 01:32 PM, Ian Campbell wrote:
> On Fri, 2014-06-06 at 13:28 +0100, Julien Grall wrote:
>> On 06/06/2014 01:24 PM, Fu Wei wrote:
>>> On 06/06/2014 02:31 AM, Ian Campbell wrote:
>>>> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
>>>>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
>>>>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
>>>>>>> will change his behavior depending of the DOM0 image.
>>>>
>>>> Actually thinking about this some more I think you are right. Xen
>>>> already probes the kernel it gets so we can safely implement this as
>>>> multiboot,kernel, since we don't really need the more specific type. If
>>>> in the future some non-probable kernel comes along which we want to
>>>> support we still have the option of adding more specific compatibility
>>>> strings.
>>>>
>>>> Fu Wei -- if this is OK with you I will modify the wiki page to
>>>> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
>>>
>>> This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
>>>
>>>>
>>>> Can we do something similar with linux-ramdisk? I'm not sure since we
>>>> cannot easily probe the ramdisk contents. We could base the ramdisk
>>>> behaviour on the probed behaviour of the kernel. Anyone got any
>>>> thoughts?
>>>
>>> My thought looks exactly the same as yours :
>>> The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
>>
>> cpio is not Linux specific. Probing just the file won't help here to to
>> determine if we have to add the properties linux,initrd-* or another set.
> 
> This can/should be determined by probing the type of kernel which the
> initrd is being passed to, it is not a property of the initrd.

I don't find anything documentation in Linux tree that claim the
linux,initrd-* properties is part of the zImage protocol.

FYI, the device tree documentation in Linux
(Documentation/devicetree/usage-model.txt) is talking about
initrd-{start,end}, not linux,initrd-{start,end}.

If we plan to assume that the zImage always go with linux,initrd*
properties then we have to document this specification somewhere.

Regards,

-- 
Julien Grall

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Fri, 2014-06-06 at 14:25 +0100, Julien Grall wrote:
> On 06/06/2014 01:32 PM, Ian Campbell wrote:
> > On Fri, 2014-06-06 at 13:28 +0100, Julien Grall wrote:
> >> On 06/06/2014 01:24 PM, Fu Wei wrote:
> >>> On 06/06/2014 02:31 AM, Ian Campbell wrote:
> >>>> On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
> >>>>>>> While we are modifying the protocol, "linux-zImage" is confusing in the
> >>>>>>> name. Actually we can use it for an ELF, another OS... I don't think Xen
> >>>>>>> will change his behavior depending of the DOM0 image.
> >>>>
> >>>> Actually thinking about this some more I think you are right. Xen
> >>>> already probes the kernel it gets so we can safely implement this as
> >>>> multiboot,kernel, since we don't really need the more specific type. If
> >>>> in the future some non-probable kernel comes along which we want to
> >>>> support we still have the option of adding more specific compatibility
> >>>> strings.
> >>>>
> >>>> Fu Wei -- if this is OK with you I will modify the wiki page to
> >>>> s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
> >>>
> >>> This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
> >>>
> >>>>
> >>>> Can we do something similar with linux-ramdisk? I'm not sure since we
> >>>> cannot easily probe the ramdisk contents. We could base the ramdisk
> >>>> behaviour on the probed behaviour of the kernel. Anyone got any
> >>>> thoughts?
> >>>
> >>> My thought looks exactly the same as yours :
> >>> The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
> >>
> >> cpio is not Linux specific. Probing just the file won't help here to to
> >> determine if we have to add the properties linux,initrd-* or another set.
> > 
> > This can/should be determined by probing the type of kernel which the
> > initrd is being passed to, it is not a property of the initrd.
> 
> I don't find anything documentation in Linux tree that claim the
> linux,initrd-* properties is part of the zImage protocol.
> 
> FYI, the device tree documentation in Linux
> (Documentation/devicetree/usage-model.txt) is talking about
> initrd-{start,end}, not linux,initrd-{start,end}.
> 
> If we plan to assume that the zImage always go with linux,initrd*
> properties then we have to document this specification somewhere.

If something is going to pretend to be Linux (by pretending to be a
Linux zImage) then it should behave like Linux on boot. I think this
extends to initrd handling.

If what Linux does is undocumented (or the docs are out of step with
reality), well, that's too bad.

Feel free to work on improving Linux's documentation. I think it is
orthogonal to the questions raised in this thread though.

Ian.

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Julien Grall]

On 06/06/14 15:24, Ian Campbell wrote:
>> If we plan to assume that the zImage always go with linux,initrd*
>> properties then we have to document this specification somewhere.
>
> If something is going to pretend to be Linux (by pretending to be a
> Linux zImage) then it should behave like Linux on boot. I think this
> extends to initrd handling.
>
> If what Linux does is undocumented (or the docs are out of step with
> reality), well, that's too bad.
>
> Feel free to work on improving Linux's documentation. I think it is
> orthogonal to the questions raised in this thread though.

Thanks I will try to send a patch for the Linux documentation to clarify it.

-- 
Julien Grall

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Leif Lindholm]

On Thu, Jun 05, 2014 at 05:55:31PM +0100, Ian Campbell wrote:
> > > -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
> > > +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> > > +         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
> > 
> > While we are modifying the protocol, "linux-zImage" is confusing in the
> > name. Actually we can use it for an ELF, another OS... I don't think Xen
> > will change his behavior depending of the DOM0 image.
> 
> zImage defines the boot protocol to use. Since the protocol is defined
> by Linux as zImage I think that is the appropriate name, if some other
> OS wants to mimic Linux then fine. But if we end up supporting some OS
> with its own  boot protocol which doesn't match Linux's then that should
> have a distinct name of its own.

Actually, there is no zImage support in arm64 - only Image.

/
    Leif

Re: [Linaro-uefi] [PATCH] xen: arm: implement generic multiboot compatibility strings (Was: Re: The GRUB multiboot support patch for aarch64(V3.1))

[Ian Campbell]

On Thu, 2014-06-05 at 18:05 +0100, Leif Lindholm wrote:
> On Thu, Jun 05, 2014 at 05:55:31PM +0100, Ian Campbell wrote:
> > > > -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
> > > > +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> > > > +         fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
> > > 
> > > While we are modifying the protocol, "linux-zImage" is confusing in the
> > > name. Actually we can use it for an ELF, another OS... I don't think Xen
> > > will change his behavior depending of the DOM0 image.
> > 
> > zImage defines the boot protocol to use. Since the protocol is defined
> > by Linux as zImage I think that is the appropriate name, if some other
> > OS wants to mimic Linux then fine. But if we end up supporting some OS
> > with its own  boot protocol which doesn't match Linux's then that should
> > have a distinct name of its own.
> 
> Actually, there is no zImage support in arm64 - only Image.

Right, that's a slight wrinkle in the naming. arm64's image is
essentially equivalent (from a calling into it PoV) to zImage, it
doesn't seem worth dupping the name here.

Ian.