Re: [PATCH v10 18/32] ARM: vITS: introduce translation table walks

From: Andre Przywara <andre.przywara@arm.com>
To: Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org,
	Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>,
	Vijay Kilari <vijay.kilari@gmail.com>,
	Shanker Donthineni <shankerd@codeaurora.org>
Subject: Re: [PATCH v10 18/32] ARM: vITS: introduce translation table walks
Date: Thu, 8 Jun 2017 10:45:59 +0100	[thread overview]
Message-ID: <bdba6f8c-6e1d-3e72-3a6e-d97daff54ff9@arm.com> (raw)
In-Reply-To: <58ae8104-c3bd-5348-c783-e0b5b82433c4@arm.com>

Hi,

On 08/06/17 10:35, Julien Grall wrote:
> Hi Andre,
> 
> On 26/05/17 18:35, Andre Przywara wrote:
>> +/*
>> + * Queries the collection and device tables to translate the device
>> ID and
>> + * event ID and find the appropriate ITTE. The given collection ID
>> and the
>> + * virtual LPI number are then stored into that entry.
>> + * If vcpu_ptr is provided, returns the VCPU belonging to that
>> collection.
>> + * Must be called with the ITS lock held.
>> + */
>> +bool write_itte_locked(struct virt_its *its, uint32_t devid,
>> +                       uint32_t evid, uint32_t collid, uint32_t vlpi,
>> +                       struct vcpu **vcpu_ptr)
>> +{
>> +    paddr_t addr;
>> +    struct vits_itte itte;
>> +
>> +    ASSERT(spin_is_locked(&its->its_lock));
>> +
>> +    if ( collid >= its->max_collections )
>> +        return false;
> 
> This check will always fail with the command DISCARD because collid ==
> UNMAPPED_COLLECTION (~0).

Good point, thanks for catching this.

> Looking at the code, I am not sure why you need to validate collid and
> nr_lpis in write_itte_locked. This should have been made by the caller.

Indeed, I just fixed that.

>> +
>> +    if ( vlpi >= its->d->arch.vgic.nr_lpis )
>> +        return false;
>> +
>> +    addr = its_get_itte_address(its, devid, evid);
>> +    if ( addr == INVALID_PADDR )
>> +        return false;
>> +
>> +    itte.collection = collid;
>> +    itte.vlpi = vlpi;
>> +
>> +    if ( vgic_access_guest_memory(its->d, addr, &itte, sizeof(itte),
>> true) )
>> +        return false;
>> +
>> +    if ( vcpu_ptr )
>> +        *vcpu_ptr = get_vcpu_from_collection(its, collid);
> 
> I guess this is why you check the collection in this function. However,
> I am not sure how this is related to write_itte_locked.

Looks like some artefact from some previous revisions of the code
(possibly due to some locking, where someone was calling write_itte(),
which would drop the lock before returning).

But indeed this is independent and actually there is only one user of
this, so I removed the vcpu_ptr parameter and do the lookup outside of
this function.

On the way I removed this whole {read,write}_itte_locked naming, just
providing functions which assume we hold the lock. There is only one
caller which didn't hold the lock, so I can just do the locking there.

Thanks for pointing this out!

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel