Changeset View
Standalone View
sys/amd64/include/vmm.h
Show First 20 Lines • Show All 153 Lines • ▼ Show 20 Lines | ||||||||||||
struct vlapic; | struct vlapic; | |||||||||||
struct vmspace; | struct vmspace; | |||||||||||
struct vm_object; | struct vm_object; | |||||||||||
struct vm_guest_paging; | struct vm_guest_paging; | |||||||||||
struct pmap; | struct pmap; | |||||||||||
enum snapshot_req; | enum snapshot_req; | |||||||||||
struct vm_eventinfo { | struct vm_eventinfo { | |||||||||||
void *rptr; /* rendezvous cookie */ | cpuset_t *rptr; /* rendezvous cookie */ | |||||||||||
int *sptr; /* suspend cookie */ | int *sptr; /* suspend cookie */ | |||||||||||
gusev.vitaliy_gmail.com: I think you don't need mutex here (&vm->rendezvous_mtx), because taking the mutex every time… | ||||||||||||
int *iptr; /* reqidle cookie */ | int *iptr; /* reqidle cookie */ | |||||||||||
}; | }; | |||||||||||
typedef int (*vmm_init_func_t)(int ipinum); | typedef int (*vmm_init_func_t)(int ipinum); | |||||||||||
typedef int (*vmm_cleanup_func_t)(void); | typedef int (*vmm_cleanup_func_t)(void); | |||||||||||
typedef void (*vmm_resume_func_t)(void); | typedef void (*vmm_resume_func_t)(void); | |||||||||||
typedef void * (*vmi_init_func_t)(struct vm *vm, struct pmap *pmap); | typedef void * (*vmi_init_func_t)(struct vm *vm, struct pmap *pmap); | |||||||||||
typedef int (*vmi_run_func_t)(void *vcpui, register_t rip, | typedef int (*vmi_run_func_t)(void *vcpui, register_t rip, | |||||||||||
▲ Show 20 Lines • Show All 157 Lines • ▼ Show 20 Lines | ||||||||||||
cpuset_t vm_active_cpus(struct vm *vm); | cpuset_t vm_active_cpus(struct vm *vm); | |||||||||||
cpuset_t vm_debug_cpus(struct vm *vm); | cpuset_t vm_debug_cpus(struct vm *vm); | |||||||||||
cpuset_t vm_suspended_cpus(struct vm *vm); | cpuset_t vm_suspended_cpus(struct vm *vm); | |||||||||||
cpuset_t vm_start_cpus(struct vm *vm, const cpuset_t *tostart); | cpuset_t vm_start_cpus(struct vm *vm, const cpuset_t *tostart); | |||||||||||
void vm_await_start(struct vm *vm, const cpuset_t *waiting); | void vm_await_start(struct vm *vm, const cpuset_t *waiting); | |||||||||||
#endif /* _SYS__CPUSET_H_ */ | #endif /* _SYS__CPUSET_H_ */ | |||||||||||
static __inline int | static __inline int | |||||||||||
vcpu_rendezvous_pending(struct vm_eventinfo *info) | vcpu_rendezvous_pending(struct vcpu *vcpu, struct vm_eventinfo *info) | |||||||||||
{ | { | |||||||||||
/* | ||||||||||||
return (*((uintptr_t *)(info->rptr)) != 0); | * This check isn't done with atomic operations or under a lock because | |||||||||||
* there's no need to. If the vcpuid bit is set, the vcpu is part of a | ||||||||||||
* rendezvous and the bit won't be cleared until the vcpu enters the | ||||||||||||
* rendezvous. On rendezvous exit, the cpuset is cleared and the vcpu | ||||||||||||
* will see an empty cpuset. So, the races are harmless. | ||||||||||||
Not Done Inline ActionsPerhaps more accurate to say the the races are harmless. They still exist, they are just all handled jhb: Perhaps more accurate to say the the races are harmless. They still exist, they are just all… | ||||||||||||
*/ | ||||||||||||
return (CPU_ISSET(vcpu_vcpuid(vcpu), info->rptr)); | ||||||||||||
Not Done Inline ActionsOTOH, this probably needs to be something like CPU_EMPTY() instead of just checking the first word, or even the check you have here of checking the cpuid might be sensible. That would be a separate followup though. Until we have more vCPUs than bits in a pointer though (which we can't until my other series lands) this isn't yet a problem. I probably should fix this in my other series actually. jhb: OTOH, this probably needs to be something like `CPU_EMPTY()` instead of just checking the first… | ||||||||||||
Not Done Inline ActionsNo, this shouldn't be CPU_EMPTY. This function should check if the vcpu identified by vcpuid has a pending rendevzous or not. So, this should only test if the bit at position vcpuid is set or not. That's exactly what CPU_ISSET does and it will work even with thousands of vcpus. corvink: No, this shouldn't be `CPU_EMPTY`. This function should check if the vcpu identified by vcpuid… | ||||||||||||
Not Done Inline ActionsI had thought the current code was setting rptr to the cpuset, and CPU_EMPTY() would be a more correct version of checking for an empty mask in that case (to preserve existing semantics). I do think if we want a comment anywhere we want it here as this is the one read outside of the rendezvous_mtx. The comment could just say that the check is racy, but that's ok as if we see a stale value we will get an IPI to force an exit, and if we see a stale value with it set, the handler will ignore the spurious rendezvous. jhb: I had thought the current code was setting `rptr` to the cpuset, and `CPU_EMPTY()` would be a… | ||||||||||||
Not Done Inline ActionsI can add a comment. corvink: I can add a comment. | ||||||||||||
Not Done Inline ActionsWhat if use additional check on 'rendezvous_func' ? It should be faster than use only CPU_ISSET() on every vmexit that doesn't break vmx/svm loop. Of course that optimisation makes sense if there are a lot of such vmexits. vcpu_rendezvous_pending() { return (*((uintptr_t *)(info->rptr)) != 0) && CPU_ISSET(vcpuid, (cpuset_t *)info->rptrset); } rptr -> rendezvous_func gusev.vitaliy_gmail.com: What if use additional check on 'rendezvous_func' ? It should be faster than use only CPU_ISSET… | ||||||||||||
Not Done Inline ActionsNot sure if it's worth. CPU_ISSET is a single lookup (_cpuset[bitoffset] & bitmask). So, it should be fast. corvink: Not sure if it's worth. CPU_ISSET is a single lookup (_cpuset[bitoffset] & bitmask). So, it… | ||||||||||||
Not Done Inline ActionsI don't think we need to overcomplicate the check with checking both things. Rendezvous should be rare. Modern guests should be using MSI interrupts and not I/O APIC interrupts in which case rendezvous will only occur during boot to start APs. jhb: I don't think we need to overcomplicate the check with checking both things. Rendezvous should… | ||||||||||||
Not Done Inline Actions
We could also change the type of info->rptr to be cpuset_t * which would remove the need for casts. It's only used for this. jhb: We could also change the type of `info->rptr` to be `cpuset_t *` which would remove the need… | ||||||||||||
} | } | |||||||||||
static __inline int | static __inline int | |||||||||||
vcpu_suspended(struct vm_eventinfo *info) | vcpu_suspended(struct vm_eventinfo *info) | |||||||||||
{ | { | |||||||||||
return (*info->sptr); | return (*info->sptr); | |||||||||||
} | } | |||||||||||
▲ Show 20 Lines • Show All 476 Lines • Show Last 20 Lines |
I think you don't need mutex here (&vm->rendezvous_mtx), because taking the mutex every time during check can be too expensive for VM with many VCPUs. So this code can introduce bottleneck.
Instead I would suggest to move that engine (rendezvous_req_cpus ) to atomic operations (CPU_SET_ATOMIC, etc) and barriers, it should be fast on reading (check) and safe.