Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V3 0/8] IOMMU probe deferral support
From: Sricharan @ 2016-11-28 17:42 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <f2ad6976-0f8f-d116-bc43-37337dd9f41f@arm.com>

Hi Robin,

>> <snip..>
>>
>>>
>>>>>>>> iommu_group_get_for_dev which gets called in the add_device
>>>>>>>> callback, increases the reference count of the iommu_group,
>>>>>>>> so we do an iommu_group_put after that. iommu_group_get_for_dev
>>>>>>>> inturn calls device_group callback and in the case of arm-smmu
>>>>>>>> we call generic_device_group/pci_device_group which takes
>>>>>>>> care of increasing the group's reference. But when we return
>>>>>>>> an already existing group(when multiple devices have same group)
>>>>>>>> the reference is not incremented, resulting in issues when the
>>>>>>>> remove_device callback for the devices is invoked.
>>>>>>>> Fixing the same here.
>>>>>>>
>>>>>>> Bah, yes, this does look like my fault - after flip-flopping between
>>>>>>> about 3 different ways to keep refcounts for the S2CR entries, none of
>>>>>>> which would quite work, I ripped it all out but apparently still got
>>>>>>> things wrong, oh well. Thanks for figuring it out.
>>>>>>>
>>>>>>> On the probe-deferral angle, whilst it's useful to have uncovered this
>>>>>>> bug, I don't think we should actually be calling remove_device() from
>>>>>>> DMA teardown. I think it's preferable from a user perspective if group
>>>>>>> numbering remains stable, rather than changing depending on the order in
>>>>>>> which they unbind/rebind VFIO drivers. I'm really keen to try and get
>>>>>>> this in shape for 4.10, so I've taken the liberty of hacking up my own
>>>>>>> branch (iommu/defer) based on v3 - would you mind taking a look at the
>>>>>>> two "iommu/of:" commits to see what you think? (Ignore the PCI changes
>>>>>>> to your later patches - that was an experiment which didn't really work out)
>>>>>>
>>>>>> Ok, will take a look at this now and respond more on this.
>>>>>>
>>>>> Sorry for the delayed response on this. I was OOO for the last few days.
>>>>> So i tested this branch and it worked fine. I tested it with a pci device
>>>>> for both normal and deferred probe cases.  The of/iommu patches
>>>>> are the cleanup/preparation patches and it looks fine. One thing is without
>>>>> calling the remove_device callback, the resources like (smes for exmaple)
>>>>> and the group association of the device all remain allocated. That does not
>>>>> feel correct, given that the associated device does not exist. So to
>>>>> understand that, what happens with VFIO in this case which makes the
>>>>> group renumbering/rebinding a problem ?
>>>>>
>>>>
>>>> Would it be ok if i post a V4 based on your branch above ?
>>>
>>> Sure, as long as none of the hacks slip through :) - I've just pushed
>>> out a mild rework based on Lorenzo's v9, which I hope shouldn't break
>>> anything for you.
>>>
>>
>> Ok sure, i will test and just the post out the stuff from your branch then
>> mostly by tomorrow.
>
>Cool. We're rather hoping that the ACPI stuff is good to go for 4.10
>now, so it's probably worth pulling the rest of that in (beyond the one
>patch I picked) to make sure the of_dma_configure/acpi_dma_configure
>paths don't inadvertently diverge.
>

I rebased and was testing your branch with Lorenzo's series. One thing
i am still trying to get right is the acpi_dma_configure call. With your
series dma_configure calls pci_dma/of_dma configure, so i am just adding
acpi_dma_configure call there for non-pci ACPI devices as well. I see that
acpi_dma_configure right now is called from acpi_bind_one and 
iort_add_smmu_platform_device, both go through the really_probe function
path, so moving acpi_dma_configure from the above the two functions
to dma_configure. I remember we discussed this on another thread, so
hopefully it is correct. I do not have an platform to test the ACPI though.
I will take some testing help on V4 for this.

>>> Having thought a bit more about the add/remove thing, I'm inclined to
>>> agree that the group numbering itself may not be that big an issue in
>>> practice - sure, it could break my little script, but it looks like QEMU
>>> and such work with the device ID rather than the group number directly,
>>> so might not even notice. However, the fact remains that the callbacks
>>> are intended to handle a device being added to/removed from its bus, and
>>> will continue to do so on other platforms, so I don't like the idea of
>>> introducing needlessly different behaviour. If you unbind a driver, the
>>> stream IDs and everything don't stop existing at the hardware level; the
>>> struct device to which the in-kernel data belongs still exists and
>>> doesn't stop being associated with its bus. There's no good reason for
>>> freeing SMEs that we'll only reallocate again (inadequately-specced
>>> hardware with not enough SMRs/contexts is not a *good* reason), and
>>
>> ok, so SMRs/contexts was the reason i was adding the remove_dev
>> callback, but if thats not good enough then there was no other
>> intention.
>>
>>> there are also some strong arguments against letting any stream IDs the
>>> kernel knows about go back to bypass after a driver has been bound - by
>>
>> ok, but not sure why is this so ?
>
>Any device the kernel is in control of, having bound a driver to it,
>definitely should not be doing DMA after that driver is unbound...
>

ok.

>>> keeping groups around as expected that's something we can implement
>>> quite easily without having to completely lock down bypass for stream
>>> IDs the kernel *doesn't* know about.
>>>
>>
>> So do you mean in this case to keep the unbound device's group/context bank
>> to bypass rather than resetting the streamids ?
>
>...which we can easily enforce by keeping the device attached to its
>default domain, in which nothing should be mapped by that point (we
>could even have a group notifier switch its S2CRs to faulting entries
>for extreme paranoia). Freeing the SMRs means those stream IDs would
>instead fall back to the default "unmatched" behaviour, which in general
>is going to be bypass, and thus allow DMA attacks.
>
>It's harder to disable unmatched bypass in general, because we may have
>devices which physically master through the SMMU but want low latency
>more than they want translation (at the moment the best we can do is
>leave the kernel unaware of those stream IDs), or there could be unknown
>devices under control of the firmware or other agents which we would
>disrupt by hitting a system-wide switch.
>

ok thanks, understand the point now. Agree that putting the previously bound
devices to fault is right than putting it to bypass. So the notifier to switch the
S2CRs to faulting entries can be added as a separate patch on top of this.

Regards,
 Sricharan

^ permalink raw reply

