[PATCH v2 00/17] arm64: Mediate access to GICv3 sysregs at EL2

[PATCH v2 00/17] arm64: Mediate access to GICv3 sysregs at EL2

[Manish Jaggi]

This patchset is based on Marc's patchset below.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1].

As these patches are ported to xen specifically for cavium errata 30115
few changes are made:
- Xen coding style is used

- group1_enable / group0_enable command line options not used.
  CONFIG_CAVIUM_ERRATUM_30115 would enable trapping of group0/1 registers

- check_workaround_cavium_30115 function is used instead to check if emulation
  has to be done.

- Not every patch in [1] is ported to xen, ported are ones which are relevant
  to Cavium Errata 30115.

- vreg_emulate_XXX functions are defined for emulating g0/g1 registers,
  basically these functions wrap calling of read/write functions based on
  isread in a separate function.

- read/write_gicreg is replaced by READ/WRITE_SYSREG32 which is already
  present in xen code.

Most of the trap code is added in vgic-v3-sr.c. Trap handler function is kept
independent of the usual guest trap handling code.

Cavium 30115 Errata Workaround:
  Hypervisor to trap and emulate the following registers:
  Group 0: ICC_IAR0_EL1, ICC_EOIR0_EL1, ICC_HPPIR0_EL1, ICC_BPR0_EL1,
           ICC_AP0R0_EL1, ICC_IGRPEN0_EL1
  Group 1: ICC_IAR1_EL1, ICC_EOIR1_EL1, ICC_HPPIR1_EL1, ICC_BPR1_EL1,
           ICC_AP1R0_EL1, ICC_IGRPEN1_EL1

The errata workaround has been validated on Cavium ThunderX1 platform.
Steps to reproduce the errata
- Boot Xen with 2 cores.
- Disable group1 interrupts in domU kernel
- start domU, the kill and start again.
One of the Xen core would hang.

[1] https://www.spinics.net/lists/arm-kernel/msg587082.html

Changes from v1
- removed __ prefix from function names
- Patches have been realigned to map closely with original patchset

Changes from v0
- Added Group0 traps.
- Some cleanups and documentation

Changes from RFC
- Added commit information on ported patches from linux
- Added skip_hyp_tail to control calling leave_hypervisor_tail 
- Added CAVIUM_CONFIG_ERRATUM_30115 which will auto enable workaround


Manish Jaggi (17):
  arm: Placeholder for handling Group0/1 traps
  arm64: vgic-v3: Add ICV_BPR1_EL1 handler
  arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler
  arm64: Add accessors for the ICH_APxRn_EL2 registers
  Expose ich_read/write_lr in vgic-v3-sr.c
  arm64: Add ICV_IAR1_EL1 handler
  arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
  arm64: vgic-v3: Add ICV_AP1Rn_EL1 handler
  arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler
  arm64: vgic-v3: Add ICV_BPR0_EL1 handler
  arm64: vgic-v3: Add ICV_IGRPEN0_EL1 handler
  arm64: vgic-v3: Add misc Group-0 handlers
  arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family
  arm64: Add config for Cavium Thunder erratum 30115
  arm: Hook workaround handler from traps.c based on Cavium
    workaround_30115
  arm64: Inject Undef exception in guest if trapping a
    write-to-read-only GICv3 access
  arm64: Inject undef exception in guest if trapping a
    read-from-write-only GICv3 access

 docs/misc/arm/silicon-errata.txt    |   1 +
 xen/arch/arm/Kconfig                |  11 +
 xen/arch/arm/arm64/Makefile         |   1 +
 xen/arch/arm/arm64/vgic-v3-sr.c     | 866 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/cpuerrata.c            |  21 +
 xen/arch/arm/traps.c                |  31 ++
 xen/include/asm-arm/arm64/sysregs.h |  12 +
 xen/include/asm-arm/arm64/traps.h   |   2 +
 xen/include/asm-arm/cpuerrata.h     |   1 +
 xen/include/asm-arm/cpufeature.h    |   3 +-
 xen/include/asm-arm/current.h       |   3 +-
 xen/include/asm-arm/gic_v3_defs.h   |  29 ++
 xen/include/asm-arm/processor.h     |   9 +
 13 files changed, 988 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

-- 
2.14.1


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

[PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Manish Jaggi]

The errata will require to emulate the GIC virtual CPU interface in Xen. 
Because the hypervisor will update its internal state of the vGIC, we want
to avoid messing up with it. So the errata is handled separately from the
rest of the hypervisor.

New file vgic-v3-sr.c is added which will hold trap and emulate code
for group0 / group1 registers. Workaround for cavium Errata 30115
needs this emulation code.

vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
in subsequent patches based on errata macros.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/traps.h |  2 ++
 2 files changed, 62 insertions(+)
 create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
new file mode 100644
index 0000000000..39ab1ed6ca
--- /dev/null
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -0,0 +1,60 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * Manish Jaggi <manish.jaggi@cavium.com>
+ * Copyright (c) 2018 Cavium.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/current.h>
+#include <asm/regs.h>
+#include <asm/system.h>
+#include <asm/traps.h>
+
+/*
+ * returns true if the register is emulated.
+ */
+bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
+{
+    bool ret = true;
+    const union hsr hsr = { .bits = regs->hsr };
+
+    /* Disabling interrupts to prevent change in guest state */
+    local_irq_disable();
+    if ( hsr.ec != HSR_EC_SYSREG )
+    {
+        ret = false;
+        goto end;
+    }
+
+    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
+    {
+    default:
+        ret = false;
+        break;
+    }
+end:
+    local_irq_enable();
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/arm64/traps.h b/xen/include/asm-arm/arm64/traps.h
index 2379b578cb..3c3911a69c 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -3,6 +3,8 @@
 
 void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
 
+bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs);
+
 void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr);
 
-- 
2.14.1


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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Marc Zyngier]

On 27/03/18 10:07, Manish Jaggi wrote:
> The errata will require to emulate the GIC virtual CPU interface in Xen. 
> Because the hypervisor will update its internal state of the vGIC, we want
> to avoid messing up with it. So the errata is handled separately from the
> rest of the hypervisor.
> 
> New file vgic-v3-sr.c is added which will hold trap and emulate code
> for group0 / group1 registers. Workaround for cavium Errata 30115
> needs this emulation code.
> 
> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
> in subsequent patches based on errata macros.
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>  xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/arm64/traps.h |  2 ++
>  2 files changed, 62 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
> 
> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> new file mode 100644
> index 0000000000..39ab1ed6ca
> --- /dev/null
> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> @@ -0,0 +1,60 @@
> +/*
> + * xen/arch/arm/arm64/vgic-v3-sr.c
> + *
> + * Code to emulate group0/group1 traps for handling
> + * cavium erratum 30115
> + *
> + * Manish Jaggi <manish.jaggi@cavium.com>
> + * Copyright (c) 2018 Cavium.

IANAL, but I don't think this copyright notice is correct.

I wrote about 90% of this series, and the copyright for that code is
owned by ARM, and licensed under the GPLv2. You have the right to
duplicate that code and do almost whatever you want with (within the
limits of the GPLv2), but you still don't own the copyright.

I suggest you get in touch with your legal department for clarification
on the matter.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Manish Jaggi]

On 03/27/2018 03:31 PM, Marc Zyngier wrote:
> On 27/03/18 10:07, Manish Jaggi wrote:
>> The errata will require to emulate the GIC virtual CPU interface in Xen.
>> Because the hypervisor will update its internal state of the vGIC, we want
>> to avoid messing up with it. So the errata is handled separately from the
>> rest of the hypervisor.
>>
>> New file vgic-v3-sr.c is added which will hold trap and emulate code
>> for group0 / group1 registers. Workaround for cavium Errata 30115
>> needs this emulation code.
>>
>> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
>> in subsequent patches based on errata macros.
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/arm64/traps.h |  2 ++
>>   2 files changed, 62 insertions(+)
>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
>>
>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>> new file mode 100644
>> index 0000000000..39ab1ed6ca
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>> @@ -0,0 +1,60 @@
>> +/*
>> + * xen/arch/arm/arm64/vgic-v3-sr.c
>> + *
>> + * Code to emulate group0/group1 traps for handling
>> + * cavium erratum 30115
>> + *
>> + * Manish Jaggi <manish.jaggi@cavium.com>
>> + * Copyright (c) 2018 Cavium.
> IANAL, but I don't think this copyright notice is correct.
>
> I wrote about 90% of this series, and the copyright for that code is
> owned by ARM, and licensed under the GPLv2. You have the right to
> duplicate that code and do almost whatever you want with (within the
> limits of the GPLv2), but you still don't own the copyright.
>
> I suggest you get in touch with your legal department for clarification
> on the matter.
I will remove the copyright line, and add this
Original Author: Marc Zyngier <>
Ported to Xen by: Manish Jaggi <>

Hope that is fine.
I can resend this single patch
> Thanks,
>
> 	M.


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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Marc Zyngier]

On 27/03/18 11:10, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 03:31 PM, Marc Zyngier wrote:
>> On 27/03/18 10:07, Manish Jaggi wrote:
>>> The errata will require to emulate the GIC virtual CPU interface in Xen.
>>> Because the hypervisor will update its internal state of the vGIC, we want
>>> to avoid messing up with it. So the errata is handled separately from the
>>> rest of the hypervisor.
>>>
>>> New file vgic-v3-sr.c is added which will hold trap and emulate code
>>> for group0 / group1 registers. Workaround for cavium Errata 30115
>>> needs this emulation code.
>>>
>>> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
>>> in subsequent patches based on errata macros.
>>>
>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>> ---
>>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/arm64/traps.h |  2 ++
>>>   2 files changed, 62 insertions(+)
>>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
>>>
>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>> new file mode 100644
>>> index 0000000000..39ab1ed6ca
>>> --- /dev/null
>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>> @@ -0,0 +1,60 @@
>>> +/*
>>> + * xen/arch/arm/arm64/vgic-v3-sr.c
>>> + *
>>> + * Code to emulate group0/group1 traps for handling
>>> + * cavium erratum 30115
>>> + *
>>> + * Manish Jaggi <manish.jaggi@cavium.com>
>>> + * Copyright (c) 2018 Cavium.
>> IANAL, but I don't think this copyright notice is correct.
>>
>> I wrote about 90% of this series, and the copyright for that code is
>> owned by ARM, and licensed under the GPLv2. You have the right to
>> duplicate that code and do almost whatever you want with (within the
>> limits of the GPLv2), but you still don't own the copyright.
>>
>> I suggest you get in touch with your legal department for clarification
>> on the matter.
> I will remove the copyright line, and add this
> Original Author: Marc Zyngier <>
> Ported to Xen by: Manish Jaggi <>

You're missing the point. I don't give a damn about the authorship (I'm
not exactly proud to have written that code). The problem at hand is the
ARM copyright, which should be preserved (as no-one in Cavium wrote a
single line of the original code).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Stefano Stabellini]

On Tue, 27 Mar 2018, Marc Zyngier wrote:
> On 27/03/18 11:10, Manish Jaggi wrote:
> > 
> > 
> > On 03/27/2018 03:31 PM, Marc Zyngier wrote:
> >> On 27/03/18 10:07, Manish Jaggi wrote:
> >>> The errata will require to emulate the GIC virtual CPU interface in Xen.
> >>> Because the hypervisor will update its internal state of the vGIC, we want
> >>> to avoid messing up with it. So the errata is handled separately from the
> >>> rest of the hypervisor.
> >>>
> >>> New file vgic-v3-sr.c is added which will hold trap and emulate code
> >>> for group0 / group1 registers. Workaround for cavium Errata 30115
> >>> needs this emulation code.
> >>>
> >>> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
> >>> in subsequent patches based on errata macros.
> >>>
> >>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> >>> ---
> >>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
> >>>   xen/include/asm-arm/arm64/traps.h |  2 ++
> >>>   2 files changed, 62 insertions(+)
> >>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
> >>>
> >>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> >>> new file mode 100644
> >>> index 0000000000..39ab1ed6ca
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> >>> @@ -0,0 +1,60 @@
> >>> +/*
> >>> + * xen/arch/arm/arm64/vgic-v3-sr.c
> >>> + *
> >>> + * Code to emulate group0/group1 traps for handling
> >>> + * cavium erratum 30115
> >>> + *
> >>> + * Manish Jaggi <manish.jaggi@cavium.com>
> >>> + * Copyright (c) 2018 Cavium.
> >> IANAL, but I don't think this copyright notice is correct.
> >>
> >> I wrote about 90% of this series, and the copyright for that code is
> >> owned by ARM, and licensed under the GPLv2. You have the right to
> >> duplicate that code and do almost whatever you want with (within the
> >> limits of the GPLv2), but you still don't own the copyright.
> >>
> >> I suggest you get in touch with your legal department for clarification
> >> on the matter.
> > I will remove the copyright line, and add this
> > Original Author: Marc Zyngier <>
> > Ported to Xen by: Manish Jaggi <>
> 
> You're missing the point. I don't give a damn about the authorship (I'm
> not exactly proud to have written that code).

:-D

> The problem at hand is the
> ARM copyright, which should be preserved (as no-one in Cavium wrote a
> single line of the original code).

I have been asking myself similar questions for a while now when we
import code from Linux. The copyright/author line at the top of the file
is somewhat arbitrary as every person that touched the code has
copyright over her modifications. This is why at some point I thought we
had to retain the full list of Signed-off-by lines that ever touched the
code in question, but then, reading the DCO terms one more time:

 (b) The contribution is based upon previous work that, to the best
     of my knowledge, is covered under an appropriate open source
     license and I have the right under that license to submit that
     work with modifications, whether created in whole or in part
     by me, under the same open source license (unless I am
     permitted to submit under a different license), as indicated
     in the file; or

I checked in the Linux kernel when code was imported from the Xen
hypervisor to KVM a long time ago, and only a singled Signed-off-by of
the person importing the code was used.

This is how I came to the conclusion that actually we don't need to do
anything special, although it would be nice as a courtesy to retain the
copyright/author lines at the top of the file. It would also be nice to
have the original Linux commit id in the commit message to help tracking
the history.

But I would love to have a clear guideline on this from a lawyer.

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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 4811 bytes --]

Hi,

Sorry for the formatting.

On Wed, 28 Mar 2018, 04:18 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Tue, 27 Mar 2018, Marc Zyngier wrote:
> > On 27/03/18 11:10, Manish Jaggi wrote:
> > >
> > >
> > > On 03/27/2018 03:31 PM, Marc Zyngier wrote:
> > >> On 27/03/18 10:07, Manish Jaggi wrote:
> > >>> The errata will require to emulate the GIC virtual CPU interface in
> Xen.
> > >>> Because the hypervisor will update its internal state of the vGIC,
> we want
> > >>> to avoid messing up with it. So the errata is handled separately
> from the
> > >>> rest of the hypervisor.
> > >>>
> > >>> New file vgic-v3-sr.c is added which will hold trap and emulate code
> > >>> for group0 / group1 registers. Workaround for cavium Errata 30115
> > >>> needs this emulation code.
> > >>>
> > >>> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
> > >>> in subsequent patches based on errata macros.
> > >>>
> > >>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> > >>> ---
> > >>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60
> +++++++++++++++++++++++++++++++++++++++
> > >>>   xen/include/asm-arm/arm64/traps.h |  2 ++
> > >>>   2 files changed, 62 insertions(+)
> > >>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
> > >>>
> > >>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c
> b/xen/arch/arm/arm64/vgic-v3-sr.c
> > >>> new file mode 100644
> > >>> index 0000000000..39ab1ed6ca
> > >>> --- /dev/null
> > >>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> > >>> @@ -0,0 +1,60 @@
> > >>> +/*
> > >>> + * xen/arch/arm/arm64/vgic-v3-sr.c
> > >>> + *
> > >>> + * Code to emulate group0/group1 traps for handling
> > >>> + * cavium erratum 30115
> > >>> + *
> > >>> + * Manish Jaggi <manish.jaggi@cavium.com>
> > >>> + * Copyright (c) 2018 Cavium.
> > >> IANAL, but I don't think this copyright notice is correct.
> > >>
> > >> I wrote about 90% of this series, and the copyright for that code is
> > >> owned by ARM, and licensed under the GPLv2. You have the right to
> > >> duplicate that code and do almost whatever you want with (within the
> > >> limits of the GPLv2), but you still don't own the copyright.
> > >>
> > >> I suggest you get in touch with your legal department for
> clarification
> > >> on the matter.
> > > I will remove the copyright line, and add this
> > > Original Author: Marc Zyngier <>
> > > Ported to Xen by: Manish Jaggi <>
> >
> > You're missing the point. I don't give a damn about the authorship (I'm
> > not exactly proud to have written that code).
>
> :-D
>
> > The problem at hand is the
> > ARM copyright, which should be preserved (as no-one in Cavium wrote a
> > single line of the original code).
>
> I have been asking myself similar questions for a while now when we
> import code from Linux. The copyright/author line at the top of the file
> is somewhat arbitrary as every person that touched the code has
> copyright over her modifications. This is why at some point I thought we
> had to retain the full list of Signed-off-by lines that ever touched the
> code in question, but then, reading the DCO terms one more time:
>
>  (b) The contribution is based upon previous work that, to the best
>      of my knowledge, is covered under an appropriate open source
>      license and I have the right under that license to submit that
>      work with modifications, whether created in whole or in part
>      by me, under the same open source license (unless I am
>      permitted to submit under a different license), as indicated
>      in the file; or
>
> I checked in the Linux kernel when code was imported from the Xen
> hypervisor to KVM a long time ago, and only a singled Signed-off-by of
> the person importing the code was used.


It does not mean it was a good practice ;).


> This is how I came to the conclusion that actually we don't need to do
> anything special, although it would be nice as a courtesy to retain the
> copyright/author lines at the top of the file. It would also be nice to
> have the original Linux commit id in the commit message to help tracking
> the history.
>

We had a similar discussion on the spectre patches a couple of months ago.
The original author is useful to know if we need to relicense some files.

Having just the commit id from Linux would create more overhead.

Lars suggested to create a file listing where all the work come from. For
commit, I think it would be ok to just add the Signed-off in the commit and
retain copyright in the code.

Of course, it is ok add the Cavium one if code is changed. But no copyright
should be dropped.

Cheers,


> But I would love to have a clear guideline on this from a lawyer.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 6691 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Manish Jaggi]

[-- Attachment #1.1: Type: text/plain, Size: 5900 bytes --]



On 03/28/2018 06:18 AM, Julien Grall wrote:
>
> Hi,
>
> Sorry for the formatting.
>
> On Wed, 28 Mar 2018, 04:18 Stefano Stabellini, <sstabellini@kernel.org 
> <mailto:sstabellini@kernel.org>> wrote:
>
>     On Tue, 27 Mar 2018, Marc Zyngier wrote:
>     > On 27/03/18 11:10, Manish Jaggi wrote:
>     > >
>     > >
>     > > On 03/27/2018 03:31 PM, Marc Zyngier wrote:
>     > >> On 27/03/18 10:07, Manish Jaggi wrote:
>     > >>> The errata will require to emulate the GIC virtual CPU
>     interface in Xen.
>     > >>> Because the hypervisor will update its internal state of the
>     vGIC, we want
>     > >>> to avoid messing up with it. So the errata is handled
>     separately from the
>     > >>> rest of the hypervisor.
>     > >>>
>     > >>> New file vgic-v3-sr.c is added which will hold trap and
>     emulate code
>     > >>> for group0 / group1 registers. Workaround for cavium Errata
>     30115
>     > >>> needs this emulation code.
>     > >>>
>     > >>> vgic_v3_handle_cpuif_access would be called from
>     do_trap_guest_sync
>     > >>> in subsequent patches based on errata macros.
>     > >>>
>     > >>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com
>     <mailto:manish.jaggi@cavium.com>>
>     > >>> ---
>     > >>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60
>     +++++++++++++++++++++++++++++++++++++++
>     > >>>   xen/include/asm-arm/arm64/traps.h |  2 ++
>     > >>>   2 files changed, 62 insertions(+)
>     > >>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
>     > >>>
>     > >>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c
>     b/xen/arch/arm/arm64/vgic-v3-sr.c
>     > >>> new file mode 100644
>     > >>> index 0000000000..39ab1ed6ca
>     > >>> --- /dev/null
>     > >>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>     > >>> @@ -0,0 +1,60 @@
>     > >>> +/*
>     > >>> + * xen/arch/arm/arm64/vgic-v3-sr.c
>     > >>> + *
>     > >>> + * Code to emulate group0/group1 traps for handling
>     > >>> + * cavium erratum 30115
>     > >>> + *
>     > >>> + * Manish Jaggi <manish.jaggi@cavium.com
>     <mailto:manish.jaggi@cavium.com>>
>     > >>> + * Copyright (c) 2018 Cavium.
>     > >> IANAL, but I don't think this copyright notice is correct.
>     > >>
>     > >> I wrote about 90% of this series, and the copyright for that
>     code is
>     > >> owned by ARM, and licensed under the GPLv2. You have the right to
>     > >> duplicate that code and do almost whatever you want with
>     (within the
>     > >> limits of the GPLv2), but you still don't own the copyright.
>     > >>
>     > >> I suggest you get in touch with your legal department for
>     clarification
>     > >> on the matter.
>     > > I will remove the copyright line, and add this
>     > > Original Author: Marc Zyngier <>
>     > > Ported to Xen by: Manish Jaggi <>
>     >
>     > You're missing the point. I don't give a damn about the
>     authorship (I'm
>     > not exactly proud to have written that code).
>
>     :-D
>
>     > The problem at hand is the
>     > ARM copyright, which should be preserved (as no-one in Cavium
>     wrote a
>     > single line of the original code).
>
>     I have been asking myself similar questions for a while now when we
>     import code from Linux. The copyright/author line at the top of
>     the file
>     is somewhat arbitrary as every person that touched the code has
>     copyright over her modifications. This is why at some point I
>     thought we
>     had to retain the full list of Signed-off-by lines that ever
>     touched the
>     code in question, but then, reading the DCO terms one more time:
>
>      (b) The contribution is based upon previous work that, to the best
>          of my knowledge, is covered under an appropriate open source
>          license and I have the right under that license to submit that
>          work with modifications, whether created in whole or in part
>          by me, under the same open source license (unless I am
>          permitted to submit under a different license), as indicated
>          in the file; or
>
>     I checked in the Linux kernel when code was imported from the Xen
>     hypervisor to KVM a long time ago, and only a singled Signed-off-by of
>     the person importing the code was used.
>
>
> It does not mean it was a good practice ;).
>
>
>     This is how I came to the conclusion that actually we don't need to do
>     anything special, although it would be nice as a courtesy to
>     retain the
>     copyright/author lines at the top of the file. It would also be
>     nice to
>     have the original Linux commit id in the commit message to help
>     tracking
>     the history.
>
>
> We had a similar discussion on the spectre patches a couple of months 
> ago. The original author is useful to know if we need to relicense 
> some files.
>
> Having just the commit id from Linux would create more overhead.
>
> Lars suggested to create a file listing where all the work come from. 
> For commit, I think it would be ok to just add the Signed-off in the 
> commit and retain copyright in the code.
>
> Of course, it is ok add the Cavium one if code is changed. But no 
> copyright should be dropped.
>
How about

+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * This file merges code from Linux virt/kvm/arm/hyp/vgic-v3-sr.c
+ * Copyright (C) 2012-2015 - ARM Ltd
+ * Original Author: Marc Zyngier <marc.zyngier@arm.com>
+ * Code Ported to Xen by: Manish Jaggi <manish.jaggi@cavium.com>
...

> Cheers,
>
>
>     But I would love to have a clear guideline on this from a lawyer.
>
>     _______________________________________________
>     Xen-devel mailing list
>     Xen-devel@lists.xenproject.org <mailto:Xen-devel@lists.xenproject.org>
>     https://lists.xenproject.org/mailman/listinfo/xen-devel
>


[-- Attachment #1.2: Type: text/html, Size: 10072 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 01/17] arm: Placeholder for handling Group0/1 traps

[Julien Grall]

Hi,

On 28/03/18 04:48, Manish Jaggi wrote:
> 
> 
> On 03/28/2018 06:18 AM, Julien Grall wrote:
>>
>> Hi,
>>
>> Sorry for the formatting.
>>
>> On Wed, 28 Mar 2018, 04:18 Stefano Stabellini, <sstabellini@kernel.org 
>> <mailto:sstabellini@kernel.org>> wrote:
>>
>>     On Tue, 27 Mar 2018, Marc Zyngier wrote:
>>     > On 27/03/18 11:10, Manish Jaggi wrote:
>>     > >
>>     > >
>>     > > On 03/27/2018 03:31 PM, Marc Zyngier wrote:
>>     > >> On 27/03/18 10:07, Manish Jaggi wrote:
>>     > >>> The errata will require to emulate the GIC virtual CPU
>>     interface in Xen.
>>     > >>> Because the hypervisor will update its internal state of the
>>     vGIC, we want
>>     > >>> to avoid messing up with it. So the errata is handled
>>     separately from the
>>     > >>> rest of the hypervisor.
>>     > >>>
>>     > >>> New file vgic-v3-sr.c is added which will hold trap and
>>     emulate code
>>     > >>> for group0 / group1 registers. Workaround for cavium Errata
>>     30115
>>     > >>> needs this emulation code.
>>     > >>>
>>     > >>> vgic_v3_handle_cpuif_access would be called from
>>     do_trap_guest_sync
>>     > >>> in subsequent patches based on errata macros.
>>     > >>>
>>     > >>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com
>>     <mailto:manish.jaggi@cavium.com>>
>>     > >>> ---
>>     > >>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60
>>     +++++++++++++++++++++++++++++++++++++++
>>     > >>>   xen/include/asm-arm/arm64/traps.h |  2 ++
>>     > >>>   2 files changed, 62 insertions(+)
>>     > >>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
>>     > >>>
>>     > >>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c
>>     b/xen/arch/arm/arm64/vgic-v3-sr.c
>>     > >>> new file mode 100644
>>     > >>> index 0000000000..39ab1ed6ca
>>     > >>> --- /dev/null
>>     > >>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>     > >>> @@ -0,0 +1,60 @@
>>     > >>> +/*
>>     > >>> + * xen/arch/arm/arm64/vgic-v3-sr.c
>>     > >>> + *
>>     > >>> + * Code to emulate group0/group1 traps for handling
>>     > >>> + * cavium erratum 30115
>>     > >>> + *
>>     > >>> + * Manish Jaggi <manish.jaggi@cavium.com
>>     <mailto:manish.jaggi@cavium.com>>
>>     > >>> + * Copyright (c) 2018 Cavium.
>>     > >> IANAL, but I don't think this copyright notice is correct.
>>     > >>
>>     > >> I wrote about 90% of this series, and the copyright for that
>>     code is
>>     > >> owned by ARM, and licensed under the GPLv2. You have the right to
>>     > >> duplicate that code and do almost whatever you want with
>>     (within the
>>     > >> limits of the GPLv2), but you still don't own the copyright.
>>     > >>
>>     > >> I suggest you get in touch with your legal department for
>>     clarification
>>     > >> on the matter.
>>     > > I will remove the copyright line, and add this
>>     > > Original Author: Marc Zyngier <>
>>     > > Ported to Xen by: Manish Jaggi <>
>>     >
>>     > You're missing the point. I don't give a damn about the
>>     authorship (I'm
>>     > not exactly proud to have written that code).
>>
>>     :-D
>>
>>     > The problem at hand is the
>>     > ARM copyright, which should be preserved (as no-one in Cavium
>>     wrote a
>>     > single line of the original code).
>>
>>     I have been asking myself similar questions for a while now when we
>>     import code from Linux. The copyright/author line at the top of
>>     the file
>>     is somewhat arbitrary as every person that touched the code has
>>     copyright over her modifications. This is why at some point I
>>     thought we
>>     had to retain the full list of Signed-off-by lines that ever
>>     touched the
>>     code in question, but then, reading the DCO terms one more time:
>>
>>      (b) The contribution is based upon previous work that, to the best
>>          of my knowledge, is covered under an appropriate open source
>>          license and I have the right under that license to submit that
>>          work with modifications, whether created in whole or in part
>>          by me, under the same open source license (unless I am
>>          permitted to submit under a different license), as indicated
>>          in the file; or
>>
>>     I checked in the Linux kernel when code was imported from the Xen
>>     hypervisor to KVM a long time ago, and only a singled Signed-off-by of
>>     the person importing the code was used.
>>
>>
>> It does not mean it was a good practice ;).
>>
>>
>>     This is how I came to the conclusion that actually we don't need to do
>>     anything special, although it would be nice as a courtesy to
>>     retain the
>>     copyright/author lines at the top of the file. It would also be
>>     nice to
>>     have the original Linux commit id in the commit message to help
>>     tracking
>>     the history.
>>
>>
>> We had a similar discussion on the spectre patches a couple of months 
>> ago. The original author is useful to know if we need to relicense 
>> some files.
>>
>> Having just the commit id from Linux would create more overhead.
>>
>> Lars suggested to create a file listing where all the work come from. 
>> For commit, I think it would be ok to just add the Signed-off in the 
>> commit and retain copyright in the code.
>>
>> Of course, it is ok add the Cavium one if code is changed. But no 
>> copyright should be dropped.
>>
> How about
> 
> +/*
> + * xen/arch/arm/arm64/vgic-v3-sr.c
> + *
> + * Code to emulate group0/group1 traps for handling
> + * cavium erratum 30115
> + *
> + * This file merges code from Linux virt/kvm/arm/hyp/vgic-v3-sr.c
> + * Copyright (C) 2012-2015 - ARM Ltd
> + * Original Author: Marc Zyngier <marc.zyngier@arm.com>
> + * Code Ported to Xen by: Manish Jaggi <manish.jaggi@cavium.com>
> ...

That would be ok for me.

Cheers,

-- 
Julien Grall

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

[PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
 3 files changed, 77 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index 39ab1ed6ca..ed4254acf9 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -18,10 +18,76 @@
  */
 
 #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
 #include <asm/regs.h>
 #include <asm/system.h>
 #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 084d2a1e5d..025a27b0b4 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -89,6 +89,7 @@
 #define HSR_SYSREG_ICC_ASGI1R_EL1 HSR_SYSREG(3,1,c12,c11,6)
 #define HSR_SYSREG_ICC_SGI0R_EL1  HSR_SYSREG(3,2,c12,c11,7)
 #define HSR_SYSREG_ICC_SRE_EL1    HSR_SYSREG(3,0,c12,c12,5)
+#define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 65c9dc47cf..68a34cc353 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -157,6 +157,12 @@
 
 #define GICH_VMCR_EOI                (1 << 9)
 #define GICH_VMCR_VENG1              (1 << 1)
+#define ICH_VMCR_CBPR_SHIFT          4
+#define ICH_VMCR_CBPR_MASK           (1 << ICH_VMCR_CBPR_SHIFT)
+#define ICH_VMCR_BPR0_SHIFT          21
+#define ICH_VMCR_BPR0_MASK           (7 << ICH_VMCR_BPR0_SHIFT)
+#define ICH_VMCR_BPR1_SHIFT          18
+#define ICH_VMCR_BPR1_MASK           (7 << ICH_VMCR_BPR1_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
-- 
2.14.1


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier]

On 27/03/18 10:07, Manish Jaggi wrote:
> This patch is ported to xen from linux commit
> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
> 
> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
> register, which is located in the ICH_VMCR_EL2.BPR1 field.
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>  xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/arm64/sysregs.h |  1 +
>  xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> index 39ab1ed6ca..ed4254acf9 100644
> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> @@ -18,10 +18,76 @@
>   */
>  
>  #include <asm/current.h>
> +#include <asm/gic_v3_defs.h>
>  #include <asm/regs.h>
>  #include <asm/system.h>
>  #include <asm/traps.h>
>  
> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
> +
> +static int vgic_v3_bpr_min(void)
> +{
> +    /* See Pseudocode for VPriorityGroup */
> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
> +}
> +
> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
> +{
> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
> +}
> +
> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
> +{
> +    unsigned int bpr;
> +
> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
> +    {
> +        bpr = vgic_v3_get_bpr0(vmcr);
> +        if ( bpr < 7 )
> +            bpr++;
> +    }
> +    else
> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
> +
> +    return bpr;
> +}
> +
> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
> +{
> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
> +
> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
> +}
> +
> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
> +{
> +    register_t val = get_user_reg(regs, regidx);
> +    uint8_t bpr_min = vgic_v3_bpr_min();
> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
> +
> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
> +        return;
> +
> +    /* Enforce BPR limiting */
> +    if ( val < bpr_min )
> +        val = bpr_min;
> +
> +    val <<= ICH_VMCR_BPR1_SHIFT;
> +    val &= ICH_VMCR_BPR1_MASK;
> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
> +    vmcr |= val;
> +
> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
> +}
> +
> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
> +{
> +    if ( hsr.sysreg.read )
> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
> +    else
> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
> +}
> +
>  /*
>   * returns true if the register is emulated.
>   */
> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>  
>      switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>      {
> +    case HSR_SYSREG_ICC_BPR1_EL1:
> +         vreg_emulate_bpr1(regs, hsr);
> +         break;

What is the rational for indirecting through a function and moving the
reading of VMCR to the leaf functions? I appreciate that this doesn't
change much, but since this is a port of existing code, it will make
more complex the port of potential fixes.

This comment applies throughout the series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 04:00 PM, Marc Zyngier wrote:
> On 27/03/18 10:07, Manish Jaggi wrote:
>> This patch is ported to xen from linux commit
>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>
>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/arm64/sysregs.h |  1 +
>>   xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>   3 files changed, 77 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>> index 39ab1ed6ca..ed4254acf9 100644
>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>> @@ -18,10 +18,76 @@
>>    */
>>   
>>   #include <asm/current.h>
>> +#include <asm/gic_v3_defs.h>
>>   #include <asm/regs.h>
>>   #include <asm/system.h>
>>   #include <asm/traps.h>
>>   
>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>> +
>> +static int vgic_v3_bpr_min(void)
>> +{
>> +    /* See Pseudocode for VPriorityGroup */
>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>> +}
>> +
>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>> +{
>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>> +}
>> +
>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>> +{
>> +    unsigned int bpr;
>> +
>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>> +    {
>> +        bpr = vgic_v3_get_bpr0(vmcr);
>> +        if ( bpr < 7 )
>> +            bpr++;
>> +    }
>> +    else
>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>> +
>> +    return bpr;
>> +}
>> +
>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>> +{
>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>> +
>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>> +}
>> +
>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>> +{
>> +    register_t val = get_user_reg(regs, regidx);
>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>> +
>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>> +        return;
>> +
>> +    /* Enforce BPR limiting */
>> +    if ( val < bpr_min )
>> +        val = bpr_min;
>> +
>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>> +    val &= ICH_VMCR_BPR1_MASK;
>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>> +    vmcr |= val;
>> +
>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>> +}
>> +
>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>> +{
>> +    if ( hsr.sysreg.read )
>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>> +    else
>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>> +}
>> +
>>   /*
>>    * returns true if the register is emulated.
>>    */
>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>   
>>       switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>       {
>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>> +         vreg_emulate_bpr1(regs, hsr);
>> +         break;
> What is the rational for indirecting through a function and moving the
> reading of VMCR to the leaf functions? I appreciate that this doesn't
> change much, but since this is a port of existing code, it will make
> more complex the port of potential fixes.
I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr)
{
   ...
     /*
      * MDCR_EL2.TDRA
      *
      * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
      */
     case HSR_SYSREG_MDRAR_EL1:
         return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);

> This comment applies throughout the series.
>
> Thanks,
>
> 	M.


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier]

