[RFC PATCH v1 0/4] KVM: arm64: Add VLPI migration support on GICv4.1

[RFC PATCH v1 0/4] KVM: arm64: Add VLPI migration support on GICv4.1

[Shenming Lu]

In GICv4.1, migration has been supported except for (directly-injected)
VLPI. And GICv4.1 spec explicitly gives a way to get the VLPI's pending
state (which was crucially missing in GICv4.0). So we make VLPI migration
capable on GICv4.1 in this patch set.

In order to support VLPI migration, we need to save and restore all
required configuration information and pending states of VLPIs. But
in fact, the configuration information of VLPIs has already been saved
(or will be reallocated on the dst host...) in vgic(kvm) migration.
So we only have to migrate the pending states of VLPIs specially.

Below is the related workflow in migration.

On the save path:
	In migration completion:
		pause all vCPUs
				|
		call each VM state change handler:
			pause other devices (just keep from sending interrupts, and
			such as VFIO migration protocol has already realized it [1])
					|
			flush ITS tables into guest RAM
					|
			flush RDIST pending tables (also flush VLPI state here)
				|
		...
On the resume path:
	load each device's state:
		restore ITS tables (include pending tables) from guest RAM
				|
		for other (PCI) devices (paused), if configured to have VLPIs,
		establish the forwarding paths of their VLPIs (and transfer
		the pending states from kvm's vgic to VPT here)

Yet TODO:
 - For some reason, such as for VFIO PCI devices, there may be repeated
   resettings of HW VLPI configuration in load_state, resulting in the
   loss of pending state. A very intuitive solution is to retrieve the
   pending state in unset_forwarding (and this should be so regardless
   of migration). But at normal run time, this function may be called
   when all devices are running, in which case the unmapping of VPE is
   not allowed. It seems to be an almost insoluble bug...
   There are other possible solutions as follows:
   1) avoid unset_forwarding being called from QEMU in resuming (simply
   allocate all needed vectors first), which is more reasonable and
   efficient.
   2) add a new dedicated interface to transfer these pending states to
   HW in GIC VM state change handler corresponding to save_pending_tables.
   ...

Any comments and suggestions are very welcome.

Besides, we have tested this series in VFIO migration, and nothing else
goes wrong (with two issues committed [2][3]).

Links:
[1] vfio: UAPI for migration interface for device state:
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
    commit/?id=a8a24f3f6e38103b77cf399c38eb54e1219d00d6
[2] vfio: Move the saving of the config space to the right place in VFIO migration:
    https://patchwork.ozlabs.org/patch/1400246/
[3] vfio: Set the priority of VFIO VM state change handler explicitly:
    https://patchwork.ozlabs.org/patch/1401280/

Shenming Lu (2):
  KVM: arm64: GICv4.1: Try to save hw pending state in
    save_pending_tables
  KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state

Zenghui Yu (2):
  irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback
  KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

 .../virt/kvm/devices/arm-vgic-its.rst         |  2 +-
 arch/arm64/kvm/vgic/vgic-its.c                |  6 +-
 arch/arm64/kvm/vgic/vgic-v3.c                 | 62 +++++++++++++++++--
 arch/arm64/kvm/vgic/vgic-v4.c                 | 12 ++++
 drivers/irqchip/irq-gic-v3-its.c              | 38 ++++++++++++
 5 files changed, 110 insertions(+), 10 deletions(-)

-- 
2.23.0

[RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[Shenming Lu]

From: Zenghui Yu <yuzenghui@huawei.com>

Up to now, the irq_get_irqchip_state() callback of its_irq_chip
leaves unimplemented since there is no architectural way to get
the VLPI's pending state before GICv4.1. Yeah, there has one in
v4.1 for VLPIs.

With GICv4.1, after unmapping the vPE, which cleans and invalidates
any caching of the VPT, we can get the VLPI's pending state by
peeking at the VPT. So we implement the irq_get_irqchip_state()
callback of its_irq_chip to do it.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0fec31931e11..287003cacac7 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
 	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
 }
 
+static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t hwirq)
+{
+	int mask = hwirq % BITS_PER_BYTE;
+	void *va;
+	u8 *pt;
+
+	va = page_address(vpe->vpt_page);
+	pt = va + hwirq / BITS_PER_BYTE;
+
+	return !!(*pt & (1U << mask));
+}
+
+static int its_irq_get_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool *val)
+{
+	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+	struct its_vlpi_map *map = get_vlpi_map(d);
+
+	if (which != IRQCHIP_STATE_PENDING)
+		return -EINVAL;
+
+	/* not intended for physical LPI's pending state */
+	if (!map)
+		return -EINVAL;
+
+	/*
+	 * In GICv4.1, a VMAPP with {V,Alloc}=={0,1} cleans and invalidates
+	 * any caching of the VPT associated with the vPEID held in the GIC.
+	 */
+	if (!is_v4_1(its_dev->its) || atomic_read(&map->vpe->vmapp_count))
+		return -EACCES;
+
+	*val = its_peek_vpt(map->vpe, map->vintid);
+
+	return 0;
+}
+
 static int its_irq_set_irqchip_state(struct irq_data *d,
 				     enum irqchip_irq_state which,
 				     bool state)
@@ -1975,6 +2012,7 @@ static struct irq_chip its_irq_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_set_affinity	= its_set_affinity,
 	.irq_compose_msi_msg	= its_irq_compose_msi_msg,
+	.irq_get_irqchip_state	= its_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= its_irq_set_irqchip_state,
 	.irq_retrigger		= its_irq_retrigger,
 	.irq_set_vcpu_affinity	= its_irq_set_vcpu_affinity,
-- 
2.23.0

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[Marc Zyngier]

On 2020-11-23 06:54, Shenming Lu wrote:
> From: Zenghui Yu <yuzenghui@huawei.com>
> 
> Up to now, the irq_get_irqchip_state() callback of its_irq_chip
> leaves unimplemented since there is no architectural way to get
> the VLPI's pending state before GICv4.1. Yeah, there has one in
> v4.1 for VLPIs.
> 
> With GICv4.1, after unmapping the vPE, which cleans and invalidates
> any caching of the VPT, we can get the VLPI's pending state by

This is a crucial note: without this unmapping and invalidation,
the pending bits are not generally accessible (they could be cached
in a GIC private structure, cache or otherwise).

> peeking at the VPT. So we implement the irq_get_irqchip_state()
> callback of its_irq_chip to do it.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 0fec31931e11..287003cacac7 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct
> irq_data *d, struct msi_msg *msg)
>  	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
>  }
> 
> +static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t hwirq)
> +{
> +	int mask = hwirq % BITS_PER_BYTE;

nit: this isn't a mask, but a shift instead. BIT(hwirq % BPB) would give
you a mask.

> +	void *va;
> +	u8 *pt;
> +
> +	va = page_address(vpe->vpt_page);
> +	pt = va + hwirq / BITS_PER_BYTE;
> +
> +	return !!(*pt & (1U << mask));
> +}
> +
> +static int its_irq_get_irqchip_state(struct irq_data *d,
> +				     enum irqchip_irq_state which, bool *val)
> +{
> +	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> +	struct its_vlpi_map *map = get_vlpi_map(d);
> +
> +	if (which != IRQCHIP_STATE_PENDING)
> +		return -EINVAL;
> +
> +	/* not intended for physical LPI's pending state */
> +	if (!map)
> +		return -EINVAL;
> +
> +	/*
> +	 * In GICv4.1, a VMAPP with {V,Alloc}=={0,1} cleans and invalidates
> +	 * any caching of the VPT associated with the vPEID held in the GIC.
> +	 */
> +	if (!is_v4_1(its_dev->its) || atomic_read(&map->vpe->vmapp_count))

It isn't clear to me what prevents this from racing against a mapping of
the VPE. Actually, since we only hold the LPI irqdesc lock, I'm pretty 
sure
nothing prevents it.

> +		return -EACCES;

I can sort of buy EACCESS for a VPE that is currently mapped, but a 
non-4.1
ITS should definitely return EINVAL.

> +
> +	*val = its_peek_vpt(map->vpe, map->vintid);
> +
> +	return 0;
> +}
> +
>  static int its_irq_set_irqchip_state(struct irq_data *d,
>  				     enum irqchip_irq_state which,
>  				     bool state)
> @@ -1975,6 +2012,7 @@ static struct irq_chip its_irq_chip = {
>  	.irq_eoi		= irq_chip_eoi_parent,
>  	.irq_set_affinity	= its_set_affinity,
>  	.irq_compose_msi_msg	= its_irq_compose_msi_msg,
> +	.irq_get_irqchip_state	= its_irq_get_irqchip_state,

My biggest issue with this is that it isn't a reliable interface.
It happens to work in the context of KVM, because you make sure it
is called at the right time, but that doesn't make it safe in general
(anyone with the interrupt number is allowed to call this at any time).

Is there a problem with poking at the VPT page from the KVM side?
The code should be exactly the same (maybe simpler even), and at least
you'd be guaranteed to be in the correct context.

>  	.irq_set_irqchip_state	= its_irq_set_irqchip_state,
>  	.irq_retrigger		= its_irq_retrigger,
>  	.irq_set_vcpu_affinity	= its_irq_set_vcpu_affinity,

Thanks,

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

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[Shenming Lu]

On 2020/11/23 17:01, Marc Zyngier wrote:
> On 2020-11-23 06:54, Shenming Lu wrote:
>> From: Zenghui Yu <yuzenghui@huawei.com>
>>
>> Up to now, the irq_get_irqchip_state() callback of its_irq_chip
>> leaves unimplemented since there is no architectural way to get
>> the VLPI's pending state before GICv4.1. Yeah, there has one in
>> v4.1 for VLPIs.
>>
>> With GICv4.1, after unmapping the vPE, which cleans and invalidates
>> any caching of the VPT, we can get the VLPI's pending state by
> 
> This is a crucial note: without this unmapping and invalidation,
> the pending bits are not generally accessible (they could be cached
> in a GIC private structure, cache or otherwise).
> 
>> peeking at the VPT. So we implement the irq_get_irqchip_state()
>> callback of its_irq_chip to do it.
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 0fec31931e11..287003cacac7 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct
>> irq_data *d, struct msi_msg *msg)
>>      iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
>>  }
>>
>> +static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t hwirq)
>> +{
>> +    int mask = hwirq % BITS_PER_BYTE;
> 
> nit: this isn't a mask, but a shift instead. BIT(hwirq % BPB) would give
> you a mask.

Ok, I will correct it.

> 
>> +    void *va;
>> +    u8 *pt;
>> +
>> +    va = page_address(vpe->vpt_page);
>> +    pt = va + hwirq / BITS_PER_BYTE;
>> +
>> +    return !!(*pt & (1U << mask));
>> +}
>> +
>> +static int its_irq_get_irqchip_state(struct irq_data *d,
>> +                     enum irqchip_irq_state which, bool *val)
>> +{
>> +    struct its_device *its_dev = irq_data_get_irq_chip_data(d);
>> +    struct its_vlpi_map *map = get_vlpi_map(d);
>> +
>> +    if (which != IRQCHIP_STATE_PENDING)
>> +        return -EINVAL;
>> +
>> +    /* not intended for physical LPI's pending state */
>> +    if (!map)
>> +        return -EINVAL;
>> +
>> +    /*
>> +     * In GICv4.1, a VMAPP with {V,Alloc}=={0,1} cleans and invalidates
>> +     * any caching of the VPT associated with the vPEID held in the GIC.
>> +     */
>> +    if (!is_v4_1(its_dev->its) || atomic_read(&map->vpe->vmapp_count))
> 
> It isn't clear to me what prevents this from racing against a mapping of
> the VPE. Actually, since we only hold the LPI irqdesc lock, I'm pretty sure
> nothing prevents it.

Yes, should have the vmovp_lock held?
And is it necessary to also hold this lock in its_vpe_irq_domain_activate/deactivate?

> 
>> +        return -EACCES;
> 
> I can sort of buy EACCESS for a VPE that is currently mapped, but a non-4.1
> ITS should definitely return EINVAL.

Alright, EINVAL looks better.

> 
>> +
>> +    *val = its_peek_vpt(map->vpe, map->vintid);
>> +
>> +    return 0;
>> +}
>> +
>>  static int its_irq_set_irqchip_state(struct irq_data *d,
>>                       enum irqchip_irq_state which,
>>                       bool state)
>> @@ -1975,6 +2012,7 @@ static struct irq_chip its_irq_chip = {
>>      .irq_eoi        = irq_chip_eoi_parent,
>>      .irq_set_affinity    = its_set_affinity,
>>      .irq_compose_msi_msg    = its_irq_compose_msi_msg,
>> +    .irq_get_irqchip_state    = its_irq_get_irqchip_state,
> 
> My biggest issue with this is that it isn't a reliable interface.
> It happens to work in the context of KVM, because you make sure it
> is called at the right time, but that doesn't make it safe in general
> (anyone with the interrupt number is allowed to call this at any time).

We check the vmapp_count in it to ensure the unmapping of the vPE, and
let the caller do the unmapping (they should know whether it is the right
time). If the unmapping is not done, just return a failure.

> 
> Is there a problem with poking at the VPT page from the KVM side?
> The code should be exactly the same (maybe simpler even), and at least
> you'd be guaranteed to be in the correct context.

Yeah, that also seems a good choice.
If you prefer it, we can try to realize it in v2.

> 
>>      .irq_set_irqchip_state    = its_irq_set_irqchip_state,
>>      .irq_retrigger        = its_irq_retrigger,
>>      .irq_set_vcpu_affinity    = its_irq_set_vcpu_affinity,
> 
> Thanks,
> 
>         M.

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[Marc Zyngier]

On 2020-11-24 07:38, Shenming Lu wrote:
> On 2020/11/23 17:01, Marc Zyngier wrote:
>> On 2020-11-23 06:54, Shenming Lu wrote:
>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>> 
>>> Up to now, the irq_get_irqchip_state() callback of its_irq_chip
>>> leaves unimplemented since there is no architectural way to get
>>> the VLPI's pending state before GICv4.1. Yeah, there has one in
>>> v4.1 for VLPIs.
>>> 
>>> With GICv4.1, after unmapping the vPE, which cleans and invalidates
>>> any caching of the VPT, we can get the VLPI's pending state by
>> 
>> This is a crucial note: without this unmapping and invalidation,
>> the pending bits are not generally accessible (they could be cached
>> in a GIC private structure, cache or otherwise).
>> 
>>> peeking at the VPT. So we implement the irq_get_irqchip_state()
>>> callback of its_irq_chip to do it.
>>> 
>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>> ---
>>>  drivers/irqchip/irq-gic-v3-its.c | 38 
>>> ++++++++++++++++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>> 
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index 0fec31931e11..287003cacac7 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct
>>> irq_data *d, struct msi_msg *msg)
>>>      iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
>>>  }
>>> 
>>> +static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t hwirq)
>>> +{
>>> +    int mask = hwirq % BITS_PER_BYTE;
>> 
>> nit: this isn't a mask, but a shift instead. BIT(hwirq % BPB) would 
>> give
>> you a mask.
> 
> Ok, I will correct it.
> 
>> 
>>> +    void *va;
>>> +    u8 *pt;
>>> +
>>> +    va = page_address(vpe->vpt_page);
>>> +    pt = va + hwirq / BITS_PER_BYTE;
>>> +
>>> +    return !!(*pt & (1U << mask));
>>> +}
>>> +
>>> +static int its_irq_get_irqchip_state(struct irq_data *d,
>>> +                     enum irqchip_irq_state which, bool *val)
>>> +{
>>> +    struct its_device *its_dev = irq_data_get_irq_chip_data(d);
>>> +    struct its_vlpi_map *map = get_vlpi_map(d);
>>> +
>>> +    if (which != IRQCHIP_STATE_PENDING)
>>> +        return -EINVAL;
>>> +
>>> +    /* not intended for physical LPI's pending state */
>>> +    if (!map)
>>> +        return -EINVAL;
>>> +
>>> +    /*
>>> +     * In GICv4.1, a VMAPP with {V,Alloc}=={0,1} cleans and 
>>> invalidates
>>> +     * any caching of the VPT associated with the vPEID held in the 
>>> GIC.
>>> +     */
>>> +    if (!is_v4_1(its_dev->its) || 
>>> atomic_read(&map->vpe->vmapp_count))
>> 
>> It isn't clear to me what prevents this from racing against a mapping 
>> of
>> the VPE. Actually, since we only hold the LPI irqdesc lock, I'm pretty 
>> sure
>> nothing prevents it.
> 
> Yes, should have the vmovp_lock held?

That's not helping because of the VPE activation.

> And is it necessary to also hold this lock in
> its_vpe_irq_domain_activate/deactivate?

Well, you'd need that, but that's unnecessary complex AFAICT.

> 
>> 
>>> +        return -EACCES;
>> 
>> I can sort of buy EACCESS for a VPE that is currently mapped, but a 
>> non-4.1
>> ITS should definitely return EINVAL.
> 
> Alright, EINVAL looks better.
> 
>> 
>>> +
>>> +    *val = its_peek_vpt(map->vpe, map->vintid);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static int its_irq_set_irqchip_state(struct irq_data *d,
>>>                       enum irqchip_irq_state which,
>>>                       bool state)
>>> @@ -1975,6 +2012,7 @@ static struct irq_chip its_irq_chip = {
>>>      .irq_eoi        = irq_chip_eoi_parent,
>>>      .irq_set_affinity    = its_set_affinity,
>>>      .irq_compose_msi_msg    = its_irq_compose_msi_msg,
>>> +    .irq_get_irqchip_state    = its_irq_get_irqchip_state,
>> 
>> My biggest issue with this is that it isn't a reliable interface.
>> It happens to work in the context of KVM, because you make sure it
>> is called at the right time, but that doesn't make it safe in general
>> (anyone with the interrupt number is allowed to call this at any 
>> time).
> 
> We check the vmapp_count in it to ensure the unmapping of the vPE, and
> let the caller do the unmapping (they should know whether it is the 
> right
> time). If the unmapping is not done, just return a failure.

And without guaranteeing mutual exclusion against a concurrent VMAPP,
checking the vmapp_count means nothing (you need the lock described
above, and start sprinkling it around in other places as well).

>> 
>> Is there a problem with poking at the VPT page from the KVM side?
>> The code should be exactly the same (maybe simpler even), and at least
>> you'd be guaranteed to be in the correct context.
> 
> Yeah, that also seems a good choice.
> If you prefer it, we can try to realize it in v2.

I'd certainly prefer that. Let me know if you spot any implementation
issue with that.

Thanks,

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

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[luojiaxing]

Hi, shenming


I got few questions about this patch.

Although it's a bit late and not very appropriate, I'd like to ask 
before you send next version.

On 2020/11/23 14:54, Shenming Lu wrote:
> From: Zenghui Yu <yuzenghui@huawei.com>
>
> Up to now, the irq_get_irqchip_state() callback of its_irq_chip
> leaves unimplemented since there is no architectural way to get
> the VLPI's pending state before GICv4.1. Yeah, there has one in
> v4.1 for VLPIs.


I checked the invoking scenario of irq_get_irqchip_state and found no 
scenario related to vLPI.

For example, synchronize_irq(), it pass IRQCHIP_STATE_ACTIVE to which, 
so in your patch, you will directly return and other is for vSGI, 
GICD_ISPENDR, GICD_ICPENDR and so on.

The only one I am not sure is vgic_get_phys_line_level(), is it your 
purpose to fill this callback, or some scenarios I don't know about that 
use this callback.


>
> With GICv4.1, after unmapping the vPE, which cleans and invalidates
> any caching of the VPT, we can get the VLPI's pending state by
> peeking at the VPT. So we implement the irq_get_irqchip_state()
> callback of its_irq_chip to do it.
>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>   drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 0fec31931e11..287003cacac7 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
>   	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
>   }
>   
> +static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t hwirq)
> +{
> +	int mask = hwirq % BITS_PER_BYTE;
> +	void *va;
> +	u8 *pt;
> +
> +	va = page_address(vpe->vpt_page);
> +	pt = va + hwirq / BITS_PER_BYTE;
> +
> +	return !!(*pt & (1U << mask));


How can you confirm that the interrupt pending status is the latest? Is 
it possible that the interrupt pending status is still cached in the 
GICR but not synchronized to the memory.


Thanks

Jiaxing


> +}
> +
> +static int its_irq_get_irqchip_state(struct irq_data *d,
> +				     enum irqchip_irq_state which, bool *val)
> +{
> +	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> +	struct its_vlpi_map *map = get_vlpi_map(d);
> +
> +	if (which != IRQCHIP_STATE_PENDING)
> +		return -EINVAL;
> +
> +	/* not intended for physical LPI's pending state */
> +	if (!map)
> +		return -EINVAL;
> +
> +	/*
> +	 * In GICv4.1, a VMAPP with {V,Alloc}=={0,1} cleans and invalidates
> +	 * any caching of the VPT associated with the vPEID held in the GIC.
> +	 */
> +	if (!is_v4_1(its_dev->its) || atomic_read(&map->vpe->vmapp_count))
> +		return -EACCES;
> +
> +	*val = its_peek_vpt(map->vpe, map->vintid);
> +
> +	return 0;
> +}
> +
>   static int its_irq_set_irqchip_state(struct irq_data *d,
>   				     enum irqchip_irq_state which,
>   				     bool state)
> @@ -1975,6 +2012,7 @@ static struct irq_chip its_irq_chip = {
>   	.irq_eoi		= irq_chip_eoi_parent,
>   	.irq_set_affinity	= its_set_affinity,
>   	.irq_compose_msi_msg	= its_irq_compose_msi_msg,
> +	.irq_get_irqchip_state	= its_irq_get_irqchip_state,
>   	.irq_set_irqchip_state	= its_irq_set_irqchip_state,
>   	.irq_retrigger		= its_irq_retrigger,
>   	.irq_set_vcpu_affinity	= its_irq_set_vcpu_affinity,

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[Marc Zyngier]

On Sat, 28 Nov 2020 07:19:48 +0000,
luojiaxing <luojiaxing@huawei.com> wrote:
> 
> Hi, shenming
> 
> 
> I got few questions about this patch.
> 
> Although it's a bit late and not very appropriate, I'd like to ask
> before you send next version.
> 
> On 2020/11/23 14:54, Shenming Lu wrote:
> > From: Zenghui Yu <yuzenghui@huawei.com>
> > 
> > Up to now, the irq_get_irqchip_state() callback of its_irq_chip
> > leaves unimplemented since there is no architectural way to get
> > the VLPI's pending state before GICv4.1. Yeah, there has one in
> > v4.1 for VLPIs.
> 
> 
> I checked the invoking scenario of irq_get_irqchip_state and found no
> scenario related to vLPI.
> 
> For example, synchronize_irq(), it pass IRQCHIP_STATE_ACTIVE to which,
> so in your patch, you will directly return and other is for vSGI,
> GICD_ISPENDR, GICD_ICPENDR and so on.

You do realise that LPIs have no active state, right? And that LPIs
have a radically different programming interface to the rest of the GIC?

> The only one I am not sure is vgic_get_phys_line_level(), is it your
> purpose to fill this callback, or some scenarios I don't know about
> that use this callback.

LPIs only offer edge signalling, so the concept of "line level" means
absolutely nothing.

> 
> 
> > 
> > With GICv4.1, after unmapping the vPE, which cleans and invalidates
> > any caching of the VPT, we can get the VLPI's pending state by
> > peeking at the VPT. So we implement the irq_get_irqchip_state()
> > callback of its_irq_chip to do it.
> > 
> > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> > Signed-off-by: Shenming Lu <lushenming@huawei.com>
> > ---
> >   drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
> >   1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 0fec31931e11..287003cacac7 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> >   	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
> >   }
> >   +static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t
> > hwirq)
> > +{
> > +	int mask = hwirq % BITS_PER_BYTE;
> > +	void *va;
> > +	u8 *pt;
> > +
> > +	va = page_address(vpe->vpt_page);
> > +	pt = va + hwirq / BITS_PER_BYTE;
> > +
> > +	return !!(*pt & (1U << mask));
> 
> 
> How can you confirm that the interrupt pending status is the latest?
> Is it possible that the interrupt pending status is still cached in
> the GICR but not synchronized to the memory.

That's a consequence of the vPE having been unmapped:

"A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of
the Virtual Pending Table and Virtual Configuration Table associated
with the vPEID held in the GIC."

An implementation that wouldn't follow this simple rule would simply
be totally broken, and unsupported.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[luojiaxing]

On 2020/11/28 18:18, Marc Zyngier wrote:
> On Sat, 28 Nov 2020 07:19:48 +0000,
> luojiaxing <luojiaxing@huawei.com> wrote:
>> Hi, shenming
>>
>>
>> I got few questions about this patch.
>>
>> Although it's a bit late and not very appropriate, I'd like to ask
>> before you send next version.
>>
>> On 2020/11/23 14:54, Shenming Lu wrote:
>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>>
>>> Up to now, the irq_get_irqchip_state() callback of its_irq_chip
>>> leaves unimplemented since there is no architectural way to get
>>> the VLPI's pending state before GICv4.1. Yeah, there has one in
>>> v4.1 for VLPIs.
>>
>> I checked the invoking scenario of irq_get_irqchip_state and found no
>> scenario related to vLPI.
>>
>> For example, synchronize_irq(), it pass IRQCHIP_STATE_ACTIVE to which,
>> so in your patch, you will directly return and other is for vSGI,
>> GICD_ISPENDR, GICD_ICPENDR and so on.
> You do realise that LPIs have no active state, right?


yes, I know


> And that LPIs
> have a radically different programming interface to the rest of the GIC?


I found out that my mailbox software filtered out the other two patches, 
which led me to look at the patch alone, so it was weird. I already got 
the answer now.


>> The only one I am not sure is vgic_get_phys_line_level(), is it your
>> purpose to fill this callback, or some scenarios I don't know about
>> that use this callback.
> LPIs only offer edge signalling, so the concept of "line level" means
> absolutely nothing.
>
>>
>>> With GICv4.1, after unmapping the vPE, which cleans and invalidates
>>> any caching of the VPT, we can get the VLPI's pending state by
>>> peeking at the VPT. So we implement the irq_get_irqchip_state()
>>> callback of its_irq_chip to do it.
>>>
>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>> ---
>>>    drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
>>>    1 file changed, 38 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 0fec31931e11..287003cacac7 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -1695,6 +1695,43 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
>>>    	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
>>>    }
>>>    +static bool its_peek_vpt(struct its_vpe *vpe, irq_hw_number_t
>>> hwirq)
>>> +{
>>> +	int mask = hwirq % BITS_PER_BYTE;
>>> +	void *va;
>>> +	u8 *pt;
>>> +
>>> +	va = page_address(vpe->vpt_page);
>>> +	pt = va + hwirq / BITS_PER_BYTE;
>>> +
>>> +	return !!(*pt & (1U << mask));
>>
>> How can you confirm that the interrupt pending status is the latest?
>> Is it possible that the interrupt pending status is still cached in
>> the GICR but not synchronized to the memory.
> That's a consequence of the vPE having been unmapped:
>
> "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of
> the Virtual Pending Table and Virtual Configuration Table associated
> with the vPEID held in the GIC."


Yes, in addition to that, if a vPE is scheduled out of the PE, the cache 
clearing and write-back to VPT are also performed, I think.


However, I feel a litter confusing to read this comment at first ,  
because it is not only VMAPP that causes cache clearing.

I don't know why VMAPP was mentioned here until I check the other two 
patches ("KVM: arm64: GICv4.1: Try to save hw pending state in 
save_pending_tables").


So I think may be it's better to add some background description here.


Thanks

Jiaxing


>
> An implementation that wouldn't follow this simple rule would simply
> be totally broken, and unsupported.
>
> 	M.
>

Re: [RFC PATCH v1 1/4] irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback

[Marc Zyngier]

On 2020-12-01 09:38, luojiaxing wrote:
> On 2020/11/28 18:18, Marc Zyngier wrote:
>> On Sat, 28 Nov 2020 07:19:48 +0000,
>> luojiaxing <luojiaxing@huawei.com> wrote:

>>> How can you confirm that the interrupt pending status is the latest?
>>> Is it possible that the interrupt pending status is still cached in
>>> the GICR but not synchronized to the memory.
>> That's a consequence of the vPE having been unmapped:
>> 
>> "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of
>> the Virtual Pending Table and Virtual Configuration Table associated
>> with the vPEID held in the GIC."
> 
> 
> Yes, in addition to that, if a vPE is scheduled out of the PE, the
> cache clearing and write-back to VPT are also performed, I think.

There is no such architectural requirement.

> However, I feel a litter confusing to read this comment at first , 
> because it is not only VMAPP that causes cache clearing.

I can't see anything else that guarantee that the caches are clean,
and that there is no possible write to the PE table.

> I don't know why VMAPP was mentioned here until I check the other two
> patches ("KVM: arm64: GICv4.1: Try to save hw pending state in
> save_pending_tables").
> 
> So I think may be it's better to add some background description here.

Well, relying on the standard irqchip state methods to peek at the
pending state isn't very reliable, as you could be temped to call into
this even when the VPE is mapped. Which is why I've suggested
a different implementation.

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

[RFC PATCH v1 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables

[Shenming Lu]

After pausing all vCPUs and devices capable of interrupting, in order
to save the information of all interrupts, besides flushing the pending
states in kvm’s vgic, we also try to flush the states of VLPIs in the
virtual pending tables into guest RAM, but we need to have GICv4.1 and
safely unmap the vPEs first.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 9cdf39a94a63..e1b3aa4b2b12 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <kvm/arm_vgic.h>
@@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
 	return 0;
 }
 
+/*
+ * With GICv4.1, we can get the VLPI's pending state after unmapping
+ * the vPE. The deactivation of the doorbell interrupt will trigger
+ * the unmapping of the associated vPE.
+ */
+static void get_vlpi_state_pre(struct vgic_dist *dist)
+{
+	struct irq_desc *desc;
+	int i;
+
+	if (!kvm_vgic_global_state.has_gicv4_1)
+		return;
+
+	for (i = 0; i < dist->its_vm.nr_vpes; i++) {
+		desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
+		irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
+	}
+}
+
+static void get_vlpi_state_post(struct vgic_dist *dist)
+{
+	struct irq_desc *desc;
+	int i;
+
+	if (!kvm_vgic_global_state.has_gicv4_1)
+		return;
+
+	for (i = 0; i < dist->its_vm.nr_vpes; i++) {
+		desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
+		irq_domain_activate_irq(irq_desc_get_irq_data(desc), false);
+	}
+}
+
 /**
  * vgic_v3_save_pending_tables - Save the pending tables into guest RAM
  * kvm lock and all vcpu lock must be held
@@ -365,14 +400,17 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	struct vgic_irq *irq;
 	gpa_t last_ptr = ~(gpa_t)0;
-	int ret;
+	int ret = 0;
 	u8 val;
 
+	get_vlpi_state_pre(dist);
+
 	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
 		int byte_offset, bit_nr;
 		struct kvm_vcpu *vcpu;
 		gpa_t pendbase, ptr;
 		bool stored;
+		bool is_pending = irq->pending_latch;
 
 		vcpu = irq->target_vcpu;
 		if (!vcpu)
@@ -387,24 +425,36 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 		if (ptr != last_ptr) {
 			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
 			if (ret)
-				return ret;
+				goto out;
 			last_ptr = ptr;
 		}
 
 		stored = val & (1U << bit_nr);
-		if (stored == irq->pending_latch)
+
+		/* also flush hw pending state */
+		if (irq->hw) {
+			WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
+						IRQCHIP_STATE_PENDING, &is_pending),
+				       "IRQ %d", irq->host_irq);
+		}
+
+		if (stored == is_pending)
 			continue;
 
-		if (irq->pending_latch)
+		if (is_pending)
 			val |= 1 << bit_nr;
 		else
 			val &= ~(1 << bit_nr);
 
 		ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
 		if (ret)
-			return ret;
+			goto out;
 	}
