On Sun, Dec 11, 2011 at 11:03 AM, Peter Maydell
<peter.maydell at linaro.org> wrote:
> 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).
>
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.