On 27/03/18 11:35, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>> On 27/03/18 10:07, Manish Jaggi wrote:
>>> This patch is ported to xen from linux commit
>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>
>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>
>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>> ---
>>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>   xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>   3 files changed, 77 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>> index 39ab1ed6ca..ed4254acf9 100644
>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>> @@ -18,10 +18,76 @@
>>>    */
>>>   
>>>   #include <asm/current.h>
>>> +#include <asm/gic_v3_defs.h>
>>>   #include <asm/regs.h>
>>>   #include <asm/system.h>
>>>   #include <asm/traps.h>
>>>   
>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>> +
>>> +static int vgic_v3_bpr_min(void)
>>> +{
>>> +    /* See Pseudocode for VPriorityGroup */
>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>> +}
>>> +
>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>> +{
>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>> +}
>>> +
>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>> +{
>>> +    unsigned int bpr;
>>> +
>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>> +    {
>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>> +        if ( bpr < 7 )
>>> +            bpr++;
>>> +    }
>>> +    else
>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>> +
>>> +    return bpr;
>>> +}
>>> +
>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>> +{
>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>> +
>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>> +}
>>> +
>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>> +{
>>> +    register_t val = get_user_reg(regs, regidx);
>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>> +
>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>> +        return;
>>> +
>>> +    /* Enforce BPR limiting */
>>> +    if ( val < bpr_min )
>>> +        val = bpr_min;
>>> +
>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>> +    val &= ICH_VMCR_BPR1_MASK;
>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>> +    vmcr |= val;
>>> +
>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>> +}
>>> +
>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>> +{
>>> +    if ( hsr.sysreg.read )
>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>> +    else
>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>> +}
>>> +
>>>   /*
>>>    * returns true if the register is emulated.
>>>    */
>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>   
>>>       switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>       {
>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>> +         vreg_emulate_bpr1(regs, hsr);
>>> +         break;
>> What is the rational for indirecting through a function and moving the
>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>> change much, but since this is a port of existing code, it will make
>> more complex the port of potential fixes.
> I used xen template of handling sysreg traps
> If you see the file xen/arch/arm/arm64/sysreg.c
> a handle_XXX function is used throughout...

Sure, but this is not sysreg.c. This is a separate file for a reason
(i.e. it is imported code). Anyway, that's for the Xen maintainers to
decide.

More importantly, my other question still stand: most trap functions do
require VMCR as an input. Why moving it to the leaf functions?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 04:15 PM, Marc Zyngier wrote:
> On 27/03/18 11:35, Manish Jaggi wrote:
>>
>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>> This patch is ported to xen from linux commit
>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>
>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>
>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>> ---
>>>>    xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>    xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>    xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>    3 files changed, 77 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>> @@ -18,10 +18,76 @@
>>>>     */
>>>>    
>>>>    #include <asm/current.h>
>>>> +#include <asm/gic_v3_defs.h>
>>>>    #include <asm/regs.h>
>>>>    #include <asm/system.h>
>>>>    #include <asm/traps.h>
>>>>    
>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>> +
>>>> +static int vgic_v3_bpr_min(void)
>>>> +{
>>>> +    /* See Pseudocode for VPriorityGroup */
>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>> +}
>>>> +
>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>> +{
>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>> +}
>>>> +
>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>> +{
>>>> +    unsigned int bpr;
>>>> +
>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>> +    {
>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>> +        if ( bpr < 7 )
>>>> +            bpr++;
>>>> +    }
>>>> +    else
>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>> +
>>>> +    return bpr;
>>>> +}
>>>> +
>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>> +{
>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>> +
>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>> +}
>>>> +
>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>> +{
>>>> +    register_t val = get_user_reg(regs, regidx);
>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>> +
>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>> +        return;
>>>> +
>>>> +    /* Enforce BPR limiting */
>>>> +    if ( val < bpr_min )
>>>> +        val = bpr_min;
>>>> +
>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>> +    vmcr |= val;
>>>> +
>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>> +}
>>>> +
>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>> +{
>>>> +    if ( hsr.sysreg.read )
>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>> +    else
>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>> +}
>>>> +
>>>>    /*
>>>>     * returns true if the register is emulated.
>>>>     */
>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>    
>>>>        switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>        {
>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>> +         break;
>>> What is the rational for indirecting through a function and moving the
>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>> change much, but since this is a port of existing code, it will make
>>> more complex the port of potential fixes.
>> I used xen template of handling sysreg traps
>> If you see the file xen/arch/arm/arm64/sysreg.c
>> a handle_XXX function is used throughout...
> Sure, but this is not sysreg.c. This is a separate file for a reason
> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
> decide.
>
> More importantly, my other question still stand: most trap functions do
> require VMCR as an input. Why moving it to the leaf functions?
Same reason, I was keeping the interface same of all handle_XXX functions
handle_XXX(regs, hsr, ...)

Do you want me to change both to match with your patch or it is ok?
>
> Thanks,
>
> 	M.


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier]

On 27/03/18 11:56, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>> This patch is ported to xen from linux commit
>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>
>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>
>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>> ---
>>>>>    xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>    xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>    xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>    3 files changed, 77 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>> @@ -18,10 +18,76 @@
>>>>>     */
>>>>>    
>>>>>    #include <asm/current.h>
>>>>> +#include <asm/gic_v3_defs.h>
>>>>>    #include <asm/regs.h>
>>>>>    #include <asm/system.h>
>>>>>    #include <asm/traps.h>
>>>>>    
>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>> +
>>>>> +static int vgic_v3_bpr_min(void)
>>>>> +{
>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>> +}
>>>>> +
>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>> +{
>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>> +}
>>>>> +
>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>> +{
>>>>> +    unsigned int bpr;
>>>>> +
>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>> +    {
>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>> +        if ( bpr < 7 )
>>>>> +            bpr++;
>>>>> +    }
>>>>> +    else
>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>> +
>>>>> +    return bpr;
>>>>> +}
>>>>> +
>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>> +{
>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>> +
>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>> +}
>>>>> +
>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>> +{
>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>> +
>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>> +        return;
>>>>> +
>>>>> +    /* Enforce BPR limiting */
>>>>> +    if ( val < bpr_min )
>>>>> +        val = bpr_min;
>>>>> +
>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>> +    vmcr |= val;
>>>>> +
>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>> +}
>>>>> +
>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>> +{
>>>>> +    if ( hsr.sysreg.read )
>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>> +    else
>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>> +}
>>>>> +
>>>>>    /*
>>>>>     * returns true if the register is emulated.
>>>>>     */
>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>    
>>>>>        switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>        {
>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>> +         break;
>>>> What is the rational for indirecting through a function and moving the
>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>> change much, but since this is a port of existing code, it will make
>>>> more complex the port of potential fixes.
>>> I used xen template of handling sysreg traps
>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>> a handle_XXX function is used throughout...
>> Sure, but this is not sysreg.c. This is a separate file for a reason
>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>> decide.
>>
>> More importantly, my other question still stand: most trap functions do
>> require VMCR as an input. Why moving it to the leaf functions?
> Same reason, I was keeping the interface same of all handle_XXX functions
> handle_XXX(regs, hsr, ...)
> 
> Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're
pointlessly changing the flow. Again, that's for the maintainers to
comment, but you should at the very least indicate that change in the
commit log.

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 04:35 PM, Marc Zyngier wrote:
> On 27/03/18 11:56, Manish Jaggi wrote:
>>
>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>> This patch is ported to xen from linux commit
>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>
>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>
>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>> ---
>>>>>>     xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>     xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>     xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>     3 files changed, 77 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>> @@ -18,10 +18,76 @@
>>>>>>      */
>>>>>>     
>>>>>>     #include <asm/current.h>
>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>     #include <asm/regs.h>
>>>>>>     #include <asm/system.h>
>>>>>>     #include <asm/traps.h>
>>>>>>     
>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>> +
>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>> +{
>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>> +}
>>>>>> +
>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>> +{
>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>> +}
>>>>>> +
>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>> +{
>>>>>> +    unsigned int bpr;
>>>>>> +
>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>> +    {
>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>> +        if ( bpr < 7 )
>>>>>> +            bpr++;
>>>>>> +    }
>>>>>> +    else
>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>> +
>>>>>> +    return bpr;
>>>>>> +}
>>>>>> +
>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>> +{
>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>> +
>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>> +}
>>>>>> +
>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>> +{
>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>> +
>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>> +        return;
>>>>>> +
>>>>>> +    /* Enforce BPR limiting */
>>>>>> +    if ( val < bpr_min )
>>>>>> +        val = bpr_min;
>>>>>> +
>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>> +    vmcr |= val;
>>>>>> +
>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>> +}
>>>>>> +
>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>> +{
>>>>>> +    if ( hsr.sysreg.read )
>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>> +    else
>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>> +}
>>>>>> +
>>>>>>     /*
>>>>>>      * returns true if the register is emulated.
>>>>>>      */
>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>     
>>>>>>         switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>         {
>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>> +         break;
>>>>> What is the rational for indirecting through a function and moving the
>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>> change much, but since this is a port of existing code, it will make
>>>>> more complex the port of potential fixes.
>>>> I used xen template of handling sysreg traps
>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>> a handle_XXX function is used throughout...
>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>> decide.
>>>
>>> More importantly, my other question still stand: most trap functions do
>>> require VMCR as an input. Why moving it to the leaf functions?
>> Same reason, I was keeping the interface same of all handle_XXX functions
>> handle_XXX(regs, hsr, ...)
>>
>> Do you want me to change both to match with your patch or it is ok?
> My preference would be to keep the code as initially written, as you're
> pointlessly changing the flow. Again, that's for the maintainers to
> comment, but you should at the very least indicate that change in the
> commit log.
I did in the cover letter

few changes are made:
...

- vreg_emulate_XXX functions are defined for emulating g0/g1 registers,
   basically these functions wrap calling of read/write functions based on
   isread in a separate function.

- read/write_gicreg is replaced by READ/WRITE_SYSREG32 which is already
   present in xen code.

> 	M.


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier]

On 27/03/18 12:07, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>> This patch is ported to xen from linux commit
>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>
>>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>>
>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>>> ---
>>>>>>>     xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>>     xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>     xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>     3 files changed, 77 insertions(+)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>      */
>>>>>>>     
>>>>>>>     #include <asm/current.h>
>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>     #include <asm/regs.h>
>>>>>>>     #include <asm/system.h>
>>>>>>>     #include <asm/traps.h>
>>>>>>>     
>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>> +
>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>> +{
>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>> +}
>>>>>>> +
>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>> +{
>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>>> +{
>>>>>>> +    unsigned int bpr;
>>>>>>> +
>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>> +    {
>>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>>> +        if ( bpr < 7 )
>>>>>>> +            bpr++;
>>>>>>> +    }
>>>>>>> +    else
>>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>>> +
>>>>>>> +    return bpr;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>> +{
>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>> +
>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>> +{
>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>> +
>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    /* Enforce BPR limiting */
>>>>>>> +    if ( val < bpr_min )
>>>>>>> +        val = bpr_min;
>>>>>>> +
>>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>>> +    vmcr |= val;
>>>>>>> +
>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>> +{
>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>> +    else
>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>> +}
>>>>>>> +
>>>>>>>     /*
>>>>>>>      * returns true if the register is emulated.
>>>>>>>      */
>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>>     
>>>>>>>         switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>         {
>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>> +         break;
>>>>>> What is the rational for indirecting through a function and moving the
>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>> change much, but since this is a port of existing code, it will make
>>>>>> more complex the port of potential fixes.
>>>>> I used xen template of handling sysreg traps
>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>> a handle_XXX function is used throughout...
>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>> decide.
>>>>
>>>> More importantly, my other question still stand: most trap functions do
>>>> require VMCR as an input. Why moving it to the leaf functions?
>>> Same reason, I was keeping the interface same of all handle_XXX functions
>>> handle_XXX(regs, hsr, ...)
>>>
>>> Do you want me to change both to match with your patch or it is ok?
>> My preference would be to keep the code as initially written, as you're
>> pointlessly changing the flow. Again, that's for the maintainers to
>> comment, but you should at the very least indicate that change in the
>> commit log.
> I did in the cover letter
which, crucially, doesn't end-up in the commit. Oh well.

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 04:41 PM, Marc Zyngier wrote:
> On 27/03/18 12:07, Manish Jaggi wrote:
>>
>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>>> This patch is ported to xen from linux commit
>>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>>
>>>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>>>
>>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>>>> ---
>>>>>>>>      xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>>>      xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>>      xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>>      3 files changed, 77 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>>       */
>>>>>>>>      
>>>>>>>>      #include <asm/current.h>
>>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>>      #include <asm/regs.h>
>>>>>>>>      #include <asm/system.h>
>>>>>>>>      #include <asm/traps.h>
>>>>>>>>      
>>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>>> +
>>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>>> +{
>>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>>> +{
>>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>>>> +{
>>>>>>>> +    unsigned int bpr;
>>>>>>>> +
>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>> +    {
>>>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>>>> +        if ( bpr < 7 )
>>>>>>>> +            bpr++;
>>>>>>>> +    }
>>>>>>>> +    else
>>>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>>>> +
>>>>>>>> +    return bpr;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>> +{
>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>> +
>>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>> +{
>>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>> +
>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    /* Enforce BPR limiting */
>>>>>>>> +    if ( val < bpr_min )
>>>>>>>> +        val = bpr_min;
>>>>>>>> +
>>>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>>>> +    vmcr |= val;
>>>>>>>> +
>>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>>> +{
>>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>>> +    else
>>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>      /*
>>>>>>>>       * returns true if the register is emulated.
>>>>>>>>       */
>>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>>>      
>>>>>>>>          switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>>          {
>>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>>> +         break;
>>>>>>> What is the rational for indirecting through a function and moving the
>>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>>> change much, but since this is a port of existing code, it will make
>>>>>>> more complex the port of potential fixes.
>>>>>> I used xen template of handling sysreg traps
>>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>>> a handle_XXX function is used throughout...
>>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>>> decide.
>>>>>
>>>>> More importantly, my other question still stand: most trap functions do
>>>>> require VMCR as an input. Why moving it to the leaf functions?
>>>> Same reason, I was keeping the interface same of all handle_XXX functions
>>>> handle_XXX(regs, hsr, ...)
>>>>
>>>> Do you want me to change both to match with your patch or it is ok?
>>> My preference would be to keep the code as initially written, as you're
>>> pointlessly changing the flow. Again, that's for the maintainers to
>>> comment, but you should at the very least indicate that change in the
>>> commit log.
>> I did in the cover letter
> which, crucially, doesn't end-up in the commit. Oh well.
I am working on to address the two points, will send v3 later today.
- will remove emulate functions
- and pass vmcr as a param.

Will it be possible to review the remaining part of the code, so that I 
can address
other comments in v3 as well.

Thanks,
Manish
>
> 	M.


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier]

On 27/03/18 12:15, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 04:41 PM, Marc Zyngier wrote:
>> On 27/03/18 12:07, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>>>> This patch is ported to xen from linux commit
>>>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>>>
>>>>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>>>>> ---
>>>>>>>>>      xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>>>>      xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>>>      xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>>>      3 files changed, 77 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>>>       */
>>>>>>>>>      
>>>>>>>>>      #include <asm/current.h>
>>>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>>>      #include <asm/regs.h>
>>>>>>>>>      #include <asm/system.h>
>>>>>>>>>      #include <asm/traps.h>
>>>>>>>>>      
>>>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>>>> +
>>>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>>>> +{
>>>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>>>> +{
>>>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>>>>> +{
>>>>>>>>> +    unsigned int bpr;
>>>>>>>>> +
>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>> +    {
>>>>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>>>>> +        if ( bpr < 7 )
>>>>>>>>> +            bpr++;
>>>>>>>>> +    }
>>>>>>>>> +    else
>>>>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>>>>> +
>>>>>>>>> +    return bpr;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>> +{
>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>> +
>>>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>> +{
>>>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>> +
>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>> +        return;
>>>>>>>>> +
>>>>>>>>> +    /* Enforce BPR limiting */
>>>>>>>>> +    if ( val < bpr_min )
>>>>>>>>> +        val = bpr_min;
>>>>>>>>> +
>>>>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>>>>> +    vmcr |= val;
>>>>>>>>> +
>>>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>>>> +{
>>>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>> +    else
>>>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>      /*
>>>>>>>>>       * returns true if the register is emulated.
>>>>>>>>>       */
>>>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>>>>      
>>>>>>>>>          switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>>>          {
>>>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>>>> +         break;
>>>>>>>> What is the rational for indirecting through a function and moving the
>>>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>>>> change much, but since this is a port of existing code, it will make
>>>>>>>> more complex the port of potential fixes.
>>>>>>> I used xen template of handling sysreg traps
>>>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>>>> a handle_XXX function is used throughout...
>>>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>>>> decide.
>>>>>>
>>>>>> More importantly, my other question still stand: most trap functions do
>>>>>> require VMCR as an input. Why moving it to the leaf functions?
>>>>> Same reason, I was keeping the interface same of all handle_XXX functions
>>>>> handle_XXX(regs, hsr, ...)
>>>>>
>>>>> Do you want me to change both to match with your patch or it is ok?
>>>> My preference would be to keep the code as initially written, as you're
>>>> pointlessly changing the flow. Again, that's for the maintainers to
>>>> comment, but you should at the very least indicate that change in the
>>>> commit log.
>>> I did in the cover letter
>> which, crucially, doesn't end-up in the commit. Oh well.
> I am working on to address the two points, will send v3 later today.
> - will remove emulate functions
> - and pass vmcr as a param.
> 
> Will it be possible to review the remaining part of the code, so that I 
> can address
> other comments in v3 as well.

I suggest you wait until some other folks have a chance to properly
review the series. You only posted the stuff this morning, give them a
chance. A week between two versions is probably the right timing.

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 04:55 PM, Marc Zyngier wrote:
> On 27/03/18 12:15, Manish Jaggi wrote:
>>
>> On 03/27/2018 04:41 PM, Marc Zyngier wrote:
>>> On 27/03/18 12:07, Manish Jaggi wrote:
>>>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>>>>> This patch is ported to xen from linux commit
>>>>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>>>>
>>>>>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>>>>>> ---
>>>>>>>>>>       xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>>>>>       xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>>>>       xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>>>>       3 files changed, 77 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>>>>        */
>>>>>>>>>>       
>>>>>>>>>>       #include <asm/current.h>
>>>>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>>>>       #include <asm/regs.h>
>>>>>>>>>>       #include <asm/system.h>
>>>>>>>>>>       #include <asm/traps.h>
>>>>>>>>>>       
>>>>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>>>>> +
>>>>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>>>>> +{
>>>>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>>>>> +{
>>>>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>>>>>> +{
>>>>>>>>>> +    unsigned int bpr;
>>>>>>>>>> +
>>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>>> +    {
>>>>>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>>>>>> +        if ( bpr < 7 )
>>>>>>>>>> +            bpr++;
>>>>>>>>>> +    }
>>>>>>>>>> +    else
>>>>>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>>>>>> +
>>>>>>>>>> +    return bpr;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>>> +{
>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>> +
>>>>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>>> +{
>>>>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>> +
>>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>>> +        return;
>>>>>>>>>> +
>>>>>>>>>> +    /* Enforce BPR limiting */
>>>>>>>>>> +    if ( val < bpr_min )
>>>>>>>>>> +        val = bpr_min;
>>>>>>>>>> +
>>>>>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>>>>>> +    vmcr |= val;
>>>>>>>>>> +
>>>>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>>>>> +{
>>>>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>> +    else
>>>>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>       /*
>>>>>>>>>>        * returns true if the register is emulated.
>>>>>>>>>>        */
>>>>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>>>>>       
>>>>>>>>>>           switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>>>>           {
>>>>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>>>>> +         break;
>>>>>>>>> What is the rational for indirecting through a function and moving the
>>>>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>>>>> change much, but since this is a port of existing code, it will make
>>>>>>>>> more complex the port of potential fixes.
>>>>>>>> I used xen template of handling sysreg traps
>>>>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>>>>> a handle_XXX function is used throughout...
>>>>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>>>>> decide.
>>>>>>>
>>>>>>> More importantly, my other question still stand: most trap functions do
>>>>>>> require VMCR as an input. Why moving it to the leaf functions?
>>>>>> Same reason, I was keeping the interface same of all handle_XXX functions
>>>>>> handle_XXX(regs, hsr, ...)
>>>>>>
>>>>>> Do you want me to change both to match with your patch or it is ok?
>>>>> My preference would be to keep the code as initially written, as you're
>>>>> pointlessly changing the flow. Again, that's for the maintainers to
>>>>> comment, but you should at the very least indicate that change in the
>>>>> commit log.
>>>> I did in the cover letter
>>> which, crucially, doesn't end-up in the commit. Oh well.
>> I am working on to address the two points, will send v3 later today.
>> - will remove emulate functions
>> - and pass vmcr as a param.
>>
>> Will it be possible to review the remaining part of the code, so that I
>> can address
>> other comments in v3 as well.
> I suggest you wait until some other folks have a chance to properly
> review the series. You only posted the stuff this morning, give them a
> chance. A week between two versions is probably the right timing.
Xen 4.11 window closes this week, so I have only few days.
I am hoping to get your ack before that.
> 	M.


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Marc Zyngier]

On 27/03/18 12:27, Manish Jaggi wrote:
> 
> 
> On 03/27/2018 04:55 PM, Marc Zyngier wrote:
>> On 27/03/18 12:15, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 04:41 PM, Marc Zyngier wrote:
>>>> On 27/03/18 12:07, Manish Jaggi wrote:
>>>>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>>>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>>>>>> This patch is ported to xen from linux commit
>>>>>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>>>>>
>>>>>>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>>>>>       xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>>>>>       3 files changed, 77 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>>>>>        */
>>>>>>>>>>>       
>>>>>>>>>>>       #include <asm/current.h>
>>>>>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>>>>>       #include <asm/regs.h>
>>>>>>>>>>>       #include <asm/system.h>
>>>>>>>>>>>       #include <asm/traps.h>
>>>>>>>>>>>       
>>>>>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>>>>>> +
>>>>>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>>>>>> +{
>>>>>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>>>>>> +{
>>>>>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>>>>>>> +{
>>>>>>>>>>> +    unsigned int bpr;
>>>>>>>>>>> +
>>>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>>>> +    {
>>>>>>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>>>>>>> +        if ( bpr < 7 )
>>>>>>>>>>> +            bpr++;
>>>>>>>>>>> +    }
>>>>>>>>>>> +    else
>>>>>>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>>>>>>> +
>>>>>>>>>>> +    return bpr;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>>>> +{
>>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>>> +
>>>>>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>>>> +{
>>>>>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>>> +
>>>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>>>> +        return;
>>>>>>>>>>> +
>>>>>>>>>>> +    /* Enforce BPR limiting */
>>>>>>>>>>> +    if ( val < bpr_min )
>>>>>>>>>>> +        val = bpr_min;
>>>>>>>>>>> +
>>>>>>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>>>>>>> +    vmcr |= val;
>>>>>>>>>>> +
>>>>>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>>>>>> +{
>>>>>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>>> +    else
>>>>>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>>       /*
>>>>>>>>>>>        * returns true if the register is emulated.
>>>>>>>>>>>        */
>>>>>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>>>>>>       
>>>>>>>>>>>           switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>>>>>           {
>>>>>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>>>>>> +         break;
>>>>>>>>>> What is the rational for indirecting through a function and moving the
>>>>>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>>>>>> change much, but since this is a port of existing code, it will make
>>>>>>>>>> more complex the port of potential fixes.
>>>>>>>>> I used xen template of handling sysreg traps
>>>>>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>>>>>> a handle_XXX function is used throughout...
>>>>>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>>>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>>>>>> decide.
>>>>>>>>
>>>>>>>> More importantly, my other question still stand: most trap functions do
>>>>>>>> require VMCR as an input. Why moving it to the leaf functions?
>>>>>>> Same reason, I was keeping the interface same of all handle_XXX functions
>>>>>>> handle_XXX(regs, hsr, ...)
>>>>>>>
>>>>>>> Do you want me to change both to match with your patch or it is ok?
>>>>>> My preference would be to keep the code as initially written, as you're
>>>>>> pointlessly changing the flow. Again, that's for the maintainers to
>>>>>> comment, but you should at the very least indicate that change in the
>>>>>> commit log.
>>>>> I did in the cover letter
>>>> which, crucially, doesn't end-up in the commit. Oh well.
>>> I am working on to address the two points, will send v3 later today.
>>> - will remove emulate functions
>>> - and pass vmcr as a param.
>>>
>>> Will it be possible to review the remaining part of the code, so that I
>>> can address
>>> other comments in v3 as well.
>> I suggest you wait until some other folks have a chance to properly
>> review the series. You only posted the stuff this morning, give them a
>> chance. A week between two versions is probably the right timing.
> Xen 4.11 window closes this week, so I have only few days.
> I am hoping to get your ack before that.
My Ack is not the maintainers' (which you'd need anyway), and your
series is not the only one I need to review. I don't plan on having
another look at it this week anyway.

As for the Xen deadline, I'm sure there will be another one.

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 05:08 PM, Marc Zyngier wrote:
> On 27/03/18 12:27, Manish Jaggi wrote:
>>
>> On 03/27/2018 04:55 PM, Marc Zyngier wrote:
>>> On 27/03/18 12:15, Manish Jaggi wrote:
>>>> On 03/27/2018 04:41 PM, Marc Zyngier wrote:
>>>>> On 27/03/18 12:07, Manish Jaggi wrote:
>>>>>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>>>>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>>>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>>>>>>>> This patch is ported to xen from linux commit
>>>>>>>>>>>> d70c7b31a60f2458f35c226131f2a01a7a98b6cf
>>>>>>>>>>>> KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler
>>>>>>>>>>>>
>>>>>>>>>>>> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
>>>>>>>>>>>> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>        xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>        xen/include/asm-arm/arm64/sysregs.h |  1 +
>>>>>>>>>>>>        xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
>>>>>>>>>>>>        3 files changed, 77 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>>> index 39ab1ed6ca..ed4254acf9 100644
>>>>>>>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>>>>>>>> @@ -18,10 +18,76 @@
>>>>>>>>>>>>         */
>>>>>>>>>>>>        
>>>>>>>>>>>>        #include <asm/current.h>
>>>>>>>>>>>> +#include <asm/gic_v3_defs.h>
>>>>>>>>>>>>        #include <asm/regs.h>
>>>>>>>>>>>>        #include <asm/system.h>
>>>>>>>>>>>>        #include <asm/traps.h>
>>>>>>>>>>>>        
>>>>>>>>>>>> +#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
>>>>>>>>>>>> +
>>>>>>>>>>>> +static int vgic_v3_bpr_min(void)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    /* See Pseudocode for VPriorityGroup */
>>>>>>>>>>>> +    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    unsigned int bpr;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>>>>> +    {
>>>>>>>>>>>> +        bpr = vgic_v3_get_bpr0(vmcr);
>>>>>>>>>>>> +        if ( bpr < 7 )
>>>>>>>>>>>> +            bpr++;
>>>>>>>>>>>> +    }
>>>>>>>>>>>> +    else
>>>>>>>>>>>> +        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    return bpr;
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    register_t val = get_user_reg(regs, regidx);
>>>>>>>>>>>> +    uint8_t bpr_min = vgic_v3_bpr_min();
>>>>>>>>>>>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if ( vmcr & ICH_VMCR_CBPR_MASK )
>>>>>>>>>>>> +        return;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    /* Enforce BPR limiting */
>>>>>>>>>>>> +    if ( val < bpr_min )
>>>>>>>>>>>> +        val = bpr_min;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    val <<= ICH_VMCR_BPR1_SHIFT;
>>>>>>>>>>>> +    val &= ICH_VMCR_BPR1_MASK;
>>>>>>>>>>>> +    vmcr &= ~ICH_VMCR_BPR1_MASK;
>>>>>>>>>>>> +    vmcr |= val;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    if ( hsr.sysreg.read )
>>>>>>>>>>>> +        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>>>> +    else
>>>>>>>>>>>> +        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>>        /*
>>>>>>>>>>>>         * returns true if the register is emulated.
>>>>>>>>>>>>         */
>>>>>>>>>>>> @@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>>>>>>>>>>>        
>>>>>>>>>>>>            switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>>>>>>>>>>>>            {
>>>>>>>>>>>> +    case HSR_SYSREG_ICC_BPR1_EL1:
>>>>>>>>>>>> +         vreg_emulate_bpr1(regs, hsr);
>>>>>>>>>>>> +         break;
>>>>>>>>>>> What is the rational for indirecting through a function and moving the
>>>>>>>>>>> reading of VMCR to the leaf functions? I appreciate that this doesn't
>>>>>>>>>>> change much, but since this is a port of existing code, it will make
>>>>>>>>>>> more complex the port of potential fixes.
>>>>>>>>>> I used xen template of handling sysreg traps
>>>>>>>>>> If you see the file xen/arch/arm/arm64/sysreg.c
>>>>>>>>>> a handle_XXX function is used throughout...
>>>>>>>>> Sure, but this is not sysreg.c. This is a separate file for a reason
>>>>>>>>> (i.e. it is imported code). Anyway, that's for the Xen maintainers to
>>>>>>>>> decide.
>>>>>>>>>
>>>>>>>>> More importantly, my other question still stand: most trap functions do
>>>>>>>>> require VMCR as an input. Why moving it to the leaf functions?
>>>>>>>> Same reason, I was keeping the interface same of all handle_XXX functions
>>>>>>>> handle_XXX(regs, hsr, ...)
>>>>>>>>
>>>>>>>> Do you want me to change both to match with your patch or it is ok?
>>>>>>> My preference would be to keep the code as initially written, as you're
>>>>>>> pointlessly changing the flow. Again, that's for the maintainers to
>>>>>>> comment, but you should at the very least indicate that change in the
>>>>>>> commit log.
>>>>>> I did in the cover letter
>>>>> which, crucially, doesn't end-up in the commit. Oh well.
>>>> I am working on to address the two points, will send v3 later today.
>>>> - will remove emulate functions
>>>> - and pass vmcr as a param.
>>>>
>>>> Will it be possible to review the remaining part of the code, so that I
>>>> can address
>>>> other comments in v3 as well.
>>> I suggest you wait until some other folks have a chance to properly
>>> review the series. You only posted the stuff this morning, give them a
>>> chance. A week between two versions is probably the right timing.
>> Xen 4.11 window closes this week, so I have only few days.
>> I am hoping to get your ack before that.
> My Ack is not the maintainers' (which you'd need anyway), and your
> series is not the only one I need to review. I don't plan on having
> another look at it this week anyway.
>
> As for the Xen deadline, I'm sure there will be another one.
Hi Julien,
Is it prudent to send v3 with the two changes of marc suggested in this 
mail ?
Since this is a errata workaround, and hopefully it should make it to 4.11.
I have tried to address the v1 comments in v2, could you please have a 
look at it.

Thanks
manish
>
> 	M.


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

Re: [PATCH v2 02/17] arm64: vgic-v3: Add ICV_BPR1_EL1 handler

[Julien Grall]

Hi,

On 28/03/18 04:51, Manish Jaggi wrote:
> On 03/27/2018 05:08 PM, Marc Zyngier wrote:
>> On 27/03/18 12:27, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 04:55 PM, Marc Zyngier wrote:
>>>> On 27/03/18 12:15, Manish Jaggi wrote:
>>>>> On 03/27/2018 04:41 PM, Marc Zyngier wrote:
>>>>>> On 27/03/18 12:07, Manish Jaggi wrote:
>>>>>>> On 03/27/2018 04:35 PM, Marc Zyngier wrote:
>>>>>>>> On 27/03/18 11:56, Manish Jaggi wrote:
>>>>>>>>> On 03/27/2018 04:15 PM, Marc Zyngier wrote:
>>>>>>>>>> On 27/03/18 11:35, Manish Jaggi wrote:
>>>>>>>>>>> On 03/27/2018 04:00 PM, Marc Zyngier wrote:
>>>>>>>>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>> Will it be possible to review the remaining part of the code, so 
>>>>> that I
>>>>> can address
>>>>> other comments in v3 as well.
>>>> I suggest you wait until some other folks have a chance to properly
>>>> review the series. You only posted the stuff this morning, give them a
>>>> chance. A week between two versions is probably the right timing.
>>> Xen 4.11 window closes this week, so I have only few days.

I agree with Marc on giving a week between each version. 
Maintainers/reviewers have other duty and you can't expect them to 
review your series promptly. There are no difference with merge window 
closing.

Instead you should plan well in advance if you want to get your code in 
a specific release.

>>> I am hoping to get your ack before that.
>> My Ack is not the maintainers' (which you'd need anyway), and your
>> series is not the only one I need to review. I don't plan on having
>> another look at it this week anyway.
>>
>> As for the Xen deadline, I'm sure there will be another one.
> Hi Julien,
> Is it prudent to send v3 with the two changes of marc suggested in this 
> mail ?
> Since this is a errata workaround, and hopefully it should make it to 4.11.
> I have tried to address the v1 comments in v2, could you please have a 
> look at it.

You were probably aware from your management that I was traveling for 
the past 2 weeks. So the cut-off for any patches I needed to review for 
Xen 4.11 was right before my holidays (the week before easter).

Even if the code freeze was moved to end of this week, I am unlikely 
going to have time to fully review the series by then. You can still 
resend version while Xen is in code freeze, so this could be merge as 
soon as the tree re-open.

Cheers,

-- 
Julien Grall

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

[PATCH v2 03/17] arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler

[Manish Jaggi]

This patch is ported to xen from linux commit:
f8b630bc542e0368886ae193d3519c832b270359
KVM: arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler

Add a handler for reading/writing the guest's view of ICC_IGRPEN1_EL1
register, which is located in the ICH_VMCR_EL2.VENG1 field.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 32 ++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 xen/include/asm-arm/gic_v3_defs.h   |  2 ++
 3 files changed, 35 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index ed4254acf9..edaa13ec1c 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -88,6 +88,34 @@ static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
         vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
 }
 
+static void vgic_v3_read_igrpen1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, !!(vmcr & ICH_VMCR_ENG1_MASK));
+}
+
+static void vgic_v3_write_igrpen1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( val & 1 )
+        vmcr |= ICH_VMCR_ENG1_MASK;
+    else
+        vmcr &= ~ICH_VMCR_ENG1_MASK;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_igrpen1(struct cpu_user_regs *regs,
+                                 const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_igrpen1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_igrpen1(regs, hsr.sysreg.reg);
+}
 /*
  * returns true if the register is emulated.
  */
@@ -110,6 +138,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
          vreg_emulate_bpr1(regs, hsr);
          break;
 
+    case HSR_SYSREG_ICC_IGRPEN1_EL1:
+        vreg_emulate_igrpen1(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 025a27b0b4..731cabc74a 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -90,6 +90,7 @@
 #define HSR_SYSREG_ICC_SGI0R_EL1  HSR_SYSREG(3,2,c12,c11,7)
 #define HSR_SYSREG_ICC_SRE_EL1    HSR_SYSREG(3,0,c12,c12,5)
 #define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
+#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 68a34cc353..ff8bda37d1 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -163,6 +163,8 @@
 #define ICH_VMCR_BPR0_MASK           (7 << ICH_VMCR_BPR0_SHIFT)
 #define ICH_VMCR_BPR1_SHIFT          18
 #define ICH_VMCR_BPR1_MASK           (7 << ICH_VMCR_BPR1_SHIFT)
+#define ICH_VMCR_ENG1_SHIFT          1
+#define ICH_VMCR_ENG1_MASK           (1 << ICH_VMCR_ENG1_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
-- 
2.14.1


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

[PATCH v2 04/17] arm64: Add accessors for the ICH_APxRn_EL2 registers

[Manish Jaggi]

This patch is ported to xen from linux commit
63000dd8006dc987db31ba670edc23142ea91e01
KVM: arm/arm64: vgic-v3: Add accessors for the ICH_APxRn_EL2 registers

As we're about to access the Active Priority registers a lot more,
let's define accessors that take the register number as a parameter.

This patch only has accessors, it does not modify the vgic code.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c | 93 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index edaa13ec1c..48346ed628 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -116,6 +116,99 @@ static void vreg_emulate_igrpen1(struct cpu_user_regs *regs,
     else
         vgic_v3_write_igrpen1(regs, hsr.sysreg.reg);
 }
+
+static void vgic_v3_write_ap0rn(uint32_t val, int n)
+{
+    switch (n)
+    {
+    case 0:
+        WRITE_SYSREG32(val, ICH_AP0R0_EL2);
+        break;
+    case 1:
+        WRITE_SYSREG32(val, ICH_AP0R1_EL2);
+        break;
+    case 2:
+        WRITE_SYSREG32(val, ICH_AP0R2_EL2);
+        break;
+    case 3:
+        WRITE_SYSREG32(val, ICH_AP0R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+}
+
+static void vgic_v3_write_ap1rn(uint32_t val, int n)
+{
+    switch (n)
+    {
+    case 0:
+        WRITE_SYSREG32(val, ICH_AP1R0_EL2);
+        break;
+    case 1:
+        WRITE_SYSREG32(val, ICH_AP1R1_EL2);
+        break;
+    case 2:
+        WRITE_SYSREG32(val, ICH_AP1R2_EL2);
+        break;
+    case 3:
+        WRITE_SYSREG32(val, ICH_AP1R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+}
+
+static uint32_t vgic_v3_read_ap0rn(int n)
+{
+    uint32_t val;
+
+    switch (n)
+    {
+    case 0:
+        val = READ_SYSREG32(ICH_AP0R0_EL2);
+        break;
+    case 1:
+        val = READ_SYSREG32(ICH_AP0R1_EL2);
+        break;
+    case 2:
+        val = READ_SYSREG32(ICH_AP0R2_EL2);
+        break;
+    case 3:
+        val = READ_SYSREG32(ICH_AP0R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+
+    return val;
+}
+
+static uint32_t vgic_v3_read_ap1rn(int n)
+{
+    uint32_t val;
+
+    switch (n)
+    {
+    case 0:
+        val = READ_SYSREG32(ICH_AP1R0_EL2);
+        break;
+    case 1:
+        val = READ_SYSREG32(ICH_AP1R1_EL2);
+        break;
+    case 2:
+        val = READ_SYSREG32(ICH_AP1R2_EL2);
+        break;
+    case 3:
+        val = READ_SYSREG32(ICH_AP1R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+
+    return val;
+}
+
 /*
  * returns true if the register is emulated.
  */
-- 
2.14.1


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

[PATCH v2 05/17] Expose ich_read/write_lr in vgic-v3-sr.c

[Manish Jaggi]

gicv3_ich_read/write_lr functions are duplicated in vgic-v3-sr.c
This is done to make the file independent of the xen vgic code for
handling the errata.

Both the functions in the patch are static, so this patch needs
subsequent patches to compile without error (unused function)

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c | 83 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index 48346ed628..fc5246539e 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -209,6 +209,89 @@ static uint32_t vgic_v3_read_ap1rn(int n)
     return val;
 }
 
+static uint64_t gicv3_ich_read_lr(int lr)
+{
+    switch ( lr )
+    {
+    case 0: return READ_SYSREG(ICH_LR0_EL2);
+    case 1: return READ_SYSREG(ICH_LR1_EL2);
+    case 2: return READ_SYSREG(ICH_LR2_EL2);
+    case 3: return READ_SYSREG(ICH_LR3_EL2);
+    case 4: return READ_SYSREG(ICH_LR4_EL2);
+    case 5: return READ_SYSREG(ICH_LR5_EL2);
+    case 6: return READ_SYSREG(ICH_LR6_EL2);
+    case 7: return READ_SYSREG(ICH_LR7_EL2);
+    case 8: return READ_SYSREG(ICH_LR8_EL2);
+    case 9: return READ_SYSREG(ICH_LR9_EL2);
+    case 10: return READ_SYSREG(ICH_LR10_EL2);
+    case 11: return READ_SYSREG(ICH_LR11_EL2);
+    case 12: return READ_SYSREG(ICH_LR12_EL2);
+    case 13: return READ_SYSREG(ICH_LR13_EL2);
+    case 14: return READ_SYSREG(ICH_LR14_EL2);
+    case 15: return READ_SYSREG(ICH_LR15_EL2);
+    default:
+        unreachable();
+    }
+}
+
+static void gicv3_ich_write_lr(int lr, uint64_t val)
+{
+    switch ( lr )
+    {
+    case 0:
+        WRITE_SYSREG(val, ICH_LR0_EL2);
+        break;
+    case 1:
+        WRITE_SYSREG(val, ICH_LR1_EL2);
+        break;
+    case 2:
+        WRITE_SYSREG(val, ICH_LR2_EL2);
+        break;
+    case 3:
+        WRITE_SYSREG(val, ICH_LR3_EL2);
+        break;
+    case 4:
+        WRITE_SYSREG(val, ICH_LR4_EL2);
+        break;
+    case 5:
+        WRITE_SYSREG(val, ICH_LR5_EL2);
+        break;
+    case 6:
+        WRITE_SYSREG(val, ICH_LR6_EL2);
+        break;
+    case 7:
+        WRITE_SYSREG(val, ICH_LR7_EL2);
+        break;
+    case 8:
+        WRITE_SYSREG(val, ICH_LR8_EL2);
+        break;
+    case 9:
+        WRITE_SYSREG(val, ICH_LR9_EL2);
+        break;
+    case 10:
+        WRITE_SYSREG(val, ICH_LR10_EL2);
+        break;
+    case 11:
+        WRITE_SYSREG(val, ICH_LR11_EL2);
+        break;
+    case 12:
+        WRITE_SYSREG(val, ICH_LR12_EL2);
+        break;
+    case 13:
+        WRITE_SYSREG(val, ICH_LR13_EL2);
+        break;
+    case 14:
+        WRITE_SYSREG(val, ICH_LR14_EL2);
+        break;
+    case 15:
+        WRITE_SYSREG(val, ICH_LR15_EL2);
+        break;
+    default:
+        return;
+    }
+    isb();
+}
+
 /*
  * returns true if the register is emulated.
  */
-- 
2.14.1


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

[PATCH v2 06/17] arm64: Add ICV_IAR1_EL1 handler

[Manish Jaggi]

This patch is ported to xen from linux commit
132a324ab62fe4fb8d6dcc2ab4eddb0e93b69afe.
KVM: arm64: vgic-v3: Add ICV_IAR1_EL1 handler

Add a handler for reading the guest's view of the ICC_IAR1_EL1
register. This involves finding the highest priority Group-1
interrupt, checking against both PMR and the active group
priority, activating the interrupt and setting the group
priority as active.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 194 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |   1 +
 xen/include/asm-arm/gic_v3_defs.h   |  17 ++++
 3 files changed, 212 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index fc5246539e..026d64506f 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -22,8 +22,17 @@
 #include <asm/regs.h>
 #include <asm/system.h>
 #include <asm/traps.h>
+#include <asm/vtimer.h>
+#include <xen/sched.h>
 
 #define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+#define vtr_to_nr_apr_regs(v)     (1 << (vtr_to_nr_pre_bits(v) - 5))
+
+#define ESR_ELx_SYS64_ISS_CRM_SHIFT 1
+#define ESR_ELx_SYS64_ISS_CRM_MASK (0xf << ESR_ELx_SYS64_ISS_CRM_SHIFT)
+
+#define ICC_IAR1_EL1_SPURIOUS    0x3ff
+#define VGIC_MAX_SPI             1019
 
 static int vgic_v3_bpr_min(void)
 {
@@ -292,6 +301,187 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
     isb();
 }
 
+static int vgic_v3_get_group(const union hsr hsr)
+{
+    uint8_t crm = (hsr.bits & ESR_ELx_SYS64_ISS_CRM_MASK) >>
+                  ESR_ELx_SYS64_ISS_CRM_SHIFT;
+
+    return crm != 8;
+}
+
+static unsigned int gic_get_num_lrs(void)
+{
+    uint32_t vtr;
+
+    vtr = READ_SYSREG32(ICH_VTR_EL2);
+    return (vtr & GICH_VTR_NRLRGS) + 1;
+}
+
+static int vgic_v3_highest_priority_lr(struct cpu_user_regs *regs,
+                                       uint32_t vmcr, uint64_t *lr_val)
+{
+    unsigned int i, lr = -1;
+    unsigned int used_lrs =  gic_get_num_lrs();
+    uint8_t priority = GICV3_IDLE_PRIORITY;
+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        uint64_t val =  gicv3_ich_read_lr(i);
+        uint8_t lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+
+        /* Not pending in the state? */
+        if ( (val & ICH_LR_STATE) != ICH_LR_PENDING_BIT )
+            continue;
+
+        /* Group-0 interrupt, but Group-0 disabled? */
+        if ( !(val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG0_MASK) )
+            continue;
+
+        /* Group-1 interrupt, but Group-1 disabled? */
+        if ( (val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG1_MASK) )
+            continue;
+
+        /* Not the highest priority? */
+        if ( lr_prio >= priority )
+            continue;
+
+        /* This is a candidate */
+        priority = lr_prio;
+        *lr_val = val;
+        lr = i;
+    }
+
+    if ( lr == -1 )
+        *lr_val = ICC_IAR1_EL1_SPURIOUS;
+
+    return lr;
+}
+
+static int vgic_v3_get_highest_active_priority(void)
+{
+    unsigned int i;
+    uint32_t hap = 0;
+    uint8_t nr_apr_regs = vtr_to_nr_apr_regs(READ_SYSREG32(ICH_VTR_EL2));
+
+    for ( i = 0; i < nr_apr_regs; i++ )
+    {
+        uint32_t val;
+
+        /*
+         * The ICH_AP0Rn_EL2 and ICH_AP1Rn_EL2 registers
+         * contain the active priority levels for this VCPU
+         * for the maximum number of supported priority
+         * levels, and we return the full priority level only
+         * if the BPR is programmed to its minimum, otherwise
+         * we return a combination of the priority level and
+         * subpriority, as determined by the setting of the
+         * BPR, but without the full subpriority.
+         */
+        val  = vgic_v3_read_ap0rn(i);
+        val |= vgic_v3_read_ap1rn(i);
+        if ( !val )
+        {
+            hap += 32;
+            continue;
+        }
+
+        return (hap + __ffs(val)) << vgic_v3_bpr_min();
+    }
+
+    return GICV3_IDLE_PRIORITY;
+}
+
+/*
+ * Convert a priority to a preemption level, taking the relevant BPR
+ * into account by zeroing the sub-priority bits.
+ */
+static uint8_t vgic_v3_pri_to_pre(uint8_t pri, uint32_t vmcr, int grp)
+{
+    unsigned int bpr;
+
+    if ( !grp )
+        bpr = vgic_v3_get_bpr0(vmcr) + 1;
+    else
+        bpr = vgic_v3_get_bpr1(vmcr);
+
+    return pri & (GENMASK(7, 0) << bpr);
+}
+
+/*
+ * The priority value is independent of any of the BPR values, so we
+ * normalize it using the minumal BPR value. This guarantees that no
+ * matter what the guest does with its BPR, we can always set/get the
+ * same value of a priority.
+ */
+static void vgic_v3_set_active_priority(uint8_t pri, uint32_t vmcr, int grp)
+{
+    uint8_t pre, ap;
+    uint32_t val;
+    int apr;
+
+    pre = vgic_v3_pri_to_pre(pri, vmcr, grp);
+    ap = pre >> vgic_v3_bpr_min();
+    apr = ap / 32;
+
+    if ( !grp )
+    {
+        val = vgic_v3_read_ap0rn(apr);
+        vgic_v3_write_ap0rn(val | BIT(ap % 32), apr);
+    }
+    else
+    {
+        val = vgic_v3_read_ap1rn(apr);
+        vgic_v3_write_ap1rn(val | BIT(ap % 32), apr);
+    }
+}
+
+static void vgic_v3_read_iar(struct cpu_user_regs *regs,
+                             const union hsr hsr)
+{
+    uint64_t lr_val;
+    uint8_t lr_prio, pmr;
+    int lr, grp;
+    int regidx = hsr.sysreg.reg;
+
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+    grp = vgic_v3_get_group(hsr);
+
+    lr = vgic_v3_highest_priority_lr(regs, vmcr, &lr_val);
+    if ( lr < 0 )
+        goto spurious;
+
+    if ( grp != !!(lr_val & ICH_LR_GROUP) )
+        goto spurious;
+
+    pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT;
+    lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+    if ( pmr <= lr_prio )
+        goto spurious;
+
+    if ( vgic_v3_get_highest_active_priority() <=
+         vgic_v3_pri_to_pre(lr_prio, vmcr, grp) )
+        goto spurious;
+
+    lr_val &= ~ICH_LR_STATE;
+    /* No active state for LPIs */
+    if ( (lr_val & ICH_LR_VIRTUAL_ID_MASK) <= VGIC_MAX_SPI )
+        lr_val |= ICH_LR_ACTIVE_BIT;
+
+    gicv3_ich_write_lr(lr, lr_val);
+    vgic_v3_set_active_priority(lr_prio, vmcr, grp);
+    set_user_reg(regs, regidx,  lr_val & ICH_LR_VIRTUAL_ID_MASK);
+
+    return;
+
+spurious:
+     set_user_reg(regs, regidx, ICC_IAR1_EL1_SPURIOUS);
+}
+
+static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    vgic_v3_read_iar(regs, hsr);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -318,6 +508,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_igrpen1(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_IAR1_EL1:
+        vreg_emulate_iar(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 731cabc74a..53d2251840 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -91,6 +91,7 @@
 #define HSR_SYSREG_ICC_SRE_EL1    HSR_SYSREG(3,0,c12,c12,5)
 #define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
 #define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
+#define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index ff8bda37d1..884fce0fd0 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -67,6 +67,7 @@
  */
 #define GICV3_GICD_IIDR_VAL          0x34c
 #define GICV3_GICR_IIDR_VAL          GICV3_GICD_IIDR_VAL
+#define GICV3_IDLE_PRIORITY          0xff
 
 #define GICR_CTLR                    (0x0000)
 #define GICR_IIDR                    (0x0004)
@@ -165,6 +166,10 @@
 #define ICH_VMCR_BPR1_MASK           (7 << ICH_VMCR_BPR1_SHIFT)
 #define ICH_VMCR_ENG1_SHIFT          1
 #define ICH_VMCR_ENG1_MASK           (1 << ICH_VMCR_ENG1_SHIFT)
+#define ICH_VMCR_ENG0_SHIFT          0
+#define ICH_VMCR_ENG0_MASK           (1 << ICH_VMCR_ENG0_SHIFT)
+#define ICH_VMCR_PMR_SHIFT           24
+#define ICH_VMCR_PMR_MASK            (0xffUL << ICH_VMCR_PMR_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
@@ -182,6 +187,18 @@
 #define GICH_LR_GRP1                 (1UL<<60)
 #define GICH_LR_HW                   (1UL<<61)
 
+#define ICH_LR_PRIORITY_SHIFT        48
+#define ICH_LR_PRIORITY_MASK         (0xffULL << ICH_LR_PRIORITY_SHIFT)
+#define ICH_LR_EOI                   (1ULL << 41)
+#define ICH_LR_GROUP                 (1ULL << 60)
+#define ICH_LR_HW                    (1ULL << 61)
+#define ICH_LR_STATE                 (3ULL << 62)
+#define ICH_LR_PENDING_BIT           (1ULL << 62)
+#define ICH_LR_ACTIVE_BIT            (1ULL << 63)
+#define ICH_LR_PHYS_ID_SHIFT         32
+#define ICH_LR_PHYS_ID_MASK          (0x3ffULL << ICH_LR_PHYS_ID_SHIFT)
+#define ICH_LR_VIRTUAL_ID_MASK       ((1ULL << 32) - 1)
+
 #define GICH_VTR_NRLRGS              0x3f
 #define GICH_VTR_PRIBITS_MASK        0x7
 #define GICH_VTR_PRIBITS_SHIFT       29
-- 
2.14.1


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

[PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Manish Jaggi]

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1
register. This involves dropping the priority of the interrupt,
and deactivating it if required (EOImode == 0).

Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 136 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |   1 +
 xen/include/asm-arm/gic_v3_defs.h   |   4 ++
 3 files changed, 141 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index 026d64506f..e32ec01f56 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -33,6 +33,7 @@
 
 #define ICC_IAR1_EL1_SPURIOUS    0x3ff
 #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
 
 static int vgic_v3_bpr_min(void)
 {
@@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr)
     vgic_v3_read_iar(regs, hsr);
 }
 
+static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();
+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        uint64_t val = gicv3_ich_read_lr(i);
+
+        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
+            (val & ICH_LR_ACTIVE_BIT) )
+        {
+            *lr_val = val;
+            return i;
+        }
+    }
+
+    *lr_val = ICC_IAR1_EL1_SPURIOUS;
+    return -1;
+}
+
+static int vgic_v3_clear_highest_active_priority(void)
+{
+    int i;
+    uint32_t hap = 0;
+    uint8_t nr_apr_regs = vtr_to_nr_apr_regs(READ_SYSREG32(ICH_VTR_EL2));
+
+    for ( i = 0; i < nr_apr_regs; i++ )
+    {
+        uint32_t ap0, ap1;
+        int c0, c1;
+
+        ap0 = vgic_v3_read_ap0rn(i);
+        ap1 = vgic_v3_read_ap1rn(i);
+        if ( !ap0 && !ap1 )
+        {
+            hap += 32;
+            continue;
+        }
+
+        c0 = ap0 ? __ffs(ap0) : 32;
+        c1 = ap1 ? __ffs(ap1) : 32;
+
+        /* Always clear the LSB, which is the highest priority */
+        if ( c0 < c1 )
+        {
+            ap0 &= ~BIT(c0);
+            vgic_v3_write_ap0rn(ap0, i);
+            hap += c0;
+        }
+        else
+        {
+            ap1 &= ~BIT(c1);
+            vgic_v3_write_ap1rn(ap1, i);
+            hap += c1;
+        }
+
+        /* Rescale to 8 bits of priority */
+        return hap << vgic_v3_bpr_min();
+    }
+
+    return GICV3_IDLE_PRIORITY;
+}
+
+static void vgic_v3_clear_active_lr(int lr, uint64_t lr_val)
+{
+    lr_val &= ~ICH_LR_ACTIVE_BIT;
+    if ( lr_val & ICH_LR_HW )
+    {
+        uint32_t pid;
+
+        pid = (lr_val & ICH_LR_PHYS_ID_MASK) >> ICH_LR_PHYS_ID_SHIFT;
+        WRITE_SYSREG32(pid, ICC_DIR_EL1);
+    }
+    gicv3_ich_write_lr(lr, lr_val);
+}
+
+static void vgic_v3_bump_eoicount(void)
+{
+    uint32_t hcr;
+
+    hcr = READ_SYSREG32(ICH_HCR_EL2);
+    hcr += 1 << ICH_HCR_EOIcount_SHIFT;
+    WRITE_SYSREG32(hcr, ICH_HCR_EL2);
+}
+
+static void vgic_v3_write_eoir(struct cpu_user_regs *regs,
+                               const union hsr hsr)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+    register_t vid = get_user_reg(regs, hsr.sysreg.reg);
+    uint64_t lr_val;
+    uint8_t lr_prio, act_prio;
+    int lr, grp;
+
+    grp = vgic_v3_get_group(hsr);
+
+    /* Drop priority in any case */
+    act_prio = vgic_v3_clear_highest_active_priority();
+
+    /* If EOIing an LPI, no deactivate to be performed */
+    if ( vid >= VGIC_MIN_LPI )
+        return;
+
+    /* EOImode == 1, nothing to be done here */
+    if ( vmcr & ICH_VMCR_EOIM_MASK )
+        return;
+
+    lr = vgic_v3_find_active_lr(vid, &lr_val);
+    if ( lr == -1 )
+    {
+        vgic_v3_bump_eoicount();
+        return;
+    }
+
+    lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+
+    /* If priorities or group do not match, the guest has fscked-up. */
+    if ( grp != !!(lr_val & ICH_LR_GROUP) ||
+         vgic_v3_pri_to_pre(lr_prio, vmcr, grp) != act_prio )
+        return;
+
+    /* Let's now perform the deactivation */
+    vgic_v3_clear_active_lr(lr, lr_val);
+}
+
+static void vreg_emulate_eoi(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    vgic_v3_write_eoir(regs, hsr);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -512,6 +644,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_iar(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_EOIR1_EL1:
+        vreg_emulate_eoi(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 53d2251840..f9110ebf9c 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -92,6 +92,7 @@
 #define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
 #define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
 #define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
+#define HSR_SYSREG_ICC_EOIR1_EL1  HSR_SYSREG(3,0,c12,c12,1)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 884fce0fd0..b169e2cb78 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -170,6 +170,10 @@
 #define ICH_VMCR_ENG0_MASK           (1 << ICH_VMCR_ENG0_SHIFT)
 #define ICH_VMCR_PMR_SHIFT           24
 #define ICH_VMCR_PMR_MASK            (0xffUL << ICH_VMCR_PMR_SHIFT)
+#define ICH_VMCR_EOIM_SHIFT          9
+#define ICH_VMCR_EOIM_MASK           (1 << ICH_VMCR_EOIM_SHIFT)
+#define ICH_HCR_EOIcount_SHIFT       27
+#define ICH_HCR_EOIcount_MASK        (0x1f << ICH_HCR_EOIcount_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
-- 
2.14.1


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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Marc Zyngier]

On 27/03/18 10:07, Manish Jaggi wrote:
> This patch is ported to xen from linux commit
> b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
> KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
> 
> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
> register. This involves dropping the priority of the interrupt,
> and deactivating it if required (EOImode == 0).
> 
> Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
> ---
>  xen/arch/arm/arm64/vgic-v3-sr.c     | 136 ++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/arm64/sysregs.h |   1 +
>  xen/include/asm-arm/gic_v3_defs.h   |   4 ++
>  3 files changed, 141 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> index 026d64506f..e32ec01f56 100644
> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> @@ -33,6 +33,7 @@
>  
>  #define ICC_IAR1_EL1_SPURIOUS    0x3ff
>  #define VGIC_MAX_SPI             1019
> +#define VGIC_MIN_LPI             8192
>  
>  static int vgic_v3_bpr_min(void)
>  {
> @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr)
>      vgic_v3_read_iar(regs, hsr);
>  }
>  
> +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
> +{
> +    int i;
> +    unsigned int used_lrs =  gic_get_num_lrs();

This is quite a departure from the existing code. KVM always allocate
LRs sequentially, and used_lrs represents the current upper bound. Here,
you seem to be looking at *all* the LRs. Is that safe? Are you
guaranteed not to have any stale state?

In any case, the change should be documented.

> +
> +    for ( i = 0; i < used_lrs; i++ )
> +    {
> +        uint64_t val = gicv3_ich_read_lr(i);
> +
> +        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
> +            (val & ICH_LR_ACTIVE_BIT) )
> +        {
> +            *lr_val = val;
> +            return i;
> +        }
> +    }
> +
> +    *lr_val = ICC_IAR1_EL1_SPURIOUS;
> +    return -1;
> +}

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 03:48 PM, Marc Zyngier wrote:
> On 27/03/18 10:07, Manish Jaggi wrote:
>> This patch is ported to xen from linux commit
>> b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
>> KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
>>
>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>> register. This involves dropping the priority of the interrupt,
>> and deactivating it if required (EOImode == 0).
>>
>> Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 136 ++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/arm64/sysregs.h |   1 +
>>   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
>>   3 files changed, 141 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>> index 026d64506f..e32ec01f56 100644
>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>> @@ -33,6 +33,7 @@
>>   
>>   #define ICC_IAR1_EL1_SPURIOUS    0x3ff
>>   #define VGIC_MAX_SPI             1019
>> +#define VGIC_MIN_LPI             8192
>>   
>>   static int vgic_v3_bpr_min(void)
>>   {
>> @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr)
>>       vgic_v3_read_iar(regs, hsr);
>>   }
>>   
>> +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
>> +{
>> +    int i;
>> +    unsigned int used_lrs =  gic_get_num_lrs();
> This is quite a departure from the existing code. KVM always allocate
> LRs sequentially, and used_lrs represents the current upper bound.
IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.

xen/arch/arm/gic.c:
gic_raise_guest_irq
     gic_find_unused_lr
> Here,
> you seem to be looking at *all* the LRs. Is that safe?
IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but that 
is static in gic.c

To do something like
+for_each_set_bit(i, lr_mask, nr_lrs)
+ {
+      u64 val = __gic_v3_get_lr(i);
+      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+     /* Not pending in the state? */
+     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
+         continue;


I need to do some jugglery to make lr_mask visible outside of 
xen/arch/arm/gic.c
The easiest would be to add an extern function, harder way would be to 
add it in gic_hw_operations

- vgic_v3_highest_priority_lr iterates is interested in used LR's which 
sre in Pending state.
- emulating IAR is done with interrupts disabled
- iterating over all the LRs and finding which ones are in Pending.


>   Are you
> guaranteed not to have any stale state?
I would request Stefano/Andre/Julien to comment here...
>
> In any case, the change should be documented.
>
>> +
>> +    for ( i = 0; i < used_lrs; i++ )
>> +    {
>> +        uint64_t val = gicv3_ich_read_lr(i);
>> +
>> +        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
>> +            (val & ICH_LR_ACTIVE_BIT) )
>> +        {
>> +            *lr_val = val;
>> +            return i;
>> +        }
>> +    }
>> +
>> +    *lr_val = ICC_IAR1_EL1_SPURIOUS;
>> +    return -1;
>> +}
> Thanks,
>
> 	M.


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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Manish Jaggi]

On 04/02/2018 04:33 PM, Manish Jaggi wrote:
>
> On 03/27/2018 03:48 PM, Marc Zyngier wrote:
>> On 27/03/18 10:07, Manish Jaggi wrote:
>>> This patch is ported to xen from linux commit
>>> b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
>>> KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
>>>
>>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>>> register. This involves dropping the priority of the interrupt,
>>> and deactivating it if required (EOImode == 0).
>>>
>>> Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
>>> ---
>>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 136 
>>> ++++++++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/arm64/sysregs.h |   1 +
>>>   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
>>>   3 files changed, 141 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c 
>>> b/xen/arch/arm/arm64/vgic-v3-sr.c
>>> index 026d64506f..e32ec01f56 100644
>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>> @@ -33,6 +33,7 @@
>>>     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
>>>   #define VGIC_MAX_SPI             1019
>>> +#define VGIC_MIN_LPI             8192
>>>     static int vgic_v3_bpr_min(void)
>>>   {
>>> @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
>>> cpu_user_regs *regs, const union hsr hsr)
>>>       vgic_v3_read_iar(regs, hsr);
>>>   }
>>>   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
>>> +{
>>> +    int i;
>>> +    unsigned int used_lrs =  gic_get_num_lrs();
>> This is quite a departure from the existing code. KVM always allocate
>> LRs sequentially, and used_lrs represents the current upper bound.
> IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.
>
> xen/arch/arm/gic.c:
> gic_raise_guest_irq
>     gic_find_unused_lr
>> Here,
>> you seem to be looking at *all* the LRs. Is that safe?
> IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but 
> that is static in gic.c
>
> To do something like
> +for_each_set_bit(i, lr_mask, nr_lrs)
> + {
> +      u64 val = __gic_v3_get_lr(i);
> +      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> 
> ICH_LR_PRIORITY_SHIFT;
> +     /* Not pending in the state? */
> +     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
> +         continue;
>
>
> I need to do some jugglery to make lr_mask visible outside of 
> xen/arch/arm/gic.c
> The easiest would be to add an extern function, harder way would be to 
> add it in gic_hw_operations
>
> - vgic_v3_highest_priority_lr iterates is interested in used LR's 
> which sre in Pending state.
> - emulating IAR is done with interrupts disabled
> - iterating over all the LRs and finding which ones are in Pending.
>
>
Just to add I was answering for using num_lrs for used_lrs, above was 
for IAR flow.
This holds the same for EOIR flow as well.

The bigger point is unless I add some jugglery to access static value 
outside gic.c
this is the only solution.

Stefano/Andre/Julien
Please suggest if there is some better way...
>>   Are you
>> guaranteed not to have any stale state?
> I would request Stefano/Andre/Julien to comment here...
>>
>> In any case, the change should be documented.
>>
>>> +
>>> +    for ( i = 0; i < used_lrs; i++ )
>>> +    {
>>> +        uint64_t val = gicv3_ich_read_lr(i);
>>> +
>>> +        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
>>> +            (val & ICH_LR_ACTIVE_BIT) )
>>> +        {
>>> +            *lr_val = val;
>>> +            return i;
>>> +        }
>>> +    }
>>> +
>>> +    *lr_val = ICC_IAR1_EL1_SPURIOUS;
>>> +    return -1;
>>> +}
>> Thanks,
>>
>>     M.
>


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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Julien Grall]

Hi,

On 02/04/18 12:17, Manish Jaggi wrote:
> 
> 
> On 04/02/2018 04:33 PM, Manish Jaggi wrote:
>>
>> On 03/27/2018 03:48 PM, Marc Zyngier wrote:
>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>> This patch is ported to xen from linux commit
>>>> b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
>>>> KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
>>>>
>>>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>>>> register. This involves dropping the priority of the interrupt,
>>>> and deactivating it if required (EOImode == 0).
>>>>
>>>> Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
>>>> ---
>>>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 136 
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   xen/include/asm-arm/arm64/sysregs.h |   1 +
>>>>   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
>>>>   3 files changed, 141 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c 
>>>> b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>> index 026d64506f..e32ec01f56 100644
>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>> @@ -33,6 +33,7 @@
>>>>     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
>>>>   #define VGIC_MAX_SPI             1019
>>>> +#define VGIC_MIN_LPI             8192
>>>>     static int vgic_v3_bpr_min(void)
>>>>   {
>>>> @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
>>>> cpu_user_regs *regs, const union hsr hsr)
>>>>       vgic_v3_read_iar(regs, hsr);
>>>>   }
>>>>   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
>>>> +{
>>>> +    int i;
>>>> +    unsigned int used_lrs =  gic_get_num_lrs();
>>> This is quite a departure from the existing code. KVM always allocate
>>> LRs sequentially, and used_lrs represents the current upper bound.
>> IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.
>>
>> xen/arch/arm/gic.c:
>> gic_raise_guest_irq
>>     gic_find_unused_lr
>>> Here,
>>> you seem to be looking at *all* the LRs. Is that safe?
>> IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but 
>> that is static in gic.c
>>
>> To do something like
>> +for_each_set_bit(i, lr_mask, nr_lrs)
>> + {
>> +      u64 val = __gic_v3_get_lr(i);
>> +      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> 
>> ICH_LR_PRIORITY_SHIFT;
>> +     /* Not pending in the state? */
>> +     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
>> +         continue;
>>
>>
>> I need to do some jugglery to make lr_mask visible outside of 
>> xen/arch/arm/gic.c
>> The easiest would be to add an extern function, harder way would be to 
>> add it in gic_hw_operations
>>
>> - vgic_v3_highest_priority_lr iterates is interested in used LR's 
>> which sre in Pending state.
>> - emulating IAR is done with interrupts disabled
>> - iterating over all the LRs and finding which ones are in Pending.
>>
>>
> Just to add I was answering for using num_lrs for used_lrs, above was 
> for IAR flow.
> This holds the same for EOIR flow as well.
> 
> The bigger point is unless I add some jugglery to access static value 
> outside gic.c
> this is the only solution.
> 
> Stefano/Andre/Julien
> Please suggest if there is some better way...

lr_mask is already exported. So I am not sure what you need here.

However, despite the fact the variable is living in gic.c it is only 
used by the old vGIC. Newer vGIC is based on KVM, so used_lrs would be 
fine to use.

For the old vGIC, see below.

>>>   Are you
>>> guaranteed not to have any stale state?

LRs are zeroed when unused and AFAICT they should always be accurate. 
Stefano can you confirm it?

So it would be ok go through all the LRs (thought with a performance hit).

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4170 bytes --]

On Thu, 5 Apr 2018, Julien Grall wrote:
> On 02/04/18 12:17, Manish Jaggi wrote:
> > 
> > 
> > On 04/02/2018 04:33 PM, Manish Jaggi wrote:
> > > 
> > > On 03/27/2018 03:48 PM, Marc Zyngier wrote:
> > > > On 27/03/18 10:07, Manish Jaggi wrote:
> > > > > This patch is ported to xen from linux commit
> > > > > b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
> > > > > KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
> > > > > 
> > > > > Add a handler for writing the guest's view of the ICC_EOIR1_EL1
> > > > > register. This involves dropping the priority of the interrupt,
> > > > > and deactivating it if required (EOImode == 0).
> > > > > 
> > > > > Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
> > > > > ---
> > > > >   xen/arch/arm/arm64/vgic-v3-sr.c     | 136
> > > > > ++++++++++++++++++++++++++++++++++++
> > > > >   xen/include/asm-arm/arm64/sysregs.h |   1 +
> > > > >   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
> > > > >   3 files changed, 141 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c
> > > > > b/xen/arch/arm/arm64/vgic-v3-sr.c
> > > > > index 026d64506f..e32ec01f56 100644
> > > > > --- a/xen/arch/arm/arm64/vgic-v3-sr.c
> > > > > +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> > > > > @@ -33,6 +33,7 @@
> > > > >     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
> > > > >   #define VGIC_MAX_SPI             1019
> > > > > +#define VGIC_MIN_LPI             8192
> > > > >     static int vgic_v3_bpr_min(void)
> > > > >   {
> > > > > @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct
> > > > > cpu_user_regs *regs, const union hsr hsr)
> > > > >       vgic_v3_read_iar(regs, hsr);
> > > > >   }
> > > > >   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
> > > > > +{
> > > > > +    int i;
> > > > > +    unsigned int used_lrs =  gic_get_num_lrs();
> > > > This is quite a departure from the existing code. KVM always allocate
> > > > LRs sequentially, and used_lrs represents the current upper bound.
> > > IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.
> > > 
> > > xen/arch/arm/gic.c:
> > > gic_raise_guest_irq
> > >     gic_find_unused_lr
> > > > Here,
> > > > you seem to be looking at *all* the LRs. Is that safe?
> > > IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but that
> > > is static in gic.c
> > > 
> > > To do something like
> > > +for_each_set_bit(i, lr_mask, nr_lrs)
> > > + {
> > > +      u64 val = __gic_v3_get_lr(i);
> > > +      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
> > > +     /* Not pending in the state? */
> > > +     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
> > > +         continue;
> > > 
> > > 
> > > I need to do some jugglery to make lr_mask visible outside of
> > > xen/arch/arm/gic.c
> > > The easiest would be to add an extern function, harder way would be to add
> > > it in gic_hw_operations
> > > 
> > > - vgic_v3_highest_priority_lr iterates is interested in used LR's which
> > > sre in Pending state.
> > > - emulating IAR is done with interrupts disabled
> > > - iterating over all the LRs and finding which ones are in Pending.
> > > 
> > > 
> > Just to add I was answering for using num_lrs for used_lrs, above was for
> > IAR flow.
> > This holds the same for EOIR flow as well.
> > 
> > The bigger point is unless I add some jugglery to access static value
> > outside gic.c
> > this is the only solution.
> > 
> > Stefano/Andre/Julien
> > Please suggest if there is some better way...
> 
> lr_mask is already exported. So I am not sure what you need here.
> 
> However, despite the fact the variable is living in gic.c it is only used by
> the old vGIC. Newer vGIC is based on KVM, so used_lrs would be fine to use.
> 
> For the old vGIC, see below.
> 
> > > >   Are you
> > > > guaranteed not to have any stale state?
> 
> LRs are zeroed when unused and AFAICT they should always be accurate. Stefano
> can you confirm it?
> 
> So it would be ok go through all the LRs (thought with a performance hit).

Yes, in the old vgic LRs are always accurate (obvioulsy when read on the
right pcpu).

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Manish Jaggi]

On 04/05/2018 03:10 PM, Julien Grall wrote:
> Hi,
>
> On 02/04/18 12:17, Manish Jaggi wrote:
>>
>>
>> On 04/02/2018 04:33 PM, Manish Jaggi wrote:
>>>
>>> On 03/27/2018 03:48 PM, Marc Zyngier wrote:
>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>> This patch is ported to xen from linux commit
>>>>> b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
>>>>> KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
>>>>>
>>>>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>>>>> register. This involves dropping the priority of the interrupt,
>>>>> and deactivating it if required (EOImode == 0).
>>>>>
>>>>> Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
>>>>> ---
>>>>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 136 
>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>   xen/include/asm-arm/arm64/sysregs.h |   1 +
>>>>>   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
>>>>>   3 files changed, 141 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c 
>>>>> b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>> index 026d64506f..e32ec01f56 100644
>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>> @@ -33,6 +33,7 @@
>>>>>     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
>>>>>   #define VGIC_MAX_SPI             1019
>>>>> +#define VGIC_MIN_LPI             8192
>>>>>     static int vgic_v3_bpr_min(void)
>>>>>   {
>>>>> @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
>>>>> cpu_user_regs *regs, const union hsr hsr)
>>>>>       vgic_v3_read_iar(regs, hsr);
>>>>>   }
>>>>>   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
>>>>> +{
>>>>> +    int i;
>>>>> +    unsigned int used_lrs =  gic_get_num_lrs();
>>>> This is quite a departure from the existing code. KVM always allocate
>>>> LRs sequentially, and used_lrs represents the current upper bound.
>>> IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.
>>>
>>> xen/arch/arm/gic.c:
>>> gic_raise_guest_irq
>>>     gic_find_unused_lr
>>>> Here,
>>>> you seem to be looking at *all* the LRs. Is that safe?
>>> IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but 
>>> that is static in gic.c
>>>
>>> To do something like
>>> +for_each_set_bit(i, lr_mask, nr_lrs)
>>> + {
>>> +      u64 val = __gic_v3_get_lr(i);
>>> +      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> 
>>> ICH_LR_PRIORITY_SHIFT;
>>> +     /* Not pending in the state? */
>>> +     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
>>> +         continue;
>>>
>>>
>>> I need to do some jugglery to make lr_mask visible outside of 
>>> xen/arch/arm/gic.c
>>> The easiest would be to add an extern function, harder way would be 
>>> to add it in gic_hw_operations
>>>
>>> - vgic_v3_highest_priority_lr iterates is interested in used LR's 
>>> which sre in Pending state.
>>> - emulating IAR is done with interrupts disabled
>>> - iterating over all the LRs and finding which ones are in Pending.
>>>
>>>
>> Just to add I was answering for using num_lrs for used_lrs, above was 
>> for IAR flow.
>> This holds the same for EOIR flow as well.
>>
>> The bigger point is unless I add some jugglery to access static value 
>> outside gic.c
>> this is the only solution.
>>
>> Stefano/Andre/Julien
>> Please suggest if there is some better way...
>
> lr_mask is already exported. So I am not sure what you need here.
>
> However, despite the fact the variable is living in gic.c it is only 
> used by the old vGIC. Newer vGIC is based on KVM, so used_lrs would be 
> fine to use.
>
So I should send the next version based on old vGIC or new vGIC ?
> For the old vGIC, see below.
>
>>>>   Are you
>>>> guaranteed not to have any stale state?
>
> LRs are zeroed when unused and AFAICT they should always be accurate. 
> Stefano can you confirm it?
>
> So it would be ok go through all the LRs (thought with a performance 
> hit).
>
> Cheers,
>


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

Re: [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Julien Grall]

Hi,

On 04/06/2018 09:37 AM, Manish Jaggi wrote:
> 
> 
> On 04/05/2018 03:10 PM, Julien Grall wrote:
>> Hi,
>>
>> On 02/04/18 12:17, Manish Jaggi wrote:
>>>
>>>
>>> On 04/02/2018 04:33 PM, Manish Jaggi wrote:
>>>>
>>>> On 03/27/2018 03:48 PM, Marc Zyngier wrote:
>>>>> On 27/03/18 10:07, Manish Jaggi wrote:
>>>>>> This patch is ported to xen from linux commit
>>>>>> b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
>>>>>> KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
>>>>>>
>>>>>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>>>>>> register. This involves dropping the priority of the interrupt,
>>>>>> and deactivating it if required (EOImode == 0).
>>>>>>
>>>>>> Signed-off-by : Manish Jaggi <manish.jaggi@cavium.com>
>>>>>> ---
>>>>>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 136 
>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>   xen/include/asm-arm/arm64/sysregs.h |   1 +
>>>>>>   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
>>>>>>   3 files changed, 141 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c 
>>>>>> b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>> index 026d64506f..e32ec01f56 100644
>>>>>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>>>>>> @@ -33,6 +33,7 @@
>>>>>>     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
>>>>>>   #define VGIC_MAX_SPI             1019
>>>>>> +#define VGIC_MIN_LPI             8192
>>>>>>     static int vgic_v3_bpr_min(void)
>>>>>>   {
>>>>>> @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
>>>>>> cpu_user_regs *regs, const union hsr hsr)
>>>>>>       vgic_v3_read_iar(regs, hsr);
>>>>>>   }
>>>>>>   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
>>>>>> +{
>>>>>> +    int i;
>>>>>> +    unsigned int used_lrs =  gic_get_num_lrs();
>>>>> This is quite a departure from the existing code. KVM always allocate
>>>>> LRs sequentially, and used_lrs represents the current upper bound.
>>>> IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.
>>>>
>>>> xen/arch/arm/gic.c:
>>>> gic_raise_guest_irq
>>>>     gic_find_unused_lr
>>>>> Here,
>>>>> you seem to be looking at *all* the LRs. Is that safe?
>>>> IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but 
>>>> that is static in gic.c
>>>>
>>>> To do something like
>>>> +for_each_set_bit(i, lr_mask, nr_lrs)
>>>> + {
>>>> +      u64 val = __gic_v3_get_lr(i);
>>>> +      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> 
>>>> ICH_LR_PRIORITY_SHIFT;
>>>> +     /* Not pending in the state? */
>>>> +     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
>>>> +         continue;
>>>>
>>>>
>>>> I need to do some jugglery to make lr_mask visible outside of 
>>>> xen/arch/arm/gic.c
>>>> The easiest would be to add an extern function, harder way would be 
>>>> to add it in gic_hw_operations
>>>>
>>>> - vgic_v3_highest_priority_lr iterates is interested in used LR's 
>>>> which sre in Pending state.
>>>> - emulating IAR is done with interrupts disabled
>>>> - iterating over all the LRs and finding which ones are in Pending.
>>>>
>>>>
>>> Just to add I was answering for using num_lrs for used_lrs, above was 
>>> for IAR flow.
>>> This holds the same for EOIR flow as well.
>>>
>>> The bigger point is unless I add some jugglery to access static value 
>>> outside gic.c
>>> this is the only solution.
>>>
>>> Stefano/Andre/Julien
>>> Please suggest if there is some better way...
>>
>> lr_mask is already exported. So I am not sure what you need here.
>>
>> However, despite the fact the variable is living in gic.c it is only 
>> used by the old vGIC. Newer vGIC is based on KVM, so used_lrs would be 
>> fine to use.
>>
> So I should send the next version based on old vGIC or new vGIC ?

The new vGIC has been merged for Xen 4.11 and will cohabite with the old 
vGIC for a couple of release for hardening. So you want to add support 
for both.

Note that the new vGIC does not yet have support for GICv3, but it 
should not be a big issue for keeping the code around. I think it can 
easily be reviewed.

Cheers,

-- 
Julien Grall

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

[PATCH v2 08/17] arm64: vgic-v3: Add ICV_AP1Rn_EL1 handler

[Manish Jaggi]

This patch is a xen port of linux commit
f9e7449c780f688bf61a13dfa8c344afeb4ad6e0
KVM: arm64: vgic-v3: Add ICV_AP1Rn_EL1 handler

Add a handler for reading/writing the guest's view of the ICV_AP1Rn_EL1
registers. We just map them to the corresponding ICH_AP1Rn_EL2
registers.

This patch calls vreg_emulate_apxrN which has a if (hsr.sysreg.read)
and based on that calls read and write functions. This code placement
is slight different from linux code, which calls read/write functions
from within switch case.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 76 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 2 files changed, 77 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index e32ec01f56..c67e7c6ada 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -614,6 +614,66 @@ static void vreg_emulate_eoi(struct cpu_user_regs *regs, const union hsr hsr)
     vgic_v3_write_eoir(regs, hsr);
 }
 
+static void vgic_v3_read_apxrn(struct cpu_user_regs *regs,
+                               const union hsr hsr, int n)
+{
+    uint32_t val;
+
+    if ( !vgic_v3_get_group(hsr) )
+        val = vgic_v3_read_ap0rn(n);
+    else
+        val = vgic_v3_read_ap1rn(n);
+
+   set_user_reg(regs, hsr.sysreg.reg, val);
+}
+
+static void vgic_v3_write_apxrn(struct cpu_user_regs *regs,
+                                const union hsr hsr, int n)
+{
+    uint32_t val = get_user_reg(regs, hsr.sysreg.reg);
+
+    if ( !vgic_v3_get_group(hsr) )
+        vgic_v3_write_ap0rn(val, n);
+    else
+        vgic_v3_write_ap1rn(val, n);
+}
+
+static void vreg_emulate_apxr0(struct cpu_user_regs *regs,
+                               const union hsr hsr)
+{
+    if( hsr.sysreg.read )
+        vgic_v3_read_apxrn(regs, hsr, 0);
+    else
+        vgic_v3_write_apxrn(regs, hsr, 0);
+}
+
+static void vreg_emulate_apxr1(struct cpu_user_regs *regs,
+                               const union hsr hsr)
+{
+    if( hsr.sysreg.read )
+        vgic_v3_read_apxrn(regs, hsr, 1);
+    else
+        vgic_v3_write_apxrn(regs, hsr, 1);
+}
+
+static void vreg_emulate_apxr2(struct cpu_user_regs *regs,
+                               const union hsr hsr)
+{
+    if( hsr.sysreg.read )
+        vgic_v3_read_apxrn(regs, hsr, 2);
+    else
+        vgic_v3_write_apxrn(regs, hsr, 2);
+}
+
+static void vreg_emulate_apxr3(struct cpu_user_regs *regs,
+                               const union hsr hsr)
+{
+    if( hsr.sysreg.read )
+        vgic_v3_read_apxrn(regs, hsr, 3);
+    else
+        vgic_v3_write_apxrn(regs, hsr, 3);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -648,6 +708,22 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_eoi(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_AP1Rn_EL1(0):
+        vreg_emulate_apxr0(regs, hsr);
+        break;
+
+    case HSR_SYSREG_ICC_AP1Rn_EL1(1):
+        vreg_emulate_apxr1(regs, hsr);
+        break;
+
+    case HSR_SYSREG_ICC_AP1Rn_EL1(2):
+        vreg_emulate_apxr2(regs, hsr);
+        break;
+
+    case HSR_SYSREG_ICC_AP1Rn_EL1(3):
+        vreg_emulate_apxr3(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index f9110ebf9c..f3cc9ff7b5 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -85,6 +85,7 @@
 #define HSR_SYSREG_PMINTENCLR_EL1 HSR_SYSREG(3,0,c9,c14,2)
 #define HSR_SYSREG_MAIR_EL1       HSR_SYSREG(3,0,c10,c2,0)
 #define HSR_SYSREG_AMAIR_EL1      HSR_SYSREG(3,0,c10,c3,0)
+#define HSR_SYSREG_ICC_AP1Rn_EL1(n) HSR_SYSREG(3,0,c12,c9, n)
 #define HSR_SYSREG_ICC_SGI1R_EL1  HSR_SYSREG(3,0,c12,c11,5)
 #define HSR_SYSREG_ICC_ASGI1R_EL1 HSR_SYSREG(3,1,c12,c11,6)
 #define HSR_SYSREG_ICC_SGI0R_EL1  HSR_SYSREG(3,2,c12,c11,7)
-- 
2.14.1


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

[PATCH v2 09/17] arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler

[Manish Jaggi]

This patch is ported from linux to xen
commit: 2724c11a1df4b22ee966c04809ea0e808f66b04e
(KVM: arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler)

Add a handler for reading the guest's view of the ICV_HPPIR1_EL1
register. This is a simple parsing of the available LRs, extracting the
highest available interrupt.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 31 +++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 2 files changed, 32 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index c67e7c6ada..f11c7646da 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -674,6 +674,33 @@ static void vreg_emulate_apxr3(struct cpu_user_regs *regs,
         vgic_v3_write_apxrn(regs, hsr, 3);
 }
 
+static void vgic_v3_read_hppir1(struct cpu_user_regs *regs,
+                                const union hsr hsr)
+{
+    uint64_t lr_val;
+    int lr, lr_grp, grp;
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    grp = vgic_v3_get_group(hsr);
+    lr = vgic_v3_highest_priority_lr(regs, vmcr, &lr_val);
+
+    if ( lr == -1 )
+        goto spurious;
+
+    lr_grp = !!(lr_val & ICH_LR_GROUP);
+    if ( lr_grp != grp )
+        lr_val = ICC_IAR1_EL1_SPURIOUS;
+
+spurious:
+    set_user_reg(regs, hsr.sysreg.reg, lr_val & ICH_LR_VIRTUAL_ID_MASK);
+}
+
+static void vreg_emulate_hppir1(struct cpu_user_regs *regs,
+                                const union hsr hsr)
+{
+    vgic_v3_read_hppir1(regs, hsr);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -724,6 +751,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_apxr3(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_HPPIR1_EL1:
+        vreg_emulate_hppir1(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index f3cc9ff7b5..8a6345c2d2 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -94,6 +94,7 @@
 #define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
 #define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
 #define HSR_SYSREG_ICC_EOIR1_EL1  HSR_SYSREG(3,0,c12,c12,1)
+#define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
-- 
2.14.1


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

Re: [PATCH v2 09/17] arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler

[Marc Zyngier]

On 27/03/18 10:07, Manish Jaggi wrote:
> This patch is ported from linux to xen
> commit: 2724c11a1df4b22ee966c04809ea0e808f66b04e
> (KVM: arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler)
> 
> Add a handler for reading the guest's view of the ICV_HPPIR1_EL1
> register. This is a simple parsing of the available LRs, extracting the
> highest available interrupt.
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>  xen/arch/arm/arm64/vgic-v3-sr.c     | 31 +++++++++++++++++++++++++++++++
>  xen/include/asm-arm/arm64/sysregs.h |  1 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> index c67e7c6ada..f11c7646da 100644
> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> @@ -674,6 +674,33 @@ static void vreg_emulate_apxr3(struct cpu_user_regs *regs,
>          vgic_v3_write_apxrn(regs, hsr, 3);
>  }
>  
> +static void vgic_v3_read_hppir1(struct cpu_user_regs *regs,
> +                                const union hsr hsr)
> +{
> +    uint64_t lr_val;
> +    int lr, lr_grp, grp;
> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
> +
> +    grp = vgic_v3_get_group(hsr);
> +    lr = vgic_v3_highest_priority_lr(regs, vmcr, &lr_val);
> +
> +    if ( lr == -1 )
> +        goto spurious;
> +
> +    lr_grp = !!(lr_val & ICH_LR_GROUP);
> +    if ( lr_grp != grp )
> +        lr_val = ICC_IAR1_EL1_SPURIOUS;
> +
> +spurious:
> +    set_user_reg(regs, hsr.sysreg.reg, lr_val & ICH_LR_VIRTUAL_ID_MASK);
> +}
> +
> +static void vreg_emulate_hppir1(struct cpu_user_regs *regs,
> +                                const union hsr hsr)
> +{
> +    vgic_v3_read_hppir1(regs, hsr);
> +}

See why I said that this "emulate" function idea didn't hold much water?

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 09/17] arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler

[Manish Jaggi]

On 03/27/2018 04:26 PM, Marc Zyngier wrote:
> On 27/03/18 10:07, Manish Jaggi wrote:
>> This patch is ported from linux to xen
>> commit: 2724c11a1df4b22ee966c04809ea0e808f66b04e
>> (KVM: arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler)
>>
>> Add a handler for reading the guest's view of the ICV_HPPIR1_EL1
>> register. This is a simple parsing of the available LRs, extracting the
>> highest available interrupt.
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 31 +++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/arm64/sysregs.h |  1 +
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>> index c67e7c6ada..f11c7646da 100644
>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>> @@ -674,6 +674,33 @@ static void vreg_emulate_apxr3(struct cpu_user_regs *regs,
>>           vgic_v3_write_apxrn(regs, hsr, 3);
>>   }
>>   
>> +static void vgic_v3_read_hppir1(struct cpu_user_regs *regs,
>> +                                const union hsr hsr)
>> +{
>> +    uint64_t lr_val;
>> +    int lr, lr_grp, grp;
>> +    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
>> +
>> +    grp = vgic_v3_get_group(hsr);
>> +    lr = vgic_v3_highest_priority_lr(regs, vmcr, &lr_val);
>> +
>> +    if ( lr == -1 )
>> +        goto spurious;
>> +
>> +    lr_grp = !!(lr_val & ICH_LR_GROUP);
>> +    if ( lr_grp != grp )
>> +        lr_val = ICC_IAR1_EL1_SPURIOUS;
>> +
>> +spurious:
>> +    set_user_reg(regs, hsr.sysreg.reg, lr_val & ICH_LR_VIRTUAL_ID_MASK);
>> +}
>> +
>> +static void vreg_emulate_hppir1(struct cpu_user_regs *regs,
>> +                                const union hsr hsr)
>> +{
>> +    vgic_v3_read_hppir1(regs, hsr);
>> +}
> See why I said that this "emulate" function idea didn't hold much water?
I wanted to put a check for read call, but I was just trying to match 
the patch,
So that a later patch would inject an undef exception to guest if a 
write is issue by guest on hppir1
>
> 	M.


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

[PATCH v2 10/17] arm64: vgic-v3: Add ICV_BPR0_EL1 handler

[Manish Jaggi]

This patch is ported to xen from linux commit:
423de85a98c2b50715a0784a74f6124fbc0b1548
(KVM: arm64: vgic-v3: Add ICV_BPR0_EL1 handler)

Add a handler for reading/writing the guest's view of the ICC_BPR0_EL1
register, which is located in the ICH_VMCR_EL2.BPR0 field.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 41 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 2 files changed, 42 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index f11c7646da..b938e795a8 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -701,6 +701,43 @@ static void vreg_emulate_hppir1(struct cpu_user_regs *regs,
     vgic_v3_read_hppir1(regs, hsr);
 }
 
+static void vgic_v3_read_bpr0(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr0(vmcr));
+}
+
+static void vgic_v3_write_bpr0(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR0_SHIFT;
+    val &= ICH_VMCR_BPR0_MASK;
+    vmcr &= ~ICH_VMCR_BPR0_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr0(struct cpu_user_regs *regs,
+                              const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr0(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr0(regs, hsr.sysreg.reg);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -755,6 +792,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_hppir1(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_BPR0_EL1:
+        vreg_emulate_bpr0(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 8a6345c2d2..55e8185f6a 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -95,6 +95,7 @@
 #define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
 #define HSR_SYSREG_ICC_EOIR1_EL1  HSR_SYSREG(3,0,c12,c12,1)
 #define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)
+#define HSR_SYSREG_ICC_BPR0_EL1   HSR_SYSREG(3,0,c12,c8,3)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
-- 
2.14.1


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

[PATCH v2 11/17] arm64: vgic-v3: Add ICV_IGRPEN0_EL1 handler

[Manish Jaggi]

This patch is ported to xen from linux commit:
fbc48a0011deb3d51cb657ca9c0f9083f41c0665
(KVM: arm64: vgic-v3: Add ICV_IGNREN0_EL1 handler)

Add a handler for reading/writing the guest's view of the
ICC_IGRPEN0_EL1 register, which is located in the ICH_VMCR_EL2.VENG0
field.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 33 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index b938e795a8..d854b1070d 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -738,6 +738,35 @@ static void vreg_emulate_bpr0(struct cpu_user_regs *regs,
         vgic_v3_write_bpr0(regs, hsr.sysreg.reg);
 }
 
+static void vgic_v3_read_igrpen0(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, !!(vmcr & ICH_VMCR_ENG0_MASK));
+}
+
+static void vgic_v3_write_igrpen0(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( val & 1 )
+        vmcr |= ICH_VMCR_ENG0_MASK;
+    else
+        vmcr &= ~ICH_VMCR_ENG0_MASK;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_igrpen0(struct cpu_user_regs *regs,
+                                 const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_igrpen0(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_igrpen0(regs, hsr.sysreg.reg);
+}
+
 /*
  * returns true if the register is emulated.
  */
@@ -796,6 +825,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_bpr0(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_IGRPEN0_EL1:
+        vreg_emulate_igrpen0(regs, hsr);
+        break;
+
     default:
         ret = false;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 55e8185f6a..8a4f5b45cb 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -96,6 +96,7 @@
 #define HSR_SYSREG_ICC_EOIR1_EL1  HSR_SYSREG(3,0,c12,c12,1)
 #define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)
 #define HSR_SYSREG_ICC_BPR0_EL1   HSR_SYSREG(3,0,c12,c8,3)
+#define HSR_SYSREG_ICC_IGRPEN0_EL1 HSR_SYSREG(3,0,c12,c12,6)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
-- 
2.14.1


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

[PATCH v2 12/17] arm64: vgic-v3: Add misc Group-0 handlers

[Manish Jaggi]

This patch is ported to xen from linux commit:
eab0b2dc4f6f34147e3d10da49ab8032e15dbea0
(KVM: arm64: vgic-v3: Add misc Group-0 handlers)

A number of Group-0 registers can be handled by the same accessors
as that of Group-1, so let's add the required system register encodings
and catch them in the dispatching function.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 7 +++++++
 xen/include/asm-arm/arm64/sysregs.h | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index d854b1070d..201194c713 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -793,30 +793,37 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
         vreg_emulate_igrpen1(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_IAR0_EL1:
     case HSR_SYSREG_ICC_IAR1_EL1:
         vreg_emulate_iar(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_EOIR0_EL1:
     case HSR_SYSREG_ICC_EOIR1_EL1:
         vreg_emulate_eoi(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_AP0Rn_EL1(0):
     case HSR_SYSREG_ICC_AP1Rn_EL1(0):
         vreg_emulate_apxr0(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_AP0Rn_EL1(1):
     case HSR_SYSREG_ICC_AP1Rn_EL1(1):
         vreg_emulate_apxr1(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_AP0Rn_EL1(2):
     case HSR_SYSREG_ICC_AP1Rn_EL1(2):
         vreg_emulate_apxr2(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_AP0Rn_EL1(3):
     case HSR_SYSREG_ICC_AP1Rn_EL1(3):
         vreg_emulate_apxr3(regs, hsr);
         break;
 
+    case HSR_SYSREG_ICC_HPPIR0_EL1:
     case HSR_SYSREG_ICC_HPPIR1_EL1:
         vreg_emulate_hppir1(regs, hsr);
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 8a4f5b45cb..6d346d84db 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -97,6 +97,10 @@
 #define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)
 #define HSR_SYSREG_ICC_BPR0_EL1   HSR_SYSREG(3,0,c12,c8,3)
 #define HSR_SYSREG_ICC_IGRPEN0_EL1 HSR_SYSREG(3,0,c12,c12,6)
+#define HSR_SYSREG_ICC_IAR0_EL1   HSR_SYSREG(3,0,c12,c8,0)
+#define HSR_SYSREG_ICC_EOIR0_EL1  HSR_SYSREG(3,0,c12,c8,1)
+#define HSR_SYSREG_ICC_HPPIR0_EL1 HSR_SYSREG(3,0,c12,c8,2)
+#define HSR_SYSREG_ICC_AP0Rn_EL1(n) HSR_SYSREG(3,0,c12,c8,4|n)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
-- 
2.14.1


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

Re: [PATCH v2 12/17] arm64: vgic-v3: Add misc Group-0 handlers

[Marc Zyngier]

On 27/03/18 10:07, Manish Jaggi wrote:
> This patch is ported to xen from linux commit:
> eab0b2dc4f6f34147e3d10da49ab8032e15dbea0
> (KVM: arm64: vgic-v3: Add misc Group-0 handlers)
> 
> A number of Group-0 registers can be handled by the same accessors
> as that of Group-1, so let's add the required system register encodings
> and catch them in the dispatching function.
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>  xen/arch/arm/arm64/vgic-v3-sr.c     | 7 +++++++
>  xen/include/asm-arm/arm64/sysregs.h | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> index d854b1070d..201194c713 100644
> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> @@ -793,30 +793,37 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>          vreg_emulate_igrpen1(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_IAR0_EL1:
>      case HSR_SYSREG_ICC_IAR1_EL1:
>          vreg_emulate_iar(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_EOIR0_EL1:
>      case HSR_SYSREG_ICC_EOIR1_EL1:
>          vreg_emulate_eoi(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_AP0Rn_EL1(0):
>      case HSR_SYSREG_ICC_AP1Rn_EL1(0):
>          vreg_emulate_apxr0(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_AP0Rn_EL1(1):
>      case HSR_SYSREG_ICC_AP1Rn_EL1(1):
>          vreg_emulate_apxr1(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_AP0Rn_EL1(2):
>      case HSR_SYSREG_ICC_AP1Rn_EL1(2):
>          vreg_emulate_apxr2(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_AP0Rn_EL1(3):
>      case HSR_SYSREG_ICC_AP1Rn_EL1(3):
>          vreg_emulate_apxr3(regs, hsr);
>          break;
>  
> +    case HSR_SYSREG_ICC_HPPIR0_EL1:
>      case HSR_SYSREG_ICC_HPPIR1_EL1:
>          vreg_emulate_hppir1(regs, hsr);

This doesn't shock you a tiny bit?

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2 12/17] arm64: vgic-v3: Add misc Group-0 handlers

[Manish Jaggi]

On 03/27/2018 04:28 PM, Marc Zyngier wrote:
> On 27/03/18 10:07, Manish Jaggi wrote:
>> This patch is ported to xen from linux commit:
>> eab0b2dc4f6f34147e3d10da49ab8032e15dbea0
>> (KVM: arm64: vgic-v3: Add misc Group-0 handlers)
>>
>> A number of Group-0 registers can be handled by the same accessors
>> as that of Group-1, so let's add the required system register encodings
>> and catch them in the dispatching function.
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/arm64/vgic-v3-sr.c     | 7 +++++++
>>   xen/include/asm-arm/arm64/sysregs.h | 4 ++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
>> index d854b1070d..201194c713 100644
>> --- a/xen/arch/arm/arm64/vgic-v3-sr.c
>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
>> @@ -793,30 +793,37 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
>>           vreg_emulate_igrpen1(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_IAR0_EL1:
>>       case HSR_SYSREG_ICC_IAR1_EL1:
>>           vreg_emulate_iar(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_EOIR0_EL1:
>>       case HSR_SYSREG_ICC_EOIR1_EL1:
>>           vreg_emulate_eoi(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_AP0Rn_EL1(0):
>>       case HSR_SYSREG_ICC_AP1Rn_EL1(0):
>>           vreg_emulate_apxr0(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_AP0Rn_EL1(1):
>>       case HSR_SYSREG_ICC_AP1Rn_EL1(1):
>>           vreg_emulate_apxr1(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_AP0Rn_EL1(2):
>>       case HSR_SYSREG_ICC_AP1Rn_EL1(2):
>>           vreg_emulate_apxr2(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_AP0Rn_EL1(3):
>>       case HSR_SYSREG_ICC_AP1Rn_EL1(3):
>>           vreg_emulate_apxr3(regs, hsr);
>>           break;
>>   
>> +    case HSR_SYSREG_ICC_HPPIR0_EL1:
>>       case HSR_SYSREG_ICC_HPPIR1_EL1:
>>           vreg_emulate_hppir1(regs, hsr);
> This doesn't shock you a tiny bit?
Ah, it should be vgic_emulate_hppir.
> 	M.


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

[PATCH v2 13/17] arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family

[Manish Jaggi]

Add MIDR values for Cavium ThunderX1 SoC family: ThunderX1, 81XX, 83XX.

This patch copies the below defines as is from linux kernel code.
arch/arm64/include/asm/cputype.h

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/include/asm-arm/processor.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 65eb1071e1..62ad244785 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -43,15 +43,24 @@
 })
 
 #define ARM_CPU_IMP_ARM             0x41
+#define ARM_CPU_IMP_CAVIUM          0x43
 
 #define ARM_CPU_PART_CORTEX_A15     0xC0F
 #define ARM_CPU_PART_CORTEX_A53     0xD03
 #define ARM_CPU_PART_CORTEX_A57     0xD07
 
+#define CAVIUM_CPU_PART_THUNDERX      0x0A1
+#define CAVIUM_CPU_PART_THUNDERX_81XX 0x0A2
+#define CAVIUM_CPU_PART_THUNDERX_83XX 0x0A3
+
 #define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A15)
 #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
 #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
 
+#define MIDR_THUNDERX      MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
+#define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
+#define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX)
+
 /* MPIDR Multiprocessor Affinity Register */
 #define _MPIDR_UP           (30)
 #define MPIDR_UP            (_AC(1,U) << _MPIDR_UP)
-- 
2.14.1


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

[PATCH v2 14/17] arm64: Add config for Cavium Thunder erratum 30115

[Manish Jaggi]

Some Cavium Thunder CPUs suffer a problem where a Xen guest may
inadvertently cause the host kernel to quit receiving interrupts.
This patch adds CONFIG_CAVIUM_ERRATUM_30115. Subsequent patches will
provide workaround.

This patch is ported to xen from linux kernel commit:
690a341577f9adf2c275ababe0dcefe91898bbf0
arm64: Add workaround for Cavium Thunder erratum 30115

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 docs/misc/arm/silicon-errata.txt |  1 +
 xen/arch/arm/Kconfig             | 11 +++++++++++
 xen/arch/arm/cpuerrata.c         | 21 +++++++++++++++++++++
 xen/include/asm-arm/cpuerrata.h  |  1 +
 xen/include/asm-arm/cpufeature.h |  3 ++-
 5 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/docs/misc/arm/silicon-errata.txt b/docs/misc/arm/silicon-errata.txt
index c9854c39f4..a2546d4bb5 100644
--- a/docs/misc/arm/silicon-errata.txt
+++ b/docs/misc/arm/silicon-errata.txt
@@ -48,3 +48,4 @@ stable hypervisors.
 | ARM            | Cortex-A57      | #852523         | N/A                     |
 | ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    |
 | ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220    |
+| CAVIUM         | ThunderX1       | #30115          | CAVIUM_ERRATUM_30115    |
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index f58019d6ed..762b761f7d 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -169,6 +169,17 @@ config ARM64_ERRATUM_834220
 
 	  If unsure, say Y.
 
+config CAVIUM_ERRATUM_30115
+	bool "Cavium Erratum 30115"
+	depends on HAS_GICV3
+	help
+	  On ThunderX T88 pass 1.x through 2.2, T81 pass 1.0 through
+	  1.2, and T83 Pass 1.0, guest execution may disable
+	  interrupts in host. Trapping both GICv3 group-0 and group-1
+	  accesses sidesteps the issue.
+
+	  If unsure, say Y.
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index fe9e9facbe..d49698f785 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -56,6 +56,27 @@ static const struct arm_cpu_capabilities arm_errata[] = {
         MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
                    (1 << MIDR_VARIANT_SHIFT) | 2),
     },
+#endif
+#ifdef CONFIG_CAVIUM_ERRATUM_30115
+    {
+        /* Cavium ThunderX, T88 pass 1.x - 2.2 */
+        .desc = "Cavium erratum 30115",
+        .capability = ARM64_WORKAROUND_CAVIUM_30115,
+        MIDR_RANGE(MIDR_THUNDERX, 0x00,
+                   (1 << MIDR_VARIANT_SHIFT) | 2),
+    },
+    {
+        /* Cavium ThunderX, T81 pass 1.0 - 1.2 */
+        .desc = "Cavium erratum 30115",
+        .capability = ARM64_WORKAROUND_CAVIUM_30115,
+        MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x02),
+    },
+    {
+        /* Cavium ThunderX, T83 pass 1.0 */
+        .desc = "Cavium erratum 30115",
+        .capability = ARM64_WORKAROUND_CAVIUM_30115,
+        MIDR_RANGE(MIDR_THUNDERX_83XX, 0x00, 0x00),
+    },
 #endif
     {},
 };
diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index 8b158429c7..eff606c422 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -41,6 +41,7 @@ static inline bool check_workaround_##erratum(void)             \
 
 CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)
 CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
+CHECK_WORKAROUND_HELPER(cavium_30115, ARM64_WORKAROUND_CAVIUM_30115, CONFIG_ARM_64)
 
 #undef CHECK_WORKAROUND_HELPER
 
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index f00b6dbd39..d409636bf0 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -42,8 +42,9 @@
 #define LIVEPATCH_FEATURE   4
 #define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5
 #define SKIP_CTXT_SWITCH_SERROR_SYNC 6
+#define ARM64_WORKAROUND_CAVIUM_30115 7
 
-#define ARM_NCAPS           7
+#define ARM_NCAPS           8
 
 #ifndef __ASSEMBLY__
 
-- 
2.14.1


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

[PATCH v2 15/17] arm: Hook workaround handler from traps.c based on Cavium workaround_30115

[Manish Jaggi]

Function vgic_v3_handle_cpuif_access is called from do_trap_guest_sync
if ARM64_WORKAROUND_CAVIUM_30115 capability is found.

A flag skip_hyp_tail is introduced in struct cpu_info. This flag
is used to skip leave_hypervisor_tail when enter_hypervisor_head
is not invoked. enter_hypervisor_head andleave_hypervisor_tail are
invoked in sync, if one is not called other one should be skipped,
otherwise guest vGIC state be out-of-date.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/Makefile   |  1 +
 xen/arch/arm/traps.c          | 31 +++++++++++++++++++++++++++++++
 xen/include/asm-arm/current.h |  3 ++-
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 718fe44455..eed3c9e913 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -11,3 +11,4 @@ obj-y += smpboot.o
 obj-y += traps.o
 obj-y += vfp.o
 obj-y += vsysreg.o
+obj-$(CONFIG_CAVIUM_ERRATUM_30115) += vgic-v3-sr.o
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..1dc34275b3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,27 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
 {
     const union hsr hsr = { .bits = regs->hsr };
 
+    if ( check_workaround_cavium_30115() )
+    {
+        if ( vgic_v3_handle_cpuif_access(regs) )
+        {
+	        /*
+            * if true, g0/g1 vgic register trap is emulated for errata
+	         * so rest of handling of do_trap_guest_sync is not required.
+	         */
+            advance_pc(regs, hsr);
+            /*
+             * enter_hypervisor_head is not invoked when workaround 30115
+             * is in place. enter_hypervisor_head and leave_hypervisor_tail
+             * are invoked in sync, if one is not called other one should be
+             * skipped, otherwise guest vGIC state be out-of-date.
+             */
+            get_cpu_info()->skip_hyp_tail = true;
+
+            return;
+        }
+    }
+
     enter_hypervisor_head(regs);
 
     switch (hsr.ec) {
@@ -2295,6 +2316,16 @@ void do_trap_fiq(struct cpu_user_regs *regs)
 
 void leave_hypervisor_tail(void)
 {
+    /*
+     * if skip_hyp_tail is set simply retrun;
+     */
+    if ( unlikely(get_cpu_info()->skip_hyp_tail) )
+    {
+        /* clear it, so that it is false when not handling g0/g1 traps */
+        get_cpu_info()->skip_hyp_tail = false;
+        return;
+    }
+
     while (1)
     {
         local_irq_disable();
diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
index 7a0971fdea..63b7e68f0b 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -21,7 +21,14 @@ DECLARE_PER_CPU(struct vcpu *, curr_vcpu);
 struct cpu_info {
     struct cpu_user_regs guest_cpu_user_regs;
     unsigned long elr;
-    unsigned int pad;
+/*
+ * Flag is used to skip leave_hypervisor_tail when enter_hypervisor_head
+ * is not invoked. enter_hypervisor_head andleave_hypervisor_tail are
+ * invoked in sync, if one is not called other one should be skipped,
+ * otherwise guest vGIC state be out-of-date.
+ */
+    bool skip_hyp_tail:1;
+    unsigned int pad:31;
 };
 
 static inline struct cpu_info *get_cpu_info(void)
-- 
2.14.1


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

[PATCH v2 16/17] arm64: if trapping a write-to-read-only GICv3 access inject Undef exception in guest

[Manish Jaggi]

This patch is a port to xen from linux commit:
7b1dba1f7325629427c0e5bdf014159b229d16c8
KVM: arm64: Log an error if trapping a write-to-read-only GICv3 access

A write-to-read-only GICv3 access should UNDEF at EL1. But since
we're in complete paranoia-land with broken CPUs, let's assume the
worse and gracefully handle the case.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index 201194c713..14276a4c92 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -480,7 +480,10 @@ spurious:
 
 static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    vgic_v3_read_iar(regs, hsr);
+    if ( unlikely(!hsr.sysreg.read) )
+        inject_undef_exception(regs, hsr);
+    else
+        vgic_v3_read_iar(regs, hsr);
 }
 
 static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
@@ -698,7 +701,10 @@ spurious:
 static void vreg_emulate_hppir1(struct cpu_user_regs *regs,
                                 const union hsr hsr)
 {
-    vgic_v3_read_hppir1(regs, hsr);
+    if ( unlikely(!hsr.sysreg.read) )
+        inject_undef_exception(regs, hsr);
+    else
+        vgic_v3_read_hppir1(regs, hsr);
 }
 
 static void vgic_v3_read_bpr0(struct cpu_user_regs *regs, int regidx)
-- 
2.14.1


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

[PATCH v2 17/17] arm64: if trapping a read-from-write-only GICv3 access inject undef exception in guest

[Manish Jaggi]

This patch is ported to xen from linux commit:
e7f1d1eef482150a64a6e6ad8faf40f8f97eed67
KVM: arm64: Log an error if trapping a read-from-write-only GICv3 access

A read-from-write-only GICv3 access should UNDEF at EL1. But since
we're in complete paranoia-land with broken CPUs, let's assume the
worse and gracefully handle the case.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vgic-v3-sr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
index 14276a4c92..d5dc8168c8 100644
--- a/xen/arch/arm/arm64/vgic-v3-sr.c
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -614,7 +614,11 @@ static void vgic_v3_write_eoir(struct cpu_user_regs *regs,
 
 static void vreg_emulate_eoi(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    vgic_v3_write_eoir(regs, hsr);
+
+    if ( unlikely(hsr.sysreg.read) )
+        inject_undef_exception(regs, hsr);
+    else
+        vgic_v3_write_eoir(regs, hsr);
 }
 
 static void vgic_v3_read_apxrn(struct cpu_user_regs *regs,
-- 
2.14.1


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