-	return 0;
+
+out:
+	get_vlpi_state_post(dist);
+
+	return ret;
 }
 
 /**
-- 
2.23.0

Re: [RFC PATCH v1 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables

[Marc Zyngier]

On 2020-11-23 06:54, Shenming Lu wrote:
> After pausing all vCPUs and devices capable of interrupting, in order
         ^^^^^^^^^^^^^^^^^
See my comment below about this.

> to save the information of all interrupts, besides flushing the pending
> states in kvm’s vgic, we also try to flush the states of VLPIs in the
> virtual pending tables into guest RAM, but we need to have GICv4.1 and
> safely unmap the vPEs first.
> 
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>  arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c 
> b/arch/arm64/kvm/vgic/vgic-v3.c
> index 9cdf39a94a63..e1b3aa4b2b12 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> 
>  #include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/kvm.h>
>  #include <linux/kvm_host.h>
>  #include <kvm/arm_vgic.h>
> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
> *kvm, struct vgic_irq *irq)
>  	return 0;
>  }
> 
> +/*
> + * With GICv4.1, we can get the VLPI's pending state after unmapping
> + * the vPE. The deactivation of the doorbell interrupt will trigger
> + * the unmapping of the associated vPE.
> + */
> +static void get_vlpi_state_pre(struct vgic_dist *dist)
> +{
> +	struct irq_desc *desc;
> +	int i;
> +
> +	if (!kvm_vgic_global_state.has_gicv4_1)
> +		return;
> +
> +	for (i = 0; i < dist->its_vm.nr_vpes; i++) {
> +		desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
> +		irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
> +	}
> +}
> +
> +static void get_vlpi_state_post(struct vgic_dist *dist)

nit: the naming feels a bit... odd. Pre/post what?

> +{
> +	struct irq_desc *desc;
> +	int i;
> +
> +	if (!kvm_vgic_global_state.has_gicv4_1)
> +		return;
> +
> +	for (i = 0; i < dist->its_vm.nr_vpes; i++) {
> +		desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
> +		irq_domain_activate_irq(irq_desc_get_irq_data(desc), false);
> +	}
> +}
> +
>  /**
>   * vgic_v3_save_pending_tables - Save the pending tables into guest 
> RAM
>   * kvm lock and all vcpu lock must be held
> @@ -365,14 +400,17 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct vgic_irq *irq;
>  	gpa_t last_ptr = ~(gpa_t)0;
> -	int ret;
> +	int ret = 0;
>  	u8 val;
> 
> +	get_vlpi_state_pre(dist);
> +
>  	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>  		int byte_offset, bit_nr;
>  		struct kvm_vcpu *vcpu;
>  		gpa_t pendbase, ptr;
>  		bool stored;
> +		bool is_pending = irq->pending_latch;
> 
>  		vcpu = irq->target_vcpu;
>  		if (!vcpu)
> @@ -387,24 +425,36 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  		if (ptr != last_ptr) {
>  			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>  			if (ret)
> -				return ret;
> +				goto out;
>  			last_ptr = ptr;
>  		}
> 
>  		stored = val & (1U << bit_nr);
> -		if (stored == irq->pending_latch)
> +
> +		/* also flush hw pending state */

This comment looks out of place, as we aren't flushing anything.

> +		if (irq->hw) {
> +			WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
> +						IRQCHIP_STATE_PENDING, &is_pending),
> +				       "IRQ %d", irq->host_irq);

Isn't this going to warn like mad on a GICv4.0 system where this, by 
definition,
will generate an error?

> +		}
> +
> +		if (stored == is_pending)
>  			continue;
> 
> -		if (irq->pending_latch)
> +		if (is_pending)
>  			val |= 1 << bit_nr;
>  		else
>  			val &= ~(1 << bit_nr);
> 
>  		ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>  		if (ret)
> -			return ret;
> +			goto out;
>  	}
> -	return 0;
> +
> +out:
> +	get_vlpi_state_post(dist);

This bit worries me: you have unmapped the VPEs, so any interrupt that 
has been
generated during that phase is now forever lost (the GIC doesn't have 
ownership
of the pending tables).

Do you really expect the VM to be restartable from that point? I don't 
see how
this is possible.

> +
> +	return ret;
>  }
> 
>  /**

Thanks,

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

Re: [RFC PATCH v1 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables

[Shenming Lu]

On 2020/11/23 17:18, Marc Zyngier wrote:
> On 2020-11-23 06:54, Shenming Lu wrote:
>> After pausing all vCPUs and devices capable of interrupting, in order
>         ^^^^^^^^^^^^^^^^^
> See my comment below about this.
> 
>> to save the information of all interrupts, besides flushing the pending
>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>> virtual pending tables into guest RAM, but we need to have GICv4.1 and
>> safely unmap the vPEs first.
>>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
>>  1 file changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>> @@ -1,6 +1,8 @@
>>  // SPDX-License-Identifier: GPL-2.0-only
>>
>>  #include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>>  #include <linux/kvm.h>
>>  #include <linux/kvm_host.h>
>>  #include <kvm/arm_vgic.h>
>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>> *kvm, struct vgic_irq *irq)
>>      return 0;
>>  }
>>
>> +/*
>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>> + * the unmapping of the associated vPE.
>> + */
>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>> +{
>> +    struct irq_desc *desc;
>> +    int i;
>> +
>> +    if (!kvm_vgic_global_state.has_gicv4_1)
>> +        return;
>> +
>> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>> +        irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>> +    }
>> +}
>> +
>> +static void get_vlpi_state_post(struct vgic_dist *dist)
> 
> nit: the naming feels a bit... odd. Pre/post what?

My understanding is that the unmapping is a preparation for get_vlpi_state...
Maybe just call it unmap/map_all_vpes?

> 
>> +{
>> +    struct irq_desc *desc;
>> +    int i;
>> +
>> +    if (!kvm_vgic_global_state.has_gicv4_1)
>> +        return;
>> +
>> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>> +        irq_domain_activate_irq(irq_desc_get_irq_data(desc), false);
>> +    }
>> +}
>> +
>>  /**
>>   * vgic_v3_save_pending_tables - Save the pending tables into guest RAM
>>   * kvm lock and all vcpu lock must be held
>> @@ -365,14 +400,17 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>      struct vgic_dist *dist = &kvm->arch.vgic;
>>      struct vgic_irq *irq;
>>      gpa_t last_ptr = ~(gpa_t)0;
>> -    int ret;
>> +    int ret = 0;
>>      u8 val;
>>
>> +    get_vlpi_state_pre(dist);
>> +
>>      list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>>          int byte_offset, bit_nr;
>>          struct kvm_vcpu *vcpu;
>>          gpa_t pendbase, ptr;
>>          bool stored;
>> +        bool is_pending = irq->pending_latch;
>>
>>          vcpu = irq->target_vcpu;
>>          if (!vcpu)
>> @@ -387,24 +425,36 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>          if (ptr != last_ptr) {
>>              ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>>              if (ret)
>> -                return ret;
>> +                goto out;
>>              last_ptr = ptr;
>>          }
>>
>>          stored = val & (1U << bit_nr);
>> -        if (stored == irq->pending_latch)
>> +
>> +        /* also flush hw pending state */
> 
> This comment looks out of place, as we aren't flushing anything.

