Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-02-26 Thread Julien Grall 

Hi Manish,

On 26/02/18 06:42, Manish Jaggi wrote:



On 02/01/2018 04:24 PM, Julien Grall wrote:

Hi Manish,

On 01/02/18 08:51, Manish Jaggi wrote:

On 01/25/2018 11:37 PM, Julien Grall wrote:

Hi,

I forgot to mention one thing about the placement of 
do_fixup_vgic_errata.


On 16/01/18 15:42, mja...@caviumnetworks.com wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
*regs)

  {
  const union hsr hsr = { .bits = regs->hsr };
  +#ifdef CONFIG_VGIC_ERRATA
+    int ret;
+
+    ret  = do_fixup_vgic_errata(regs,hsr);
+    if ( !ret )
+    {
+    advance_pc(regs, hsr);
+    return;


I am fully aware that I suggested this solution and still support 
that the vGIC errata should be fully separated. After all, it deals 
with hardware bug and the errata will just update the LRs as the 
hardware would do.


enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.


As you rightfully did, the do_fixup_vgic_errata should be called 
before syncing the LRs. However, even if you return early here, you 
will still execute leave_hypervisor_tail(). This mean that pending 
softirqs will be processed and potentially the vCPU rescheduled. 
Because the LRs were not synced (enter_hypervisor_head()) was not 
called, then the vGIC state will not out-of-date and would lead to 
all sort of potential issues.


As the vGIC errata implies trapping the register such as IAR1 
(reading interrupt), we want to get a fastpath for it (e.g not 
trying to execute softirq...). So I think we should bypass 
leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
thought.




How about adding a check for group1_trap enable in 
leave_hypervisor_tail().


void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;


I guess you mean the variable you introduced in patch #10. In that 
case, this would be totally wrong. You only want to skip 
leave_hypervisor_tail() when you are handling a ICV_* System registers.


What you want is adding a bool in the structure cpu_info to tell 
whether leave_hypervisor_tail() should be skipped.



Why would it be wrong? I don't follow our point.


As I mentioned in one of my previous e-mail, leave_hypervisor_tail() 
will process pending softirq and write/update the LRs based on the 
internal vGIC state. Softirq are used for scheduling, handling timer,...


So unless you want to make Xen unusable on Thunder-X, you really don't 
want to bypass leave_hypervisor_tail() in all the cases. Hence the 
suggest of a flag in cpu_info to provide a fastpath in certain cases.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-02-25 Thread Manish Jaggi 



On 02/26/2018 12:12 PM, Manish Jaggi wrote:



On 02/01/2018 04:24 PM, Julien Grall wrote:

Hi Manish,

On 01/02/18 08:51, Manish Jaggi wrote:

On 01/25/2018 11:37 PM, Julien Grall wrote:

Hi,

I forgot to mention one thing about the placement of 
do_fixup_vgic_errata.


On 16/01/18 15:42, mja...@caviumnetworks.com wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct 
cpu_user_regs *regs)

  {
  const union hsr hsr = { .bits = regs->hsr };
  +#ifdef CONFIG_VGIC_ERRATA
+    int ret;
+
+    ret  = do_fixup_vgic_errata(regs,hsr);
+    if ( !ret )
+    {
+    advance_pc(regs, hsr);
+    return;


I am fully aware that I suggested this solution and still support 
that the vGIC errata should be fully separated. After all, it deals 
with hardware bug and the errata will just update the LRs as the 
hardware would do.


enter_hypervisor_head() will sync the LRs state to the internal 
vGIC state. leave_hypervisor_head() will process pending softirq 
and write/update the LRs based on the internal vGIC state.


As you rightfully did, the do_fixup_vgic_errata should be called 
before syncing the LRs. However, even if you return early here, you 
will still execute leave_hypervisor_tail(). This mean that pending 
softirqs will be processed and potentially the vCPU rescheduled. 
Because the LRs were not synced (enter_hypervisor_head()) was not 
called, then the vGIC state will not out-of-date and would lead to 
all sort of potential issues.


As the vGIC errata implies trapping the register such as IAR1 
(reading interrupt), we want to get a fastpath for it (e.g not 
trying to execute softirq...). So I think we should bypass 
leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
thought.




How about adding a check for group1_trap enable in 
leave_hypervisor_tail().


void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;


I guess you mean the variable you introduced in patch #10. In that 
case, this would be totally wrong. You only want to skip 
leave_hypervisor_tail() when you are handling a ICV_* System registers.


What you want is adding a bool in the structure cpu_info to tell 
whether leave_hypervisor_tail() should be skipped.



Why would it be wrong? I don't follow our point.
Also adding a flag in cpu_info would be overkill, use of a global 
variable is simpler.

I am planning to remove group1_trap as a command line, and use it as

static void gicv3_hyp_init(void)
{
...
#ifdef CONFIG_CAVIUM_ERRATUM_30115
    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
    group1_trap = 1;
#endif
...
}

Attaching test patch

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 473e26111f..581c07b274 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -64,6 +64,7 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
 #define GICD   (gicv3.map_dbase)
 #define GICD_RDIST_BASE    (this_cpu(rbase))
 #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
+static bool group1_trap = 0;

 /*
  * Saves all 16(Max) LR registers. Though number of LRs implemented
@@ -825,7 +826,7 @@ static void gicv3_cpu_disable(void)

 static void gicv3_hyp_init(void)
 {
-    uint32_t vtr;
+    uint32_t vtr, reg32;

 vtr = READ_SYSREG32(ICH_VTR_EL2);
 gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
@@ -837,6 +838,10 @@ static void gicv3_hyp_init(void)

 WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
 WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
+
+    reg32 = GICH_HCR_EN;
+    reg32 |= (group1_trap) ? GICH_HCR_TALL1 : 0;
+    WRITE_SYSREG32(reg32, ICH_HCR_EL2);
 }

 /* Set up the per-CPU parts of the GIC for a secondary CPU */
@@ -1651,6 +1656,11 @@ static int __init gicv3_init(void)
 return -ENODEV;
 }

+#ifdef CONFIG_CAVIUM_ERRATUM_30115
+    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
+    group1_trap = 1;
+#endif
+
 if ( acpi_disabled )
 gicv3_dt_init();
 else
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..dbee0c322f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2295,6 +2295,10 @@ void do_trap_fiq(struct cpu_user_regs *regs)

 void leave_hypervisor_tail(void)
 {
+#ifdef CONFIG_CAVIUM_ERRATUM_30115
+    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
+    return;
+#endif
 while (1)
 {
 local_irq_disable();
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d3d7bda50d..e4c77fefd6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -117,6 +117,7 @@
 #define GICH_HCR_VGRP0DIE (1 << 5)
 #define GICH_HCR_VGRP1EIE (1 << 6)
 #define GICH_HCR_VGRP1DIE (1 << 7)
+#define GICH_HCR_TALL1    (1 << 12)

 #define GICH_MISR_EOI (1 << 0)
 #define GICH_MISR_U   (1 << 1)




Cheers,

Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-02-25 Thread Manish Jaggi 



On 02/01/2018 04:24 PM, Julien Grall wrote:

Hi Manish,

On 01/02/18 08:51, Manish Jaggi wrote:

On 01/25/2018 11:37 PM, Julien Grall wrote:

Hi,

I forgot to mention one thing about the placement of 
do_fixup_vgic_errata.


On 16/01/18 15:42, mja...@caviumnetworks.com wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
*regs)

  {
  const union hsr hsr = { .bits = regs->hsr };
  +#ifdef CONFIG_VGIC_ERRATA
+    int ret;
+
+    ret  = do_fixup_vgic_errata(regs,hsr);
+    if ( !ret )
+    {
+    advance_pc(regs, hsr);
+    return;


I am fully aware that I suggested this solution and still support 
that the vGIC errata should be fully separated. After all, it deals 
with hardware bug and the errata will just update the LRs as the 
hardware would do.


enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.


As you rightfully did, the do_fixup_vgic_errata should be called 
before syncing the LRs. However, even if you return early here, you 
will still execute leave_hypervisor_tail(). This mean that pending 
softirqs will be processed and potentially the vCPU rescheduled. 
Because the LRs were not synced (enter_hypervisor_head()) was not 
called, then the vGIC state will not out-of-date and would lead to 
all sort of potential issues.


As the vGIC errata implies trapping the register such as IAR1 
(reading interrupt), we want to get a fastpath for it (e.g not 
trying to execute softirq...). So I think we should bypass 
leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
thought.




How about adding a check for group1_trap enable in 
leave_hypervisor_tail().


void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;


I guess you mean the variable you introduced in patch #10. In that 
case, this would be totally wrong. You only want to skip 
leave_hypervisor_tail() when you are handling a ICV_* System registers.


What you want is adding a bool in the structure cpu_info to tell 
whether leave_hypervisor_tail() should be skipped.



Why would it be wrong? I don't follow our point.
Also adding a flag in cpu_info would be overkill, use of a global 
variable is simpler.

I am planning to remove group1_trap as a command line, and use it as

static void gicv3_hyp_init(void)
{
...
#ifdef CONFIG_CAVIUM_ERRATUM_30115
    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
    group1_trap = 1;
#endif
...
}


Cheers,




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

Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-02-01 Thread Julien Grall 

Hi Manish,

On 01/02/18 08:51, Manish Jaggi wrote:

On 01/25/2018 11:37 PM, Julien Grall wrote:

Hi,

I forgot to mention one thing about the placement of 
do_fixup_vgic_errata.


On 16/01/18 15:42, mja...@caviumnetworks.com wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
*regs)

  {
  const union hsr hsr = { .bits = regs->hsr };
  +#ifdef CONFIG_VGIC_ERRATA
+    int ret;
+
+    ret  = do_fixup_vgic_errata(regs,hsr);
+    if ( !ret )
+    {
+    advance_pc(regs, hsr);
+    return;


I am fully aware that I suggested this solution and still support that 
the vGIC errata should be fully separated. After all, it deals with 
hardware bug and the errata will just update the LRs as the hardware 
would do.


enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.


As you rightfully did, the do_fixup_vgic_errata should be called 
before syncing the LRs. However, even if you return early here, you 
will still execute leave_hypervisor_tail(). This mean that pending 
softirqs will be processed and potentially the vCPU rescheduled. 
Because the LRs were not synced (enter_hypervisor_head()) was not 
called, then the vGIC state will not out-of-date and would lead to all 
sort of potential issues.


As the vGIC errata implies trapping the register such as IAR1 (reading 
interrupt), we want to get a fastpath for it (e.g not trying to 
execute softirq...). So I think we should bypass 
leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
thought.




How about adding a check for group1_trap enable in leave_hypervisor_tail().

void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;


I guess you mean the variable you introduced in patch #10. In that case, 
this would be totally wrong. You only want to skip 
leave_hypervisor_tail() when you are handling a ICV_* System registers.


What you want is adding a bool in the structure cpu_info to tell whether 
leave_hypervisor_tail() should be skipped.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-02-01 Thread Manish Jaggi 



On 01/25/2018 11:37 PM, Julien Grall wrote:

Hi,

I forgot to mention one thing about the placement of 
do_fixup_vgic_errata.


On 16/01/18 15:42, mja...@caviumnetworks.com wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
*regs)

  {
  const union hsr hsr = { .bits = regs->hsr };
  +#ifdef CONFIG_VGIC_ERRATA
+    int ret;
+
+    ret  = do_fixup_vgic_errata(regs,hsr);
+    if ( !ret )
+    {
+    advance_pc(regs, hsr);
+    return;


I am fully aware that I suggested this solution and still support that 
the vGIC errata should be fully separated. After all, it deals with 
hardware bug and the errata will just update the LRs as the hardware 
would do.


enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.


As you rightfully did, the do_fixup_vgic_errata should be called 
before syncing the LRs. However, even if you return early here, you 
will still execute leave_hypervisor_tail(). This mean that pending 
softirqs will be processed and potentially the vCPU rescheduled. 
Because the LRs were not synced (enter_hypervisor_head()) was not 
called, then the vGIC state will not out-of-date and would lead to all 
sort of potential issues.


As the vGIC errata implies trapping the register such as IAR1 (reading 
interrupt), we want to get a fastpath for it (e.g not trying to 
execute softirq...). So I think we should bypass 
leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
thought.




How about adding a check for group1_trap enable in leave_hypervisor_tail().

void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;
    while (1)



Cheers,


+    }
+#endif
+
  enter_hypervisor_head(regs);
    switch (hsr.ec) {
diff --git a/xen/include/asm-arm/arm64/traps.h 
b/xen/include/asm-arm/arm64/traps.h

index 2379b578cb..7378a1b022 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs 
*regs, int instr_len);

    void do_sysreg(struct cpu_user_regs *regs,
 const union hsr hsr);
+int do_fixup_vgic_errata(struct cpu_user_regs *regs,
+   const union hsr hsr);
    #endif /* __ASM_ARM64_TRAPS__ */
  /*






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

Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-01-25 Thread Julien Grall 

Hi,

I forgot to mention one thing about the placement of do_fixup_vgic_errata.

On 16/01/18 15:42, mja...@caviumnetworks.com wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
  {
  const union hsr hsr = { .bits = regs->hsr };
  
+#ifdef CONFIG_VGIC_ERRATA

+int ret;
+
+ret  = do_fixup_vgic_errata(regs,hsr);
+if ( !ret )
+{
+advance_pc(regs, hsr);
+return;


I am fully aware that I suggested this solution and still support that 
the vGIC errata should be fully separated. After all, it deals with 
hardware bug and the errata will just update the LRs as the hardware 
would do.


enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.


As you rightfully did, the do_fixup_vgic_errata should be called before 
syncing the LRs. However, even if you return early here, you will still 
execute leave_hypervisor_tail(). This mean that pending softirqs will be 
processed and potentially the vCPU rescheduled. Because the LRs were not 
synced (enter_hypervisor_head()) was not called, then the vGIC state 
will not out-of-date and would lead to all sort of potential issues.


As the vGIC errata implies trapping the register such as IAR1 (reading 
interrupt), we want to get a fastpath for it (e.g not trying to execute 
softirq...). So I think we should bypass leave_hypervisor_tail(). I am 
not entirely sure how to do it nicely thought.


Cheers,


+}
+#endif
+
  enter_hypervisor_head(regs);
  
  switch (hsr.ec) {

diff --git a/xen/include/asm-arm/arm64/traps.h 
b/xen/include/asm-arm/arm64/traps.h
index 2379b578cb..7378a1b022 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int 
instr_len);
  
  void do_sysreg(struct cpu_user_regs *regs,

 const union hsr hsr);
+int do_fixup_vgic_errata(struct cpu_user_regs *regs,
+   const union hsr hsr);
  
  #endif /* __ASM_ARM64_TRAPS__ */

  /*



--
Julien Grall

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

[Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

2018-01-16 Thread mjaggi 
From: Manish Jaggi 

In order to start handling guest access to GICv3 system registers,
let's add a hook that will get called when we trap a system register
access.
This handling code is kept independent of other traps.
Set CONFIG_VGIC_ERRATA to enable this code.

Signed-off-by: Manish Jaggi 
---
 xen/arch/arm/arm64/vsysreg.c  | 17 +
 xen/arch/arm/traps.c  | 11 +++
 xen/include/asm-arm/arm64/traps.h |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index c57ac12503..a5c17e460f 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -219,6 +219,23 @@ void do_sysreg(struct cpu_user_regs *regs,
 regs->pc += 4;
 }
 
+#ifdef CONFIG_VGIC_ERRATA
+int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
+{
+int ret = 0;
+
+local_irq_disable();
+switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
+{
+default:
+ret = -1;
+break;
+}
+local_irq_enable();
+
+return ret;
+}
+#endif
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
 {
 const union hsr hsr = { .bits = regs->hsr };
 
+#ifdef CONFIG_VGIC_ERRATA
+int ret;
+
+ret  = do_fixup_vgic_errata(regs,hsr);
+if ( !ret )
+{
+advance_pc(regs, hsr);
+return;
+}
+#endif
+
 enter_hypervisor_head(regs);
 
 switch (hsr.ec) {
diff --git a/xen/include/asm-arm/arm64/traps.h 
b/xen/include/asm-arm/arm64/traps.h
index 2379b578cb..7378a1b022 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int 
instr_len);
 
 void do_sysreg(struct cpu_user_regs *regs,
const union hsr hsr);
+int do_fixup_vgic_errata(struct cpu_user_regs *regs,
+   const union hsr hsr);
 
 #endif /* __ASM_ARM64_TRAPS__ */
 /*
-- 
2.14.1


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