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

[Andrii Anisov <Andrii_Anisov@xxxxxxxx>]

Hello Julien,

I just wonder, do you plan to upstream the patch below?

Andrii Anisov


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