xen: introduce XEN_CPUID_TO_VCPUID()

[ehem_freebsd_m5p.com]

Summary

Part of the series for allowing FreeBSD/ARM to run on Xen. On ARM the
function is a trivial pass-through, other architectures need distinct
implementations.

Submitted by: Elliott Mitchell <[e-mail]>
Original implementation: Julien Grall <[e-mail]>, 2014-04-19 08:57:40
Original implementation: Julien Grall <[e-mail]>, 2014-04-19 14:32:01

[royger]

I'm not sure why this is needed, as I would expect that just using PCPU_GET(vcpu_id) or PCPU_ID_GET(cpu, vcpu_id) if you need to fetch the vcpu id of a different CPU to be enough.

Maybe there's something that I'm missing, but I would say the cpu_to_vcpu_id helper is not needed at all.

sys/dev/xen/debug/debug.c

82

I think I'm confused, why do you need to fetch the cpuid to get the vcpu id? The previous code already gives you the vcpu id directly using PCPU_GET(vcpu_id).

[ehem_freebsd_m5p.com]

I hope Julien Grall can remember why this was originally done. I can't really speak to this part.

[mhorne]

Agreed with @royger, this appears to be an indirect way of achieving the same behaviour.

[julien_xen.org]

sys/dev/xen/debug/debug.c

82

This is where the implementation for the Arm counterpart could help to understand the approach :).

I don't know why on x86 you need a specific field vcpu_id. On Arm the vCPU ID will be equivalent to the physical CPU ID.

The field pc_vcpu_id doesn't seem to exist on Arm today and it looks like on x86 it is only used by Xen code. So to me it sounds a bit wasteful to introduce the field on Arm.

[royger]

sys/dev/xen/debug/debug.c

82

This is where the implementation for the Arm counterpart could help to understand the approach :).

I don't know why on x86 you need a specific field vcpu_id. On Arm the vCPU ID will be equivalent to the physical CPU ID.

On x86 the vCPU ID should be fetched from the Xen cpuid leaves. While currently there's a relation between the ACPI CPU ID and the vCPU ID, it's not written down anywhere, and a proper implementation should fetch the vCPU ID from cpuid and expect it to be a random value.

The field pc_vcpu_id doesn't seem to exist on Arm today and it looks like on x86 it is only used by Xen code. So to me it sounds a bit wasteful to introduce the field on Arm.

Fine. TBH I don't think adding an unsigned int to each CPU data area is going to make that much of a difference (it would be conditional to enabling XENHVM).

I don't seem to be able to find how the vCPU ID is fetched on Arm from the public headers, but as long as it's clear that's fine. I'm however slightly worried that we will now have a mix of cpu_to_vcpu_id and PCPU_GET(vcpu_id), which makes the code confusing IMO, that's another reason why I would prefer Arm to just add a vcpu_id field.

sys/x86/include/xen/xen-os.h

55

PCPU_ID_GET please.

[royger]

If this patch has to make progress the commit message needs to be updated to reflect why a vCPU ID filed might not be needed, and that having a per-arch helper allows to abstract away whether it's a completely different CPU field or just abstracted away from existing data.

[ehem_freebsd_m5p.com]

Updating, definitely taking care of one comment, hopefully working towards others. Squashing the implementation commit since the rest of that disappeared with PVHv1.

[ehem_freebsd_m5p.com]

Renamed to PCPU_GET() as requested (wondering if VCPU_GET() might be better).

I've squashed the commit where this was put into place, hopefully the greater context makes it overt the direction things go. Plus mentioning for aarch64 the function is a straight pass-through.

[ehem_freebsd_m5p.com]

Updating Phabricator's in light of this having okay via e-mail. The function got renamed, but it is still pretty similar to what was present before.

[ehem_freebsd_m5p.com]

This has an e-mail okay by @royger, just noticed the reject from before is still here.

[mhorne]

sys/xen/xen-os.h

47

This should just be PCPU_GET(vcpu_id). Otherwise you are fetching from the PCPU region twice when you don't need to.

If for ARM cpuid == vcpuid, then you can define XEN_VCPUID separately in each arch's xen-os.h header.

[royger]

You will likely need to introduce a wrapper for xen_intr_assign_cpu() in order to keep the second parameter as an apic_id.

sys/dev/xen/bus/xen_intr.c

881

I'm afraid you cannot just change the parameter here from an apic_id into a cpuid, as this function is used by xen_intr_pic in order to fill the pic_assign_cpu hook, and that expects the second parameter of the hook to be an apic_id.

[ehem_freebsd_m5p.com]

Restoring a lost hunk. Seems as some point I lost the xen_intr_pic_assign_cpu() function. Helpfully it wasn't lost from a later point.