Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally
On Sat, 10 Mar 2018 03:23:18 +,
peng hao wrote:
> >> For emulation devices just like vga, keeping coherent dcache between
> >> guest and host timely is needed.
> >> Now the display of vnc-viewer will not update continuously and the
> >> patch can fix up.
> >>
> >> Signed-off-by: Peng Hao
> >> ---
> >> virt/kvm/arm/mmu.c | 6 ++
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ec62d1c..4a28395e 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> >> phys_addr_t fault_ipa,
> >> kvm_set_pfn_dirty(pfn);
> >> }
> >>
> >> -if (fault_status != FSC_PERM)
> >> -clean_dcache_guest_page(pfn, PMD_SIZE);
> >> +clean_dcache_guest_page(pfn, PMD_SIZE);
> >>
> >> if (exec_fault) {
> >> new_pmd = kvm_s2pmd_mkexec(new_pmd);
> >> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> >> phys_addr_t fault_ipa,
> >> mark_page_dirty(kvm, gfn);
> >> }
> >>
> >> -if (fault_status != FSC_PERM)
> >> -clean_dcache_guest_page(pfn, PAGE_SIZE);
> >> +clean_dcache_guest_page(pfn, PAGE_SIZE);
> >>
> >> if (exec_fault) {
> >> new_pte = kvm_s2pte_mkexec(new_pte);
> >>
>
> >I'm sorry, but I have to NAK this.
>
> >You're papering over the fundamental issue that you're accessing a
> >cacheable alias of a non cacheable memory. The architecture is very
> >clear about why this doesn't work, and KVM implements the architecture.
>
>
>
> I find that I just encounter the problem after the commit
> '15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .
This should really be a9c0e12ebee5 ("KVM: arm/arm64: Only clean the
dcache on translation fault").
> The commit contains "icache invalidation optimizations, improving VM
> startup time",it changes from unconditionally calling
> coherent_cache_guest_page(including dcache handle) to conditionally
> calling clean_dcache_guest_page.
>
> I trace the display of vnc abnormally and find it generate data
> abort in vga address region with FSC_PERM,and it will not call
> clean_dcache_guest_page . So I think should recover to uncontionally
> calling clean_dcache_guest_page.
[I really hate ranting on a Saturday morning as the sun is finally out
and I could be relaxing in the garden, but hey, I guess that's the
fate of a kernel hacker who made the silly mistake of reading email on
a Saturday morning instead of being out in the garden...]
Even if you enabled dirty tracking on the VGA memory (and thus making
it RO to generate a permission fault), you would end-up cleaning to
the PoC *before* any write has been committed to the page. How useful
is that? You're also pretty lucky that this does a clean+invalidate
(rather than a clean which would be good enough), meaning that QEMU
will in turn miss in the cache (PIPT caches) and read some *stale*
data from memory.
Repeat that often enough, and yes, you can get some sort of
display. Is that the right thing to do? Hell no. You might just as
well have a userspace process thrashing the cache at the PoC, running
concurrently with your VM, it would have a similar effect. It may work
on your particular platform, in some specific conditions, but it just
doesn't work in general.
What you are describing is a long standing issue. VGA emulation never
worked on ARM, because it is fundamentally incompatible with the ARM
memory architecture. Read the various discussions on the list over the
past 4 years or so, read the various presentations I and others have
given about this problem and how to address it[1].
The *real* fix is a one-liner in the Linux VGA driver: map the VGA
memory as cacheable, and stop lying to the guest. Or if you want to
lie to the guest, perform cache maintenance from userspace in QEMU
based on the dirty tracking bitmap that it uses for the VGA memory.
Anything else is out of the scope of the architecture, and I'm not
going to add more crap to satisfy requirements that cannot be
implemented on real HW, let alone in a virtualized environment.
Thanks,
M.
[1] http://events17.linuxfoundation.org/sites/events/files/slides/slides_10.pdf
--
Jazz is not dead, it just smell funny.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling
On Fri, 09 Mar 2018 21:36:12 +, Christoffer Dall wrote: > > On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote: > > I'd be more confident if we did forbid P+A for such interrupts > > altogether, as they really feel like another kind of HW interrupt. > > How about a slightly bigger hammer: Can we avoid doing P+A for level > interrupts completely? I don't think that really makes much sense, and > I think we simply everything if we just come back out and resample the > line. For an edge, something like a network card, there's a potential > performance win to appending a new pending state, but I doubt that this > is the case for level interrupts. I started implementing the same thing yesterday. Somehow, it feels slightly better to have the same flow for all level interrupts, including the timer, and we only use the MI on EOI as a way to trigger the next state of injection. Still testing, but looking good so far. I'm still puzzled that we have this level-but-not-quite behaviour for VFIO interrupts. At some point, it is going to bite us badly. M. -- Jazz is not dead, it just smell funny. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] KVM: arm/arm64: vgic: Don't populate multiple LRs with the same vintid
Hi Christoffer, On Fri, 09 Mar 2018 21:39:31 +, Christoffer Dall wrote: > > On Thu, Mar 08, 2018 at 06:39:20PM +, Marc Zyngier wrote: > > Thinking of it a bit more: MI on EOI doesn't offer much more guarantee > > in the way of priority ordering. Taking your example above: Even if > > you generate a MI when EOIing the SGI, there is no guarantee that > > you'll take the MI before you've acked the SPI. > > There's no guarantee, but at least you're attempting at processing the > SGIs in first. It's the best we can do, but not completely correct, > kinda thing... > > > > > If you really want to offer that absolute guarantee that all the > > multi-source SGIs of higher priority are delivered before anything > > else, then you must make sure that only the SGIs are present in the > > LRs, excluding any other interrupt on lower priority until you've > > queued them all. > > Yes, that sucks! Might not be too hard to implement, it's basically an > early out of the loop traversing the AP list, but just an annoying > complication. Yeah, it is a bit gross. The way I implemented it is by forcing the AP list to be sorted if there is any multi-SGI in the pipeline, and early out as soon as we see an interrupt of a lower priority than the first multi-SGI. That way, we only have an overhead in the case that combines multi-SGI and lower priority interrupts. > > At that stage, I wonder if there is a point in doing anything at > > all. The GICv2 architecture is too rubbish for words. > > > > The case we do need to worry about is the guest processing all its > interrupts and not exiting while there is actually still an SGI pending. > At that point, we can either do it with the "no interrupts pending > maintenance interrupt" or with the "EOI maintenance interrupt", and I > think the latter at least gets us slightly closer to the architecture > for a non-virtualized system. I think that this is where we disagree. I don't see anything in the architecture that mandates that we should present the SGIs before anything else. All we need to do is to ensure that interrupts of higher priority are presented before anything else. It is perfectly acceptable for an implementation to deliver SGI0, then SPI3, and SGI0 (from another CPU) after that, as long as SPI3 isn't of lesser priority than SGI0. Another thing I dislike about using EOI for that is forces us to propagate the knowledge of the multi-SGI horror further down the stack, down to both implementations of vgic_populate_lr. NPIE allows us to keep that knowledge local. But that's an orthogonal issue, and we can further argue/bikeshed about the respective merits of both solutions once we have something that fits the sorry state of the GICv2 architecture ;-). Thanks, M. -- Jazz is not dead, it just smell funny. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally
>On 09/03/18 22:15, Peng Hao wrote:
>> For emulation devices just like vga, keeping coherent dcache between
>> guest and host timely is needed.
>> Now the display of vnc-viewer will not update continuously and the
>> patch can fix up.
>>
>> Signed-off-by: Peng Hao
>> ---
>> virt/kvm/arm/mmu.c | 6 ++
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index ec62d1c..4a28395e 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> kvm_set_pfn_dirty(pfn);
>> }
>>
>> -if (fault_status != FSC_PERM)
>> -clean_dcache_guest_page(pfn, PMD_SIZE);
>> +clean_dcache_guest_page(pfn, PMD_SIZE);
>>
>> if (exec_fault) {
>> new_pmd = kvm_s2pmd_mkexec(new_pmd);
>> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> mark_page_dirty(kvm, gfn);
>> }
>>
>> -if (fault_status != FSC_PERM)
>> -clean_dcache_guest_page(pfn, PAGE_SIZE);
>> +clean_dcache_guest_page(pfn, PAGE_SIZE);
>>
>> if (exec_fault) {
>> new_pte = kvm_s2pte_mkexec(new_pte);
>>
>I'm sorry, but I have to NAK this.
>You're papering over the fundamental issue that you're accessing a
>cacheable alias of a non cacheable memory. The architecture is very
>clear about why this doesn't work, and KVM implements the architecture.
I find that I just encounter the problem after the commit
'15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .
The commit contains "icache invalidation optimizations, improving VM startup
time",it changes
from unconditionally calling coherent_cache_guest_page(including dcache handle)
to conditionally calling clean_dcache_guest_page.
I trace the display of vnc abnormally and find it generate data abort in vga
address region with FSC_PERM,and it will not call clean_dcache_guest_page . So
I think should recover to uncontionally calling clean_dcache_guest_page.
Thanks.
>If you want this to work, map your VGA device as cacheable, add cache
>maintenance to QEMU, or use another frame-buffer emulation that doesn't
>require such a gack.
>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
Re: Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally
>On 09/03/18 22:15, Peng Hao wrote:
>> For emulation devices just like vga, keeping coherent dcache between
>> guest and host timely is needed.
>> Now the display of vnc-viewer will not update continuously and the
>> patch can fix up.
>>
>> Signed-off-by: Peng Hao
>> ---
>> virt/kvm/arm/mmu.c | 6 ++
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index ec62d1c..4a28395e 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> kvm_set_pfn_dirty(pfn);
>> }
>>
>> -if (fault_status != FSC_PERM)
>> -clean_dcache_guest_page(pfn, PMD_SIZE);
>> +clean_dcache_guest_page(pfn, PMD_SIZE);
>>
>> if (exec_fault) {
>> new_pmd = kvm_s2pmd_mkexec(new_pmd);
>> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> phys_addr_t fault_ipa,
>> mark_page_dirty(kvm, gfn);
>> }
>>
>> -if (fault_status != FSC_PERM)
>> -clean_dcache_guest_page(pfn, PAGE_SIZE);
>> +clean_dcache_guest_page(pfn, PAGE_SIZE);
>>
>> if (exec_fault) {
>> new_pte = kvm_s2pte_mkexec(new_pte);
>>
>I'm sorry, but I have to NAK this.
>You're papering over the fundamental issue that you're accessing a
>cacheable alias of a non cacheable memory. The architecture is very
>clear about why this doesn't work, and KVM implements the architecture.
I find that I just encounter the problem after the commit
'15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .
The commit contains "icache invalidation optimizations, improving VM startup
time",it changes
from unconditionally calling coherent_cache_guest_page(including dcache handle)
to conditionally calling clean_dcache_guest_page.
I trace the display of vnc abnormally and find it generate data abort in vga
address region with FSC_PERM,and it will not call clean_dcache_guest_page . So
I think should recover to uncontionally calling clean_dcache_guest_page.
Thanks.
>If you want this to work, map your VGA device as cacheable, add cache
>maintenance to QEMU, or use another frame-buffer emulation that doesn't
>require such a gack.
>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
Re:Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally
>On Sat, 10 Mar 2018 03:23:18 +,
>peng hao wrote:
>> >> For emulation devices just like vga, keeping coherent dcache between
>> >> guest and host timely is needed.
>> >> Now the display of vnc-viewer will not update continuously and the
>> >> patch can fix up.
>> >>
>> >> Signed-off-by: Peng Hao
>> >> ---
>> >> virt/kvm/arm/mmu.c | 6 ++
>> >> 1 file changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> >> index ec62d1c..4a28395e 100644
>> >> --- a/virt/kvm/arm/mmu.c
>> >> +++ b/virt/kvm/arm/mmu.c
>> >> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> >> phys_addr_t fault_ipa,
>> >> kvm_set_pfn_dirty(pfn);
>> >> }
>> >>
>> >> -if (fault_status != FSC_PERM)
>> >> -clean_dcache_guest_page(pfn, PMD_SIZE);
>> >> +clean_dcache_guest_page(pfn, PMD_SIZE);
>> >>
>> >> if (exec_fault) {
>> >> new_pmd = kvm_s2pmd_mkexec(new_pmd);
>> >> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
>> >> phys_addr_t fault_ipa,
>> >> mark_page_dirty(kvm, gfn);
>> >> }
>> >>
>> >> -if (fault_status != FSC_PERM)
>> >> -clean_dcache_guest_page(pfn, PAGE_SIZE);
>> >> +clean_dcache_guest_page(pfn, PAGE_SIZE);
>> >>
>> >> if (exec_fault) {
>> >> new_pte = kvm_s2pte_mkexec(new_pte);
>> >>
>>
>> >I'm sorry, but I have to NAK this.
>>
>> >You're papering over the fundamental issue that you're accessing a
>> >cacheable alias of a non cacheable memory. The architecture is very
>> >clear about why this doesn't work, and KVM implements the architecture.
>>
>>
>>
>> I find that I just encounter the problem after the commit
>> '15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .
>This should really be a9c0e12ebee5 ("KVM: arm/arm64: Only clean the
>dcache on translation fault").
>> The commit contains "icache invalidation optimizations, improving VM
>> startup time",it changes from unconditionally calling
>> coherent_cache_guest_page(including dcache handle) to conditionally
>> calling clean_dcache_guest_page.
>>
>> I trace the display of vnc abnormally and find it generate data
>> abort in vga address region with FSC_PERM,and it will not call
>> clean_dcache_guest_page . So I think should recover to uncontionally
>> calling clean_dcache_guest_page.
>[I really hate ranting on a Saturday morning as the sun is finally out
>and I could be relaxing in the garden, but hey, I guess that's the
>fate of a kernel hacker who made the silly mistake of reading email on
>a Saturday morning instead of being out in the garden...]
>Even if you enabled dirty tracking on the VGA memory (and thus making
>it RO to generate a permission fault), you would end-up cleaning to
>the PoC *before* any write has been committed to the page. How useful
>is that? You're also pretty lucky that this does a clean+invalidate
>(rather than a clean which would be good enough), meaning that QEMU
>will in turn miss in the cache (PIPT caches) and read some *stale*
>data from memory.
>Repeat that often enough, and yes, you can get some sort of
>display. Is that the right thing to do? Hell no. You might just as
>well have a userspace process thrashing the cache at the PoC, running
>concurrently with your VM, it would have a similar effect. It may work
>on your particular platform, in some specific conditions, but it just
>doesn't work in general.
>What you are describing is a long standing issue. VGA emulation never
>worked on ARM, because it is fundamentally incompatible with the ARM
>memory architecture. Read the various discussions on the list over the
>past 4 years or so, read the various presentations I and others have
>given about this problem and how to address it[1].
>The *real* fix is a one-liner in the Linux VGA driver: map the VGA
>memory as cacheable, and stop lying to the guest. Or if you want to
>lie to the guest, perform cache maintenance from userspace in QEMU
>based on the dirty tracking bitmap that it uses for the VGA memory.
>Anything else is out of the scope of the architecture, and I'm not
>going to add more crap to satisfy requirements that cannot be
>implemented on real HW, let alone in a virtualized environment.
Thanks for your explanation in detail
>Thanks,
>M.
>[1] http://events17.linuxfoundation.org/sites/events/files/slides/slides_10.pdf
>--
>Jazz is not dead, it just smell funny.___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm64: KVM: Use SMCCC_ARCH_WORKAROUND_1 for Falkor BP hardening
Hi Will, On 03/06/2018 09:25 AM, Will Deacon wrote: > On Mon, Mar 05, 2018 at 12:03:33PM -0600, Shanker Donthineni wrote: >> On 03/05/2018 11:15 AM, Will Deacon wrote: >>> On Mon, Mar 05, 2018 at 10:57:58AM -0600, Shanker Donthineni wrote: On 03/05/2018 09:56 AM, Will Deacon wrote: > On Fri, Mar 02, 2018 at 03:50:18PM -0600, Shanker Donthineni wrote: >> @@ -199,33 +208,15 @@ static int enable_smccc_arch_workaround_1(void >> *data) >> return 0; >> } >> >> +if (((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR) || >> +((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1)) >> +cb = qcom_link_stack_sanitization; > > Is this just a performance thing? Do you actually see an advantage over > always making the firmware call? We've seen minimal impact in our testing. > Yes, we've couple of advantages using the standard SMCCC_ARCH_WOKAROUND_1 framework. - Improves the code readability. - Avoid the unnecessary MIDR checks on each vCPU exit. - Validates ID_AA64PFR0_CVS2 feature for Falkor chips. - Avoids the 2nd link stack sanitization workaround in firmware. >>> >>> What I mean is, can we drop qcom_link_stack_sanitization altogether and >>> use the SMCCC interface for everything? >>> >> >> No, We would like to keep it qcom_link_stack_sanitization for host kernel >> since it takes a few CPU cycles instead of heavyweight SMCCC call. > > Is that something that you can actually measure in the workloads and > benchmarks that you care about? If so, fine, but that doesn't seem to be the > case for the Cortex cores we've looked at internally and it would be nice to > avoid having different workarounds in the kernel just because the SMCCC > interface wasn't baked in time, rather than because there's a meaningful > performance difference. > We've seen noticeable performance improvement with the microbench workloads, ans also some of our customers have observed improvements on heavy workloads. Unfortunately I can't share those specific results here. SMCCC call overhead is much higher as compared to link stack workaround on Falkor, ~99X. Host kernel workaround takes less than ~20 CPU cycles, whereas SMCCC_ARCH_WOKAROUND_1 consumes thousands of CPU cycles to sanitize the branch prediction on Falkor. Especially workloads inside virtual machines provides much better results because no KVM involvement is required whenever guest calls qcom_link_stack_sanitization(). > Will > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- Shanker Donthineni Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: Use SMCCC_ARCH_WORKAROUND_1 for Falkor BP hardening
Hi Will, On 03/09/2018 07:48 AM, Will Deacon wrote: > Hi SHanker, > > On Mon, Mar 05, 2018 at 11:06:43AM -0600, Shanker Donthineni wrote: >> The function SMCCC_ARCH_WORKAROUND_1 was introduced as part of SMC >> V1.1 Calling Convention to mitigate CVE-2017-5715. This patch uses >> the standard call SMCCC_ARCH_WORKAROUND_1 for Falkor chips instead >> of Silicon provider service ID 0xC2001700. >> >> Signed-off-by: Shanker Donthineni >> --- >> Chnages since v1: >> - Trivial change in cpucaps.h (refresh after removing >> ARM64_HARDEN_BP_POST_GUEST_EXIT) >> >> arch/arm64/include/asm/cpucaps.h | 5 ++-- >> arch/arm64/include/asm/kvm_asm.h | 2 -- >> arch/arm64/kernel/bpi.S | 8 -- >> arch/arm64/kernel/cpu_errata.c | 55 >> ++-- >> arch/arm64/kvm/hyp/entry.S | 12 - >> arch/arm64/kvm/hyp/switch.c | 10 >> 6 files changed, 21 insertions(+), 71 deletions(-) > > Could you reply to my outstanding question on the last version of this patch > please? > I replied to your comments. This patch contents have been discussed with QCOM CPU architecture and design team. Their recommendation was to keep two variants of variant2 mitigation in order to take advantage of Falkor hardware and avoid the unnecessary overhead by calling SMMCC always. > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/564194.html > > Will > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- Shanker Donthineni Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] KVM: arm/arm64: vgic: Don't populate multiple LRs with the same vintid
On Sat, Mar 10, 2018 at 1:57 PM, Marc Zyngier wrote: > Hi Christoffer, > > On Fri, 09 Mar 2018 21:39:31 +, > Christoffer Dall wrote: >> >> On Thu, Mar 08, 2018 at 06:39:20PM +, Marc Zyngier wrote: >> > Thinking of it a bit more: MI on EOI doesn't offer much more guarantee >> > in the way of priority ordering. Taking your example above: Even if >> > you generate a MI when EOIing the SGI, there is no guarantee that >> > you'll take the MI before you've acked the SPI. >> >> There's no guarantee, but at least you're attempting at processing the >> SGIs in first. It's the best we can do, but not completely correct, >> kinda thing... >> >> > >> > If you really want to offer that absolute guarantee that all the >> > multi-source SGIs of higher priority are delivered before anything >> > else, then you must make sure that only the SGIs are present in the >> > LRs, excluding any other interrupt on lower priority until you've >> > queued them all. >> >> Yes, that sucks! Might not be too hard to implement, it's basically an >> early out of the loop traversing the AP list, but just an annoying >> complication. > > Yeah, it is a bit gross. The way I implemented it is by forcing the AP > list to be sorted if there is any multi-SGI in the pipeline, and early > out as soon as we see an interrupt of a lower priority than the first > multi-SGI. That way, we only have an overhead in the case that > combines multi-SGI and lower priority interrupts. > yes, that's what I had in mind as well. >> > At that stage, I wonder if there is a point in doing anything at >> > all. The GICv2 architecture is too rubbish for words. >> > >> >> The case we do need to worry about is the guest processing all its >> interrupts and not exiting while there is actually still an SGI pending. >> At that point, we can either do it with the "no interrupts pending >> maintenance interrupt" or with the "EOI maintenance interrupt", and I >> think the latter at least gets us slightly closer to the architecture >> for a non-virtualized system. > > I think that this is where we disagree. I don't think we disagree, I must have expressed myself poorly... > I don't see anything in the > architecture that mandates that we should present the SGIs before > anything else. Neither do I. > All we need to do is to ensure that interrupts of > higher priority are presented before anything else. Agreed. > It is perfectly > acceptable for an implementation to deliver SGI0, then SPI3, and SGI0 > (from another CPU) after that, as long as SPI3 isn't of lesser > priority than SGI0. Yes, but what we cannot do is let the guest deliver SGI0, then SPI3, and then loop forever without delivering SGI0 from another CPU. That's why I said "the guest processing all its interrupts and not exiting while there is actually still an SGI pending" and said that we could use either the EOI or the NPIE trick. > > Another thing I dislike about using EOI for that is forces us to > propagate the knowledge of the multi-SGI horror further down the > stack, down to both implementations of vgic_populate_lr. NPIE allows > us to keep that knowledge local. But that's an orthogonal issue, and > we can further argue/bikeshed about the respective merits of both > solutions once we have something that fits the sorry state of the > GICv2 architecture ;-). > Yeah, I don't care deeply. If NPIE is prettier in the implementations, let's do that. Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling
On Sat, Mar 10, 2018 at 12:20 PM, Marc Zyngier wrote: > On Fri, 09 Mar 2018 21:36:12 +, > Christoffer Dall wrote: >> >> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote: >> > I'd be more confident if we did forbid P+A for such interrupts >> > altogether, as they really feel like another kind of HW interrupt. >> >> How about a slightly bigger hammer: Can we avoid doing P+A for level >> interrupts completely? I don't think that really makes much sense, and >> I think we simply everything if we just come back out and resample the >> line. For an edge, something like a network card, there's a potential >> performance win to appending a new pending state, but I doubt that this >> is the case for level interrupts. > > I started implementing the same thing yesterday. Somehow, it feels > slightly better to have the same flow for all level interrupts, > including the timer, and we only use the MI on EOI as a way to trigger > the next state of injection. Still testing, but looking good so far. > > I'm still puzzled that we have this level-but-not-quite behaviour for > VFIO interrupts. At some point, it is going to bite us badly. > Where is the departure from level-triggered behavior with VFIO? As far as I can tell, the GIC flow of the interrupts will be just a level interrupt, but we just need to make sure the resamplefd mechanism is supported for both types of interrupts. Whether or not that's a decent mechanism seems orthogonal to me, but that's a discussion for another day I think. Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
