Damien Zammit, le sam. 07 mars 2026 21:57:17 +0000, a ecrit:
> index 8bf69a68..c0d9f630 100644
> --- a/i386/i386/ast_check.c
> +++ b/i386/i386/ast_check.c
> @@ -50,7 +50,11 @@ void init_ast_check(const processor_t processor)
> */
> void cause_ast_check(const processor_t processor)
> {
> +#ifndef MACH_XEN
> smp_remote_ast(APIC_LOGICAL_ID(processor->slot_num));
> +#else
> +#warning cause_ast_check not implemented on xen?
> +#endif
> }
>
> #endif /* NCPUS > 1 */
Mmmm.
As long as we do know that this is missing, we want to shout it very
loudly, and not just as a build warning.
I'd say rather put an assertion failure
> diff --git a/i386/i386/cpu_number.h b/i386/i386/cpu_number.h
> index 4e894a00..af0b93cb 100644
> --- a/i386/i386/cpu_number.h
> +++ b/i386/i386/cpu_number.h
> @@ -38,8 +38,13 @@
> #define CX8(addr, reg) addr(,reg,8)
>
> /* Fastest version, requires gs being set up */
> +#ifdef MACH_XEN
> +#define CPU_NUMBER(reg) \
> + movl MY(VCPU_ID), reg;
> +#else
> #define CPU_NUMBER(reg) \
> movl MY(CPU_ID), reg;
> +#endif
I don't see why using a different field?
Isn't vcpu equal to cpu?
> diff --git a/i386/i386/locore.S b/i386/i386/locore.S
> index 905e1af9..19e674c3 100644
> --- a/i386/i386/locore.S
> +++ b/i386/i386/locore.S
> @@ -492,7 +492,7 @@ ENTRY(t_page_fault)
> pushl $(T_PAGE_FAULT) /* mark a page fault trap */
> pusha /* save the general registers */
> #ifdef MACH_PV_PAGETABLES
> - movl %ss:hyp_shared_info+CR2,%eax
> + movl %ss:hyp_shared_info+CPU0_CR2,%eax
I don't think that will work in SMP? In t_page_fault we want the cr2
field of the current CPU, not of CPU0.
> #else /* MACH_PV_PAGETABLES */
> movl %cr2,%eax /* get the faulting address */
> #endif /* MACH_PV_PAGETABLES */
> diff --git a/i386/i386/mp_desc.c b/i386/i386/mp_desc.c
> index 1343861c..406cf2bf 100644
> --- a/i386/i386/mp_desc.c
> +++ b/i386/i386/mp_desc.c
> @@ -205,7 +207,9 @@ cpu_control(int cpu, const int *info, unsigned int count)
> void
> interrupt_processor(int cpu)
> {
> +#ifndef MACH_XEN
> smp_pmap_update(APIC_LOGICAL_ID(cpu));
> +#endif
Again, we don't want to hide that it's not implemented yet, we do want
to shout that we still need to implement sending an IPI.
Otherwise we will get odd memory mapping issues with not obvious clue
where the issue is, and only very late find out that we are simply
missing the pmap update IPI.
> }
>
> static void
> @@ -225,6 +229,7 @@ paging_enable(void)
> #endif /* MACH_HYP */
> }
>
> +#ifndef MACH_XEN
> static __attribute__((noreturn)) void
> cpu_setup(int cpu)
> {
> +void cpu_ap_main(void) {
> + int vcpu = cpu_number();
> +
> + assert (vcpu > 0);
> +
> + mp_desc_init(vcpu);
> + printf("vCPU=(%u) mpdesc done\n", vcpu);
> +
> + ap_gdt_init(vcpu);
> + printf("vCPU=(%u) gdt done\n", vcpu);
> +
> + ap_idt_init(vcpu);
> + printf("vCPU=(%u) idt done\n", vcpu);
> +
> + ap_ldt_init(vcpu);
> + printf("vCPU=(%u) ldt done\n", vcpu);
> +
> + ap_ktss_init(vcpu);
> + printf("vCPU=(%u) ktss done\n", vcpu);
> +
> + /* Initialize machine_slot fields with the cpu data */
> + machine_slot[vcpu].cpu_subtype = CPU_SUBTYPE_AT386;
> + machine_slot[vcpu].cpu_type = machine_slot[0].cpu_type;
> + machine_slot[vcpu].running = TRUE;
> +
> + cpu_launch_first_thread(THREAD_NULL);
> +}
Please factorize it with cpu_setup, putting #ifndef MACH_XEN as
appropriate. Code duplication is the path to bugs getting fixed in half
of the cases only.
> -#ifndef MACH_XEN
> void init_percpu(int cpu)
> {
> - int apic_id = apic_get_current_cpu();
> -
> percpu_array[cpu].self = &percpu_array[cpu];
> - percpu_array[cpu].apic_id = apic_id;
> +#ifdef APIC
> + percpu_array[cpu].apic_id = apic_get_current_cpu();
> +#endif
> percpu_array[cpu].cpu_id = cpu;
> -}
> +#if NCPUS > 1 && defined(MACH_XEN)
> + percpu_array[cpu].vcpu_id = cpu;
> + percpu_array[cpu].vcpu_gc.user_regs.eip = (unsigned long)cpu_ap_main;
> + percpu_array[cpu].vcpu_gc.flags = VGCF_IN_KERNEL;
> + percpu_array[cpu].vcpu_gc.user_regs.eflags = 0x1000; /* IOPL_RING1 */
> + percpu_array[cpu].vcpu_gc.user_regs.ds = USER_DS;
> + percpu_array[cpu].vcpu_gc.user_regs.es = USER_DS;
> + percpu_array[cpu].vcpu_gc.user_regs.ss = KERNEL_DS;
> + percpu_array[cpu].vcpu_gc.user_regs.cs = KERNEL_CS;
> + percpu_array[cpu].vcpu_gc.user_regs.esp = (unsigned
> long)int_stack_top[cpu];
What is user_regs supposed to be, vs kernel_ss/sp? Is that the initial
vcpu status? (in which case it should be just using all kernel segments?)
> + //XXX gdt_{frames,ents} ?
> + //XXX trap_ctxt (idt)?
Again, make it an assertion error, not just a code comment.
gdt_frames is probably simply the mfn of the corresponding mp_gdt.
trap_ctxt content is probably to be copied from idt_inittab, see how
that table is set up in idt_inittab.S with the expected format.
> + percpu_array[cpu].vcpu_gc.kernel_ss = KERNEL_DS;
> + percpu_array[cpu].vcpu_gc.kernel_sp = (unsigned long)int_stack_top[cpu];
> +#ifdef __x86_64__
> + percpu_array[cpu].vcpu_gc.gs_base_kernel = (unsigned
> long)percpu_array[cpu].self;
> #endif
> + percpu_array[cpu].vcpu_gc.event_callback_eip = (unsigned
> long)hyp_callback;
> + percpu_array[cpu].vcpu_gc.failsafe_callback_eip = (unsigned
> long)hyp_failsafe_callback;
> + percpu_array[cpu].vcpu_gc.ctrlreg[3] = (unsigned long)kernel_page_dir;
> +#endif
> +}
> diff --git a/i386/i386/percpu.h b/i386/i386/percpu.h
> index 637d2ca6..4c036bcd 100644
> --- a/i386/i386/percpu.h
> +++ b/i386/i386/percpu.h
> @@ -67,11 +67,16 @@ MACRO_END
>
> #include <kern/processor.h>
> #include <mach/mach_types.h>
> +#include <i386/xen.h>
>
> struct percpu {
> struct percpu *self;
> int apic_id;
> int cpu_id;
> +#ifdef MACH_XEN
> + int vcpu_id;
> + struct vcpu_guest_context vcpu_gc;
> +#endif
> struct processor processor;
> thread_t active_thread;
> vm_offset_t active_stack;
> diff --git a/i386/i386/spl.S b/i386/i386/spl.S
> index 2f2c8e3a..3176d81c 100644
> --- a/i386/i386/spl.S
> +++ b/i386/i386/spl.S
> @@ -42,8 +42,8 @@
> testl hyp_shared_info+PENDING, %ebx; \
> popl %ebx; \
> jz 9f; /* Check whether there was some
> pending */ \
> -lock orl $1,hyp_shared_info+CPU_PENDING_SEL; /* Yes, activate it */ \
> - movb $1,hyp_shared_info+CPU_PENDING; \
> +lock orl $1,hyp_shared_info+CPU0_PENDING_SEL; /* Yes, activate it */ \
> + movb $1,hyp_shared_info+CPU0_PENDING; \
Again, this needs to be per-cpu.
> 9:
>
> ENTRY(spl0)
> diff --git a/i386/i386/xen.h b/i386/i386/xen.h
> index 2cd81be8..be5e20c3 100644
> --- a/i386/i386/xen.h
> +++ b/i386/i386/xen.h
> @@ -360,6 +360,14 @@ _hypcall2(long, vm_assist, unsigned int, cmd, unsigned
> int, type);
>
> _hypcall0(long, iret);
>
> +#include <xen/public/vcpu.h>
> +_hypcall3(int, vcpu_op, int, cmd, int, vcpu, vm_offset_t /* void* */, arg)
> +#define hyp_vcpu_initialise(vcpu, gc) hyp_vcpu_op(VCPUOP_initialise, vcpu,
> (vm_offset_t)gc)
> +#define hyp_vcpu_register(vcpu, info) hyp_vcpu_op(VCPUOP_register_vcpu_info,
> vcpu, (vm_offset_t)info)
> +#define hyp_vcpu_is_up(vcpu) hyp_vcpu_op(VCPUOP_is_up, vcpu, 0)
> +#define hyp_vcpu_up(vcpu) hyp_vcpu_op(VCPUOP_up, vcpu, 0)
> +#define hyp_vcpu_down(vcpu) hyp_vcpu_op(VCPUOP_down, vcpu, 0)
> +
I guess you had a look at the Xen documentation for these?
> #include <xen/public/sched.h>
> _hypcall2(long, sched_op, int, cmd, vm_offset_t /* void* */, arg)
> #define hyp_yield() hyp_sched_op(SCHEDOP_yield, 0)
> @@ -403,7 +411,7 @@ static inline uint64_t hyp_cpu_clock(void) {
>
> #else /* __ASSEMBLER__ */
> /* TODO: SMP */
> -#define cli movb $0xff,hyp_shared_info+CPU_CLI
> +#define cli movb $0xff,hyp_shared_info+CPU0_CLI
Again, that won't fly, we want to access the per-cpu offset.
> #define sti call hyp_sti
> #define iretq jmp hyp_iretq
> #endif /* ASSEMBLER */
> diff --git a/i386/xen/xen_locore.S b/i386/xen/xen_locore.S
> index 1468ef80..6e20139c 100644
> --- a/i386/xen/xen_locore.S
> +++ b/i386/xen/xen_locore.S
> @@ -56,7 +56,7 @@ ENTRY(hyp_sti)
> pushl %ebp
> movl %esp, %ebp
> _hyp_sti:
> - movb $0,hyp_shared_info+CPU_CLI /* Enable interrupts */
> + movb $0,hyp_shared_info+CPU0_CLI /* Enable interrupts */
> cmpl $0,int_active /* Check whether we were already
> checking pending interrupts */
> jz 0f
> popl %ebp
> @@ -64,12 +64,12 @@ _hyp_sti:
> 0:
> /* Not active, check pending interrupts by hand */
> /* no memory barrier needed on x86 */
> - cmpb $0,hyp_shared_info+CPU_PENDING
> + cmpb $0,hyp_shared_info+CPU0_PENDING
> jne 0f
> popl %ebp
> ret
> 0:
> - movb $0xff,hyp_shared_info+CPU_CLI
> + movb $0xff,hyp_shared_info+CPU0_CLI
> 1:
> pushl %eax
> pushl %ecx
> @@ -86,7 +86,7 @@ _hyp_sti:
> popl %ecx
> popl %eax
> decl int_active /* stopped handling interrupts */
> - cmpb $0,hyp_shared_info+CPU_PENDING
> + cmpb $0,hyp_shared_info+CPU0_PENDING
> jne 1b
> jmp _hyp_sti
>
Again, per-cpu, and similar in the rest.
Samuel
