On Fri, 2006-04-14 at 11:12 -0400, Maria Butrico wrote:
> 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.
You're absolutely right: each patch should add a self-contained piece of
functionality (and ideally that means small and easy-to-understand).
However, that doesn't mean they should go in one by one, potentially
months apart. That would mean dead code hanging around in the tree for
all that time, plus later on maybe you'll discover a better way that
obviates the original patch entirely.
Also, it's hard to judge some patches in isolation. For example, maybe
we'll discover that rather than having a per-domain flag, it would make
more sense to fill in a couple per-domain function descriptors. That
observation is impossible to make without seeing users of the flag.
This is why you'll see some people in the Linux world sending in 20
patches all at once.
> >> 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.
The variable is named "xservers" (and btw the Apple product is called
"Xserve", not "Xserver"), but the string says "Power Macintosh".
I'd call it something like "nohv_models", and also make it global (or at
least a #define).
> > 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.
Yeah, please remove it.
> >> + // loop over each null terminated piece of the compatible property
Oops, forgot to comment on this: let's be consistent and not use //
commenting.
> >> + 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");
... and also let's leave out "call Mark Mergen".
> >> + 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;
Don't need to explicitly initialize this.
> >> +boolean_param("nohv", opt_nohv);
> >> +
> >> int opt_earlygdb = 0;
(Or this; it's not your code but if you submit a patch I'd happily
apply.)
> >> 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);
A tab snuck in here.
> > 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.
Don't overestimate our codebase. ;)
--
Hollis Blanchard
IBM Linux Technology Center
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
Home