Re: [PATCH v2 03/36] irqchip/gic-v3-its: Allow LPI invalidation via the DirectLPI interface

Zenghui Yu
2019-11-05T10:30:46Z

Hi Marc,

On 2019/11/1 21:26, Marc Zyngier wrote:
> On Thu, 31 Oct 2019 08:49:32 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>
>> 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

(I'm a bit late here :-).

> 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);

Indeed! I think we need this kind of abstraction for VLPI.

> +
> + 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));

Yeah, this is exactly what I was having in the mind when reporting this
problem - "maybe we should have a SW emulated VINV+VSYNC for VLPI".
So I think this patch has done the right thing.

And what about the INT and CLEAR? In response to guest's INT/CLEAR
commands, hypervisor (I mean KVM) will bother the ITS driver to send
INT/CLEAR for VLPIs. They're both followed by SYNC and might need the
same fixes?

Thanks,
Zenghui

> }
>
> static void its_vlpi_set_doorbell(struct irq_data *d, bool enable)
>