On 11 December 2011 15:18, Jan Kiszka <jan.kiszka at web.de> wrote:
> Just found two, maybe three nits while browsing by:
>
> On 2011-12-11 11:24, Christoffer Dall wrote:
>> +ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
>> +irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
>> +FIQs.
This seems to me a slightly obscure way of defining the two fields
in this word (ie bits [31..1] cpu number, bit [0] irq-vs-fiq).
>> +static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct kvm_irq_level *irq_level)
>> +{
>> + ? ? u32 mask;
>> + ? ? unsigned int vcpu_idx;
>> + ? ? struct kvm_vcpu *vcpu;
>> +
>> + ? ? vcpu_idx = irq_level->irq / 2;
>> + ? ? if (vcpu_idx >= KVM_MAX_VCPUS)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? vcpu = kvm_get_vcpu(kvm, vcpu_idx);
>> + ? ? if (!vcpu)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? switch (irq_level->irq % 2) {
>> + ? ? case KVM_ARM_IRQ_LINE:
>> + ? ? ? ? ? ? mask = HCR_VI;
>> + ? ? ? ? ? ? break;
>> + ? ? case KVM_ARM_FIQ_LINE:
>> + ? ? ? ? ? ? mask = HCR_VF;
>> + ? ? ? ? ? ? break;
>> + ? ? default:
>> + ? ? ? ? ? ? return -EINVAL;
>
> Due to % 2, default is unreachable. Remove the masking?
Removing the mask would be wrong since the irq field here
is encoding both cpu number and irq-vs-fiq. The default is
just an unreachable condition. (Why are we using % here
rather than the obvious bit operation, incidentally?)
-- PMM
