This (now shorter) series expands the existing GICv4 support to deal
with the new GICv4.1 architecture, which comes with a set of major
improvements compared to v4.0:
- One architectural doorbell per vcpu, instead of one doorbell per VLPI
- Doorbell entirely managed by the HW, with an "at most once" delivery
guarantee per non-residency phase and only when requested by the
hypervisor
- A shared memory scheme between ITSs and redistributors, allowing for an
optimised residency sequence (the use of VMOVP becomes less frequent)
- Support for direct virtual SGI delivery (the injection path still involves
the hypervisor), at the cost of losing the active state on SGIs. It
shouldn't be a big deal, but some guest operating systems might notice
(Linux definitely won't care).
On the other hand, public documentation is not available yet, so that's a
bit annoying...
The series is roughly organised in 3 parts:
(1) v4.1 doorbell management
(2) Virtual SGI support
(3) Plumbing of virtual SGIs in KVM
Notes:
- The whole thing is tested on a FVP model, which can be obtained
free of charge on ARM's developer website. It requires you to
create an account, unfortunately... You'll need a fix for the
devicetree that is in the kernel tree (should be merged before
the 5.6 release).
- This series has uncovered a behaviour that looks like a HW bug on
the Cavium ThunderX (aka TX1) platform. I'd very much welcome some
clarification from the Marvell/Cavium folks on Cc, as well as an
official erratum number if this happens to be an actual bug.
[v3 update]
People have ignored for two months now, and it is fairly obvious
that support for this machine is slowly bit-rotting. Maybe I'll
drop the patch and instead start the process of removing all TX1
support from the kernel (we'd certainly be better off without it).
[v4 update]
TX1 is now broken in mainline, and nobody cares. Make of this what
you want.
* From v3 [3]:
- Rebased on v5.6-rc1
- Considerably smaller thanks to the initial patches being merged
- Small bug fix after the v5.6 merge window
* From v2 [2]:
- Another bunch of fixes thanks to Zenghui Yu's very careful review
- HW-accelerated SGIs are now optional thanks to new architected
discovery/selection bits exposed by KVM and used by the guest kernel
- Rebased on v5.5-rc2
* From v1 [1]:
- A bunch of minor reworks after Zenghui Yu's review
- A workaround for what looks like a new and unexpected TX1 bug
- A subtle reorder of the series so that some patches can go in early
[1] https://lore.kernel.org/lkml/20190923182606.32100-1-maz@kernel.org/
[2] https://lore.kernel.org/lkml/20191027144234.8395-1-maz@kernel.org/
[3] https://lore.kernel.org/r/20191224111055.11836-1-maz@kernel.org/
Marc Zyngier (20):
irqchip/gic-v4.1: Skip absent CPUs while iterating over redistributors
irqchip/gic-v3: Use SGIs without active state if offered
irqchip/gic-v4.1: Advertise support v4.1 to KVM
irqchip/gic-v4.1: Map the ITS SGIR register page
irqchip/gic-v4.1: Plumb skeletal VSGI irqchip
irqchip/gic-v4.1: Add initial SGI configuration
irqchip/gic-v4.1: Plumb mask/unmask SGI callbacks
irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks
irqchip/gic-v4.1: Plumb set_vcpu_affinity SGI callbacks
irqchip/gic-v4.1: Move doorbell management to the GICv4 abstraction
layer
irqchip/gic-v4.1: Add VSGI allocation/teardown
irqchip/gic-v4.1: Add VSGI property setup
irqchip/gic-v4.1: Eagerly vmap vPEs
KVM: arm64: GICv4.1: Let doorbells be auto-enabled
KVM: arm64: GICv4.1: Add direct injection capability to SGI registers
KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts
KVM: arm64: GICv4.1: Plumb SGI implementation selection in the
distributor
KVM: arm64: GICv4.1: Reload VLPI configuration on distributor
enable/disable
KVM: arm64: GICv4.1: Allow non-trapping WFI when using HW SGIs
KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs
arch/arm/include/asm/kvm_host.h | 1 +
arch/arm64/include/asm/kvm_emulate.h | 3 +-
arch/arm64/include/asm/kvm_host.h | 1 +
drivers/irqchip/irq-gic-v3-its.c | 301 ++++++++++++++++++++++++-
drivers/irqchip/irq-gic-v3.c | 12 +-
drivers/irqchip/irq-gic-v4.c | 134 ++++++++++-
include/kvm/arm_vgic.h | 4 +
include/linux/irqchip/arm-gic-common.h | 2 +
include/linux/irqchip/arm-gic-v3.h | 19 +-
include/linux/irqchip/arm-gic-v4.h | 20 +-
virt/kvm/arm/arm.c | 8 +
virt/kvm/arm/vgic/vgic-debug.c | 14 +-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 68 +++++-
virt/kvm/arm/vgic/vgic-mmio.c | 88 +++++++-
virt/kvm/arm/vgic/vgic-v3.c | 6 +-
virt/kvm/arm/vgic/vgic-v4.c | 139 ++++++++++--
virt/kvm/arm/vgic/vgic.h | 1 +
17 files changed, 763 insertions(+), 58 deletions(-)
--
2.20.1
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Zenghui Yuyuzenghui@huawei.comRe: [PATCH v4 02/20] irqchip/gic-v3: Use SGIs without active state if offered2020-02-17T09:18:28Zurn:uuid:24bb61cf-f65f-0b9b-f76d-c8eadcd239fe

Hi Marc,
On 2020/2/14 22:57, Marc Zyngier wrote:
> To allow the direct injection of SGIs into a guest, the GICv4.1
> architecture has to sacrifice the Active state so that SGIs look
> a lot like LPIs (they are injected by the same mechanism).
>
> In order not to break existing software, the architecture gives
> offers guests OSs the choice: SGIs with or without an active
> state. It is the hypervisors duty to honor the guest's choice.
>
> For this, the architecture offers a discovery bit indicating whether
> the GIC supports GICv4.1 SGIs (GICD_TYPER2.nASSGIcap), and another
> bit indicating whether the guest wants Active-less SGIs or not
> (controlled by GICD_CTLR.nASSGIreq).
>
> A hypervisor not supporting GICv4.1 SGIs would leave nASSGIcap
> clear, and a guest not knowing about GICv4.1 SGIs (or definitely
> wanting an Active state) would leave nASSGIreq clear (both being
> thankfully backward compatible with oler revisions of the GIC).
older?
>
> Since Linux is perfectly happy without an active state on SGIs,
> inform the hypervisor that we'll use that if offered.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Per the description of these two bits in the commit message,
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Thanks
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Hi Marc,
On 2020/2/18 23:31, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2020-02-18 09:27, Marc Zyngier wrote:
>> Hi Zenghui,
>>
>> On 2020-02-18 07:00, Zenghui Yu wrote:
>>> Hi Marc,
>
> [...]
>
>>> There might be a race on reading the 'vpe->col_idx' against a concurrent
>>> vPE schedule (col_idx will be modified in its_vpe_set_affinity)? Will we
>>> end up accessing the GICR_VSGI* registers of the old redistributor,
>>> while the vPE is now resident on the new one? Or is it harmful?
>>
>> Very well spotted. There is a potential problem if old and new RDs are
>> not part
>> of the same CommonLPIAff group.
>>
>>> The same question for direct_lpi_inv(), where 'vpe->col_idx' will be
>>> used in irq_to_cpuid().
>>
>> Same problem indeed. We need to ensure that no VMOVP operation can
>> occur whilst
>> we use col_idx to access a redistributor. This means a vPE lock of
>> some sort
>> that will protect the affinity.
Yeah, I had the same view here, a vPE level lock might help.
>> But I think there is a slightly more general problem here, which we
>> failed to
>> see initially: the same issue exists for physical LPIs, as col_map[]
>> can be
>> updated (its_set_affinity()) in parallel with a direct invalidate.
>>
>> The good old invalidation through the ITS does guarantee that the two
>> operation
>> don't overlap, but direct invalidation breaks it.
Agreed!
>> Let me have a think about it.
>
> So I've thought about it, wrote a patch, and I don't really like the
> look of it.
> This is pretty invasive, and we end-up serializing a lot more than we
> used to
> (the repurposing of vlpi_lock to a general "lpi mapping lock" is
> probably too
> coarse).
>
> It of course needs splitting over at least three patches, but it'd be
> good if
> you could have a look (applies on top of the whole series).
So the first thing is that
1. there're races on choosing the RD against a concurrent LPI/vPE
affinity changing.
And sure, I will have a look on the following patch! But I'd first
talk about some other issues I've seen today...
2. Another potential race is on accessing the same RD by different
CPUs, which gets more obvious after introducing the GICv4.1.
We can as least take two registers for example:
- GICR_VSGIR:
Let's assume that vPE0 is just descheduled from CPU0 and then vPE1
is scheduled on. CPU0 is writing its GICR_VSGIR with vpeid1 to serve
vPE1's GICR_ISPENDR0 read trap, whilst userspace is getting vSGI's
pending state of vPE0 (i.e., by a debugfs read) thus another CPU
will try to write the same GICR_VSGIR with vpeid0... without waiting
GICR_VSGIPENDR.Busy reads as 0.
This is a CONSTRAINED UNPREDICTABLE behavior from the spec and at
least one of the queries will fail.
- GICR_INV{LPI,ALL}R:
Multiple LPIs can be targeted to the same RD, thus multiple writes to
the same GICR_INVLPIR (with different INITID, even with different V)
can happen concurrently...
Above comes from the fact that the same redistributor can be accessed
(concurrently) by multiple CPUs but we don't have a mechanism to ensure
some extent of serialization. I also had a look at how KVM will handle
this kind of access, but
3. it looks like KVM makes the assumption that the per-RD MMIO region
will only be accessed by the associated VCPU? But I think this's not
restricted by the architecture, we can do it better. Or I've just
missed some important points here.
I will look at the following patch asap but may need some time to
think about all above, and do some fix if possible :-)
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index 7656b353a95f..0ed286dba827 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
[...]
Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

On 2020/2/19 19:50, Zenghui Yu wrote:
> 3. it looks like KVM makes the assumption that the per-RD MMIO region
> will only be accessed by the associated VCPU? But I think this's not
> restricted by the architecture, we can do it better. Or I've just
> missed some important points here.
(After some basic tests, KVM actually does the right thing!)
So I must have some mis-understanding on this point, please
ignore it. I'll dig it further myself, sorry for the noisy.
Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

On 2020/2/14 22:57, Marc Zyngier wrote:
> One of the new features of GICv4.1 is to allow virtual SGIs to be
> directly signaled to a VPE. For that, the ITS has grown a new
> 64kB page containing only a single register that is used to
> signal a SGI to a given VPE.
>
> Add a second mapping covering this new 64kB range, and take this
> opportunity to limit the original mapping to 64kB, which is enough
> to cover the span of the ITS registers.
Yes, no need to do ioremap for the translation register.
>
> Signed-off-by: Marc Zyngier<maz@kernel.org>
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

On 2020/2/14 22:57, Marc Zyngier wrote:
> Since GICv4.1 has the capability to inject 16 SGIs into each VPE,
> and that I'm keen not to invent too many specific interfaces to
> manupulate these interrupts, let's pretend that each of these SGIs
manipulate?
> is an actual Linux interrupt.
>
> For that matter, let's introduce a minimal irqchip and irqdomain
> setup that will get fleshed up in the following patches.
>
> Signed-off-by: Marc Zyngier<maz@kernel.org>
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Hi Zenghui,
On 2020-02-20 03:55, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/2/14 22:57, Marc Zyngier wrote:
>> In order to let a guest buy in the new, active-less SGIs, we
>> need to be able to switch between the two modes.
>>
>> Handle this by stopping all guest activity, transfer the state
>> from one mode to the other, and resume the guest.
>>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>
> [...]
>
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 1bc09b523486..2c9fc13e2c59 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
>> goto out;
>> }
>> + if (kvm_vgic_global_state.has_gicv4_1)
>> + vgic_v4_configure_vsgis(kvm);
>> dist->ready = true;
>> out:
>
> Is there any reason to invoke vgic_v4_configure_vsgis() here?
> This is called on the first VCPU run, through kvm_vgic_map_resources().
> Shouldn't the vSGI configuration only driven by a GICD_CTLR.nASSGIreq
> writing (from guest, or from userspace maybe)?
What I'm trying to catch here is the guest that has been restored with
nASSGIreq set. At the moment, we don't do anything on the userspace
side, because the vmm could decide to write that particular bit
multiple times, and switching between the two modes is expensive (not
to mention that all the vcpus may not have been created yet).
Moving it to the first run makes all these pitfalls go away (we have the
final nASSSGIreq value, and all the vcpus are accounted for).
Does this make sense to you?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

