Zhang, Xiantao wrote:
> +static struct kvm_vcpu *lid_to_vcpu(struct kvm *kvm, unsigned long id,
> + unsigned long eid)
> +{
> + ia64_lid_t lid;
> + int i;
> +
> + for (i = 0; i < KVM_MAX_VCPUS; i++) {
> + if (kvm->vcpus[i]) {
> + lid.val = VCPU_LID(kvm->vcpus[i]);
> + if (lid.id == id && lid.eid == eid)
> + return kvm->vcpus[i];
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static int handle_ipi(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> +{
> + struct exit_ctl_data *p = kvm_get_exit_data(vcpu);
> + struct kvm_vcpu *target_vcpu;
> + struct kvm_pt_regs *regs;
> + ia64_ipi_a addr = p->u.ipi_data.addr;
> + ia64_ipi_d data = p->u.ipi_data.data;
> +
> + target_vcpu = lid_to_vcpu(vcpu->kvm, addr.id, addr.eid);
> + if (!target_vcpu)
> + return handle_vm_error(vcpu, kvm_run);
> +
> + if (!target_vcpu->arch.launched) {
> + regs = vcpu_regs(target_vcpu);
> +
> + regs->cr_iip = vcpu->kvm->arch.rdv_sal_data.boot_ip;
> + regs->r1 = vcpu->kvm->arch.rdv_sal_data.boot_gp;
> +
> + target_vcpu->arch.mp_state = VCPU_MP_STATE_RUNNABLE;
> + if (waitqueue_active(&target_vcpu->wq))
> + wake_up_interruptible(&target_vcpu->wq);
> + } else {
> + vcpu_deliver_ipi(target_vcpu, data.dm, data.vector);
> + if (target_vcpu != vcpu)
> + kvm_vcpu_kick(target_vcpu);
> + }
> +
> + return 1;
> +}
*Shrug*. This looks highly racy to me. You do access various values in
target_vcpu without any lock! I know that taking the target vcpu's
lock does'nt work because that one is held all the time during
KVM_VCPU_RUN. My solution to that was struct local_interrupt, which
has its own lock, and has the waitqueue plus everything I need to send
a sigp [that's our flavor of ipi].
> +int kvm_emulate_halt(struct kvm_vcpu *vcpu)
> +{
> +
> + ktime_t kt;
> + long itc_diff;
> + unsigned long vcpu_now_itc;
> +
> + unsigned long expires;
> + struct hrtimer *p_ht = &vcpu->arch.hlt_timer;
That makes me jealous, I'd love to have hrtimer on s390 for this. I've
got to round up to the next jiffie. *Sigh*
> +int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> + struct kvm_sregs *sregs)
> +{
> + printk(KERN_WARNING"kvm:kvm_arch_vcpu_ioctl_set_sregs
> called!!\n");
> + return 0;
> +}
> +
> +int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
> + struct kvm_sregs *sregs)
> +{
> + printk(KERN_WARNING"kvm:kvm_arch_vcpu_ioctl_get_sregs
> called!!\n");
> + return 0;
> +
> +}
Suggestion: if get/set sregs does'nt seem useful on ia64, why not
return -EINVAL? In that case, you could also not print a kern warning,
the user will either handle that situation or complain.
> +int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> +{
<snip>
> + /*FIXME:Need to removed it later!!\n*/
> + vcpu->arch.apic = kzalloc(sizeof(struct kvm_lapic), GFP_KERNEL);
> + vcpu->arch.apic->vcpu = vcpu;
Fixme!
> +static int vti_vcpu_setup(struct kvm_vcpu *vcpu, int id)
> +{
> + unsigned long psr;
> + int r;
> +
> + local_irq_save(psr);
> + r = kvm_insert_vmm_mapping(vcpu);
> + if (r)
> + goto fail;
> + r = kvm_vcpu_init(vcpu, vcpu->kvm, id);
> + if (r)
> + goto fail;
Maybe change to return r, rather then goto fail?
> +int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu
> *fpu)
> +{
> + printk(KERN_WARNING"kvm:IA64 doesn't need to export"
> + "fpu to userspace!\n");
> + return 0;
> +}
> +
> +int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu
> *fpu)
> +{
> + printk(KERN_WARNING"kvm:IA64 doesn't need to export"
> + "fpu to userspace !\n");
> + return 0;
> +}
maybe -EINVAL?
> +static int find_highest_bits(int *dat)
> +{
> + u32 bits, bitnum;
> + int i;
> +
> + /* loop for all 256 bits */
> + for (i = 7; i >= 0 ; i--) {
> + bits = dat[i];
> + if (bits) {
> + bitnum = fls(bits);
> + return i * 32 + bitnum - 1;
> + }
> + }
> +
> + return -1;
> +}
Should be in asm/bitops.h. Look at find_first_bit() and friends, this
is duplicate.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