* [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
From: Marc Zyngier @ 2016-11-28 17:43 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1480351570-11648-1-git-send-email-jintack@cs.columbia.edu>

Hi Jintack,

On 28/11/16 16:46, Jintack Lim wrote:
> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> are 11th and 10th bits respectively when E2H is set.  Current code is
> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> may allow guest OS to access physical timer. So, fix it.
> 
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> ---
>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>  include/clocksource/arm_arch_timer.h |  6 ++--
>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>  4 files changed, 103 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> 
> diff --git a/arch/arm/include/asm/kvm_timer.h b/arch/arm/include/asm/kvm_timer.h
> new file mode 100644
> index 0000000..d19d4b3
> --- /dev/null
> +++ b/arch/arm/include/asm/kvm_timer.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2016 - Columbia University
> + * Author: Jintack Lim <jintack@cs.columbia.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARM_KVM_TIMER_H__
> +#define __ARM_KVM_TIMER_H__
> +
> +#include <clocksource/arm_arch_timer.h>
> +
> +static inline u32 __hyp_text get_el1pcten(void)
> +{
> +	return CNTHCTL_EL1PCTEN_NVHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcen(void)
> +{
> +	return CNTHCTL_EL1PCEN_NVHE;
> +}
> +
> +#endif	/* __ARM_KVM_TIMER_H__ */
> diff --git a/arch/arm64/include/asm/kvm_timer.h b/arch/arm64/include/asm/kvm_timer.h
> new file mode 100644
> index 0000000..153f3da
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_timer.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (C) 2016 - Columbia University
> + * Author: Jintack Lim <jintack@cs.columbia.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARM64_KVM_TIMER_H__
> +#define __ARM64_KVM_TIMER_H__
> +
> +#include <asm/kvm_hyp.h>
> +#include <clocksource/arm_arch_timer.h>
> +
> +static inline u32 __hyp_text get_el1pcten_vhe(void)
> +{
> +	return CNTHCTL_EL1PCTEN_VHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcten_nvhe(void)
> +{
> +	return CNTHCTL_EL1PCTEN_NVHE;
> +}
> +
> +static hyp_alternate_select(get_el1pcten_arch,
> +			    get_el1pcten_nvhe, get_el1pcten_vhe,
> +			    ARM64_HAS_VIRT_HOST_EXTN);

This is pretty horrid ;-)

First, the inline qualifier doesn't apply here, as the whole 
hyp_alternate_select hack relies on thing not being inlined (it 
actually creates a function pointer).

Then, this gets potentially instantiated in any compilation unit that 
will include this file. GCC is probably clever enough to eliminate it 
if not used, but not without a well deserved warning.

> +
> +static inline u32 __hyp_text get_el1pten_vhe(void)
> +{
> +	return CNTHCTL_EL1PTEN_VHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcen_nvhe(void)
> +{
> +	return CNTHCTL_EL1PCEN_NVHE;
> +}
> +
> +static hyp_alternate_select(get_el1pcen_arch,
> +			    get_el1pcen_nvhe, get_el1pten_vhe,
> +			    ARM64_HAS_VIRT_HOST_EXTN);
> +
> +static inline u32 __hyp_text get_el1pcten(void)
> +{
> +	return get_el1pcten_arch()();
> +}
> +
> +static inline u32 __hyp_text get_el1pcen(void)
> +{
> +	return get_el1pcen_arch()();
> +}
> +
> +#endif	/* __ARM64_KVM_TIMER_H__ */
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index caedb74..4094529 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -23,8 +23,10 @@
>  #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
>  #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
>  
> -#define CNTHCTL_EL1PCTEN		(1 << 0)
> -#define CNTHCTL_EL1PCEN			(1 << 1)
> +#define CNTHCTL_EL1PCTEN_NVHE		(1 << 0)
> +#define CNTHCTL_EL1PCEN_NVHE		(1 << 1)
> +#define CNTHCTL_EL1PCTEN_VHE		(1 << 10)
> +#define CNTHCTL_EL1PTEN_VHE		(1 << 11)
>  #define CNTHCTL_EVNTEN			(1 << 2)
>  #define CNTHCTL_EVNTDIR			(1 << 3)
>  #define CNTHCTL_EVNTI			(0xF << 4)
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..f3feee0 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -15,11 +15,11 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <clocksource/arm_arch_timer.h>
>  #include <linux/compiler.h>
>  #include <linux/kvm_host.h>
>  
>  #include <asm/kvm_hyp.h>
> +#include <asm/kvm_timer.h>
>  
>  /* vcpu is already in the HYP VA space */
>  void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
> @@ -37,7 +37,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>  
>  	/* Allow physical timer/counter access for the host */
>  	val = read_sysreg(cnthctl_el2);
> -	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> +	val |= get_el1pcten() | get_el1pcen();
>  	write_sysreg(val, cnthctl_el2);
>  
>  	/* Clear cntvoff for the host */
> @@ -55,8 +55,8 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>  	 * Physical counter access is allowed
>  	 */
>  	val = read_sysreg(cnthctl_el2);
> -	val &= ~CNTHCTL_EL1PCEN;
> -	val |= CNTHCTL_EL1PCTEN;
> +	val &= ~get_el1pcen();
> +	val |= get_el1pcten();
>  	write_sysreg(val, cnthctl_el2);
>  
>  	if (timer->enabled) {
> 

Trying to solve this myself, I came up with an alternative approach,
which is ugly in its own way (#define in common code, random shifts):

diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..5cacfa8 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -21,11 +21,41 @@
 
 #include <asm/kvm_hyp.h>
 
+#ifdef CONFIG_ARM64
+static unsigned int __hyp_text get_cnthclt_shift_nvhe(void)
+{
+	return 0;
+}
+
+static unsigned int __hyp_text get_cnthclt_shift_vhe(void)
+{
+	return 10;
+}
+
+static hyp_alternate_select(__get_cnthclt_shift,
+			    get_cnthclt_shift_nvhe, get_cnthclt_shift_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN)
+
+#define cnthclt_shift()		__get_cnthclt_shift()()
+#else
+#define cnthclt_shift()		(0)
+#endif
+
+static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
+{
+	u32 val;
+	int shift = cnthclt_shift();
+
+	val = read_sysreg(cnthctl_el2);
+	val &= ~clr << shift;
+	val |= set << shift;
+	write_sysreg(val, cnthctl_el2);
+}
+
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	if (timer->enabled) {
 		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
@@ -36,9 +66,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 	write_sysreg_el0(0, cntv_ctl);
 
 	/* Allow physical timer/counter access for the host */
-	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
 
 	/* Clear cntvoff for the host */
 	write_sysreg(0, cntvoff_el2);
@@ -48,16 +76,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	/*
 	 * Disallow physical timer access for the guest
 	 * Physical counter access is allowed
 	 */
-	val = read_sysreg(cnthctl_el2);
-	val &= ~CNTHCTL_EL1PCEN;
-	val |= CNTHCTL_EL1PCTEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);

We could make it nicer (read "faster") by introducing a
hyp_alternate_select construct that only returns a value instead
of calling a function. I remember writing something like that
at some point, and dropping it...

Thoughts?

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

^ permalink raw reply related

* Adding a .platform_init callback to sdhci_arasan_ops
From: Doug Anderson @ 2016-11-28 17:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <485a747c-47e3-bc0e-093a-4ec9ab719987@laposte.net>

Hi,

On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@laposte.net> wrote:
>> I will try to send another patch with what a different approach
>>
>
> Here's a different approach (I just tested that it built, because I don't have the
> rk3399 platform):
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 410a55b..5be6e67 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>                          sdhci_arasan_resume);
>
> -static const struct of_device_id sdhci_arasan_of_match[] = {
> -       /* SoC-specific compatible strings w/ soc_ctl_map */
> -       {
> -               .compatible = "rockchip,rk3399-sdhci-5.1",
> -               .data = &rk3399_soc_ctl_map,
> -       },
> -
> -       /* Generic compatible below here */
> -       { .compatible = "arasan,sdhci-8.9a" },
> -       { .compatible = "arasan,sdhci-5.1" },
> -       { .compatible = "arasan,sdhci-4.9a" },
> -
> -       { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> -
>  /**
>   * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>   *
> @@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>         of_clk_del_provider(dev->of_node);
>  }
>
> -static int sdhci_arasan_probe(struct platform_device *pdev)
> +static int sdhci_rockchip_platform_init(struct sdhci_host *host,
> +                                       struct platform_device *pdev)
>  {
>         int ret;
> -       const struct of_device_id *match;
>         struct device_node *node;
> -       struct clk *clk_xin;
> -       struct sdhci_host *host;
>         struct sdhci_pltfm_host *pltfm_host;
>         struct sdhci_arasan_data *sdhci_arasan;
> -       struct device_node *np = pdev->dev.of_node;
> -
> -       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
> -                               sizeof(*sdhci_arasan));
> -       if (IS_ERR(host))
> -               return PTR_ERR(host);
>
>         pltfm_host = sdhci_priv(host);
>         sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> -       sdhci_arasan->host = host;
>
> -       match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
> -       sdhci_arasan->soc_ctl_map = match->data;
> +       sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
>
>         node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>         if (node) {
> @@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>                         if (ret != -EPROBE_DEFER)
>                                 dev_err(&pdev->dev, "Can't get syscon: %d\n",
>                                         ret);
> -                       goto err_pltfm_free;
> +                       return -1;
>                 }
>         }
>
> +       if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
> +               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
> +
> +       return 0;
> +}
> +
> +static int sdhci_rockchip_clock_init(struct sdhci_host *host,
> +                                       struct platform_device *pdev)
> +{
> +       struct sdhci_pltfm_host *pltfm_host;
> +       struct sdhci_arasan_data *sdhci_arasan;
> +
> +       pltfm_host = sdhci_priv(host);
> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (of_device_is_compatible(pdev->dev.of_node,
> +                                   "rockchip,rk3399-sdhci-5.1"))
> +               sdhci_arasan_update_clockmultiplier(host, 0x0);

This _does_ belong in a Rockchip-specific init function, for now.
Other platforms might want different values for
corecfg_clockmultiplier, I think.

If it becomes common to need to set this, it wouldn't be hard to make
it generic by putting it in the data matched by the device tree, then
generically call sdhci_arasan_update_clockmultiplier() in cases where
it is needed.  sdhci_arasan_update_clockmultiplier() itself should be
generic enough to handle it.


> +
> +       sdhci_arasan_update_baseclkfreq(host);

If you make soc_ctl_map always part of "struct sdhci_arasan_cs_ops"
then other platforms will be able to use it.

As argued in my original patch the field "corecfg_baseclkfreq" is
documented in the generic Arasan document
<https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
and thus is unlikely to be Rockchip specific.  It is entirely possible
that not everyone who integrates this IP block will need this register
set, but in that case they can set an offset as "-1" and they'll be
fine.

Said another way: the concept of whether or not to set "baseclkfreq"
doesn't need to be tired to whether or not we're on Rockchip.


> +
> +       return sdhci_arasan_register_sdclk(sdhci_arasan, pltfm_host->clk, &pdev->dev);

This is documented in "bindings/mmc/arasan,sdhci.txt" to be available
to all people using this driver, not just Rockchip.  You should do it
always.  If you don't specify "#clock-cells" in the device tree it
will be a no-op anyway.


> +}
> +
> +static int sdhci_tango_platform_init(struct sdhci_host *host,
> +                                    struct platform_device *pdev)
> +{
> +       return 0;

I'll comment in your other patch where you actually had an
implementation for this.


> +}
> +
> +/* Chip-specific ops */
> +struct sdhci_arasan_cs_ops {
> +       int (*platform_init)(struct sdhci_host *host,
> +                            struct platform_device *pdev);
> +       int (*clock_init)(struct sdhci_host *host,
> +                         struct platform_device *pdev);
> +};

I really think it's a good idea to add the "soc_ctl_map" into this structure.

If nothing else when the next Rockchip SoC comes out that uses this
then we won't have to do a second level of matching for Rockchip SoCs.
I'm also a firm believer that others will need "soc_ctl_map" at some
point in time, but obviously I can't predict the future.


> +
> +static const struct sdhci_arasan_cs_ops sdhci_rockchip_cs_ops = {
> +       .platform_init = sdhci_rockchip_platform_init,
> +       .clock_init = sdhci_rockchip_clock_init,
> +};
> +
> +static const struct sdhci_arasan_cs_ops sdhci_tango_cs_ops = {
> +       .platform_init = sdhci_tango_platform_init,
> +};
> +
> +static const struct of_device_id sdhci_arasan_of_match[] = {
> +       /* SoC-specific compatible strings */
> +       {
> +               .compatible = "rockchip,rk3399-sdhci-5.1",
> +               .data = &sdhci_rockchip_cs_ops,
> +       },
> +       {
> +               .compatible = "sigma,sdio-v1",
> +               .data = &sdhci_tango_cs_ops,
> +       },
> +
> +       /* Generic compatible below here */
> +       { .compatible = "arasan,sdhci-8.9a" },
> +       { .compatible = "arasan,sdhci-5.1" },
> +       { .compatible = "arasan,sdhci-4.9a" },
> +
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> +
> +static int sdhci_arasan_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       const struct of_device_id *match;
> +       struct clk *clk_xin;
> +       struct sdhci_host *host;
> +       struct sdhci_pltfm_host *pltfm_host;
> +       struct sdhci_arasan_data *sdhci_arasan;
> +       const struct sdhci_arasan_cs_ops *cs_ops;
> +
> +       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
> +                               sizeof(*sdhci_arasan));
> +       if (IS_ERR(host))
> +               return PTR_ERR(host);
> +
> +       pltfm_host = sdhci_priv(host);
> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +       sdhci_arasan->host = host;
> +
> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
> +       if (match)
> +               cs_ops = match->data;
> +
> +       /* SoC-specific platform init */
> +       if (cs_ops && cs_ops->platform_init) {
> +               ret = cs_ops->platform_init(host, pdev);
> +               if (ret)
> +                       goto err_pltfm_free;
> +       }
> +
>         sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
>         if (IS_ERR(sdhci_arasan->clk_ahb)) {
>                 dev_err(&pdev->dev, "clk_ahb clock not found.\n");
> @@ -642,21 +713,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>         }
>
>         sdhci_get_of_property(pdev);
> -
> -       if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
> -               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
> -
>         pltfm_host->clk = clk_xin;
>
> -       if (of_device_is_compatible(pdev->dev.of_node,
> -                                   "rockchip,rk3399-sdhci-5.1"))
> -               sdhci_arasan_update_clockmultiplier(host, 0x0);
> -
> -       sdhci_arasan_update_baseclkfreq(host);
> -
> -       ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
> -       if (ret)
> -               goto clk_disable_all;
> +       /* SoC-specific clock init */
> +       if (cs_ops && cs_ops->clock_init) {
> +               ret = cs_ops->clock_init(host, pdev);
> +               if (ret)
> +                       goto clk_disable_all;
> +       }
>
>         ret = mmc_of_parse(host->mmc);
>         if (ret) {
>
>
> If you are ok with it I can clean it up to submit it properly.

^ permalink raw reply

* [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue
From: Florian Fainelli @ 2016-11-28 17:54 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1480348229-25672-1-git-send-email-jbrunet@baylibre.com>

On 11/28/2016 07:50 AM, Jerome Brunet wrote:
> This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
> The platform seems to enter LPI on the Rx path too often while performing
> relatively high TX transfer. This eventually break the link (both Tx and
> Rx), and require to bring the interface down and up again to get the Rx
> path working again.
> 
> The root cause of this issue is not fully understood yet but disabling EEE
> advertisement on the PHY prevent this feature to be negotiated.
> With this change, the link is stable and reliable, with the expected
> throughput performance.
> 
> The patchset adds options in the generic phy driver to disable EEE
> advertisement, through device tree. The way it is done is very similar
> to the handling of the max-speed property.
> 
> Patch 4 is provided here for testing purpose only. Please don't merge
> patch 4, this change will go through the amlogic's tree.

Sorry, but I really don't like the route this is going, and I should
have made myself clearer before on that, I really think utilizing a PHY
fixup is more appropriate here than an extremely generic DT property.
The fixup code can be in the affected PHY driver, or it can be somewhere
else, your call. There is no shortage of option on how to implement it,
and this would be something easy to enable/disable for known good
configurations (ala PCI/USB fixups).

If we start supporting generic "enable", "disable" type of properties
with values that map directly to register definitions of the HW, we
leave too much room for these properties to be utilized to implement a
specific policy, and this is not acceptable.
-- 
Florian

^ permalink raw reply

* [PATCH] fpga zynq: Check the bitstream for validity
From: Jason Gunthorpe @ 2016-11-28 18:00 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <609bc971-bb6d-8cd2-8e64-23c0082a6216@topic.nl>

I seem to have missed being CC on this follow up from Mike and wanted
to respond:

> What you're creating here will require active maintenance.  There
> are already 7007 and 7012 devices which aren't in your list so the
> driver will refuse to load them until someone fills in the IDs.

I don't know what list you are refering to, are you sure you are
responding to the right patch?

The patch searches for the sync word, it has nothing to do with IDs,
and does not attempt to parse any of the proprietary headers. Based on
the Xilinx documentation this will work on 7 Series, US and US+ at a
minimum. Certainly on all Zynq hardware.

> The bitstream format is actually proprietary and undocumented.
> Any "checks" in code are likely based on guesswork and reverse
> engineering.

No, this part is fully documented in UG470.

> We also use partial reprogramming a lot. Did you test
> that? On all devices?

You should read the patch, it only does the check on a full bitstream.

Jason

^ permalink raw reply

* Adding a .platform_init callback to sdhci_arasan_ops
From: Doug Anderson @ 2016-11-28 18:02 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <e32d2a01-ba3c-6127-51ba-a1fd4176e32c@laposte.net>

Hi,

On Mon, Nov 28, 2016 at 5:28 AM, Sebastian Frias <sf84@laposte.net> wrote:
> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
> +{
> +       printk("%s\n", __func__);
> +
> +       /*
> +         pad_mode[2:0]=0    must be 0
> +         sel_sdio[3]=1      must be 1 for SDIO
> +         inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
> +         inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
> +       */
> +       sdhci_writel(host, 0x00000008, 0x100 + 0x0);

If I were doing this, I'd probably actually add these fields to the
"sdhci_arasan_soc_ctl_map", then add a 3rd option to
sdhci_arasan_syscon_write().  Right now it has 2 modes: "hiword
update" and "non-hiword update".  You could add a 3rd indicating that
you set config registers by just writing at some offset of the main
SDHCI registers space.

So you'd add 4 fields:

.tango_pad_mode = { .reg = 0x0000, .width = 3, .shift = 0 },
.sel_sdio = { .reg = 0x0000, .width = 1, .shift = 3},
.inv_sdwp_pol = { .reg = 0x0000, .width = 1, .shift = 4},
.inv_sdcd_pol = { .reg = 0x0000, .width = 1, .shift = 5},

In your platform-specific init you're proposing you could set
tango_pad_mode to 0.  That seems tango-specific.

You'd want to hook into "set_ios" for setting sel_sdio or not.  That's
important if anyone ever wants to plug in an external SDIO card to
your slot.  This one good argument for putting this in
sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
compatibility matching every time set_ios is called.

I'd have to look more into the whole SD/WP polarity issue.  There are
already so many levels of inversion for these signals in Linux that
it's confusing.  It seems like you might just want to hardcode them to
"0" and let users use all the existing ways to invert things...  You
could either put that hardcoding into your platform init code or (if
you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
that if anyone else needs to init similar signals then they can take
advantage of it.

--

One random side note is that as currently documented you need to
specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
aren't using.  That seems stupid--not sure why I did that.  It seems
better to clue off "width = 0" so that you could just freely not init
any fields you don't need.

-Doug

^ permalink raw reply

* [PATCH v7 2/8] drm/sun8i: Add DT bindings documentation of Allwinner DE2
From: Jean-Francois Moine @ 2016-11-28 18:02 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1480414715.git.moinejf@free.fr>

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../bindings/display/sunxi/sun8i-de2.txt           | 121 +++++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
new file mode 100644
index 0000000..edf38b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
@@ -0,0 +1,121 @@
+Allwinner sun8i Display Engine 2 subsystem
+==========================================
+
+The Allwinner DE2 subsystem contains a display controller (DE2),
+one or two LCD controllers (Timing CONtrollers) and their external
+interfaces (encoders/connectors).
+
+          +-----------+
+          | DE2       |
+          |           |
+          | +-------+ |
+  plane --->|       | |   +------+
+          | | mixer |---->| TCON |---> encoder  ---> display
+  plane --->|       | |   +------+     connector     device
+          | +-------+ |
+          |           |
+          | +-------+ |
+  plane --->|       | |   +------+
+          | | mixer |---->| TCON |---> encoder  ---> display
+  plane --->|       | |   +------+     connector     device
+          | +-------+ |
+          +-----------+
+
+The DE2 contains a processor which mixes the input planes and creates
+the images which are sent to the TCONs.
+From the software point of vue, there are 2 independent real-time
+mixers, each one being statically associated to one TCON.
+
+The TCONs adjust the image format to the one of the display device.
+
+Display controller (DE2)
+========================
+
+Required properties:
+
+- compatible: value should be one of the following
+		"allwinner,sun8i-a83t-display-engine"
+		"allwinner,sun8i-h3-display-engine"
+
+- reg: base address and size of the I/O memory
+
+- clocks: must include clock specifiers corresponding to entries in the
+		clock-names property.
+
+- clock-names: must contain
+		"bus": bus gate
+		"clock": clock
+
+- resets: phandle to the reset of the device
+
+- ports: must contain a list of 2 phandles, indexed by mixer number,
+	and pointing to display interface ports of TCONs
+
+LCD controller (TCON)
+=====================
+
+Required properties:
+
+- compatible: should be
+		"allwinner,sun8i-a83t-tcon"
+
+- reg: base address and size of the I/O memory
+
+- clocks: must include clock specifiers corresponding to entries in the
+		clock-names property.
+
+- clock-names: must contain
+		"bus": bus gate
+		"clock": pixel clock
+
+- resets: phandle to the reset of the device
+
+- interrupts: interrupt number for the TCON
+
+- port: port node with endpoint definitions as defined in
+	Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+	de: de-controller at 01000000 {
+		compatible = "allwinner,sun8i-h3-display-engine";
+		reg = <0x01000000 0x400000>;
+		clocks = <&ccu CLK_BUS_DE>, <&ccu CLK_DE>;
+		clock-names = "bus", "clock";
+		resets = <&ccu RST_BUS_DE>;
+		ports = <&tcon0_p>, <&tcon1_p>;
+	};
+
+	tcon0: lcd-controller at 01c0c000 {
+		compatible = "allwinner,sun8i-a83t-tcon";
+		reg = <0x01c0c000 0x400>;
+		clocks = <&ccu CLK_BUS_TCON0>, <&ccu CLK_TCON0>;
+		clock-names = "bus", "clock";
+		resets = <&ccu RST_BUS_TCON0>;
+		interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		tcon0_p: port {
+			tcon0_hdmi: endpoint {
+				remote-endpoint = <&hdmi_tcon0>;
+			};
+		};
+	};
+
+	/* not used */
+	tcon1: lcd-controller at 01c0d000 {
+		compatible = "allwinner,sun8i-h3-tcon";
+		reg = <0x01c0d000 0x400>;
+		clocks = <&ccu CLK_BUS_TCON1>,
+			 <&ccu CLK_TCON0>;	/* no clock */
+		clock-names = "bus", "clock";
+		interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+		status = "disabled";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		tcon1_p: port {
+			endpoint {
+				/* empty */
+			};
+		};
+	};
-- 
2.10.2

^ permalink raw reply related

* [PATCH V3 0/8] IOMMU probe deferral support
From: Lorenzo Pieralisi @ 2016-11-28 18:13 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <000001d2499e$df7f2d80$9e7d8880$@codeaurora.org>

On Mon, Nov 28, 2016 at 11:12:08PM +0530, Sricharan wrote:

[...]

> >Cool. We're rather hoping that the ACPI stuff is good to go for 4.10
> >now, so it's probably worth pulling the rest of that in (beyond the one
> >patch I picked) to make sure the of_dma_configure/acpi_dma_configure
> >paths don't inadvertently diverge.
> >
> 
> I rebased and was testing your branch with Lorenzo's series. One thing
> i am still trying to get right is the acpi_dma_configure call. With your
> series dma_configure calls pci_dma/of_dma configure, so i am just adding
> acpi_dma_configure call there for non-pci ACPI devices as well. I see that
> acpi_dma_configure right now is called from acpi_bind_one and 
> iort_add_smmu_platform_device, both go through the really_probe function
> path, so moving acpi_dma_configure from the above the two functions
> to dma_configure. I remember we discussed this on another thread, so
> hopefully it is correct. I do not have an platform to test the ACPI though.
> I will take some testing help on V4 for this.

I am happy to test it for you please just send me a pointer at your v4
code.

Thank you !
Lorenzo

^ permalink raw reply

* [PATCH] ARM64: dts: meson-gxbb: add SCPI pre-1.0 compatible
From: Kevin Hilman @ 2016-11-28 18:25 UTC (permalink / raw)
  To: linux-arm-kernel

The SCPI driver has an updated compatible to indicate the pre-released
(pre v1.0) status of the driver.  Since Amlogic used a pre-1.0
version, add that compatible as well.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index ac5ad3bf1bd6..51edd5b5c460 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -51,7 +51,7 @@
 	compatible = "amlogic,meson-gxbb";
 
 	scpi {
-		compatible = "amlogic,meson-gxbb-scpi";
+		compatible = "amlogic,meson-gxbb-scpi", "arm,scpi-pre-1.0";
 		mboxes = <&mailbox 1 &mailbox 2>;
 		shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
 
-- 
2.9.3

^ permalink raw reply related

* [GIT PULL] Amlogic 64-bit DT changes for v4.10, round 2
From: Kevin Hilman @ 2016-11-28 18:27 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAFBinCAvziwM4U6qJnTV+nEMKaZH4G9xdTM0pY6O83+Edk=Ovw@mail.gmail.com>

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Hi Kevin,
>
> On Wed, Nov 23, 2016 at 11:23 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> Arnd, Olof,
>>
>> Here's a final round of arm64 DT changes for Amlogic family SoCs.
> please note that SCPI is not working due to a change in Sudeep's
> repository which has not made it into the Amlogic tree yet:
> Sudeep's latest pull request indicates that we should use
> "arm,scpi-pre-1.0", while currently we are only using
> "amlogic,meson-gxbb-scpi": [0]

Yes, I had to drop that legacy compatible as it was still under
discsussion.  Now that it's agreed upon, I've just sent a patch to fix
that.

> I fixed that in the Amlogic .dts files in [1] but this patch has not
> been applied yet.
>
> No pressure on this one - I just want to save your time (so you don't
> have to search why SCPI is not working anymore)

Thanks!

Kevin

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/470842.html

^ permalink raw reply

* [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
From: Marc Zyngier @ 2016-11-28 18:39 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <d04fd17e-9157-07fe-4daa-9dbab149b5e6@arm.com>

On 28/11/16 17:43, Marc Zyngier wrote:
> Hi Jintack,
> 
> On 28/11/16 16:46, Jintack Lim wrote:
>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>> are 11th and 10th bits respectively when E2H is set.  Current code is
>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>> may allow guest OS to access physical timer. So, fix it.
>>
>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>> ---
>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>

[...]

> We could make it nicer (read "faster") by introducing a
> hyp_alternate_select construct that only returns a value instead
> of calling a function. I remember writing something like that
> at some point, and dropping it...

So here's what this could look like (warning, wacky code ahead,
though I fixed a stupid bug that was present in the previous patch).
The generated code is quite nice (no branch, only an extra mov
instruction on the default path)... Of course, completely untested!

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index b18e852..d173902 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -121,6 +121,17 @@ typeof(orig) * __hyp_text fname(void)					\
 	return val;							\
 }
 
+#define hyp_alternate_select_const(fname, orig, alt, cond)		\
+inline const typeof(orig) __hyp_text fname(void)			\
+{									\
+	typeof(alt) val = orig;						\
+	asm volatile(ALTERNATIVE("nop		\n",			\
+				 "mov	%0, %1	\n",			\
+				 cond)					\
+		     : "+r" (val) : "r" (alt));				\
+	return val;							\
+}
+
 void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
 void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
 int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..7a783af 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -21,11 +21,29 @@
 
 #include <asm/kvm_hyp.h>
 
+#ifdef CONFIG_ARM64
+static hyp_alternate_select_const(cnthclt_shift, 0, 10,
+				  ARM64_HAS_VIRT_HOST_EXTN)
+
+#else
+#define cnthclt_shift()		(0)
+#endif
+
+static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
+{
+	u32 val;
+	int shift = cnthclt_shift();
+
+	val = read_sysreg(cnthctl_el2);
+	val &= ~(clr << shift);
+	val |= set << shift;
+	write_sysreg(val, cnthctl_el2);
+}
+
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	if (timer->enabled) {
 		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
@@ -36,9 +54,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 	write_sysreg_el0(0, cntv_ctl);
 
 	/* Allow physical timer/counter access for the host */
-	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
 
 	/* Clear cntvoff for the host */
 	write_sysreg(0, cntvoff_el2);
@@ -48,16 +64,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	/*
 	 * Disallow physical timer access for the guest
 	 * Physical counter access is allowed
 	 */
-	val = read_sysreg(cnthctl_el2);
-	val &= ~CNTHCTL_EL1PCEN;
-	val |= CNTHCTL_EL1PCTEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);

Thanks,

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

^ permalink raw reply related

* [PATCH V5 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
From: Baicar, Tyler @ 2016-11-28 18:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <583880E0.8080705@arm.com>

Hello James,

On 11/25/2016 11:20 AM, James Morse wrote:
> Hi Tyler,
>
> On 21/11/16 22:35, Tyler Baicar wrote:
>> Currently when a RAS error is reported it is not timestamped.
>> The ACPI 6.1 spec adds the timestamp field to the generic error
>> data entry v3 structure. The timestamp of when the firmware
>> generated the error is now being reported.
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index b79abc5..9063d68 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -420,7 +420,8 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
>>   	int flags = -1;
>>   	int sec_sev = ghes_severity(gdata->error_severity);
>>   	struct cper_sec_mem_err *mem_err;
>> -	mem_err = (struct cper_sec_mem_err *)(gdata + 1);
>> +
>> +	mem_err = acpi_hest_generic_data_payload(gdata);
>>   
>>   	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
>>   		return;
>> @@ -450,14 +451,18 @@ static void ghes_do_proc(struct ghes *ghes,
>>   {
>>   	int sev, sec_sev;
>>   	struct acpi_hest_generic_data *gdata;
>> +	uuid_le sec_type;
> ghes.c doesn't include <linux/uuid.h>, but I see it already uses uuid_le_cmp().
> Worth fixing as part of this patch?

I can add it here, but it shouldn't be needed. ghes.c includes 
<linux/cper.h> and that header
includes <linux/uuid.h>. Should it be added just to make the dependency 
more clear?

>>   
>>   	sev = ghes_severity(estatus->error_severity);
>>   	apei_estatus_for_each_section(estatus, gdata) {
>>   		sec_sev = ghes_severity(gdata->error_severity);
>> -		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
>> +		sec_type = *(uuid_le *)gdata->section_type;
>> +
> You don't use sec_type again here, why change this?
> (should it be in a later patch?)

Ah, yes, this change should be moved to patch 8 in this patchset.

>> +		if (!uuid_le_cmp(sec_type,
>>   				 CPER_SEC_PLATFORM_MEM)) {
>>   			struct cper_sec_mem_err *mem_err;
>> -			mem_err = (struct cper_sec_mem_err *)(gdata+1);
>> +
>> +			mem_err = acpi_hest_generic_data_payload(gdata);
>>   			ghes_edac_report_mem_error(ghes, sev, mem_err);
>>   
>>   			arch_apei_report_mem_error(sev, mem_err);
>> @@ -467,7 +472,8 @@ static void ghes_do_proc(struct ghes *ghes,
>>   		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
>>   				      CPER_SEC_PCIE)) {
>>   			struct cper_sec_pcie *pcie_err;
>> -			pcie_err = (struct cper_sec_pcie *)(gdata+1);
>> +
>> +			pcie_err = acpi_hest_generic_data_payload(gdata);
>>   			if (sev == GHES_SEV_RECOVERABLE &&
>>   			    sec_sev == GHES_SEV_RECOVERABLE &&
>>   			    pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index d425374..7e2439e 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>> @@ -32,6 +32,9 @@
>>   #include <linux/acpi.h>
>>   #include <linux/pci.h>
>>   #include <linux/aer.h>
>> +#include <linux/printk.h>
>> +#include <linux/bcd.h>
>> +#include <acpi/ghes.h>
>>   
>>   #define INDENT_SP	" "
>>   
>> @@ -386,13 +389,37 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
>>   	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
>>   }
>>   
>> +static void cper_estatus_print_section_v300(const char *pfx,
>> +	const struct acpi_hest_generic_data_v300 *gdata)
>> +{
>> +	__u8 hour, min, sec, day, mon, year, century, *timestamp;
>> +
>> +	if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>> +		timestamp = (__u8 *)&(gdata->time_stamp);
>> +		sec = bcd2bin(timestamp[0]);
>> +		min = bcd2bin(timestamp[1]);
>> +		hour = bcd2bin(timestamp[2]);
>> +		day = bcd2bin(timestamp[4]);
>> +		mon = bcd2bin(timestamp[5]);
>> +		year = bcd2bin(timestamp[6]);
>> +		century = bcd2bin(timestamp[7]);
>> +		printk("%stime: %7s %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
>> +			0x01 & *(timestamp + 3) ? "precise" : "", century,
>> +			year, mon, day, hour, min, sec);
>> +	}
>> +}
>> +
>>   static void cper_estatus_print_section(
>> -	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
>> +	const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>>   {
>>   	uuid_le *sec_type = (uuid_le *)gdata->section_type;
>>   	__u16 severity;
>>   	char newpfx[64];
>>   
>> +	if (acpi_hest_generic_data_version(gdata) >= 3)
>> +		cper_estatus_print_section_v300(pfx,
>> +			(const struct acpi_hest_generic_data_v300 *)gdata);
>> +
>>   	severity = gdata->error_severity;
>>   	printk("%s""Error %d, type: %s\n", pfx, sec_no,
>>   	       cper_severity_str(severity));
>> @@ -403,14 +430,18 @@ static void cper_estatus_print_section(
>>   
>>   	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
>>   	if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
>> -		struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1);
>> +		struct cper_sec_proc_generic *proc_err;
>> +
>> +		proc_err = acpi_hest_generic_data_payload(gdata);
>>   		printk("%s""section_type: general processor error\n", newpfx);
>>   		if (gdata->error_data_length >= sizeof(*proc_err))
>>   			cper_print_proc_generic(newpfx, proc_err);
>>   		else
>>   			goto err_section_too_small;
>>   	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
>> -		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
>> +		struct cper_sec_mem_err *mem_err;
>> +
>> +		mem_err = acpi_hest_generic_data_payload(gdata);
>>   		printk("%s""section_type: memory error\n", newpfx);
>>   		if (gdata->error_data_length >=
>>   		    sizeof(struct cper_sec_mem_err_old))
>> @@ -419,7 +450,9 @@ static void cper_estatus_print_section(
>>   		else
>>   			goto err_section_too_small;
>>   	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
>> -		struct cper_sec_pcie *pcie = (void *)(gdata + 1);
>> +		struct cper_sec_pcie *pcie;
>> +
>> +		pcie = acpi_hest_generic_data_payload(gdata);
>>   		printk("%s""section_type: PCIe error\n", newpfx);
>>   		if (gdata->error_data_length >= sizeof(*pcie))
>>   			cper_print_pcie(newpfx, pcie, gdata);
>> @@ -438,7 +471,7 @@ void cper_estatus_print(const char *pfx,
>>   			const struct acpi_hest_generic_status *estatus)
>>   {
>>   	struct acpi_hest_generic_data *gdata;
>> -	unsigned int data_len, gedata_len;
>> +	unsigned int data_len;
>>   	int sec_no = 0;
>>   	char newpfx[64];
>>   	__u16 severity;
>> @@ -451,12 +484,12 @@ void cper_estatus_print(const char *pfx,
>>   	printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
>>   	data_len = estatus->data_length;
>>   	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
>> +
>>   	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
>> -	while (data_len >= sizeof(*gdata)) {
>> -		gedata_len = gdata->error_data_length;
>> +
>> +	while (data_len >= acpi_hest_generic_data_size(gdata)) {
>>   		cper_estatus_print_section(newpfx, gdata, sec_no);
>> -		data_len -= gedata_len + sizeof(*gdata);
>> -		gdata = (void *)(gdata + 1) + gedata_len;
>> +		gdata = acpi_hest_generic_data_next(gdata);
>>   		sec_no++;
>>   	}
>>   }
>> @@ -486,12 +519,13 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>>   		return rc;
>>   	data_len = estatus->data_length;
>>   	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
>> -	while (data_len >= sizeof(*gdata)) {
>> -		gedata_len = gdata->error_data_length;
>> -		if (gedata_len > data_len - sizeof(*gdata))
>> +
>> +	while (data_len >= acpi_hest_generic_data_size(gdata)) {
>> +		gedata_len = acpi_hest_generic_data_error_length(gdata);
>> +		if (gedata_len > data_len - acpi_hest_generic_data_size(gdata))
>>   			return -EINVAL;
>> -		data_len -= gedata_len + sizeof(*gdata);
>> -		gdata = (void *)(gdata + 1) + gedata_len;
>> +		data_len -= gedata_len + acpi_hest_generic_data_size(gdata);
>> +		gdata = acpi_hest_generic_data_next(gdata);
>>   	}
>>   	if (data_len)
>>   		return -EINVAL;
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 68f088a..56b9679 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -73,3 +73,13 @@ static inline void ghes_edac_unregister(struct ghes *ghes)
>>   {
>>   }
>>   #endif
>> +
>> +#define acpi_hest_generic_data_version(gdata)			\
>> +	(gdata->revision >> 8)
>> +
>> +static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
>> +{
>> +	return acpi_hest_generic_data_version(gdata) >= 3 ?
>> +		(void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
>> +		gdata + 1;
>> +}
>> diff --git a/include/linux/cper.h b/include/linux/cper.h
>> index dcacb1a..13ea41c 100644
>> --- a/include/linux/cper.h
>> +++ b/include/linux/cper.h
>> @@ -255,6 +255,18 @@ enum {
>>   
>>   #define CPER_PCIE_SLOT_SHIFT			3
>>   
>> +#define acpi_hest_generic_data_error_length(gdata)	\
>> +	(((struct acpi_hest_generic_data *)(gdata))->error_data_length)
>> +#define acpi_hest_generic_data_size(gdata)		\
>> +	((acpi_hest_generic_data_version(gdata) >= 3) ?	\
>> +	sizeof(struct acpi_hest_generic_data_v300) :	\
>> +	sizeof(struct acpi_hest_generic_data))
>> +#define acpi_hest_generic_data_record_size(gdata)	\
>> +	(acpi_hest_generic_data_size(gdata) +		\
>> +	acpi_hest_generic_data_error_length(gdata))
>> +#define acpi_hest_generic_data_next(gdata)		\
>> +	((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))
>> +
> How come these aren't in ghes.h?

It probably does make more sense to add these in ghes.h, I'll move them 
there in the next set.

> Reviewed-by: James Morse <james.morse@arm.com>
>
Thanks!
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

^ permalink raw reply

* [PATCH 0/2] minor GXL and GXM improvements
From: Kevin Hilman @ 2016-11-28 19:08 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161123162040.24843-1-martin.blumenstingl@googlemail.com>

Hi Martin,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> This series adds SCPI support to GXL and GXM SoCs by moving the nodes
> to meson-gx.dtsi. Additionally this updates the compatible string to
> match the recent changes, see [0]

Can you rebase onto my current v4.10/dt64 branch please?  I have updated
the compatible string there.

Thanks,

Kevin

^ permalink raw reply

* [PATCH 0/2] arm64: dts: NS2: Add GICv2m and PAXC
From: Jon Mason @ 2016-11-28 19:31 UTC (permalink / raw)
  To: linux-arm-kernel

Add support for GICv2m and PAXC.  GICv2m was tested on an e1000e
adapter, with some hacking of the driver to verify MSI and legacy
interrupts work.

Jon Mason (2):
  arm64: dts: NS2: enable GICv2m for PAXB/PAXC interfaces
  arm64: dts: NS2: enable PAXC on NS2 SVK

 arch/arm64/boot/dts/broadcom/ns2-svk.dts |   4 +
 arch/arm64/boot/dts/broadcom/ns2.dtsi    | 121 +++++++++++++++++++++++++------
 2 files changed, 101 insertions(+), 24 deletions(-)

-- 
2.7.4

^ permalink raw reply

* [PATCH 1/2] arm64: dts: NS2: enable GICv2m for PAXB/PAXC interfaces
From: Jon Mason @ 2016-11-28 19:31 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1480361491-22221-1-git-send-email-jon.mason@broadcom.com>

PAXB and PAXC PCIe interfaces on NS2 have been using the iProc event
queue to handle MSI. With the gicv2m support ready, we should now switch
to gicv2m for MSI handling

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/ns2.dtsi | 104 ++++++++++++++++++++++++++--------
 1 file changed, 80 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
index 4fcdeca..69775a8 100644
--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
@@ -115,7 +115,7 @@
 
 		#interrupt-cells = <1>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 281 IRQ_TYPE_NONE>;
+		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 281 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <0>;
 
@@ -136,18 +136,7 @@
 		phys = <&pci_phy0>;
 		phy-names = "pcie-phy";
 
-		msi-parent = <&msi0>;
-		msi0: msi at 20020000 {
-			compatible = "brcm,iproc-msi";
-			msi-controller;
-			interrupt-parent = <&gic>;
-			interrupts = <GIC_SPI 277 IRQ_TYPE_NONE>,
-				     <GIC_SPI 278 IRQ_TYPE_NONE>,
-				     <GIC_SPI 279 IRQ_TYPE_NONE>,
-				     <GIC_SPI 280 IRQ_TYPE_NONE>;
-			brcm,num-eq-region = <1>;
-			brcm,num-msi-msg-region = <1>;
-		};
+		msi-parent = <&v2m0>;
 	};
 
 	pcie4: pcie at 50020000 {
@@ -156,7 +145,7 @@
 
 		#interrupt-cells = <1>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &gic GIC_SPI 305 IRQ_TYPE_NONE>;
+		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 305 IRQ_TYPE_NONE>;
 
 		linux,pci-domain = <4>;
 
@@ -177,16 +166,7 @@
 		phys = <&pci_phy1>;
 		phy-names = "pcie-phy";
 
-		msi-parent = <&msi4>;
-		msi4: msi at 50020000 {
-			compatible = "brcm,iproc-msi";
-			msi-controller;
-			interrupt-parent = <&gic>;
-			interrupts = <GIC_SPI 301 IRQ_TYPE_NONE>,
-				     <GIC_SPI 302 IRQ_TYPE_NONE>,
-				     <GIC_SPI 303 IRQ_TYPE_NONE>,
-				     <GIC_SPI 304 IRQ_TYPE_NONE>;
-		};
+		msi-parent = <&v2m0>;
 	};
 
 	soc: soc {
@@ -331,6 +311,82 @@
 			      <0x65260000 0x1000>;
 			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0xf) |
 				      IRQ_TYPE_LEVEL_HIGH)>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x652e0000 0x80000>;
+
+			v2m0: v2m at 00000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x00000 0x1000>;
+				arm,msi-base-spi = <72>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m1: v2m at 10000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x10000 0x1000>;
+				arm,msi-base-spi = <88>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m2: v2m at 20000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x20000 0x1000>;
+				arm,msi-base-spi = <104>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m3: v2m at 30000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x30000 0x1000>;
+				arm,msi-base-spi = <120>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m4: v2m at 40000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x40000 0x1000>;
+				arm,msi-base-spi = <136>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m5: v2m at 50000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x50000 0x1000>;
+				arm,msi-base-spi = <152>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m6: v2m at 60000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x60000 0x1000>;
+				arm,msi-base-spi = <168>;
+				arm,msi-num-spis = <16>;
+			};
+
+			v2m7: v2m at 70000 {
+				compatible = "arm,gic-v2m-frame";
+				interrupt-parent = <&gic>;
+				msi-controller;
+				reg = <0x70000 0x1000>;
+				arm,msi-base-spi = <184>;
+				arm,msi-num-spis = <16>;
+			};
 		};
 
 		cci at 65590000 {
-- 
2.7.4

^ permalink raw reply related

* [PATCH 2/2] arm64: dts: NS2: enable PAXC on NS2 SVK
From: Jon Mason @ 2016-11-28 19:31 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1480361491-22221-1-git-send-email-jon.mason@broadcom.com>

This enables the PAXC based PCIe root complex on NS2 SVK. The PAXC based
root complex is connected to internally emulated PCIe endpoints

Signed-off-by: Ray Jui <rjui@broadcom.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/ns2-svk.dts |  4 ++++
 arch/arm64/boot/dts/broadcom/ns2.dtsi    | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
index de8d379..5ae0816 100644
--- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
+++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
@@ -76,6 +76,10 @@
 	status = "ok";
 };
 
+&pcie8 {
+	status = "ok";
+};
+
 &i2c0 {
 	status = "ok";
 };
diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
index 69775a8..96ed47b 100644
--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
@@ -169,6 +169,23 @@
 		msi-parent = <&v2m0>;
 	};
 
+	pcie8: pcie at 60c00000 {
+		compatible = "brcm,iproc-pcie-paxc";
+		reg = <0 0x60c00000 0 0x1000>;
+		linux,pci-domain = <8>;
+
+		bus-range = <0x0 0x1>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x83000000 0 0x00000000 0 0x60000000 0 0x00c00000>;
+
+		status = "disabled";
+
+		msi-parent = <&v2m0>;
+	};
+
 	soc: soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.7.4

^ permalink raw reply related

* Adding a .platform_init callback to sdhci_arasan_ops
From: Mason @ 2016-11-28 19:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAD=FV=XrJHFbpbhsJZAEE-HLMe47Ov=BgvHw8ZtfCazzT75pKQ@mail.gmail.com>

On 28/11/2016 18:46, Doug Anderson wrote:

> As argued in my original patch the field "corecfg_baseclkfreq" is
> documented in the generic Arasan document
> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
> and thus is unlikely to be Rockchip specific.

I downloaded the data sheet, which doesn't mention registers,
but "pins" and "signals". Does that mean it is up to every
platform to decide how to group these wires into individual
registers?

corecfg_baseclkfreq[7:0]
Base Clock Frequency for SD Clock.
This is the frequency of the xin_clk.

How can 8 bits encode a frequency?
Is there an implicit LUT? Is it a MHz count?

"For maximum efficiency this should be around 200 MHz for eMMC
or 208MHz (for SD3.0)"

Regards.

^ permalink raw reply

* [PATCH v9 06/11] arm/arm64: vgic: Implement VGICv3 CPU interface access
From: Christoffer Dall @ 2016-11-28 19:39 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1479906118-15832-7-git-send-email-vijay.kilari@gmail.com>

On Wed, Nov 23, 2016 at 06:31:53PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> VGICv3 CPU interface registers are accessed using
> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> as 64-bit. The cpu MPIDR value is passed along with register id.
> is used to identify the cpu for registers access.
> 
> The VM that supports SEIs expect it on destination machine to handle
> guest aborts and hence checked for ICC_CTLR_EL1.SEIS compatibility.
> Similarly, VM that supports Affinity Level 3 that is required for AArch64
> mode, is required to be supported on destination machine. Hence checked
> for ICC_CTLR_EL1.A3V compatibility.
> 
> The CPU system register handling is spitted into two files

spitted?  Did you mean 'split into' ?

> vgic-sys-reg-common.c and vgic-sys-reg-v3.c.
> The vgic-sys-reg-common.c handles read and write of VGIC CPU registers

So this is weird because everything in virt/kvm/arm/ is exactly supposed
to be common between arm and arm64 already.

I would rather that you had a copy of vgic-sys-reg-v3.c in arch/arm/kvm/
and in arch/arm64/kvm/ each taking care of its own architecture.

But note that I didn't actually require that you implemented support for
GICv3 migration on AArch32 hosts for these patches, I just didn't want
thigns to silently break.

If we cannot test the AArch32 implementation, we should potentially just
make sure that is not supported yet, return a proper error to userspace
and get the AArch64 host implementation correct.

I suggest you move your:
  virt/kvm/arm/vgic/vgic-sys-reg-v3.c to
  arch/arm64/kvm/vgic-sys-reg-v3.c
  
and rename
  virt/kvm/arm/vgic/vgic-sys-reg-common.c to
  virt/kvm/arm/vgic/vgic-sys-reg-v3.c

And then wait with the AArch32 host side for now, but just make sure it
compiles and returns an error as opposed to crashing the system if
someone tries to excercise this interface on an AArch32 host.

> for both AArch64 and AArch32 mode. The vgic-sys-reg-v3.c handles AArch64
> mode and is compiled only for AArch64 mode.
> 
> Updated arch/arm/include/uapi/asm/kvm.h with new definitions
> required to compile for AArch32.
> 
> The version of VGIC v3 specification is define here
> Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  arch/arm/include/uapi/asm/kvm.h         |   2 +
>  arch/arm64/include/uapi/asm/kvm.h       |   3 +
>  arch/arm64/kvm/Makefile                 |   2 +
>  include/kvm/arm_vgic.h                  |   9 ++
>  virt/kvm/arm/vgic/vgic-kvm-device.c     |  28 ++++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c        |  18 +++
>  virt/kvm/arm/vgic/vgic-sys-reg-common.c | 258 ++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c     | 142 ++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-v3.c             |   8 +
>  virt/kvm/arm/vgic/vgic.h                |  22 +++
>  10 files changed, 492 insertions(+)
> 
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 0ae6035..98658d9d 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -186,9 +186,11 @@ struct kvm_arch_memory_slot {
>  			(0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL       4
>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT    0
>  
>  /* KVM_IRQ_LINE irq field index values */
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 56dc08d..91c7137 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>  			(0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
> +
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
>  
>  /* Device Control API on vcpu fd */
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index d50a82a..5c8580e 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -32,5 +32,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-common.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 002f092..730a18a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -71,6 +71,9 @@ struct vgic_global {
>  
>  	/* GIC system register CPU interface */
>  	struct static_key_false gicv3_cpuif;
> +
> +	/* Cache ICH_VTR_EL2 reg value */
> +	u32			ich_vtr_el2;
>  };
>  
>  extern struct vgic_global kvm_vgic_global_state;
> @@ -269,6 +272,12 @@ struct vgic_cpu {
>  	u64 pendbaser;
>  
>  	bool lpis_enabled;
> +
> +	/* Cache guest priority bits */
> +	u32 num_pri_bits;
> +
> +	/* Cache guest interrupt ID bits */
> +	u32 num_id_bits;
>  };
>  
>  extern struct static_key_false vgic_v2_cpuif_trap;
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index bc7de95..b6266fe 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -16,6 +16,7 @@
>  #include <linux/kvm_host.h>
>  #include <kvm/arm_vgic.h>
>  #include <linux/uaccess.h>
> +#include <asm/kvm_emulate.h>
>  #include <asm/kvm_mmu.h>
>  #include "vgic.h"
>  
> @@ -501,6 +502,14 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
>  		if (!is_write)
>  			*reg = tmp32;
>  		break;
> +	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> +		u64 regid;
> +
> +		regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> +		ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> +						  regid, reg);
> +		break;
> +	}
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -534,6 +543,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>  		reg = tmp32;
>  		return vgic_v3_attr_regs_access(dev, attr, &reg, true);
>  	}
> +	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> +		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> +		u64 reg;
> +
> +		if (get_user(reg, uaddr))
> +			return -EFAULT;
> +
> +		return vgic_v3_attr_regs_access(dev, attr, &reg, true);
> +	}
>  	}
>  	return -ENXIO;
>  }
> @@ -560,6 +578,15 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
>  		tmp32 = reg;
>  		return put_user(tmp32, uaddr);
>  	}
> +	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> +		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> +		u64 reg;
> +
> +		ret = vgic_v3_attr_regs_access(dev, attr, &reg, false);
> +		if (ret)
> +			return ret;
> +		return put_user(reg, uaddr);
> +	}
>  	}
>  
>  	return -ENXIO;
> @@ -578,6 +605,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  		break;
>  	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>  	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +	case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
>  		return vgic_v3_has_attr_regs(dev, attr);
>  	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>  		return 0;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 2a7cd62..2f7b4ed 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -641,6 +641,24 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>  		nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
>  		break;
>  	}
> +	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> +		u64 reg, id;
> +		unsigned long vgic_mpidr, mpidr_reg;
> +		struct kvm_vcpu *vcpu;
> +
> +		vgic_mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
> +			      KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
> +
> +		/* Convert plain mpidr value to MPIDR reg format */
> +		mpidr_reg = VGIC_TO_MPIDR(vgic_mpidr);
> +
> +		vcpu = kvm_mpidr_to_vcpu(dev->kvm, mpidr_reg);
> +		if (!vcpu)
> +			return -EINVAL;
> +
> +		id = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> +		return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, id, &reg);
> +	}
>  	default:
>  		return -ENXIO;
>  	}
> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-common.c b/virt/kvm/arm/vgic/vgic-sys-reg-common.c
> new file mode 100644
> index 0000000..a1fc370c
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-common.c
> @@ -0,0 +1,258 @@
> +/*
> + * VGIC system registers handling functions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms 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 <linux/irqchip/arm-gic-v3.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <asm/kvm_emulate.h>
> +#include "vgic.h"
> +
> +bool access_gic_ctlr_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 unsigned long *reg)
> +{
> +	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
> +	struct vgic_vmcr vmcr;
> +	u64 val;
> +	u32 valid_bits, seis, a3v;
> +
> +	vgic_get_vmcr(vcpu, &vmcr);
> +	if (is_write) {
> +		val = *reg;
> +
> +		/*
> +		 * Disallow restoring VM state if not supported by this
> +		 * hardware.
> +		 */
> +		valid_bits = ((val & ICC_CTLR_EL1_PRI_BITS_MASK) >>
> +				ICC_CTLR_EL1_PRI_BITS_SHIFT) + 1;
> +		if (valid_bits > vgic_v3_cpu->num_pri_bits)
> +			return false;
> +
> +		vgic_v3_cpu->num_pri_bits = valid_bits;
> +
> +		valid_bits = (val & ICC_CTLR_EL1_ID_BITS_MASK) >>
> +			       ICC_CTLR_EL1_ID_BITS_SHIFT;
> +		if (valid_bits > vgic_v3_cpu->num_id_bits)
> +			return false;
> +
> +		vgic_v3_cpu->num_id_bits = valid_bits;

nit: could you just define the variables your need instead of reusing
this valid_bits thing, that would make the code a lot more readable.


> +
> +		valid_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
> +			ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT);

it's especially confusing that you now use valid_bits for the host's
values...

> +		seis = (val & ICC_CTLR_EL1_SEIS_MASK) >>
> +			ICC_CTLR_EL1_SEIS_SHIFT;
> +		if (valid_bits != seis)
> +			return false;



> +
> +		valid_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
> +			ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT);
> +		a3v = (val & ICC_CTLR_EL1_A3V_MASK) >>
> +			ICC_CTLR_EL1_A3V_SHIFT;
> +		if (valid_bits != a3v)
> +			return false;
> +
> +		vmcr.ctlr = (val & ICC_CTLR_EL1_CBPR_MASK);
> +		vmcr.ctlr |= (val & ICC_CTLR_EL1_EOImode_MASK);
> +		vgic_set_vmcr(vcpu, &vmcr);
> +	} else {
> +		val = 0;
> +		val |= (vgic_v3_cpu->num_pri_bits - 1) <<
> +			ICC_CTLR_EL1_PRI_BITS_SHIFT;
> +		val |= vgic_v3_cpu->num_id_bits <<
> +			ICC_CTLR_EL1_ID_BITS_SHIFT;
> +		val |= ((kvm_vgic_global_state.ich_vtr_el2 &
> +			ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT) <<
> +			ICC_CTLR_EL1_SEIS_SHIFT;
> +		val |= ((kvm_vgic_global_state.ich_vtr_el2 &
> +			ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT) <<
> +			ICC_CTLR_EL1_A3V_SHIFT;
> +		val |= (vmcr.ctlr & ICC_CTLR_EL1_CBPR_MASK);
> +		val |= (vmcr.ctlr & ICC_CTLR_EL1_EOImode_MASK);
> +
> +		*reg = val;
> +	}
> +
> +	return true;
> +}
> +
> +bool access_gic_pmr_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			unsigned long *reg)
> +{
> +	struct vgic_vmcr vmcr;
> +
> +	vgic_get_vmcr(vcpu, &vmcr);
> +	if (is_write) {
> +		vmcr.pmr = (*reg & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT;
> +		vgic_set_vmcr(vcpu, &vmcr);
> +	} else {
> +		*reg = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK;
> +	}
> +
> +	return true;
> +}
> +
> +bool access_gic_bpr0_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 unsigned long *reg)
> +{
> +	struct vgic_vmcr vmcr;
> +
> +	vgic_get_vmcr(vcpu, &vmcr);
> +	if (is_write) {
> +		vmcr.bpr = (*reg & ICC_BPR0_EL1_MASK) >>
> +			    ICC_BPR0_EL1_SHIFT;
> +		vgic_set_vmcr(vcpu, &vmcr);
> +	} else {
> +		*reg = (vmcr.bpr << ICC_BPR0_EL1_SHIFT) & ICC_BPR0_EL1_MASK;
> +	}
> +
> +	return true;
> +}
> +
> +bool access_gic_bpr1_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 unsigned long *reg)
> +{
> +	struct vgic_vmcr vmcr;
> +
> +	if (!is_write)
> +		*reg = 0;
> +
> +	vgic_get_vmcr(vcpu, &vmcr);
> +	if (!((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT)) {
> +		if (is_write) {
> +			vmcr.abpr = (*reg & ICC_BPR1_EL1_MASK) >>
> +				     ICC_BPR1_EL1_SHIFT;
> +			vgic_set_vmcr(vcpu, &vmcr);
> +		} else {
> +			*reg = (vmcr.abpr << ICC_BPR1_EL1_SHIFT) &
> +				ICC_BPR1_EL1_MASK;
> +		}
> +	} else {
> +		if (!is_write)
> +			*reg = min((vmcr.bpr + 1), 7U);
> +	}
> +
> +	return true;
> +}
> +
> +bool access_gic_grpen0_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			   unsigned long *reg)
> +{
> +	struct vgic_vmcr vmcr;
> +
> +	vgic_get_vmcr(vcpu, &vmcr);
> +	if (is_write) {
> +		vmcr.grpen0 = (*reg & ICC_IGRPEN0_EL1_MASK) >>
> +			       ICC_IGRPEN0_EL1_SHIFT;
> +		vgic_set_vmcr(vcpu, &vmcr);
> +	} else {
> +		*reg = (vmcr.grpen0 << ICC_IGRPEN0_EL1_SHIFT) &
> +			ICC_IGRPEN0_EL1_MASK;
> +	}
> +
> +	return true;
> +}
> +
> +bool access_gic_grpen1_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			   unsigned long *reg)
> +{
> +	struct vgic_vmcr vmcr;
> +
> +	vgic_get_vmcr(vcpu, &vmcr);
> +	if (is_write) {
> +		vmcr.grpen1 = (*reg & ICC_IGRPEN1_EL1_MASK) >>
> +			       ICC_IGRPEN1_EL1_SHIFT;
> +		vgic_set_vmcr(vcpu, &vmcr);
> +	} else {
> +		*reg = (vmcr.grpen1 << ICC_IGRPEN1_EL1_SHIFT) &
> +			ICC_IGRPEN1_EL1_MASK;
> +	}
> +
> +	return true;
> +}
> +
> +static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu, bool is_write,
> +				   u8 apr, u8 idx, unsigned long *reg)
> +{
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +	uint32_t *ap_reg;
> +
> +	if (apr)
> +		ap_reg = &vgicv3->vgic_ap1r[idx];
> +	else
> +		ap_reg = &vgicv3->vgic_ap0r[idx];
> +
> +	if (is_write)
> +		*ap_reg = *reg;
> +	else
> +		*reg = *ap_reg;
> +}
> +
> +static bool access_gic_aprn(struct kvm_vcpu *vcpu, bool is_write, u8 apr,
> +			    u8 idx, unsigned long *reg)
> +{
> +	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
> +
> +	/* num_pri_bits are initialized with HW supported values.
> +	 * We can rely safely on num_pri_bits even if VM has not
> +	 * restored ICC_CTLR_EL1 before restoring APnR registers.
> +	 */

nit: commenting style

> +	switch (vgic_v3_cpu->num_pri_bits) {
> +	case 7:
> +		vgic_v3_access_apr_reg(vcpu, is_write, apr, idx, reg);
> +		break;
> +	case 6:
> +		if (idx > 1)
> +			goto err;
> +		vgic_v3_access_apr_reg(vcpu, is_write, apr, idx, reg);
> +		break;
> +	default:
> +		if (idx > 0)
> +			goto err;
> +		vgic_v3_access_apr_reg(vcpu, is_write, apr, idx, reg);
> +	}

It looks to me like userspace can then program active priorities with
higher numbers than what it will program num_pri_bits to later.  Is that
not weird, or am I missing something?

> +
> +	return true;
> +err:
> +	if (!is_write)
> +		*reg = 0;
> +
> +	return false;
> +}
> +
> +bool access_gic_ap0r_reg(struct kvm_vcpu *vcpu, bool is_write, u8 idx,
> +			 unsigned long *reg)
> +{
> +	return access_gic_aprn(vcpu, is_write, 0, idx, reg);
> +}
> +
> +bool access_gic_ap1r_reg(struct kvm_vcpu *vcpu, bool is_write, u8 idx,
> +			 unsigned long *reg)
> +{
> +	return access_gic_aprn(vcpu, is_write, 1, idx, reg);
> +}
> +
> +bool access_gic_sre_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			unsigned long *reg)
> +{
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	/* Validate SRE bit */
> +	if (is_write) {
> +		if (!(*reg & ICC_SRE_EL1_SRE))
> +			return false;
> +	} else {
> +		*reg = vgicv3->vgic_sre;
> +	}
> +
> +	return true;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> new file mode 100644
> index 0000000..82c2f02
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> @@ -0,0 +1,142 @@
> +/*
> + * VGIC system registers handling functions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms 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 <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <asm/kvm_emulate.h>
> +#include "vgic.h"
> +#include "sys_regs.h"
> +
> +#define ACCESS_SYS_REG(REG)						\
> +static bool access_gic_##REG##_sys_reg(struct kvm_vcpu *vcpu,		\
> +				    struct sys_reg_params *p,		\
> +				    const struct sys_reg_desc *r)	\
> +{									\
> +	unsigned long tmp;						\
> +	bool ret;							\
> +									\
> +	if (p->is_write)						\
> +		tmp = p->regval;					\
> +	ret = access_gic_##REG##_reg(vcpu, p->is_write, &tmp);		\
> +	if (!p->is_write)						\
> +		p->regval = tmp;					\
> +									\
> +	return ret;							\
> +}
> +
> +ACCESS_SYS_REG(ctlr)
> +ACCESS_SYS_REG(pmr)
> +ACCESS_SYS_REG(bpr0)
> +ACCESS_SYS_REG(bpr1)
> +ACCESS_SYS_REG(sre)
> +ACCESS_SYS_REG(grpen0)
> +ACCESS_SYS_REG(grpen1)
> +
> +#define ACCESS_APNR_SYS_REG(REG)					\
> +static bool access_gic_##REG##_sys_reg(struct kvm_vcpu *vcpu,		\
> +				    struct sys_reg_params *p,		\
> +				    const struct sys_reg_desc *r)	\
> +{									\
> +	unsigned long tmp;						\
> +	u8 idx = p->Op2 & 3;						\
> +	bool ret;							\
> +									\
> +	if (p->is_write)						\
> +		tmp = p->regval;					\
> +	ret = access_gic_##REG##_reg(vcpu, p->is_write, idx, &tmp);	\
> +	if (!p->is_write)						\
> +		p->regval = tmp;					\
> +									\
> +	return ret;							\
> +}
> +
> +ACCESS_APNR_SYS_REG(ap0r)
> +ACCESS_APNR_SYS_REG(ap1r)

I don't get these indirections.  Why can't you call the functions
directly?

> +
> +static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
> +	/* ICC_PMR_EL1 */
> +	{ Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr_sys_reg },
> +	/* ICC_BPR0_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0_sys_reg },
> +	/* ICC_AP0R0_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r_sys_reg },
> +	/* ICC_AP0R1_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r_sys_reg },
> +	/* ICC_AP0R2_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r_sys_reg },
> +	/* ICC_AP0R3_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r_sys_reg },
> +	/* ICC_AP1R0_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r_sys_reg },
> +	/* ICC_AP1R1_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r_sys_reg },
> +	/* ICC_AP1R2_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r_sys_reg },
> +	/* ICC_AP1R3_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r_sys_reg },
> +	/* ICC_BPR1_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1_sys_reg },
> +	/* ICC_CTLR_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr_sys_reg },
> +	/* ICC_SRE_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(12), Op2(5), access_gic_sre_sys_reg },
> +	/* ICC_IGRPEN0_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0_sys_reg },
> +	/* ICC_GRPEN1_EL1 */
> +	{ Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1_sys_reg },
> +};
> +
> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> +				u64 *reg)
> +{
> +	struct sys_reg_params params;
> +	u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> +
> +	params.regval = *reg;
> +	params.is_write = is_write;
> +	params.is_aarch32 = false;
> +	params.is_32bit = false;
> +
> +	if (find_reg_by_id(sysreg, &params, gic_v3_icc_reg_descs,
> +			      ARRAY_SIZE(gic_v3_icc_reg_descs)))
> +		return 0;
> +
> +	return -ENXIO;
> +}
> +
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> +				u64 *reg)
> +{
> +	struct sys_reg_params params;
> +	const struct sys_reg_desc *r;
> +	u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> +
> +	if (is_write)
> +		params.regval = *reg;
> +	params.is_write = is_write;
> +	params.is_aarch32 = false;
> +	params.is_32bit = false;
> +
> +	r = find_reg_by_id(sysreg, &params, gic_v3_icc_reg_descs,
> +			   ARRAY_SIZE(gic_v3_icc_reg_descs));
> +	if (!r)
> +		return -ENXIO;
> +
> +	if (!r->access(vcpu, &params, r))
> +		return -EINVAL;
> +
> +	if (!is_write)
> +		*reg = params.regval;
> +
> +	return 0;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index a3ff04b..6e7400e 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -240,6 +240,13 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
>  		vgic_v3->vgic_sre = 0;
>  	}
>  
> +	vcpu->arch.vgic_cpu.num_id_bits = (kvm_vgic_global_state.ich_vtr_el2 &
> +					   ICH_VTR_ID_BITS_MASK) >>
> +					   ICH_VTR_ID_BITS_SHIFT;
> +	vcpu->arch.vgic_cpu.num_pri_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
> +					    ICH_VTR_PRI_BITS_MASK) >>
> +					    ICH_VTR_PRI_BITS_SHIFT) + 1;
> +
>  	/* Get the show on the road... */
>  	vgic_v3->vgic_hcr = ICH_HCR_EN;
>  }
> @@ -340,6 +347,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>  	 */
>  	kvm_vgic_global_state.nr_lr = (ich_vtr_el2 & 0xf) + 1;
>  	kvm_vgic_global_state.can_emulate_gicv2 = false;
> +	kvm_vgic_global_state.ich_vtr_el2 = ich_vtr_el2;
>  
>  	if (!info->vcpu.start) {
>  		kvm_info("GICv3: no GICV resource entry\n");
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 9232791..af23399 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -140,6 +140,28 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>  			 int offset, u32 *val);
>  int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>  			 int offset, u32 *val);
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 u64 id, u64 *val);
> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> +				u64 *reg);
> +bool access_gic_ctlr_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 unsigned long *reg);
> +bool access_gic_pmr_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			unsigned long *reg);
> +bool access_gic_bpr0_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 unsigned long *reg);
> +bool access_gic_bpr1_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 unsigned long *reg);
> +bool access_gic_grpen0_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			   unsigned long *reg);
> +bool access_gic_grpen1_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			   unsigned long *reg);
> +bool access_gic_ap0r_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 u8 idx, unsigned long *reg);
> +bool access_gic_ap1r_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			 u8 idx, unsigned long *reg);
> +bool access_gic_sre_reg(struct kvm_vcpu *vcpu, bool is_write,
> +			unsigned long *reg);

nit: since all of this is exported, I would name them vgic_access_ctlr()
and so on. The _reg postfix is probably also unnecessary for all of
these.

>  int kvm_register_vgic_device(unsigned long type);
>  void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>  void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
> -- 
> 1.9.1
> 

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
From: Christoffer Dall @ 2016-11-28 19:42 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <799d03f5-a929-9547-1ae7-94026b76f116@arm.com>

On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
> On 28/11/16 17:43, Marc Zyngier wrote:
> > Hi Jintack,
> > 
> > On 28/11/16 16:46, Jintack Lim wrote:
> >> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> >> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> >> are 11th and 10th bits respectively when E2H is set.  Current code is
> >> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> >> may allow guest OS to access physical timer. So, fix it.
> >>
> >> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> >> ---
> >>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
> >>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
> >>  include/clocksource/arm_arch_timer.h |  6 ++--
> >>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
> >>  4 files changed, 103 insertions(+), 6 deletions(-)
> >>  create mode 100644 arch/arm/include/asm/kvm_timer.h
> >>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> >>
> 
> [...]
> 
> > We could make it nicer (read "faster") by introducing a
> > hyp_alternate_select construct that only returns a value instead
> > of calling a function. I remember writing something like that
> > at some point, and dropping it...
> 
> So here's what this could look like (warning, wacky code ahead,
> though I fixed a stupid bug that was present in the previous patch).
> The generated code is quite nice (no branch, only an extra mov
> instruction on the default path)... Of course, completely untested!

