On 12/07/16 12:33, Andre Przywara wrote:
> Hi,
>
> On 11/07/16 17:50, Marc Zyngier wrote:
>> On 05/07/16 12:23, Andre Przywara wrote:
>>> The LPI pending status for a GICv3 redistributor is held in a table
>>> in (guest) memory. To achieve reasonable performance, we cache this
>>> data in our struct vgic_irq. The initial pending state must be read
>>> from guest memory upon enabling LPIs for this redistributor.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>> ---
>>> virt/kvm/arm/vgic/vgic-its.c | 81 ++++++++++++++++++++++++++++++++++++++++++++
>>> virt/kvm/arm/vgic/vgic.h | 6 ++++
>>> 2 files changed, 87 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> index 1e2e649..29bb4fe 100644
>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -93,6 +93,81 @@ struct its_itte {
>>> list_for_each_entry(itte, &(dev)->itt_head, itte_list)
>>>
>>> #define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 12))
>>> +#define PENDBASER_ADDRESS(x) ((x) & GENMASK_ULL(51, 16))
>>
>> 52 bits again. Pick a side!
>>
>>> +
>>> +static int vgic_its_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr)
>>> +{
>>> + struct vgic_dist *dist = &kvm->arch.vgic;
>>> + struct vgic_irq *irq;
>>> + u32 *intids;
>>> + int irq_count = dist->lpi_list_count, i = 0;
>>> +
>>> + /*
>>> + * We use the current value of the list length, which may change
>>> + * after the kmalloc. We don't care, because the guest shouldn't
>>> + * change anything while the command handling is still running,
>>> + * and in the worst case we would miss a new IRQ, which one wouldn't
>>> + * expect to be covered by this command anyway.
>>> + */
>>> + intids = kmalloc_array(irq_count, sizeof(intids[0]), GFP_KERNEL);
>>> + if (!intids)
>>> + return -ENOMEM;
>>> +
>>> + spin_lock(&dist->lpi_list_lock);
>>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_entry) {
>>> + if (kref_get_unless_zero(&irq->refcount)) {
>>> + intids[i] = irq->intid;
>>> + vgic_put_irq_locked(kvm, irq);
>>
>> This is ugly. You know you're not going to free the irq, since it was at
>> least one when you did kref_get_unless_zero(). Why not doing a simple
>> kref_put (possibly in a macro so that you can hide the dummy release
>> function)?
>
> I think I don't need the get and put at all, which would allow to
> totally drop the vgic_put_irq_locked version:
> 1) We have the lpi_list_lock, so if we find the IRQ in the list, it's
> still valid (we free it only after having it removed).
> 2) It can't be removed without dropping the lock.
> 3) We just store the number, not the pointer.
> 4) An LPI can be unmapped anyway after we dropped the lock and before we
> actually use the copy. We take care of that already by calling get again
> and coping with a NULL return.
>
> So is it feasible to remove the get and put here completely or is that
> dodgy since we technically use the reference?
> Shall I document that one doesn't need get and put if holding the
> lpi_list_lock?
Yes please. Also put a comment in here, as this is the only place (I
think) where we are iterating the list without using refcounts.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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