On 2020-02-28 19:37, Marc Zyngier wrote:
> On 2020-02-20 03:11, Zenghui Yu wrote:
>> Do we really need to grab the vpe_lock for those which are belong to
>> the same irqchip with its_vpe_set_affinity()? The IRQ core code should
>> already ensure the mutual exclusion among them, wrong?
>
> I've been trying to think about that, but jet-lag keeps getting in the
> way.
> I empirically think that you are right, but I need to go and check the
> various
> code paths to be sure. Hopefully I'll have a bit more brain space next
> week.
So I slept on it and came back to my senses. The only case we actually
need
to deal with is when an affinity change impacts *another* interrupt.
There is only two instances of this issue:
- vLPIs have their *physical* affinity impacted by the affinity of the
vPE. Their virtual affinity is of course unchanged, but the physical
one becomes important with direct invalidation. Taking a per-VPE lock
in such context should address the issue.
- vSGIs have the exact same issue, plus the matter of requiring some
*extra* one when reading the pending state, which requires a RMW
on two different registers. This requires an extra per-RD lock.
My original patch was stupidly complex, and the irq_desc lock is
perfectly enough to deal with anything that only affects the interrupt
state itself.
GICv4 + direct invalidation for vLPIs breaks this by bypassing the
serialization initially provided by the ITS, as the RD is completely
out of band. The per-vPE lock brings back this serialization.
I've updated the branch, which seems to run OK on D05. I still need
to run the usual tests on the FVP model though.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Hi Marc,
On 2020/2/29 3:16, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2020-02-20 03:55, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/2/14 22:57, Marc Zyngier wrote:
>>> In order to let a guest buy in the new, active-less SGIs, we
>>> need to be able to switch between the two modes.
>>>
>>> Handle this by stopping all guest activity, transfer the state
>>> from one mode to the other, and resume the guest.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>
>> [...]
>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>>> index 1bc09b523486..2c9fc13e2c59 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
>>> goto out;
>>> }
>>> + if (kvm_vgic_global_state.has_gicv4_1)
>>> + vgic_v4_configure_vsgis(kvm);
>>> dist->ready = true;
>>> out:
>>
>> Is there any reason to invoke vgic_v4_configure_vsgis() here?
>> This is called on the first VCPU run, through kvm_vgic_map_resources().
>> Shouldn't the vSGI configuration only driven by a GICD_CTLR.nASSGIreq
>> writing (from guest, or from userspace maybe)?
>
> What I'm trying to catch here is the guest that has been restored with
> nASSGIreq set. At the moment, we don't do anything on the userspace
> side, because the vmm could decide to write that particular bit
> multiple times, and switching between the two modes is expensive (not
> to mention that all the vcpus may not have been created yet).
>
> Moving it to the first run makes all these pitfalls go away (we have the
> final nASSSGIreq value, and all the vcpus are accounted for).
So what will happen on restoration is (roughly):
- for GICR_ISPENR0: We will restore the pending status of vSGIs into
software pending_latch, just like what we've done for normal SGIs.
- for GICD_CTLR.nASSGIreq: We will only record the written value.
(Note to myself: No invocation of configure_vsgis() in uaccess_write
callback, I previously mixed it up with the guest write callback.)
- Finally, you choose the first vcpu run as the appropriate point to
potentially flush the pending status to HW according to the final
nASSGIreq value.
>
> Does this make sense to you?
Yeah, it sounds like a good idea! And please ignore what I've replied to
patch #15, I obviously missed your intention at that time, sorry...
But can we move this hunk to some places more appropriate, for example,
put it together with the GICD_CTLR's uaccess_write change? It might make
things a bit clearer for other reviewers. :-)
Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

