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;
Date:   Fri, 01 Nov 2019 13:26:37 +0000
Message-ID: <86ftj7ybg2.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Zenghui Yu <yuzenghui@huawei.com>
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>
Subject: Re: [PATCH v2 03/36] irqchip/gic-v3-its: Allow LPI invalidation via the DirectLPI interface
In-Reply-To: <a263e264-298c-57cf-31b7-a781160a3929@huawei.com>
References: <20191027144234.8395-1-maz@kernel.org>
        <20191027144234.8395-4-maz@kernel.org>
        <a263e264-298c-57cf-31b7-a781160a3929@huawei.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 31 Oct 2019 08:49:32 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Hi Marc,
> 
> On 2019/10/27 22:42, Marc Zyngier wrote:
> > We currently don't make much use of the DirectLPI feature, and it would
> > be beneficial to do this more, if only because it becomes a mandatory
> > feature for GICv4.1.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> I have no objection to this patch, which says:
> 
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
> 
> 
> But this patch really drives me to look through all callsites of
> dev_event_to_col(), the abstraction which can be used _only_ with
> physical LPI mappings.
> 
> I find that when building the INV command, we use dev_event_to_col()
> to find the "sync_obj" and then pass it to the following SYNC command.
> But the "INV+SYNC" will be performed both on physical LPI and *VLPI*
> (lpi_update_config/its_send_inv).
> So I have two questions about the way we sending INV on VLPI:
> 
> 1) Which "sync" command should be followed?  SYNC or VSYNC?
> (we currently use SYNC, while the spec says, SYNC "ensures all
> outstanding ITS operations associated with *physical* interrupts
> for the Redistributor are globally observed ...")
> 
> 2) The "sync_obj" we are currently using seems to be wrong.

I think you're right on both counts (where were you when I wrote the
initial GICv4 support? ;-). I think the confusion stems from the fact
that there is no 'VINV' command, and we simply overlooked the sync
object issue. It is quite likely that existing implementations don't
care much about the difference (otherwise we'd have seen the problem
before), but it doesn't hurt to do the right thing.

I have the following patch as part of a set of fixes that I'm about to
post (once I get a chance to test them), let me know what you think.

	M.

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index a47ed2ba2907..75ab3716a870 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -702,6 +702,24 @@ static struct its_vpe *its_build_vmovp_cmd(struct its_node *its,
 	return valid_vpe(its, desc->its_vmovp_cmd.vpe);
 }
 
+static struct its_vpe *its_build_vinv_cmd(struct its_node *its,
+					  struct its_cmd_block *cmd,
+					  struct its_cmd_desc *desc)
+{
+	struct its_vlpi_map *map;
+
+	map = dev_event_to_vlpi_map(desc->its_inv_cmd.dev,
+				    desc->its_inv_cmd.event_id);
+
+	its_encode_cmd(cmd, GITS_CMD_INV);
+	its_encode_devid(cmd, desc->its_inv_cmd.dev->device_id);
+	its_encode_event_id(cmd, desc->its_inv_cmd.event_id);
+
+	its_fixup_cmd(cmd);
+
+	return valid_vpe(its, map->vpe);
+}
+
 static u64 its_cmd_ptr_to_offset(struct its_node *its,
 				 struct its_cmd_block *ptr)
 {
@@ -1068,6 +1086,20 @@ static void its_send_vinvall(struct its_node *its, struct its_vpe *vpe)
 	its_send_single_vcommand(its, its_build_vinvall_cmd, &desc);
 }
 
+static void its_send_vinv(struct its_device *dev, u32 event_id)
+{
+	struct its_cmd_desc desc;
+
+	/*
+	 * There is no real VINV command. This is just a normal INV,
+	 * with a VSYNC instead of a SYNC.
+	 */
+	desc.its_inv_cmd.dev = dev;
+	desc.its_inv_cmd.event_id = event_id;
+
+	its_send_single_vcommand(dev->its, its_build_vinv_cmd, &desc);
+}
+
 /*
  * irqchip functions - assumes MSI, mostly.
  */
@@ -1142,8 +1174,10 @@ static void lpi_update_config(struct irq_data *d, u8 clr, u8 set)
 	lpi_write_config(d, clr, set);
 	if (gic_rdists->has_direct_lpi && !irqd_is_forwarded_to_vcpu(d))
 		direct_lpi_inv(d);
-	else
+	else if (!irqd_is_forwarded_to_vcpu(d))
 		its_send_inv(its_dev, its_get_event_id(d));
+	else
+		its_send_vinv(its_dev, its_get_event_id(d));
 }
 
 static void its_vlpi_set_doorbell(struct irq_data *d, bool enable)

-- 
Jazz is not dead, it just smells funny.