Re: [PATCH v2 12/36] irqchip/gic-v4.1: Implement the v4.1 flavour of VMAPP

[Marc Zyngier]

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@xxxxxxxxxx>
>
> [...]
>
> > @@ -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...