Details
- Reviewers
royger ehem_freebsd_m5p.com
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
This is wrong. Specifically machine/xen/arch-intr.h is the MI <=> MD interface whereas xen_arch_intr_handle_upcall() is the x86 assembly-language <=> C interface. Mixing the two together would be bad.
Why are you proposing this? The function isn't declared in a header since the assembler doesn't understand those. If you were thinking of calling xen_arch_intr_handle_upcall() from C that is almost certainly a Bad Idea. The function which needs to be called is xen_intr_handle_upcall(), but that function must conform to the driver_filter_t interface (since it needs to be MI and be called as a driver-filter).
Are you perhaps aiming to merge all the assembly-language <=> C bridges? You might want to look at Github #1748. It appears intr_event_handle()'s interface originated on x86 with minimal variation between PICs and cascading PICs being rare (aside from the pair of 8250s, but those are handled by the same driver). Whereas the moment cascading PICs appear intr_event_handle()'s interface really needs fixing.
It is C function, it can be called from C. What is the problem?
Why are you proposing this? The function isn't declared in a header since the assembler doesn't understand those. If you were thinking of calling xen_arch_intr_handle_upcall() from C that is almost certainly a Bad Idea. The function which needs to be called is xen_intr_handle_upcall(), but that function must conform to the driver_filter_t interface (since it needs to be MI and be called as a driver-filter).
Are you perhaps aiming to merge all the assembly-language <=> C bridges? You might want to look at Github #1748. It appears intr_event_handle()'s interface originated on x86 with minimal variation between PICs and cascading PICs being rare (aside from the pair of 8250s, but those are handled by the same driver). Whereas the moment cascading PICs appear intr_event_handle()'s interface really needs fixing.
This is the wrong header to have that in. Most of the */xen/*.h headers are at least semi-private so anything outside of the Xen interface shouldn't be including them. I'm unsure of the proper place to put such a declaration, but machine/arch-intr.h is certainly wrong. Since this isn't used internally by the Xen interface the two headers I notice are machine/intr_machdep.h and x86/apicvar.h. Since this is a semi-internal PIC function x86/apicvar.h almost seems the best of those.
This is still a distortion, but far less of one than having it in machine/xen/arch-intr.h. Naturally this means header adjustment elsewhere.
It might be easier if I just switch Xen to use lapic_ipi_alloc() like mentioned in the other review. That would prevent more Xen stuff leaking into generic headers which is not specially nice.