> That being said, instead of hardcoding the maximum number of CPUs to > be 256, you can just let the user choose whatever value is preferred. > That's what Linux does.
But this could cause coherency problems. By example, if the user sets mach_ncpus=4 , and the processor has 8 cores, It can produce an out-of-index error in the access to the arrays which store the info about the cpus. > Re-read what I wrote. I was talking about lapic being qualified > volatile, and apic_get_lapic returning it as non-volatile. Aren't you > getting a warning there? Yes, I've just fixed It. > I can't see how this declaration can't be in kern/smp.h. It's not even > returning a struct or whatever. What is the actual error message when > you put it in kern/smp.h? I've just solved It declaring *smp_info *in kern/smp.c , and adding an *extern *declaration in its header. > What for? > num_cpus is something that you make returned by a smp_get_numcpus() > function, so it's not actually useful to also expose it in a struct. > What else would go into that structure? By example, the master cpu (BSP in x86) > But the function returns an APIC id. Really not much can be done with > that without knowing details of the APIC. It's true. I didn't remember this. I can search the kernel ID of this APIC ID, but cpu_number() already will do that. Then, maybe it can be simpler to remove this function. My idea was to avoid calling directly to the apic function when I will implement cpu_number(). But maybe this is not the best solution for this. I have to rethink this. > What for? > num_cpus is something that you make returned by a smp_get_numcpus() > function, so it's not actually useful to also expose it in a struct. > What else would go into that structure? Won't we actually rather > use functions to return such information rather than imposing that > structure over all archs? I will not use this struct to share the information, simply to ease the access to the functions which really return such information. smp_get_numcpus() doesn't return NCPUS value, but returns the real number of cpus existent in the machine. I simply use a struct to store this generic information instead access to apic's ApicInfo struct (which stores this value in x86). > Re-read what I wrote. I do *not* think we want to print this as an > error like you did. Not having SMP is not an error. Just do not print > anything. Really, the mistake is in the message. I wanted to advise that the processor detection has failed in any step (although maybe the cause is not that SMP doesn't exist in the machine...) Then, I will have to change this message to a better explanation. But showing a different message for each error code can be very lazy... :( El mar., 11 ago. 2020 a las 1:39, Samuel Thibault (<samuel.thiba...@gnu.org>) escribió: > Almudena Garcia, le mar. 11 août 2020 01:23:49 +0200, a ecrit: > > > ? git diff produces the same as format-patch, in terms of formatting > > > mistakes... The disadvantage of git diff is that your patches then mix > > > things altogether, see the additions to Makefrag.am. I just cannot > apply > > > the series as it is. > > > > Yes, The problem is that I didn't write each file in a single commit. > > Then use git rebase to rewrite your patch history? > > > > ? The whole point of the mach_ncpus variable is to contain the number > > > of processors. I don't see why we could want mach_ncpus to be defined > to > > > a different value than NCPUS. > > > > As I explained in a previous mail, in Mach source code the are many > structures > > (arrays) which size is defined by NCPUS. > > Added to this, all SMP special cases are controlled by preprocessor > directives > > like "if NCPUS > 1". For this reason, removing NCPUS can be a very > difficult > > task. > > I didn't write about removing NCPUS. I wrote about your code defining > NCPUS to a value that is different from mach_ncpus, while it could just > define mach_ncpus, and let NCPUS inherit the value from mach_ncpus so > it's all coherent. > > That being said, instead of hardcoding the maximum number of CPUs to > be 256, you can just let the user choose whatever value is preferred. > That's what Linux does. > > > > Err, it's empty, so just drop the file. > > I prefer to keep this file, to use that as an arch-independent interface > for > > SMP. > > But there is nothing there to be compiled. Some compilation toolchains > would even emit a warning in such a case, or just plainly error out > because they cannot produce an empty .o file. > > > > Does it really need to be a separate function? Will we ever want to > call > > > it somewhere else than smp_init? If not just fold it into smp_init, or > > > make it static, there is no point in making it extern. > > > > I put It in a separate function because It's possible that, in next > steps, I > > will need to add more fields to the structure, which will need to be > > initialized together. > > Ok, but since smp_init is almost empty, you can as well initialize the > data in there. > > > > Is this function really useful? I mean in the long run we will want a > > > CPU number from 0, which will have to be knowing the apic enumeration, > > > and thus that's probably acpi.c that will define it anyway, and that is > > > the function whose declaration will belong to kernel/smp.h > > > > The idea about this function is to know the number of cpus (this will be > > necessary to enable the cpus in next steps), without knowing the details > about > > APIC. > > But the function returns an APIC id. Really not much can be done with > that without knowing details of the APIC. > > > > > +volatile ApicLocalUnit* lapic = NULL; > > > [...] > > > > If, by any reason, we can't find APIC structures, o we need to have a > secure > > value for lapic pointer, to take notice that APIC search or parsing has > failed. > > And I simply prefered set this value at start. When we find the APIC > table, if > > the parser is successful, this pointer will take the real value of the > common > > Local APIC memory address. > > For global variables the default value is *already* asserted to be NULL. > But anyway I was not talking about that at all. > > Re-read what I wrote. I was talking about lapic being qualified > volatile, and apic_get_lapic returning it as non-volatile. Aren't you > getting a warning there? > > > > That one belongs to kern/smp.h: non-i386 code will probably want to use > > > it. > > Yes, this was my idea. But It was very difficult to declare smp_info > structure > > in kern/apic.c , and then share this structure with i386/i386/smp.c to > > initialize its data. > > I had many compilation problems, so I had to put this function there to > ease > > the compilation. > > ????????????????????????????? > > I can't see how this declaration can't be in kern/smp.h. It's not even > returning a struct or whatever. What is the actual error message when > you put it in kern/smp.h? > > > > ? Is this really useful to expose to the rest of the kernel? Isn't that > > > structure something specific to the i386 SMP implementation? > > > > Really, the smp_data structure might be generic for all architectures. > > What for? > num_cpus is something that you make returned by a smp_get_numcpus() > function, so it's not actually useful to also expose it in a struct. > What else would go into that structure? Won't we actually rather > use functions to return such information rather than imposing that > structure over all archs? > > > The only reason because I declared smp_info in i386 files is because I > > didn't get to compile the code declaring it in kern/smp.c. > > Which is possibly another sign that it's generally simpler to keep it in > the arch-specific part, and only expose simple functions like > int smp_get_numcpus(void); which pose no declaration problems. > > > > ? I don't think we want to print this as an error? > > Is there a function to print messages as errors? If yes, then I replace > this > > print with It. > > Re-read what I wrote. I do *not* think we want to print this as an > error like you did. Not having SMP is not an error. Just do not print > anything. > > Samuel >