Ok, I will correct it.

> 
>> +        if (irq->hw) {
>> +            WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>> +                        IRQCHIP_STATE_PENDING, &is_pending),
>> +                       "IRQ %d", irq->host_irq);
> 
> Isn't this going to warn like mad on a GICv4.0 system where this, by definition,
> will generate an error?

As we have returned an error in save_its_tables if hw && !has_gicv4_1, we don't
have to warn this here?

> 
>> +        }
>> +
>> +        if (stored == is_pending)
>>              continue;
>>
>> -        if (irq->pending_latch)
>> +        if (is_pending)
>>              val |= 1 << bit_nr;
>>          else
>>              val &= ~(1 << bit_nr);
>>
>>          ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>>          if (ret)
>> -            return ret;
>> +            goto out;
>>      }
>> -    return 0;
>> +
>> +out:
>> +    get_vlpi_state_post(dist);
> 
> This bit worries me: you have unmapped the VPEs, so any interrupt that has been
> generated during that phase is now forever lost (the GIC doesn't have ownership
> of the pending tables).

In my opinion, during this phase, the devices capable of interrupting should have
already been paused (prevent from sending interrupts), such as VFIO migration protocol
has already realized it.

> 
> Do you really expect the VM to be restartable from that point? I don't see how
> this is possible.
> 

If the migration has encountered an error, the src VM might be restarted, so we have to
map the vPEs back.

>> +
>> +    return ret;
>>  }
>>
>>  /**
> 
> Thanks,
> 
>         M.

Thanks,
Shenming

Re: [RFC PATCH v1 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables

[Marc Zyngier]

On 2020-11-24 07:40, Shenming Lu wrote:
> On 2020/11/23 17:18, Marc Zyngier wrote:
>> On 2020-11-23 06:54, Shenming Lu wrote:
>>> After pausing all vCPUs and devices capable of interrupting, in order
>>         ^^^^^^^^^^^^^^^^^
>> See my comment below about this.
>> 
>>> to save the information of all interrupts, besides flushing the 
>>> pending
>>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>>> virtual pending tables into guest RAM, but we need to have GICv4.1 
>>> and
>>> safely unmap the vPEs first.
>>> 
>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-v3.c | 62 
>>> +++++++++++++++++++++++++++++++----
>>>  1 file changed, 56 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c 
>>> b/arch/arm64/kvm/vgic/vgic-v3.c
>>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>> @@ -1,6 +1,8 @@
>>>  // SPDX-License-Identifier: GPL-2.0-only
>>> 
>>>  #include <linux/irqchip/arm-gic-v3.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/irqdomain.h>
>>>  #include <linux/kvm.h>
>>>  #include <linux/kvm_host.h>
>>>  #include <kvm/arm_vgic.h>
>>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>>> *kvm, struct vgic_irq *irq)
>>>      return 0;
>>>  }
>>> 
>>> +/*
>>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>>> + * the unmapping of the associated vPE.
>>> + */
>>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>>> +{
>>> +    struct irq_desc *desc;
>>> +    int i;
>>> +
>>> +    if (!kvm_vgic_global_state.has_gicv4_1)
>>> +        return;
>>> +
>>> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>>> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>>> +        irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>>> +    }
>>> +}
>>> +
>>> +static void get_vlpi_state_post(struct vgic_dist *dist)
>> 
>> nit: the naming feels a bit... odd. Pre/post what?
> 
> My understanding is that the unmapping is a preparation for 
> get_vlpi_state...
> Maybe just call it unmap/map_all_vpes?

Yes, much better.

[...]

>>> +        if (irq->hw) {
>>> +            WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>>> +                        IRQCHIP_STATE_PENDING, &is_pending),
>>> +                       "IRQ %d", irq->host_irq);
>> 
>> Isn't this going to warn like mad on a GICv4.0 system where this, by 
>> definition,
>> will generate an error?
> 
> As we have returned an error in save_its_tables if hw && !has_gicv4_1, 
> we don't
> have to warn this here?

Are you referring to the check in vgic_its_save_itt() that occurs in 
patch 4?
Fair enough, though I think the use of irq_get_irqchip_state() isn't 
quite
what we want, as per my comments on patch #1.

>> 
>>> +        }
>>> +
>>> +        if (stored == is_pending)
>>>              continue;
>>> 
>>> -        if (irq->pending_latch)
>>> +        if (is_pending)
>>>              val |= 1 << bit_nr;
>>>          else
>>>              val &= ~(1 << bit_nr);
>>> 
>>>          ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>>>          if (ret)
>>> -            return ret;
>>> +            goto out;
>>>      }
>>> -    return 0;
>>> +
>>> +out:
>>> +    get_vlpi_state_post(dist);
>> 
>> This bit worries me: you have unmapped the VPEs, so any interrupt that 
>> has been
>> generated during that phase is now forever lost (the GIC doesn't have 
>> ownership
>> of the pending tables).
> 
> In my opinion, during this phase, the devices capable of interrupting
> should have  already been paused (prevent from sending interrupts),
> such as VFIO migration protocol has already realized it.

Is that a hard guarantee? Pausing devices *may* be possible for a 
limited
set of endpoints, but I'm not sure that is universally possible to 
restart
them and expect a consistent state (you have just dropped a bunch of 
network
packets on the floor...).

>> Do you really expect the VM to be restartable from that point? I don't 
>> see how
>> this is possible.
>> 
> 
> If the migration has encountered an error, the src VM might be
> restarted, so we have to map the vPEs back.

As I said above, I doubt it is universally possible to do so, but
after all, this probably isn't worse that restarting on the target...

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

Re: [RFC PATCH v1 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables

[Shenming Lu]

On 2020/11/24 16:26, Marc Zyngier wrote:
> On 2020-11-24 07:40, Shenming Lu wrote:
>> On 2020/11/23 17:18, Marc Zyngier wrote:
>>> On 2020-11-23 06:54, Shenming Lu wrote:
>>>> After pausing all vCPUs and devices capable of interrupting, in order
>>>         ^^^^^^^^^^^^^^^^^
>>> See my comment below about this.
>>>
>>>> to save the information of all interrupts, besides flushing the pending
>>>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>>>> virtual pending tables into guest RAM, but we need to have GICv4.1 and
>>>> safely unmap the vPEs first.
>>>>
>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>> ---
>>>>  arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
>>>>  1 file changed, 56 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>>> @@ -1,6 +1,8 @@
>>>>  // SPDX-License-Identifier: GPL-2.0-only
>>>>
>>>>  #include <linux/irqchip/arm-gic-v3.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/irqdomain.h>
>>>>  #include <linux/kvm.h>
>>>>  #include <linux/kvm_host.h>
>>>>  #include <kvm/arm_vgic.h>
>>>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>>>> *kvm, struct vgic_irq *irq)
>>>>      return 0;
>>>>  }
>>>>
>>>> +/*
>>>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>>>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>>>> + * the unmapping of the associated vPE.
>>>> + */
>>>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>>>> +{
>>>> +    struct irq_desc *desc;
>>>> +    int i;
>>>> +
>>>> +    if (!kvm_vgic_global_state.has_gicv4_1)
>>>> +        return;
>>>> +
>>>> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>>>> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>>>> +        irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>>>> +    }
>>>> +}
>>>> +
>>>> +static void get_vlpi_state_post(struct vgic_dist *dist)
>>>
>>> nit: the naming feels a bit... odd. Pre/post what?
>>
>> My understanding is that the unmapping is a preparation for get_vlpi_state...
>> Maybe just call it unmap/map_all_vpes?
> 
> Yes, much better.
> 
> [...]
> 
>>>> +        if (irq->hw) {
>>>> +            WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>>>> +                        IRQCHIP_STATE_PENDING, &is_pending),
>>>> +                       "IRQ %d", irq->host_irq);
>>>
>>> Isn't this going to warn like mad on a GICv4.0 system where this, by definition,
>>> will generate an error?
>>
>> As we have returned an error in save_its_tables if hw && !has_gicv4_1, we don't
>> have to warn this here?
> 
> Are you referring to the check in vgic_its_save_itt() that occurs in patch 4?
> Fair enough, though I think the use of irq_get_irqchip_state() isn't quite
> what we want, as per my comments on patch #1.
> 
>>>
>>>> +        }
>>>> +
>>>> +        if (stored == is_pending)
>>>>              continue;
>>>>
>>>> -        if (irq->pending_latch)
>>>> +        if (is_pending)
>>>>              val |= 1 << bit_nr;
>>>>          else
>>>>              val &= ~(1 << bit_nr);
>>>>
>>>>          ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>>>>          if (ret)
>>>> -            return ret;
>>>> +            goto out;
>>>>      }
>>>> -    return 0;
>>>> +
>>>> +out:
>>>> +    get_vlpi_state_post(dist);
>>>
>>> This bit worries me: you have unmapped the VPEs, so any interrupt that has been
>>> generated during that phase is now forever lost (the GIC doesn't have ownership
>>> of the pending tables).
>>
>> In my opinion, during this phase, the devices capable of interrupting
>> should have  already been paused (prevent from sending interrupts),
>> such as VFIO migration protocol has already realized it.
> 
> Is that a hard guarantee? Pausing devices *may* be possible for a limited
> set of endpoints, but I'm not sure that is universally possible to restart
> them and expect a consistent state (you have just dropped a bunch of network
> packets on the floor...).

