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

Commit Message

Shenming Lu Nov. 23, 2020, 6:54 a.m. UTC

From: Zenghui Yu <[e-mail]>

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 <[e-mail]>
Signed-off-by: Shenming Lu <[e-mail]>
---
 drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Marc Zyngier Nov. 23, 2020, 9:01 a.m. UTC | #1

On 2020-11-23 06:54, Shenming Lu wrote:
> From: Zenghui Yu <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Shenming Lu <[e-mail]>
> ---
>  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.

Shenming Lu Nov. 24, 2020, 7:38 a.m. UTC | #2

On 2020/11/23 17:01, Marc Zyngier wrote:
> On 2020-11-23 06:54, Shenming Lu wrote:
>> From: Zenghui Yu <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Shenming Lu <[e-mail]>
>> ---
>>  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.

Marc Zyngier Nov. 24, 2020, 8:08 a.m. UTC | #3

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 <[e-mail]>
>>> 
>>> 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 <[e-mail]>
>>> Signed-off-by: Shenming Lu <[e-mail]>
>>> ---
>>>  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.

luojiaxing Nov. 28, 2020, 7:19 a.m. UTC | #4

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 <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Shenming Lu <[e-mail]>
> ---
>   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,

Marc Zyngier Nov. 28, 2020, 10:18 a.m. UTC | #5

On Sat, 28 Nov 2020 07:19:48 +0000,
luojiaxing <[e-mail]> 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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Shenming Lu <[e-mail]>
> > ---
> >   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.

luojiaxing Dec. 1, 2020, 9:38 a.m. UTC | #6

On 2020/11/28 18:18, Marc Zyngier wrote:
> On Sat, 28 Nov 2020 07:19:48 +0000,
> luojiaxing <[e-mail]> 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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Shenming Lu <[e-mail]>
>>> ---
>>>    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.
>

Marc Zyngier Dec. 1, 2020, 10:58 a.m. UTC | #7

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 <[e-mail]> 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.

Patch

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,