On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell
<peter.maydell@xxxxxxxxxx> wrote:
> On 11 December 2011 15:18, Jan Kiszka <jan.kiszka@xxxxxx> 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).
>
Isn't that just personal preference? The other scheme was suggested by
Avi, and nobody else complained then, so I'd be inclined to just leave
it as is.
>>> +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?)
>
right, I will remove the default case.
I highly doubt that the difference in using a bitop will be measurably
more efficient, but if you feel strongly about it, I can change it to
a shift and bitwise and, which I assume is what you mean by the
obvious bit operation? I think my CS background speaks for using %,
but whatever.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
