Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
To:     Zenghui Yu <yuzenghui@huawei.com>
Subject: Re: [PATCH v2 12/36] irqchip/gic-v4.1: Implement the v4.1 flavour  of VMAPP
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 13 Nov 2019 10:56:22 +0109
From:   Marc Zyngier <maz@kernel.org>
Cc:     <kvmarm@lists.cs.columbia.edu>, <linux-kernel@vger.kernel.org>,
        Eric Auger <eric.auger@redhat.com>,
        James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Andrew Murray <andrew.murray@arm.com>,
        Jayachandran C <jnair@marvell.com>,
        Robert Richter <rrichter@marvell.com>,
        "Wanghaibin (D)" <wanghaibin.wang@huawei.com>,
        <jiayanlei@huawei.com>, <liangboyan@hisilicon.com>
In-Reply-To: <c9ef4c0f-bb34-82ff-c286-8430c1c7c583@huawei.com>
References: <20191027144234.8395-1-maz@kernel.org>
 <20191027144234.8395-13-maz@kernel.org>
 <c9ef4c0f-bb34-82ff-c286-8430c1c7c583@huawei.com>
Message-ID: <c3830faf33c7f8b983ad4863fe02b86b@www.loen.fr>
User-Agent: Roundcube Webmail/0.7.2
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Zenghui,

On 2019-11-13 09:11, Zenghui Yu wrote:
> Hi Marc,
>
> On 2019/10/27 22:42, Marc Zyngier wrote:
>> The ITS VMAPP command gains some new fields with GICv4.1:
>> - a default doorbell, which allows a single doorbell to be used for
>>    all the VLPIs routed to a given VPE
>> - a pointer to the configuration table (instead of having it in a 
>> register
>>    that gets context switched)
>> - a flag indicating whether this is the first map or the last unmap 
>> for
>>    this particulat VPE
>> - a flag indicating whether the pending table is known to be zeroed, 
>> or not
>> Plumb in the new fields in the VMAPP builder, and add the map/unmap
>> refcounting so that the ITS can do the right thing.
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>
> [...]
>
>> @@ -605,19 +626,45 @@ static struct its_vpe 
>> *its_build_vmapp_cmd(struct its_node *its,
>>   					   struct its_cmd_block *cmd,
>>   					   struct its_cmd_desc *desc)
>>   {
>> -	unsigned long vpt_addr;
>> +	unsigned long vpt_addr, vconf_addr;
>>   	u64 target;
>> -
>> -	vpt_addr = 
>> virt_to_phys(page_address(desc->its_vmapp_cmd.vpe->vpt_page));
>> -	target = desc->its_vmapp_cmd.col->target_address + 
>> its->vlpi_redist_offset;
>> +	bool alloc;
>>
>>   	its_encode_cmd(cmd, GITS_CMD_VMAPP);
>>   	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
>>   	its_encode_valid(cmd, desc->its_vmapp_cmd.valid);
>> +
>> +	if (!desc->its_vmapp_cmd.valid) {
>> +		if (is_v4_1(its)) {
>> +			alloc = 
>> !atomic_dec_return(&desc->its_vmapp_cmd.vpe->vmapp_count);
>> +			its_encode_alloc(cmd, alloc);
>> +		}
>> +
>> +		goto out;
>> +	}
>> +
>> +	vpt_addr = 
>> virt_to_phys(page_address(desc->its_vmapp_cmd.vpe->vpt_page));
>> +	target = desc->its_vmapp_cmd.col->target_address + 
>> its->vlpi_redist_offset;
>> +
>>   	its_encode_target(cmd, target);
>>   	its_encode_vpt_addr(cmd, vpt_addr);
>>   	its_encode_vpt_size(cmd, LPI_NRBITS - 1);
>>   +	if (!is_v4_1(its))
>> +		goto out;
>> +
>> +	vconf_addr = 
>> virt_to_phys(page_address(desc->its_vmapp_cmd.vpe->its_vm->vprop_page));
>> +
>> +	alloc = 
>> atomic_inc_and_test(&desc->its_vmapp_cmd.vpe->vmapp_count);
>
> As the comment block on top of atomic_inc_and_test(atomic *v) says,
>
>  * Atomically increments @v by 1
>  * and returns true if the result is zero, or false for all
>  * other cases.
>  */
>
> We will always get the 'alloc' as false here, even if this is the
> first mapping of this vPE.  This is not as expected, I think.

As usual, a very good observation!

Indeed, I cocked up the logic here, as we need to test the value before
the increment (and not after). What we want is probably something like:

   alloc = !atomic_fetch_inc(&desc->its_vmapp_cmd.vpe->vmapp_count);

> And on the other hand, I wonder what is the reason for 'vmapp_count'
> to be atomic_t?

The rational is that we could end-up with multiple VMAPP commands 
emitted
in parallel, for example. That's probably not strictly necessary right 
now,
but I'm trying to be cautious.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...