Page MenuHomeFreeBSD

Merge i386 and amd64 md_var.h and smp.h.
ClosedPublic

Authored by kib on Dec 3 2015, 1:27 PM.
Tags
None
Referenced Files
F103280322: D4358.diff
Sat, Nov 23, 12:34 AM
Unknown Object (File)
Oct 19 2024, 3:35 AM
Unknown Object (File)
Oct 19 2024, 3:35 AM
Unknown Object (File)
Oct 19 2024, 3:35 AM
Unknown Object (File)
Oct 19 2024, 3:35 AM
Unknown Object (File)
Oct 19 2024, 3:13 AM
Unknown Object (File)
Oct 4 2024, 11:14 PM
Unknown Object (File)
Oct 4 2024, 5:02 PM
Subscribers

Details

Summary

As said in the title, the common parts are moved to x86/include/x86_smp.h and x86/include/x86_var.h.

I did not tried to fix the inthand_t and alsias_for_inthand_t mess (see types and esp. types for amd64).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib retitled this revision from to Merge i386 and amd64 md_var.h and smp.h..
kib updated this object.
kib edited the test plan for this revision. (Show Details)
kib added reviewers: jhb, emaste.
kib set the repository for this revision to rS FreeBSD src repository - subversion.

Also, the question is what copyright notice to add to the x86_smp.h. I do not like beer myself and do not want to give a bootle to phk either.

I've wanted to do this for md_var.h for a while. Thanks. I'm ambivalent about the license. I think you are probably stuck using it for x86_smp.h since all the subsequent changes to those files have been under that license. Also, it's not clear from arc, but are you copying these files from amd64 to x86?

sys/amd64/include/smp.h
24–25

Even though i386 doesn't use it, this one could move to the shared header since it is declared in the shared code.

sys/x86/include/x86_smp.h
17

Might be nice to update the comment here to x86_mp.c. Not sure if any of these are
declared in mp_machdep.c still?

Oh, I'm not sure what to do to fix the alias_for_inthand_t silliness. I must be a bit daft as I don't understand why setidt() can't just accept inthand_t by default? Also, inthand_t on amd64 (intr_machdep.h) is arguably "wrong", but you'd have to fix the alias. I wonder if this is just so that md_var.h doesn't require intr_machdep.h? Maybe we could move inthand_t to a more "basic" header instead?

kib edited edge metadata.

I am trying to answer all raised questions, I am sorry if I missed any.

  1. I did not copied any file technically, when I produced the patch. I took empty x86_var.h and x86_smp.h and moved definition one-by-one. This does not have any significance on the copyright ownership.
  1. I moved smp_tlb_pmap into the x86_smp.h header. My motivation for keeping it only in amd64/include/smp.h was to reduce scope.
  1. Comments in x86_mp.c are updated, I claim that almost all of the functions are in x86_mp.c.
  1. inthand_t is a mess. Mostly because amd64 interrupt handler calling conventions cannot be expressed in C. Might be, for amd64 we should use void ()(void) type ? But fixing this is definitely for the follow-up change.
  1. inthand_t alias indeed was added because authors avoided intr_machdep.h. Looking at the intent of intr_machdep.h, it is about code for high-level interrupt handling, which is subtly unrelated to the CPU IDT stuff. I believe this explains parts of the mess. setidt() and low-level handlers do not related to intr_machdep.c.

For items 4 and 5, I will do something after the current patch is committed.

In D4358#92018, @kib wrote:

I am trying to answer all raised questions, I am sorry if I missed any.

  1. I did not copied any file technically, when I produced the patch. I took empty x86_var.h and x86_smp.h and moved definition one-by-one. This does not have any significance on the copyright ownership.

I was asking more if you were going to use 'svn cp' for the purposes of annotate history.

  1. I moved smp_tlb_pmap into the x86_smp.h header. My motivation for keeping it only in amd64/include/smp.h was to reduce scope.

Thanks.

  1. Comments in x86_mp.c are updated, I claim that almost all of the functions are in x86_mp.c.

Thanks.

  1. inthand_t is a mess. Mostly because amd64 interrupt handler calling conventions cannot be expressed in C. Might be, for amd64 we should use void ()(void) type ? But fixing this is definitely for the follow-up change.
  1. inthand_t alias indeed was added because authors avoided intr_machdep.h. Looking at the intent of intr_machdep.h, it is about code for high-level interrupt handling, which is subtly unrelated to the CPU IDT stuff. I believe this explains parts of the mess. setidt() and low-level handlers do not related to intr_machdep.c.

Yes, I think that it is a quite reasonable split, and yes I agree that this patch shouldn't deal with it.

I think the current C prototype is wonky anyway because we never define any handlers in C anyway. In that case I think void ()(void) is more appropriate.

In D4358#92260, @jhb wrote:
In D4358#92018, @kib wrote:
  1. I did not copied any file technically, when I produced the patch. I took empty x86_var.h and x86_smp.h and moved definition one-by-one. This does not have any significance on the copyright ownership.

I was asking more if you were going to use 'svn cp' for the purposes of annotate history.

Most likely yes, I did it for the previous merges. I think that i386 version has longer history and thus is preferred.

I think the current C prototype is wonky anyway because we never define any handlers in C anyway. In that case I think void ()(void) is more appropriate.

Yes, this sounds as a good idea to change i386 to void ()(void) as well.

jhb edited edge metadata.
This revision is now accepted and ready to land.Dec 7 2015, 5:02 PM

I'd prefer we end up with a standard 2 clause license if phk agrees, but am happy for you to commit this version and then make that change later.

emaste edited edge metadata.
This revision was automatically updated to reflect the committed changes.