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

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

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