No, as far as I know, if the VFIO device does not support pause, the migration would
fail early... And the specific action is decided by the vendor driver.
In fact, the VFIO migration is still in an experimental phase... I will pay attention
to the follow-up development.

> 
>>> Do you really expect the VM to be restartable from that point? I don't see how
>>> this is possible.
>>>
>>
>> If the migration has encountered an error, the src VM might be
>> restarted, so we have to map the vPEs back.
> 
> As I said above, I doubt it is universally possible to do so, but
> after all, this probably isn't worse that restarting on the target...
> 
>         M.

[RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

From: Zenghui Yu <yuzenghui@huawei.com>

When setting the forwarding path of a VLPI, it is more consistent to
also transfer the pending state from irq->pending_latch to VPT (especially
in migration, the pending states of VLPIs are restored into kvm’s vgic
first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index b5fa73c9fd35..cc3ab9cea182 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 	irq->host_irq	= virq;
 	atomic_inc(&map.vpe->vlpi_count);
 
+	/* Transfer pending state */
+	ret = irq_set_irqchip_state(irq->host_irq,
+				    IRQCHIP_STATE_PENDING,
+				    irq->pending_latch);
+	WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
+
+	/*
+	 * Let it be pruned from ap_list later and don't bother
+	 * the List Register.
+	 */
+	irq->pending_latch = false;
+
 out:
 	mutex_unlock(&its->its_lock);
 	return ret;
-- 
2.23.0

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Marc Zyngier]

On 2020-11-23 06:54, Shenming Lu wrote:
> From: Zenghui Yu <yuzenghui@huawei.com>
> 
> When setting the forwarding path of a VLPI, it is more consistent to

I'm not sure it is more consistent. It is a *new* behaviour, because it 
only
matters for migration, which has been so far unsupported.

> also transfer the pending state from irq->pending_latch to VPT 
> (especially
> in migration, the pending states of VLPIs are restored into kvm’s vgic
> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>  arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c 
> b/arch/arm64/kvm/vgic/vgic-v4.c
> index b5fa73c9fd35..cc3ab9cea182 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, 
> int virq,
>  	irq->host_irq	= virq;
>  	atomic_inc(&map.vpe->vlpi_count);
> 
> +	/* Transfer pending state */
> +	ret = irq_set_irqchip_state(irq->host_irq,
> +				    IRQCHIP_STATE_PENDING,
> +				    irq->pending_latch);
> +	WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
> +
> +	/*
> +	 * Let it be pruned from ap_list later and don't bother
> +	 * the List Register.
> +	 */
> +	irq->pending_latch = false;

It occurs to me that calling into irq_set_irqchip_state() for a large
number of interrupts can take a significant amount of time. It is also
odd that you dump the VPT with the VPE unmapped, but rely on the VPE
being mapped for the opposite operation.

Shouldn't these be symmetric, all performed while the VPE is unmapped?
It would also save a lot of ITS traffic.

> +
>  out:
>  	mutex_unlock(&its->its_lock);
>  	return ret;

Thanks,

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

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/11/23 17:27, Marc Zyngier wrote:
> On 2020-11-23 06:54, Shenming Lu wrote:
>> From: Zenghui Yu <yuzenghui@huawei.com>
>>
>> When setting the forwarding path of a VLPI, it is more consistent to
> 
> I'm not sure it is more consistent. It is a *new* behaviour, because it only
> matters for migration, which has been so far unsupported.

Alright, consistent may not be accurate...
But I have doubt that whether there is really no need to transfer the pending states
from kvm'vgic to VPT in set_forwarding regardless of migration, and the similar
for unset_forwarding.

> 
>> also transfer the pending state from irq->pending_latch to VPT (especially
>> in migration, the pending states of VLPIs are restored into kvm’s vgic
>> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>> index b5fa73c9fd35..cc3ab9cea182 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>      irq->host_irq    = virq;
>>      atomic_inc(&map.vpe->vlpi_count);
>>
>> +    /* Transfer pending state */
>> +    ret = irq_set_irqchip_state(irq->host_irq,
>> +                    IRQCHIP_STATE_PENDING,
>> +                    irq->pending_latch);
>> +    WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>> +
>> +    /*
>> +     * Let it be pruned from ap_list later and don't bother
>> +     * the List Register.
>> +     */
>> +    irq->pending_latch = false;
> 
> It occurs to me that calling into irq_set_irqchip_state() for a large
> number of interrupts can take a significant amount of time. It is also
> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
> being mapped for the opposite operation.
> 
> Shouldn't these be symmetric, all performed while the VPE is unmapped?
> It would also save a lot of ITS traffic.
> 

My thought was to use the existing interface directly without unmapping...

If you want to unmap the vPE and poke the VPT here, as I said in the cover
letter, set/unset_forwarding might also be called when all devices are running
at normal run time, in which case the unmapping of the vPE is not allowed...

Another possible solution is to add a new dedicated interface to QEMU to transfer
these pending states to HW in GIC VM state change handler corresponding to
save_pending_tables?

>> +
>>  out:
>>      mutex_unlock(&its->its_lock);
>>      return ret;
> 
> Thanks,
> 
>         M.

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Marc Zyngier]

On 2020-11-24 08:10, Shenming Lu wrote:
> On 2020/11/23 17:27, Marc Zyngier wrote:
>> On 2020-11-23 06:54, Shenming Lu wrote:
>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>> 
>>> When setting the forwarding path of a VLPI, it is more consistent to
>> 
>> I'm not sure it is more consistent. It is a *new* behaviour, because 
>> it only
>> matters for migration, which has been so far unsupported.
> 
> Alright, consistent may not be accurate...
> But I have doubt that whether there is really no need to transfer the
> pending states
> from kvm'vgic to VPT in set_forwarding regardless of migration, and the 
> similar
> for unset_forwarding.

If you have to transfer that state outside of the a save/restore, it 
means that
you have missed the programming of the PCI endpoint. This is an 
established
restriction that the MSI programming must occur *after* the translation 
has
been established using MAPI/MAPTI (see the large comment at the 
beginning of
vgic-v4.c).

If you want to revisit this, fair enough. But you will need a lot more 
than
just opportunistically transfer the pending state.

> 
>> 
>>> also transfer the pending state from irq->pending_latch to VPT 
>>> (especially
>>> in migration, the pending states of VLPIs are restored into kvm’s 
>>> vgic
>>> first). And we currently send "INT+VSYNC" to trigger a VLPI to 
>>> pending.
>>> 
>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>> 
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c 
>>> b/arch/arm64/kvm/vgic/vgic-v4.c
>>> index b5fa73c9fd35..cc3ab9cea182 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, 
>>> int virq,
>>>      irq->host_irq    = virq;
>>>      atomic_inc(&map.vpe->vlpi_count);
>>> 
>>> +    /* Transfer pending state */
>>> +    ret = irq_set_irqchip_state(irq->host_irq,
>>> +                    IRQCHIP_STATE_PENDING,
>>> +                    irq->pending_latch);
>>> +    WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>> +
>>> +    /*
>>> +     * Let it be pruned from ap_list later and don't bother
>>> +     * the List Register.
>>> +     */
>>> +    irq->pending_latch = false;
>> 
>> It occurs to me that calling into irq_set_irqchip_state() for a large
>> number of interrupts can take a significant amount of time. It is also
>> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
>> being mapped for the opposite operation.
>> 
>> Shouldn't these be symmetric, all performed while the VPE is unmapped?
>> It would also save a lot of ITS traffic.
>> 
> 
> My thought was to use the existing interface directly without 
> unmapping...
> 
> If you want to unmap the vPE and poke the VPT here, as I said in the 
> cover
> letter, set/unset_forwarding might also be called when all devices are 
> running
> at normal run time, in which case the unmapping of the vPE is not 
> allowed...

No, I'm suggesting that you don't do anything here, but instead as a 
by-product
of restoring the ITS tables. What goes wrong if you use the
KVM_DEV_ARM_ITS_RESTORE_TABLE backend instead?

> Another possible solution is to add a new dedicated interface to QEMU
> to transfer
> these pending states to HW in GIC VM state change handler corresponding 
> to
> save_pending_tables?

Userspace has no way to know we use GICv4, and I intend to keep it
completely out of the loop. The API is already pretty tortuous, and
I really don't want to add any extra complexity to it.

Thanks,

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

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/11/24 16:44, Marc Zyngier wrote:
> On 2020-11-24 08:10, Shenming Lu wrote:
>> On 2020/11/23 17:27, Marc Zyngier wrote:
>>> On 2020-11-23 06:54, Shenming Lu wrote:
>>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>>>
>>>> When setting the forwarding path of a VLPI, it is more consistent to
>>>
>>> I'm not sure it is more consistent. It is a *new* behaviour, because it only
>>> matters for migration, which has been so far unsupported.
>>
>> Alright, consistent may not be accurate...
>> But I have doubt that whether there is really no need to transfer the
>> pending states
>> from kvm'vgic to VPT in set_forwarding regardless of migration, and the similar
>> for unset_forwarding.
> 
> If you have to transfer that state outside of the a save/restore, it means that
> you have missed the programming of the PCI endpoint. This is an established
> restriction that the MSI programming must occur *after* the translation has
> been established using MAPI/MAPTI (see the large comment at the beginning of
> vgic-v4.c).
> 
> If you want to revisit this, fair enough. But you will need a lot more than
> just opportunistically transfer the pending state.

Thanks, I will look at what you mentioned.

> 
>>
>>>
>>>> also transfer the pending state from irq->pending_latch to VPT (especially
>>>> in migration, the pending states of VLPIs are restored into kvm’s vgic
>>>> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>>>>
>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>> ---
>>>>  arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>> index b5fa73c9fd35..cc3ab9cea182 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>>      irq->host_irq    = virq;
>>>>      atomic_inc(&map.vpe->vlpi_count);
>>>>
>>>> +    /* Transfer pending state */
>>>> +    ret = irq_set_irqchip_state(irq->host_irq,
>>>> +                    IRQCHIP_STATE_PENDING,
>>>> +                    irq->pending_latch);
>>>> +    WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>> +
>>>> +    /*
>>>> +     * Let it be pruned from ap_list later and don't bother
>>>> +     * the List Register.
>>>> +     */
>>>> +    irq->pending_latch = false;
>>>
>>> It occurs to me that calling into irq_set_irqchip_state() for a large
>>> number of interrupts can take a significant amount of time. It is also
>>> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
>>> being mapped for the opposite operation.
>>>
>>> Shouldn't these be symmetric, all performed while the VPE is unmapped?
>>> It would also save a lot of ITS traffic.
>>>
>>
>> My thought was to use the existing interface directly without unmapping...
>>
>> If you want to unmap the vPE and poke the VPT here, as I said in the cover
>> letter, set/unset_forwarding might also be called when all devices are running
>> at normal run time, in which case the unmapping of the vPE is not allowed...
> 
> No, I'm suggesting that you don't do anything here, but instead as a by-product
> of restoring the ITS tables. What goes wrong if you use the
> KVM_DEV_ARM_ITS_RESTORE_TABLE backend instead?

There is an issue if we do it in the restoring of the ITS tables: the transferring
of the pending state needs the irq to be marked as hw before, which is done by the
pass-through device, but the configuring of the forwarding path of the VLPI depends
on the restoring of the vgic first... It is a circular dependency.

> 
>> Another possible solution is to add a new dedicated interface to QEMU
>> to transfer
>> these pending states to HW in GIC VM state change handler corresponding to
>> save_pending_tables?
> 
> Userspace has no way to know we use GICv4, and I intend to keep it
> completely out of the loop. The API is already pretty tortuous, and
> I really don't want to add any extra complexity to it.
> 
> Thanks,
> 
>         M.

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/11/24 21:12, Shenming Lu wrote:
> On 2020/11/24 16:44, Marc Zyngier wrote:
>> On 2020-11-24 08:10, Shenming Lu wrote:
>>> On 2020/11/23 17:27, Marc Zyngier wrote:
>>>> On 2020-11-23 06:54, Shenming Lu wrote:
>>>>> From: Zenghui Yu <yuzenghui@huawei.com>
>>>>>
>>>>> When setting the forwarding path of a VLPI, it is more consistent to
>>>>
>>>> I'm not sure it is more consistent. It is a *new* behaviour, because it only
>>>> matters for migration, which has been so far unsupported.
>>>
>>> Alright, consistent may not be accurate...
>>> But I have doubt that whether there is really no need to transfer the
>>> pending states
>>> from kvm'vgic to VPT in set_forwarding regardless of migration, and the similar
>>> for unset_forwarding.
>>
>> If you have to transfer that state outside of the a save/restore, it means that
>> you have missed the programming of the PCI endpoint. This is an established
>> restriction that the MSI programming must occur *after* the translation has
>> been established using MAPI/MAPTI (see the large comment at the beginning of
>> vgic-v4.c).
>>
>> If you want to revisit this, fair enough. But you will need a lot more than
>> just opportunistically transfer the pending state.
> 
> Thanks, I will look at what you mentioned.
> 
>>
>>>
>>>>
>>>>> also transfer the pending state from irq->pending_latch to VPT (especially
>>>>> in migration, the pending states of VLPIs are restored into kvm’s vgic
>>>>> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>>>>>
>>>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>>> ---
>>>>>  arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>> index b5fa73c9fd35..cc3ab9cea182 100644
>>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>>>      irq->host_irq    = virq;
>>>>>      atomic_inc(&map.vpe->vlpi_count);
>>>>>
>>>>> +    /* Transfer pending state */
>>>>> +    ret = irq_set_irqchip_state(irq->host_irq,
>>>>> +                    IRQCHIP_STATE_PENDING,
>>>>> +                    irq->pending_latch);
>>>>> +    WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>>> +
>>>>> +    /*
>>>>> +     * Let it be pruned from ap_list later and don't bother
>>>>> +     * the List Register.
>>>>> +     */
>>>>> +    irq->pending_latch = false;
>>>>
>>>> It occurs to me that calling into irq_set_irqchip_state() for a large
>>>> number of interrupts can take a significant amount of time. It is also
>>>> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
>>>> being mapped for the opposite operation.
>>>>
>>>> Shouldn't these be symmetric, all performed while the VPE is unmapped?
>>>> It would also save a lot of ITS traffic.
>>>>
>>>
>>> My thought was to use the existing interface directly without unmapping...
>>>
>>> If you want to unmap the vPE and poke the VPT here, as I said in the cover
>>> letter, set/unset_forwarding might also be called when all devices are running
>>> at normal run time, in which case the unmapping of the vPE is not allowed...
>>
>> No, I'm suggesting that you don't do anything here, but instead as a by-product
>> of restoring the ITS tables. What goes wrong if you use the
>> KVM_DEV_ARM_ITS_RESTORE_TABLE backend instead?
> 
> There is an issue if we do it in the restoring of the ITS tables: the transferring
> of the pending state needs the irq to be marked as hw before, which is done by the
> pass-through device, but the configuring of the forwarding path of the VLPI depends
> on the restoring of the vgic first... It is a circular dependency.
> 

Hi Marc,

We are pondering over this problem these days, but still don't get a good solution...
Could you give us some advice on this?

Or could we move the restoring of the pending states (include the sync from guest
RAM and the transfer to HW) to the GIC VM state change handler, which is completely
corresponding to save_pending_tables (more symmetric?) and don't expose GICv4...

Thanks,
Shenming

>>
>>> Another possible solution is to add a new dedicated interface to QEMU
>>> to transfer
>>> these pending states to HW in GIC VM state change handler corresponding to
>>> save_pending_tables?
>>
>> Userspace has no way to know we use GICv4, and I intend to keep it
>> completely out of the loop. The API is already pretty tortuous, and
>> I really don't want to add any extra complexity to it.
>>
>> Thanks,
>>
>>         M.

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Marc Zyngier]

On 2020-11-30 07:23, Shenming Lu wrote:

Hi Shenming,

> We are pondering over this problem these days, but still don't get a
> good solution...
> Could you give us some advice on this?
> 
> Or could we move the restoring of the pending states (include the sync
> from guest RAM and the transfer to HW) to the GIC VM state change 
> handler,
> which is completely corresponding to save_pending_tables (more 
> symmetric?)
> and don't expose GICv4...

What is "the GIC VM state change handler"? Is that a QEMU thing?
We don't really have that concept in KVM, so I'd appreciate if you could
be a bit more explicit on this.

Thanks,

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

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/12/1 18:55, Marc Zyngier wrote:
> On 2020-11-30 07:23, Shenming Lu wrote:
> 
> Hi Shenming,
> 
>> We are pondering over this problem these days, but still don't get a
>> good solution...
>> Could you give us some advice on this?
>>
>> Or could we move the restoring of the pending states (include the sync
>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>> which is completely corresponding to save_pending_tables (more symmetric?)
>> and don't expose GICv4...
> 
> What is "the GIC VM state change handler"? Is that a QEMU thing?

Yeah, it is a a QEMU thing...

> We don't really have that concept in KVM, so I'd appreciate if you could
> be a bit more explicit on this.

My thought is to add a new interface (to QEMU) for the restoring of the pending
states, which is completely corresponding to KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
And it is called from the GIC VM state change handler in QEMU, which is happening
after the restoring (call kvm_vgic_v4_set_forwarding()) but before the starting
(running) of the VFIO device.

Thanks,
Shenming

> 
> Thanks,
> 
>         M.

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Marc Zyngier]

On 2020-12-01 11:40, Shenming Lu wrote:
> On 2020/12/1 18:55, Marc Zyngier wrote:
>> On 2020-11-30 07:23, Shenming Lu wrote:
>> 
>> Hi Shenming,
>> 
>>> We are pondering over this problem these days, but still don't get a
>>> good solution...
>>> Could you give us some advice on this?
>>> 
>>> Or could we move the restoring of the pending states (include the 
>>> sync
>>> from guest RAM and the transfer to HW) to the GIC VM state change 
>>> handler,
>>> which is completely corresponding to save_pending_tables (more 
>>> symmetric?)
>>> and don't expose GICv4...
>> 
>> What is "the GIC VM state change handler"? Is that a QEMU thing?
> 
> Yeah, it is a a QEMU thing...
> 
>> We don't really have that concept in KVM, so I'd appreciate if you 
>> could
>> be a bit more explicit on this.
> 
> My thought is to add a new interface (to QEMU) for the restoring of
> the pending states, which is completely corresponding to
> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
> And it is called from the GIC VM state change handler in QEMU, which
> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
> but before the starting (running) of the VFIO device.

Right, that makes sense. I still wonder how much the GIC save/restore
stuff differs from other architectures that implement similar features,
such as x86 with VT-D.

It is obviously too late to change the userspace interface, but I wonder
whether we missed something at the time.

Thanks,

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

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/12/1 19:50, Marc Zyngier wrote:
> On 2020-12-01 11:40, Shenming Lu wrote:
>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>
>>> Hi Shenming,
>>>
>>>> We are pondering over this problem these days, but still don't get a
>>>> good solution...
>>>> Could you give us some advice on this?
>>>>
>>>> Or could we move the restoring of the pending states (include the sync
>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>> and don't expose GICv4...
>>>
>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>
>> Yeah, it is a a QEMU thing...
>>
>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>> be a bit more explicit on this.
>>
>> My thought is to add a new interface (to QEMU) for the restoring of
>> the pending states, which is completely corresponding to
>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>> And it is called from the GIC VM state change handler in QEMU, which
>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>> but before the starting (running) of the VFIO device.
> 
> Right, that makes sense. I still wonder how much the GIC save/restore
> stuff differs from other architectures that implement similar features,
> such as x86 with VT-D.

I am not familiar with it...

> 
> It is obviously too late to change the userspace interface, but I wonder
> whether we missed something at the time.

The interface seems to be really asymmetrical?...

Or is there a possibility that we could know which irq is hw before the VFIO
device calls kvm_vgic_v4_set_forwarding()?

Thanks,
Shenming

> 
> Thanks,
> 
>         M.

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/12/1 20:15, Shenming Lu wrote:
> On 2020/12/1 19:50, Marc Zyngier wrote:
>> On 2020-12-01 11:40, Shenming Lu wrote:
>>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>>
>>>> Hi Shenming,
>>>>
>>>>> We are pondering over this problem these days, but still don't get a
>>>>> good solution...
>>>>> Could you give us some advice on this?
>>>>>
>>>>> Or could we move the restoring of the pending states (include the sync
>>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>>> and don't expose GICv4...
>>>>
>>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>>
>>> Yeah, it is a a QEMU thing...
>>>
>>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>>> be a bit more explicit on this.
>>>
>>> My thought is to add a new interface (to QEMU) for the restoring of
>>> the pending states, which is completely corresponding to
>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>>> And it is called from the GIC VM state change handler in QEMU, which
>>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>>> but before the starting (running) of the VFIO device.
>>
>> Right, that makes sense. I still wonder how much the GIC save/restore
>> stuff differs from other architectures that implement similar features,
>> such as x86 with VT-D.
> 
> I am not familiar with it...
> 
>>
>> It is obviously too late to change the userspace interface, but I wonder
>> whether we missed something at the time.
> 
> The interface seems to be really asymmetrical?...
> 
> Or is there a possibility that we could know which irq is hw before the VFIO
> device calls kvm_vgic_v4_set_forwarding()?
> 
> Thanks,
> Shenming
> 
>>
>> Thanks,
>>
>>         M.
> .
> 

Hi Marc,

I am learning VT-d Posted Interrupt (PI) these days.

As far as I can tell, the posted interrupts are firstly recorded in the Posted
Interrupt Request (*PIR*) field of the Posted Interrupt Descriptor (a temporary
storage area (data structure in memory) which is specific to PI), and when the
vCPU is running, a notification event (host vector) will be generated and sent
to the CPU (the target vCPU is currently scheduled on it), which will cause the
CPU to transfer the posted interrupt in the PIR field to the *Virtual-APIC page*
(a data structure in kvm, the virtual interrupts delivered through kvm are put
here, and it is also accessed by the VMX microcode (the layout matches the register
layout seen by the guest)) of the vCPU and directly deliver it to the vCPU.

So they only have to sync the PIR field to the Virtual-APIC page for the migration
saving, and do nothing for the resuming...

Besides, on x86 the setting of the IRQ bypass is independent of the VM interrupt
setup...

Not sure if I have missed something.

In addition, I found that the enabling of the vAPIC is at the end of the migration
(just before the VM start) on x86. So I am wondering if we could move the calling
of *vgic_enable_lpis()* back, and transfer the pending state to the VPT there if the
irq is hw (and I think the semantics of this function should include the transfer).
In fact, this function is dependent on the restoring of the vgic(lpi_list)...

After exploration, there seems to be no perfect place to transfer the pending states
to HW in order to be compatible with the existing interface and under the current
architecture, but we have to choose one solution?

Thanks,
Shenming

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Auger Eric]

Hi Shenming,

On 12/1/20 1:15 PM, Shenming Lu wrote:
> On 2020/12/1 19:50, Marc Zyngier wrote:
>> On 2020-12-01 11:40, Shenming Lu wrote:
>>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>>
>>>> Hi Shenming,
>>>>
>>>>> We are pondering over this problem these days, but still don't get a
>>>>> good solution...
>>>>> Could you give us some advice on this?
>>>>>
>>>>> Or could we move the restoring of the pending states (include the sync
>>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>>> and don't expose GICv4...
>>>>
>>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>>
>>> Yeah, it is a a QEMU thing...
>>>
>>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>>> be a bit more explicit on this.
>>>
>>> My thought is to add a new interface (to QEMU) for the restoring of
>>> the pending states, which is completely corresponding to
>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>>> And it is called from the GIC VM state change handler in QEMU, which
>>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>>> but before the starting (running) of the VFIO device.
>>
>> Right, that makes sense. I still wonder how much the GIC save/restore
>> stuff differs from other architectures that implement similar features,
>> such as x86 with VT-D.
> 
> I am not familiar with it...
> 
>>
>> It is obviously too late to change the userspace interface, but I wonder
>> whether we missed something at the time.
> 
> The interface seems to be really asymmetrical?...

in qemu d5aa0c229a ("hw/intc/arm_gicv3_kvm: Implement pending table
save") commit message, it is traced:

"There is no explicit restore as the tables are implicitly sync'ed
on ITS table restore and on LPI enable at redistributor level."

At that time there was no real justification behind adding the RESTORE
fellow attr.

Maybe a stupid question but isn't it possible to unset the forwarding
when saving and rely on VFIO to automatically restore it when resuming
on destination?

Thanks

Eric


> 
> Or is there a possibility that we could know which irq is hw before the VFIO
> device calls kvm_vgic_v4_set_forwarding()?
> 
> Thanks,
> Shenming
> 
>>
>> Thanks,
>>
>>         M.
> 

Re: [RFC PATCH v1 3/4] KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side

[Shenming Lu]

On 2020/12/16 18:35, Auger Eric wrote:
> Hi Shenming,
> 
> On 12/1/20 1:15 PM, Shenming Lu wrote:
>> On 2020/12/1 19:50, Marc Zyngier wrote:
>>> On 2020-12-01 11:40, Shenming Lu wrote:
>>>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>>>
>>>>> Hi Shenming,
>>>>>
>>>>>> We are pondering over this problem these days, but still don't get a
>>>>>> good solution...
>>>>>> Could you give us some advice on this?
>>>>>>
>>>>>> Or could we move the restoring of the pending states (include the sync
>>>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>>>> and don't expose GICv4...
>>>>>
>>>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>>>
>>>> Yeah, it is a a QEMU thing...
>>>>
>>>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>>>> be a bit more explicit on this.
>>>>
>>>> My thought is to add a new interface (to QEMU) for the restoring of
>>>> the pending states, which is completely corresponding to
>>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>>>> And it is called from the GIC VM state change handler in QEMU, which
>>>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>>>> but before the starting (running) of the VFIO device.
>>>
>>> Right, that makes sense. I still wonder how much the GIC save/restore
>>> stuff differs from other architectures that implement similar features,
>>> such as x86 with VT-D.
>>
>> I am not familiar with it...
>>
>>>
>>> It is obviously too late to change the userspace interface, but I wonder
>>> whether we missed something at the time.
>>
>> The interface seems to be really asymmetrical?...
> 
> in qemu d5aa0c229a ("hw/intc/arm_gicv3_kvm: Implement pending table
> save") commit message, it is traced:
> 
> "There is no explicit restore as the tables are implicitly sync'ed
> on ITS table restore and on LPI enable at redistributor level."
> 
> At that time there was no real justification behind adding the RESTORE
> fellow attr.
> 
> Maybe a stupid question but isn't it possible to unset the forwarding
> when saving and rely on VFIO to automatically restore it when resuming
> on destination?

It seems that the unset_forwarding would not be called when saving, it would
be called after migration completion...
As for the resuming/set_forwarding, I still wonder: is it really improper to
transfer the pending states from vgic to VPT in set_forwarding (not only in
migration)?...  -_-

Thanks,
Shenming

> 
> Thanks
> 
> Eric
> 
> 
>>
>> Or is there a possibility that we could know which irq is hw before the VFIO
>> device calls kvm_vgic_v4_set_forwarding()?
>>
>> Thanks,
>> Shenming
>>
>>>
>>> Thanks,
>>>
>>>         M.
>>
> 
> .
> 

[RFC PATCH v1 4/4] KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state

[Shenming Lu]

Before GICv4.1, we do not have direct access to the VLPI's pending
state. So we simply let it fail early when encountering any VLPI.

But now we don't have to return -EACCES directly if on GICv4.1. So
let’s change the hard code and give a chance to save the VLPI's pending
state (and preserve the interfaces).

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 Documentation/virt/kvm/devices/arm-vgic-its.rst | 2 +-
 arch/arm64/kvm/vgic/vgic-its.c                  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/virt/kvm/devices/arm-vgic-its.rst b/Documentation/virt/kvm/devices/arm-vgic-its.rst
index 6c304fd2b1b4..d257eddbae29 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-its.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-its.rst
@@ -80,7 +80,7 @@ KVM_DEV_ARM_VGIC_GRP_CTRL
     -EFAULT  Invalid guest ram access
     -EBUSY   One or more VCPUS are running
     -EACCES  The virtual ITS is backed by a physical GICv4 ITS, and the
-	     state is not available
+	     state is not available without GICv4.1
     =======  ==========================================================
 
 KVM_DEV_ARM_VGIC_GRP_ITS_REGS
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 40cbaca81333..ec7543a9617c 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2218,10 +2218,10 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
 		/*
 		 * If an LPI carries the HW bit, this means that this
 		 * interrupt is controlled by GICv4, and we do not
-		 * have direct access to that state. Let's simply fail
-		 * the save operation...
+		 * have direct access to that state without GICv4.1.
+		 * Let's simply fail the save operation...
 		 */
-		if (ite->irq->hw)
+		if (ite->irq->hw && !kvm_vgic_global_state.has_gicv4_1)
 			return -EACCES;
 
 		ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
-- 
2.23.0