Isn't this all about determining which bitmask to use, statically, once,
after the system has booted?

How about a good old fashioned static variable, or global struct like
the global one we use for the VGIC, which sets the proper mit mask
during kvm init, and the world-switch code just uses a variable?

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH v9 07/11] arm/arm64: vgic: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
From: Christoffer Dall @ 2016-11-28 19:50 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1479906118-15832-8-git-send-email-vijay.kilari@gmail.com>

On Wed, Nov 23, 2016 at 06:31:54PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> Userspace requires to store and restore of line_level for
> level triggered interrupts using ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  arch/arm/include/uapi/asm/kvm.h     |  7 ++++++
>  arch/arm64/include/uapi/asm/kvm.h   |  6 +++++
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 49 ++++++++++++++++++++++++++++++++++++-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 11 +++++++++
>  virt/kvm/arm/vgic/vgic-mmio.c       | 38 ++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.h       |  5 ++++
>  virt/kvm/arm/vgic/vgic.h            |  2 ++
>  7 files changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 98658d9d..f347779 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -191,6 +191,13 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL       4
>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>  #define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
> +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
> +			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK	0x3ff
> +#define VGIC_LEVEL_INFO_LINE_LEVEL	0
> +
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT    0
>  
>  /* KVM_IRQ_LINE irq field index values */
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 91c7137..4100f8c 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -211,6 +211,12 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>  #define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
> +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
> +			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK	0x3ff
> +#define VGIC_LEVEL_INFO_LINE_LEVEL	0
>  
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
>  
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index b6266fe..52ed00b 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -510,6 +510,25 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
>  						  regid, reg);
>  		break;
>  	}
> +	case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> +		unsigned int info, intid;
> +
> +		info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
> +			KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;
> +		if (info == VGIC_LEVEL_INFO_LINE_LEVEL) {
> +			if (is_write)
> +				tmp32 = *reg;
> +			intid = attr->attr &
> +				KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;
> +			ret = vgic_v3_line_level_info_uaccess(vcpu, is_write,
> +							      intid, &tmp32);
> +			if (!is_write)
> +				*reg = tmp32;

I had a comment here about not having to use the tmp32 by modifying the
line_level_info function, that you seem to have missed.

Hint: The level info is not called from an MMIO path so you should be
able to just write it in a natural way.

> +		} else {
> +			ret = -EINVAL;
> +		}
> +		break;
> +	}
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -552,6 +571,17 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>  
>  		return vgic_v3_attr_regs_access(dev, attr, &reg, true);
>  	}
> +	case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u64 reg;
> +		u32 tmp32;
> +
> +		if (get_user(tmp32, uaddr))
> +			return -EFAULT;
> +
> +		reg = tmp32;
> +		return vgic_v3_attr_regs_access(dev, attr, &reg, true);
> +	}
>  	}
>  	return -ENXIO;
>  }
> @@ -587,8 +617,18 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
>  			return ret;
>  		return put_user(reg, uaddr);
>  	}
> -	}
> +	case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u64 reg;
> +		u32 tmp32;
>  
> +		ret = vgic_v3_attr_regs_access(dev, attr, &reg, false);
> +		if (ret)
> +			return ret;
> +		tmp32 = reg;
> +		return put_user(tmp32, uaddr);
> +	}
> +	}
>  	return -ENXIO;
>  }
>  
> @@ -609,6 +649,13 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  		return vgic_v3_has_attr_regs(dev, attr);
>  	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>  		return 0;
> +	case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> +		if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
> +		      KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) ==
> +		      VGIC_LEVEL_INFO_LINE_LEVEL)
> +			return 0;
> +		break;
> +	}
>  	case KVM_DEV_ARM_VGIC_GRP_CTRL:
>  		switch (attr->attr) {
>  		case KVM_DEV_ARM_VGIC_CTRL_INIT:
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 2f7b4ed..4d7d93d 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -808,3 +808,14 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>  		return vgic_uaccess(vcpu, &rd_dev, is_write,
>  				    offset, val);
>  }
> +
> +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +				    u32 intid, u32 *val)
> +{
> +	if (is_write)
> +		vgic_write_irq_line_level_info(vcpu, intid, *val);
> +	else
> +		*val = vgic_read_irq_line_level_info(vcpu, intid);
> +
> +	return 0;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index f81e0e5..d602081 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -371,6 +371,44 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +unsigned long vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid)
> +{
> +	int i;
> +	unsigned long val = 0;
> +
> +	for (i = 0; i < 32; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> +		if (irq->line_level)
> +			val |= (1U << i);
> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +	}
> +
> +	return val;
> +}
> +
> +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> +				    const unsigned long val)
> +{
> +	int i;
> +
> +	for (i = 0; i < 32; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> +		spin_lock(&irq->irq_lock);
> +		if (val & (1U << i)) {
> +			irq->line_level = true;
> +			vgic_queue_irq_unlock(vcpu->kvm, irq);
> +		} else {
> +			irq->line_level = false;
> +			spin_unlock(&irq->irq_lock);
> +		}

I think you also missed my comment about having to keep the pending
state in sync with the level state.

Which means you have to set the pending state when the line_level goes
up, and lower it when it goes down unless soft_pending is also set,
assuming it's configured as a level triggered interrupt.

If it's an edge-triggered interrupt, I think you only need to set the
pending state on a line being asserted and the rest should be adjusted
in case the user restores the configuration state to level triggered
later.

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH v9 0/11] arm/arm64: vgic: Implement API for vGICv3 live migration
From: Christoffer Dall @ 2016-11-28 19:51 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1479906118-15832-1-git-send-email-vijay.kilari@gmail.com>

Hi Vijaya,

On Wed, Nov 23, 2016 at 06:31:47PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> This patchset adds API for saving and restoring
> of VGICv3 registers to support live migration with new vgic feature.
> This API definition is as per version of VGICv3 specification
> Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> 
> The patch 3 & 4 are picked from the Pavel's previous implementation.
> http://www.spinics.net/lists/kvm/msg122040.html
> 
> NOTE: Only compilation tested for AArch32. No hardware to test.

I did not review the 32-bit part, because if you cannot test it, I don't
think we should merge it.

I just want clarity on what happens if someone tries to use this on
32-bit, and I want to make sure it fails gracefully.

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH][v2] arm64: Add DTS support for FSL's LS1012A SoC
From: Leo Li @ 2016-11-28 19:53 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1479320647-24460-1-git-send-email-harninder.rai@nxp.com>

