Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

[Julien Grall <julien.grall@xxxxxxx>]

On 29/10/2018 10:06, Andrii Anisov wrote:
> Hello Julien,
Hi,

> Sorry for the previous email sent as html.
Don't worry. I didn't notice it :).

> On 27.10.18 15:14, Andrii Anisov wrote:
> > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index
> > > > f6f6de3..985192b 100644 --- a/xen/arch/arm/traps.c +++
> > > > b/xen/arch/arm/traps.c @@ -2095,6 +2095,7 @@ static void
> > > > enter_hypervisor_head(struct cpu_user_regs *regs) if (
> > > > current->arch.hcr_el2 & HCR_VA ) current->arch.hcr_el2 =
> > > > READ_SYSREG(HCR_EL2);
> > > >
> > > > +        gic_store_lrs();
> > > gic_clear_lrs(...) may rewrite the LRs. To confirm this stall LRs
> > > are not due by this function, can you move that after
> > > gic_clear_lrs()?
> > Right you are, gic_clear_lrs(), in 4.10 codebase, rewrites LRs. But
> > it does not change PENDING to INVALID under no circumstances from
> > one hand. From another hand, all changes to LRs are made through
> > gic specific operations gic_hw_ops->... which are tracked by me.
> > You can see, in the code above, that I copy all updates to the
> > physical LR issued by hypervisor into the stored LRs. So it not the
> > case. But I'll check on Monday.
> In 4.12-unstable code base I moved gic_store_lrs() after
> vgic_sync_from_lrs()  and see significant changes.  Now stale lines
> are printed at very high rate, and it is the proper behavior. Because
> the correspondent check (performed when vgic_sync_from_lrs() reads
> LRs) detects that VM processes interrupts and LR values are changed
> comparing to those set by hypervisor lately.
>
> So now it is the question, why could I detect spurious changes in LRs
> without exiting to VM?
I wrote down an answer yesterday (sent it today) to your previous
answer. You may use the LRs information from the previous guest trap as
interrupts are re-enabled before storing the LRs.

Can you try the patch below?

commit 11e360b93be81a58a41832d714f33f797ad312a9
Author: Julien Grall <julien.grall@xxxxxxx>
Date:   Mon Oct 29 13:32:56 2018 +0000

    xen/arm: Re-enable interrupt later in the trap path

    Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 97b05f53ea..8f287891b6 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -195,7 +195,6 @@ hyp_error_invalid:

 hyp_error:
         entry   hyp=1
-        msr     daifclr, #2
         mov     x0, sp
         bl      do_trap_hyp_serror
         exit    hyp=1
@@ -203,7 +202,7 @@ hyp_error:
 /* Traps taken in Current EL with SP_ELx */
 hyp_sync:
         entry   hyp=1
-        msr     daifclr, #6
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_hyp_sync
         exit    hyp=1
@@ -304,7 +303,7 @@ guest_sync_slowpath:
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
-        msr     daifclr, #6
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_guest_sync
 1:
@@ -332,7 +331,7 @@ guest_fiq_invalid:

 guest_error:
         entry   hyp=0, compat=0
-        msr     daifclr, #6
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_guest_serror
         exit    hyp=0, compat=0
@@ -347,7 +346,7 @@ guest_sync_compat:
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
-        msr     daifclr, #6
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_guest_sync
 1:
@@ -375,7 +374,7 @@ guest_fiq_invalid_compat:

 guest_error_compat:
         entry   hyp=0, compat=1
-        msr     daifclr, #6
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_guest_serror
         exit    hyp=0, compat=1
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 51d2e42c77..c18f89b41f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2039,6 +2039,8 @@ static void enter_hypervisor_head(struct cpu_user_regs 
*regs)
     {
         struct vcpu *v = current;

+        ASSERT(!local_irq_is_enabled());
+
         /* If the guest has disabled the workaround, bring it back on. */
         if ( needs_ssbd_flip(v) )
             arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
@@ -2073,6 +2075,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
     const union hsr hsr = { .bits = regs->hsr };

     enter_hypervisor_head(regs);
+    local_irq_enable();

     switch ( hsr.ec )
     {
@@ -2208,6 +2211,7 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
     const union hsr hsr = { .bits = regs->hsr };

     enter_hypervisor_head(regs);
+    local_irq_enable();

     switch ( hsr.ec )
     {
@@ -2246,6 +2250,7 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
 void do_trap_hyp_serror(struct cpu_user_regs *regs)
 {
     enter_hypervisor_head(regs);
+    local_irq_enable();

     __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
 }
@@ -2253,6 +2258,7 @@ void do_trap_hyp_serror(struct cpu_user_regs *regs)
 void do_trap_guest_serror(struct cpu_user_regs *regs)
 {
     enter_hypervisor_head(regs);
+    local_irq_enable();

     __do_trap_serror(regs, true);
 }

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel