On 19/03/18 21:06, Auger Eric wrote:
> Hi Marc,
>
> On 19/03/18 16:57, Marc Zyngier wrote:
>> On 19/03/18 09:20, Eric Auger wrote:
>>> To prepare for multiple RDIST regions, let's record the RDIST
>>> Last bit field when registering the IO device.
>>>
>>> As a reminder the Last bit indicates whether the redistributor
>>> is the highest one in a series of contiguous redistributor pages.
>>>
>>> Since at the moment KVM only supports a single redist region,
>>> the RDIST tagged with the last bit set to true corresponds to the
>>> highest vCPU one.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>> ---
>>> include/kvm/arm_vgic.h | 1 +
>>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +++++-
>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index cdbd142..1c8c0ff 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -312,6 +312,7 @@ struct vgic_cpu {
>>> */
>>> struct vgic_io_device rd_iodev;
>>> struct vgic_io_device sgi_iodev;
>>> + bool rdist_last; /* Is the RDIST the last one of the RDIST region? */
>>>
>>> /* Contains the attributes and gpa of the LPI pending tables. */
>>> u64 pendbaser;
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 671fe81..75fe689 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -184,12 +184,13 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>>> gpa_t addr, unsigned int len)
>>> {
>>> unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> int target_vcpu_id = vcpu->vcpu_id;
>>> u64 value;
>>>
>>> value = (u64)(mpidr & GENMASK(23, 0)) << 32;
>>> value |= ((target_vcpu_id & 0xffff) << 8);
>>> - if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
>>> + if (vgic_cpu->rdist_last)
>>> value |= GICR_TYPER_LAST;
>>> if (vgic_has_its(vcpu->kvm))
>>> value |= GICR_TYPER_PLPIS;
>>> @@ -580,6 +581,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>> {
>>> struct kvm *kvm = vcpu->kvm;
>>> struct vgic_dist *vgic = &kvm->arch.vgic;
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> struct vgic_io_device *rd_dev = &vcpu->arch.vgic_cpu.rd_iodev;
>>> struct vgic_io_device *sgi_dev = &vcpu->arch.vgic_cpu.sgi_iodev;
>>> gpa_t rd_base, sgi_base;
>>> @@ -632,6 +634,8 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>> }
>>>
>>> vgic->vgic_redist_free_offset += 2 * SZ_64K;
>>> + vgic_cpu->rdist_last =
>>> + (vcpu->vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1);
>>> out:
>>> mutex_unlock(&kvm->slots_lock);
>>> return ret;
>>>
>>
>> I don't really like the idea of keeping this "Last" bit around, because
>> it doesn't have much to do with the state of a redistributor, and has
>> more to do with its position in the region.
>
> agreed. What about putting it in vgic_io_device then?.
I'm not sure either. See below.
>>
>> What is wrong with the current approach of dynamically computing the
>> Last bit? If you have multiple regions, all you need to know is which
>> region this redistributor belongs to. From that point (and assuming you
>> know how many redistributors have been instantiated in that region, you
>> can know whether the Last bit is set or not.
>
> Well, looked to me more natural to compute the information once when
> registering the rdist into its region instead of each time we read the
> RDIST TYPER.
That's a trade-off (memory vs time). Reading GICR_TYPER is a rare thing
(as for all MMIO), and comparing two addresses is cheap. On the other
hand, adding immutable state to each vcpu seems a bit strange.
> Especially because at that moment we map the redist, we
> need to know anyway if we have sufficient space to put it.
What do you mean by that? Is that related to the way we currently map
redistributors?
>>
>> One of the issue is that the current code works in term of "target cpu",
>> while we really want is to use the addr parameter to work out if the
>> Last bit is set or not. I'd be a lot more confident if we emulate it
>> like that (which is how the HW would generate it too).
>
> Sorry I don't understand your comment above.
What I mean is that the current code would be better written as
if (addr = redist_base + 128K * (nr_vcpus -1) + GICR_TYPER)
value |= GICR_TYPER_LAST;
rather than considering the vcpu_id of the target redistributor
associated vcpu (which I find crazy complicated). This check is the way
a HW implementation would perform it, not by storing some extra state.
You can easily expand this to multiple rdist banks.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