On Wed, Nov 16, 2016 at 12:24 PM, Harninder Rai <harninder.rai@nxp.com> wrote:
> LS1012A features an advanced 64-bit ARM v8 CortexA53 processor
> with 32 KB of parity protected L1-I cache, 32 KB of ECC protected
> L1-D cache, as well as 256 KB of ECC protected L2 cache.
>
> Features summary
>  One 64-bit ARM-v8 Cortex-A53 core with the following capabilities
>   - Arranged as a cluster of one core supporting a 256 KB L2 cache with ECC
>     protection
>   - Speed up to 800 MHz
>   - Parity-protected 32 KB L1 instruction cache and 32 KB L1 data cache
>   - Neon SIMD engine
>   - ARM v8 cryptography extensions
>  One 16-bit DDR3L SDRAM memory controller
>  ARM core-link CCI-400 cache coherent interconnect
>  Cryptography acceleration (SEC)
>  One Configurable x3 SerDes
>  One PCI Express Gen2 controller, supporting x1 operation
>  One serial ATA (SATA Gen 3.0) controller
>  One USB 3.0/2.0 controller with integrated PHY
>
>  Following levels of DTSI/DTS files have been created for the LS1012A
>    SoC family:
>
>            - fsl-ls1012a.dtsi:
>                    DTS-Include file for FSL LS1012A SoC.
>
>            - fsl-ls1012a-frdm.dts:
>                    DTS file for FSL LS1012A FRDM board.
>
>            - fsl-ls1012a-qds.dts:
>                    DTS file for FSL LS1012A QDS board.
>
>            - fsl-ls1012a-rdb.dts:
>                     DTS file for FSL LS1012A RDB board.
>
> Signed-off-by: Harninder Rai <harninder.rai@nxp.com>
> Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@nxp.com>


Hi Shawn,

Any feedback on this version?  Is it still possible for having it in
4.9?  It will be perfect that we can finalize this base platform
device tree soon so that driver developers can work on on-chip device
specific changes on top of it.

Regards,
Leo

^ permalink raw reply

* [GIT PULL] Amlogic 64-bit DT changes for v4.10, round 2
From: Kevin Hilman @ 2016-11-28 20:00 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <m2k2btx2c9.fsf@baylibre.com>

Arnd, Olof,

On Wed, Nov 23, 2016 at 2:23 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>
> Here's a final round of arm64 DT changes for Amlogic family SoCs.
>

I guess it's not the final.  Please ignore this one.  I need to respin
based on some DT comments from Rob H about node names.

Kevin

^ permalink raw reply

* [GIT PULL] Amlogic 64-bit DT changes for v4.10, round 2, v2
From: Kevin Hilman @ 2016-11-28 20:13 UTC (permalink / raw)
  To: linux-arm-kernel

Arnd, Olof,

Here's a final round of arm64 DT changes for Amlogic family SoCs.  This
superceeds the previous "round 2".  The changes are

- Rob H. pointed out some "_" in node names that needed updates
- SCPI compatible string for pre v1.0 (legacy) devices added

Please pull.

Thanks,

Kevin


The following changes since commit ab5b24fdd2d551ea729e8e19e47811a646260331:

  ARM64: dts: meson-gxbb-vega-s95: Add SD/SDIO/MMC and PWM nodes (2016-11-15 12:05:54 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-amlogic.git tags/amlogic-dt64-2-v2

for you to fetch changes up to c681ca42bfb74f74ac3187ca5289df25cb67b67f:

  ARM64: dts: meson-gxbb: add SCPI pre-1.0 compatible (2016-11-28 12:06:31 -0800)

----------------------------------------------------------------
Amlogic 64-bit DT updates for v4.10, round 2
- new SoC support: S912/GXM series (8x A53)
- new boards: Nexbox A1 (S912), Nexbox A95X (S905X)
- resets for 2nd USB PHY
- update SCPI compatible for pre-v1.0 devices

----------------------------------------------------------------
Kevin Hilman (1):
      ARM64: dts: meson-gxbb: add SCPI pre-1.0 compatible

Martin Blumenstingl (1):
      ARM64: dts: meson-gxbb: add the USB reset also to the second USB PHY

Neil Armstrong (3):
      ARM64: dts: Add support for Meson GXM
      ARM64: dts: meson-gxm: Add support for the Nexbox A1
      ARM64: dts: meson-gxl: Add support for Nexbox A95X

 Documentation/devicetree/bindings/arm/amlogic.txt                                  |   8 +++
 arch/arm64/boot/dts/amlogic/Makefile                                               |   4 ++
 arch/arm64/boot/dts/amlogic/{meson-gxl-s905d-p23x.dtsi => meson-gx-p23x-q20x.dtsi} |   4 +-
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi                                        |   3 +-
 arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts                              | 205 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts                               |   3 +-
 arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p231.dts                               |   3 +-
 arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts                                | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts                                |  77 +++++++++++++++++++++++++++
 arch/arm64/boot/dts/amlogic/meson-gxm-s912-q201.dts                                |  58 ++++++++++++++++++++
 arch/arm64/boot/dts/amlogic/meson-gxm.dtsi                                         | 114 +++++++++++++++++++++++++++++++++++++++
 11 files changed, 644 insertions(+), 4 deletions(-)
 rename arch/arm64/boot/dts/amlogic/{meson-gxl-s905d-p23x.dtsi => meson-gx-p23x-q20x.dtsi} (97%)
 create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
 create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
 create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts
 create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxm-s912-q201.dts
 create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxm.dtsi

^ permalink raw reply

* [PATCH 2/2] Add support for the Nexbox A1 board based on the Amlogic S912 SoC.
From: Kevin Hilman @ 2016-11-28 20:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161123225205.re77xff5vcccaltl@rob-hp-laptop>

Rob Herring <robh@kernel.org> writes:

> On Mon, Nov 21, 2016 at 05:29:05PM +0100, Neil Armstrong wrote:
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  Documentation/devicetree/bindings/arm/amlogic.txt  |   1 +
>>  arch/arm64/boot/dts/amlogic/Makefile               |   1 +
>>  .../arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts | 169 +++++++++++++++++++++
>>  3 files changed, 171 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
>
> A few nits below, otherwise:
>
> Acked-by: Rob Herring <robh@kernel.org>
>

Fixed up the changes locally, and applied for v4.10.

Kevin

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox