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

[Maria Butrico <butrico@xxxxxxxxxxxxx>]

Hollis Blanchard wrote:
> 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:
>
>   
I disagree.  We are trying to stage this code in easy-to-understand and 
isolated pieces, this being the first one, that is a way to detect 
hardware where hyper/supervisor are not distinguished.
> > 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.
>   
What would you use?  I am open to reasonable suggestions.
> 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.
>   
Only the ifdef'ed out part is debug code and I suspect we might need 
again.  (Otherwise I would not have sent it with the patch).  If you 
feel strongly about this I don't care, since it would be quite easy to 
add it again.
>   
> > +       // 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.
>   
I leave this one for Mark to argue one way or another.
> 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.
>
>   
Probably true, in which case we need to establish a few conventions, 
with associated clear comments, that list the include files where the 
definitions for the values of these bits are.    Probably these are 
already in place, but I don't know about them. 


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