On 2020/3/2 3:00, Marc Zyngier wrote:
> On 2020-02-28 19:37, Marc Zyngier wrote:
>> On 2020-02-20 03:11, Zenghui Yu wrote:
>
>>> Do we really need to grab the vpe_lock for those which are belong to
>>> the same irqchip with its_vpe_set_affinity()? The IRQ core code should
>>> already ensure the mutual exclusion among them, wrong?
>>
>> I've been trying to think about that, but jet-lag keeps getting in the
>> way.
>> I empirically think that you are right, but I need to go and check the
>> various
>> code paths to be sure. Hopefully I'll have a bit more brain space next
>> week.
>
> So I slept on it and came back to my senses. The only case we actually need
> to deal with is when an affinity change impacts *another* interrupt.
>
> There is only two instances of this issue:
>
> - vLPIs have their *physical* affinity impacted by the affinity of the
> vPE. Their virtual affinity is of course unchanged, but the physical
> one becomes important with direct invalidation. Taking a per-VPE lock
> in such context should address the issue.
>
> - vSGIs have the exact same issue, plus the matter of requiring some
> *extra* one when reading the pending state, which requires a RMW
> on two different registers. This requires an extra per-RD lock.
Agreed with both!
>
> My original patch was stupidly complex, and the irq_desc lock is
> perfectly enough to deal with anything that only affects the interrupt
> state itself.
>
> GICv4 + direct invalidation for vLPIs breaks this by bypassing the
> serialization initially provided by the ITS, as the RD is completely
> out of band. The per-vPE lock brings back this serialization.
>
> I've updated the branch, which seems to run OK on D05. I still need
> to run the usual tests on the FVP model though.
I have pulled the latest branch and it looks good to me, except for
one remaining concern:
GICR_INV{LPI, ALL}R + GICR_SYNCR can also be accessed concurrently
by multiple direct invalidation, should we also use the per-RD lock
to ensure mutual exclusion? It looks not so harmful though, as this
will only increase one's polling time against the Busy bit (in my view).
But I point it out again for confirmation.
Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Hi Zenghui,
On 2020-03-02 08:18, Zenghui Yu wrote:
> On 2020/3/2 3:00, Marc Zyngier wrote:
>> On 2020-02-28 19:37, Marc Zyngier wrote:
>>> On 2020-02-20 03:11, Zenghui Yu wrote:
>>
>>>> Do we really need to grab the vpe_lock for those which are belong to
>>>> the same irqchip with its_vpe_set_affinity()? The IRQ core code
>>>> should
>>>> already ensure the mutual exclusion among them, wrong?
>>>
>>> I've been trying to think about that, but jet-lag keeps getting in
>>> the way.
>>> I empirically think that you are right, but I need to go and check
>>> the various
>>> code paths to be sure. Hopefully I'll have a bit more brain space
>>> next week.
>>
>> So I slept on it and came back to my senses. The only case we actually
>> need
>> to deal with is when an affinity change impacts *another* interrupt.
>>
>> There is only two instances of this issue:
>>
>> - vLPIs have their *physical* affinity impacted by the affinity of the
>> vPE. Their virtual affinity is of course unchanged, but the
>> physical
>> one becomes important with direct invalidation. Taking a per-VPE
>> lock
>> in such context should address the issue.
>>
>> - vSGIs have the exact same issue, plus the matter of requiring some
>> *extra* one when reading the pending state, which requires a RMW
>> on two different registers. This requires an extra per-RD lock.
>
> Agreed with both!
>
>>
>> My original patch was stupidly complex, and the irq_desc lock is
>> perfectly enough to deal with anything that only affects the interrupt
>> state itself.
>>
>> GICv4 + direct invalidation for vLPIs breaks this by bypassing the
>> serialization initially provided by the ITS, as the RD is completely
>> out of band. The per-vPE lock brings back this serialization.
>>
>> I've updated the branch, which seems to run OK on D05. I still need
>> to run the usual tests on the FVP model though.
>
> I have pulled the latest branch and it looks good to me, except for
> one remaining concern:
>
> GICR_INV{LPI, ALL}R + GICR_SYNCR can also be accessed concurrently
> by multiple direct invalidation, should we also use the per-RD lock
> to ensure mutual exclusion? It looks not so harmful though, as this
> will only increase one's polling time against the Busy bit (in my
> view).
>
> But I point it out again for confirmation.
I was about to say that it doesn't really matter because it is only a
performance optimisation (and we're noty quite there yet), until I
spotted
this great nugget in the spec:
<quote>
Writing GICR_INVLPIR or GICR_INVALLR when GICR_SYNCR.Busy==1 is
CONSTRAINED
UNPREDICTABLE:
- The write is IGNORED .
- The invalidate specified by the write is performed.
</quote>
So we really need some form of mutual exclusion on a per-RD basis to
ensure
that no two invalidations occur at the same time, ensuring that Busy
clears
between the two.
Thanks for the heads up,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[-- Attachment #1.1: Type: text/plain, Size: 3344 bytes --]
What the hell?
On Mon, 2 Mar 2020 at 12:09, Marc Zyngier <maz@kernel.org> wrote:
> Hi Zenghui,
>
> On 2020-03-02 08:18, Zenghui Yu wrote:
> > On 2020/3/2 3:00, Marc Zyngier wrote:
> >> On 2020-02-28 19:37, Marc Zyngier wrote:
> >>> On 2020-02-20 03:11, Zenghui Yu wrote:
> >>
> >>>> Do we really need to grab the vpe_lock for those which are belong to
> >>>> the same irqchip with its_vpe_set_affinity()? The IRQ core code
> >>>> should
> >>>> already ensure the mutual exclusion among them, wrong?
> >>>
> >>> I've been trying to think about that, but jet-lag keeps getting in
> >>> the way.
> >>> I empirically think that you are right, but I need to go and check
> >>> the various
> >>> code paths to be sure. Hopefully I'll have a bit more brain space
> >>> next week.
> >>
> >> So I slept on it and came back to my senses. The only case we actually
> >> need
> >> to deal with is when an affinity change impacts *another* interrupt.
> >>
> >> There is only two instances of this issue:
> >>
> >> - vLPIs have their *physical* affinity impacted by the affinity of the
> >> vPE. Their virtual affinity is of course unchanged, but the
> >> physical
> >> one becomes important with direct invalidation. Taking a per-VPE
> >> lock
> >> in such context should address the issue.
> >>
> >> - vSGIs have the exact same issue, plus the matter of requiring some
> >> *extra* one when reading the pending state, which requires a RMW
> >> on two different registers. This requires an extra per-RD lock.
> >
> > Agreed with both!
> >
> >>
> >> My original patch was stupidly complex, and the irq_desc lock is
> >> perfectly enough to deal with anything that only affects the interrupt
> >> state itself.
> >>
> >> GICv4 + direct invalidation for vLPIs breaks this by bypassing the
> >> serialization initially provided by the ITS, as the RD is completely
> >> out of band. The per-vPE lock brings back this serialization.
> >>
> >> I've updated the branch, which seems to run OK on D05. I still need
> >> to run the usual tests on the FVP model though.
> >
> > I have pulled the latest branch and it looks good to me, except for
> > one remaining concern:
> >
> > GICR_INV{LPI, ALL}R + GICR_SYNCR can also be accessed concurrently
> > by multiple direct invalidation, should we also use the per-RD lock
> > to ensure mutual exclusion? It looks not so harmful though, as this
> > will only increase one's polling time against the Busy bit (in my
> > view).
> >
> > But I point it out again for confirmation.
>
> I was about to say that it doesn't really matter because it is only a
> performance optimisation (and we're noty quite there yet), until I
> spotted
> this great nugget in the spec:
>
> <quote>
> Writing GICR_INVLPIR or GICR_INVALLR when GICR_SYNCR.Busy==1 is
> CONSTRAINED
> UNPREDICTABLE:
> - The write is IGNORED .
> - The invalidate specified by the write is performed.
> </quote>
>
> So we really need some form of mutual exclusion on a per-RD basis to
> ensure
> that no two invalidations occur at the same time, ensuring that Busy
> clears
> between the two.
>
> Thanks for the heads up,
>
> M.
> --
> Jazz is not dead. It just smells funny...
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
[-- Attachment #1.2: Type: text/html, Size: 4495 bytes --][-- Attachment #2: Type: text/plain, Size: 151 bytes --]
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm