WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ppc-devel

Re: [XenPPC] [Patch] Detect non hypervisor hardware

To: Maria Butrico <butrico@xxxxxxxxxxxxxx>
Subject: Re: [XenPPC] [Patch] Detect non hypervisor hardware
From: Hollis Blanchard <hollisb@xxxxxxxxxx>
Date: Fri, 14 Apr 2006 09:54:40 -0500
Cc: xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Fri, 14 Apr 2006 07:53:58 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <E1FTPri-00024F-Jn@xxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-ppc-devel-request@lists.xensource.com?subject=help>
List-id: Xen PPC development <xen-ppc-devel.lists.xensource.com>
List-post: <mailto:xen-ppc-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ppc-devel>, <mailto:xen-ppc-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ppc-devel>, <mailto:xen-ppc-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: IBM Linux Technology Center
References: <E1FTPri-00024F-Jn@xxxxxxxxxxxxxxxxxxxxx>
Sender: xen-ppc-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Tue, 2006-04-11 at 16:53 -0400, Maria Butrico wrote:
> Signed-off-by: Maria Butrico <butrico@xxxxxxxxxxxxxx> and Mark Mergen 
> <mergen@xxxxxxxxxx>
> 
> summary:     Detect non hypervisor hardware
> 
> Added a flag, opt_nohv.  Ths flag is set if
> 1) the operator specifies the nohv boot argument; or
> 2) the hardware on which we are running has the open firmware
>    property '/compatible' such that it contains the string
>    "Power Macintosh".  Both the rack mounted G5 and the tower
>    G5 do have such string in this property.
> 
> Added a domain flag option, _DOMF_prob, that indicates that the
> domain is running in problem state.  Code that uses this flag is
> not part of this patch.  

Hi Maria, I don't think it makes sense to add these flags before there
is code that uses them.

A few additional comments below:

> diff -r 47697f5b6e13 xen/arch/ppc/boot_of.c
> --- a/xen/arch/ppc/boot_of.c  Tue Apr  4 18:21:00 2006 -0400
> +++ b/xen/arch/ppc/boot_of.c  Tue Apr 11 12:13:49 2006 -0400
> @@ -24,6 +24,7 @@
>  #include <xen/compile.h>
>  #include <public/of-devtree.h>
>  #include <asm/page.h>
> +#include <xen/string.h>
>  
>  static ulong of_vec;
>  static ulong of_msr;
> @@ -33,6 +34,7 @@ static char dom0args[256];
>  static char dom0args[256];
>  
>  extern unsigned int timebase_freq;
> +extern int opt_nohv;
>  
>  #undef OF_DEBUG
>  
> @@ -825,6 +827,56 @@ int __init boot_of_rtas(void)
>      return 1;
>  }
>  
> +/*
> + * return 0 if all ok
> + * OF_FAILURE if error
> + */
> +int __init boot_of_nohv(void)
> +{
> +    char xservers_string[] = "Power Macintosh";

"xservers" doesn't seem appropriate.

I would prefer if there were a more targeted way to detect that
hypervisor mode has been disabled. If there is none (and AFAIK there
isn't), then I'm OK with checking the compatible property.

> +    int of_root;
> +    char compatible_string[256];
> +    int compatible_length;
> +    
> +    of_root = of_finddevice("/");
> +    if (OF_FAILURE == of_root) 
> +        return OF_FAILURE;
> +
> +    compatible_length = of_getprop(of_root, "compatible", compatible_string,
> +                                   sizeof (compatible_string));
> +    if (compatible_length != OF_FAILURE) {
> +       // null separated string yuck
> +       char *cmpstr;
> +       int used_length;
> +#if 0
> +       of_printf("%s :\n", __func__);
> +       for (used_length = 0; used_length < compatible_length;
> +                           used_length++) {
> +                  if (compatible_string[used_length] == '\0' ) {
> +                          of_printf("   NULL\n");
> +                  } else {
> +                          of_printf("%c", compatible_string[used_length]);
> +                  }
> +       }
> +#endif

This looks like debugging code that we probably won't need to ever
enable again? In that case, I'd leave it out of the patch.

> +       // loop over each null terminated piece of the compatible property
> +       for (cmpstr = compatible_string, used_length = 0;
> +           used_length < compatible_length;
> +           cmpstr = &compatible_string[used_length]) {
> +           if (strstr(cmpstr, xservers_string)) {
> +               of_printf("Non hypervisor hardware found.  Call Mark 
> Mergen!\n");
> +               opt_nohv = 1;
> +               return 0;
> +           }
> +           used_length += strlen(cmpstr) + 1;
> +       }
> +    } else {
> +        return OF_FAILURE;
> +    }
> +
> +    return 0;
> +}
> +
>  multiboot_info_t __init *boot_of_init(
>          ulong r3, ulong r4, ulong vec, ulong r6, ulong r7, ulong orig_msr)
>  {
> @@ -861,6 +913,8 @@ multiboot_info_t __init *boot_of_init(
>      boot_of_serial();
>      boot_of_cpus();
>      boot_of_rtas();
> +
> +    boot_of_nohv();
>  
>      /* end of OF */
>      of_printf("closing OF stdout...\n");
> diff -r 47697f5b6e13 xen/arch/ppc/setup.c
> --- a/xen/arch/ppc/setup.c    Tue Apr  4 18:21:00 2006 -0400
> +++ b/xen/arch/ppc/setup.c    Tue Apr 11 12:13:49 2006 -0400
> @@ -43,6 +43,12 @@ int opt_noht = 0;
>  int opt_noht = 0;
>  boolean_param("noht", opt_noht);
>  
> +/* opt_nohv: If true, hardware has no hypervisor (HV) mode or nohv boot 
> + * parameter was specified. 
> + */
> +int opt_nohv = 0;
> +boolean_param("nohv", opt_nohv);
> +
>  int opt_earlygdb = 0;
>  boolean_param("earlygdb", opt_earlygdb);
>  
> @@ -282,6 +288,8 @@ static void __init __start_xen(multiboot
>      if (dom0 == NULL)
>          panic("Error creating domain 0\n");
>      set_bit(_DOMF_privileged, &dom0->domain_flags);
> +    if (opt_nohv)
> +     set_bit(_DOMF_prob, &dom0->domain_flags);
>  
>      /* Grab the DOM0 command line. Skip past the image name. */
>      cmdline = (char *)(mod[0].string ? __va((ulong)mod[0].string) : NULL);
> diff -r 47697f5b6e13 xen/include/xen/sched.h
> --- a/xen/include/xen/sched.h Tue Apr  4 18:21:00 2006 -0400
> +++ b/xen/include/xen/sched.h Tue Apr 11 12:13:49 2006 -0400
> @@ -388,6 +388,10 @@ extern struct domain *domain_list;
>   /* Domain is being debugged by controller software. */
>  #define _DOMF_debugging        4
>  #define DOMF_debugging         (1UL<<_DOMF_debugging)
> + /* run domain in prob mode */
> +#define _DOMF_prob      7
> +#define DOMF_prob       (1UL<<_DOMF_prob)

"Problem state" is a term that nobody outside PowerPC will understand,
so I would prefer a different name.

Also, sched.h is shared among all architectures, and so the DOMF_* flags
are not architecture-specific. I think it would be better to add a flag
to struct arch_domain.

-- 
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel