* [RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore
From: Mark Rutland @ 2016-09-22 12:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543806-19210-1-git-send-email-ard.biesheuvel@linaro.org>
Hi Ard,
On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:
> ================================================================================
> NOTE: this is a work in progress, and not fully functional yet. In particular,
> the actual MMC host protocol methods are stubbed out at the moment, and need to
> be wired up to the Linux device drivers.
> ================================================================================
>
> On mobile and embedded systems, there is usually only a single MMC device for
> non-volatile storage, which sits behind a controller that is owned by the OS at
> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
> since the UEFI runtime services routines expect ownership of the underlying
> device as well.
>
> This series proposes an approach to work around this. It implements the UEFI
> MMC host protocol in the kernel, in a way that makes it possible to expose it
> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
> so that the kernel can override it if it decides to expose this functionality
> to the firmware.
At a high level, and assuming a number of details from previous
discussions, I think the general approach of having the kernel mediate
access to the MMC makes sense.
However, I don't think that this series has enough detail for critical
review, even at the interface level. e.g. there is no mention of how
this caters for replay attacks and so on (which the current spec
sidesteps). I'm under the impression that there are mechanisms which
have been discussed for this, and I hope this is simply an oversight.
I also think that this needs to go via the USWG (and to the UEFI spec),
before we can consider using it. I say this because:
* This is critical to correct operation of variable storage as required
for the standard boot flow. This is a major change to the way variable
storage works today, with a number of (security) implications.
* There are others in this space trying to use the same class of
hardware, e.g. FreeBSD. We don't want a Linux-specific interface, nor
do we want a proliferation of interfaces for this purpose.
I have a few other general concerns:
* Identification of the relevant MMC device(s).
Patch 3 suggests a devicetree property. I don't think that the
'linux,' prefix makes sense, and it's not clear what we would do with
ACPI.
Does the MMC device not have some identifier we can query, so that we
can match this up without requiring additional info in ACPI/DT?
* Lifetime guarantees
When is it valid for EFI to call the MMC proxy? Can other services
(e.g. ACPI) call this?
How do we handle kexec/kdump? e.g. how do we teardown the interface
before branching to a new kernel, how do we safely tear down a crashed
kernel's interface, what can we call before doing so?
How do we handle suspend/resume? e.g. is it necessary to re-register
upon resume?
Thanks,
Mark.
> Note that these patches are based on patches in the EFI tree that are queued
> for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
> allows us to sleep in the firmware callbacks.
>
> Prerequisites for using these patches:
>
> * qemu-system-aarch64 built from this branch:
> https://git.linaro.org/people/ard.biesheuvel/qemu.git/shortlog/refs/heads/mach-virt-pl181
> which adds a PL181 SD/MMC controller to the mach-virt model, and exposes it
> via the device tree. It also sets the 'linux,uefi-varstore' property on this
> node.
>
> * UEFI firmware built from this branch:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/mmc-proxy
>
> Build using the following command
> build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuMmcVars.dsc
>
> Run using
> qemu-system-aarch64 \
> -M virt -cpu cortex-a57 -m 2048 \
> -device virtio-blk-device,drive=boot \
> -drive if=none,id=boot,file=fat:<path-of-Image>,format=raw \
> -kernel <edk2-dir>/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd \
> -sd <edk2-dir>/build/edk2/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_VARS.fd \
> -nographic $@
>
> This will give you an UEFI environment which keeps its UEFI variables in the
> emulated MMC volume, and exposes its MMC host protocol in a way that allows
> these patches to hook into it.
>
> Patches #1 and #2 implement the arch specific hooks to preserve/restore the NEON
> registers that the firmware may expect to be preserved across function calls.
>
> Patch #3 implements the plumbing to call back into the kernel from the firmware.
>
> Please comment on whether this approach seems feasible, and in particular, how
> on earth I should wire this up to the actual MMC code.
>
> Thanks,
> Ard.
>
> Ard Biesheuvel (3):
> efi/arm64: add SIMD stash/unstash operations
> efi/arm: add SIMD stash/unstash operations
> efi: implement MMC proxy support for the UEFI variable store
>
> arch/arm/include/asm/efi.h | 11 +
> arch/arm64/include/asm/efi.h | 33 +++
> drivers/firmware/efi/Kconfig | 9 +
> drivers/firmware/efi/Makefile | 1 +
> drivers/firmware/efi/arm-init.c | 2 +
> drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
> include/linux/efi.h | 1 +
> 7 files changed, 279 insertions(+)
> create mode 100644 drivers/firmware/efi/mmc-proxy.c
>
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-22 12:52 UTC (permalink / raw)
To: linux-arm-kernel
We have 2 modes for dealing with interrupts in the ARM world. We can either
handle them all using hardware acceleration through the vgic or we can emulate
a gic in user space and only drive CPU IRQ pins from there.
Unfortunately, when driving IRQs from user space, we never tell user space
about timer events that may result in interrupt line state changes, so we
lose out on timer events if we run with user space gic emulation.
This patch fixes that by syncing user space's view of the vtimer irq line
with the kvm view of that same line.
With this patch I can successfully run edk2 and Linux with user space gic
emulation.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
v1 -> v2:
- Add back curly brace that got lost
v2 -> v3:
- Split into patch set
v3 -> v4:
- Improve documentation
v4 -> v5:
- Rewrite to use pending state sync in sregs (marc)
- Remove redundant checks of vgic_initialized()
- qemu tree to try this out: https://github.com/agraf/u-boot.git no-kvm-irqchip-for-v5
---
Documentation/virtual/kvm/api.txt | 26 ++++++++
arch/arm/include/uapi/asm/kvm.h | 3 +
arch/arm/kvm/arm.c | 14 ++---
arch/arm64/include/uapi/asm/kvm.h | 3 +
include/kvm/arm_arch_timer.h | 2 +-
include/uapi/linux/kvm.h | 6 ++
virt/kvm/arm/arch_timer.c | 129 ++++++++++++++++++++++++++------------
7 files changed, 134 insertions(+), 49 deletions(-)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 739db9a..8049327 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3928,3 +3928,29 @@ In order to use SynIC, it has to be activated by setting this
capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
will disable the use of APIC hardware virtualization even if supported
by the CPU, as it's incompatible with SynIC auto-EOI behavior.
+
+8.3 KVM_CAP_ARM_TIMER
+
+Architectures: arm, arm64
+This capability, if KVM_CHECK_EXTENSION indicates that it is available and no
+in-kernel interrupt controller is in use, means that that the kernel populates
+the vcpu's run->s.regs.kernel_timer_pending field with timers that are currently
+considered pending by kvm.
+
+If active, it also allows user space to propagate its own pending state of timer
+interrupt lines using run->s.regs.user_timer_pending. If those two fields
+mismatch during CPU execution, kvm will exit to user space to give it a chance
+to update its own interrupt pending status. This usually involves triggering
+an interrupt line on a user space emulated interrupt controller.
+
+The fields run->s.regs.kernel_timer_pending and run->s.regs.user_timer_pending
+are available independent of run->kvm_valid_regs or run->kvm_dirty_regs bits.
+If no in-kernel interrupt controller is used and the capability exists, they
+will always be available and used.
+
+Currently the following bits are defined for both bitmaps:
+
+ KVM_ARM_TIMER_VTIMER - virtual timer
+
+Future versions of kvm may implement additional timer events. These will get
+indicated by additional KVM_CAP extensions.
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index a2b3eb3..caad81d 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -105,6 +105,9 @@ struct kvm_debug_exit_arch {
};
struct kvm_sync_regs {
+ /* Used with KVM_CAP_ARM_TIMER */
+ u8 kernel_timer_pending;
+ u8 user_timer_pending;
};
struct kvm_arch_memory_slot {
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 75f130e..dc19221 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_ARM_PSCI_0_2:
case KVM_CAP_READONLY_MEM:
case KVM_CAP_MP_STATE:
+ case KVM_CAP_ARM_TIMER:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
return ret;
}
- /*
- * Enable the arch timers only if we have an in-kernel VGIC
- * and it has been properly initialized, since we cannot handle
- * interrupts from the virtual timer with a userspace gic.
- */
- if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
- ret = kvm_timer_enable(vcpu);
+ ret = kvm_timer_enable(vcpu);
return ret;
}
@@ -588,7 +583,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
*/
preempt_disable();
kvm_pmu_flush_hwstate(vcpu);
- kvm_timer_flush_hwstate(vcpu);
+ if (kvm_timer_flush_hwstate(vcpu)) {
+ ret = -EINTR;
+ run->exit_reason = KVM_EXIT_INTR;
+ }
kvm_vgic_flush_hwstate(vcpu);
local_irq_disable();
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 3051f86..9aac860 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -143,6 +143,9 @@ struct kvm_debug_exit_arch {
#define KVM_GUESTDBG_USE_HW (1 << 17)
struct kvm_sync_regs {
+ /* Used with KVM_CAP_ARM_TIMER */
+ u8 kernel_timer_pending;
+ u8 user_timer_pending;
};
struct kvm_arch_memory_slot {
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index dda39d8..8cd7240 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -63,7 +63,7 @@ void kvm_timer_init(struct kvm *kvm);
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
const struct kvm_irq_level *irq);
void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
+int kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 300ef25..1fc02d7 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -870,6 +870,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_S390_USER_INSTR0 130
#define KVM_CAP_MSI_DEVID 131
#define KVM_CAP_PPC_HTM 132
+#define KVM_CAP_ARM_TIMER 133
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1327,4 +1328,9 @@ struct kvm_assigned_msix_entry {
#define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0)
#define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1)
+/* Available with KVM_CAP_ARM_TIMER */
+
+/* Bits for run->s.regs.{user,kernel}_timer_pending */
+#define KVM_ARM_TIMER_VTIMER (1 << 0)
+
#endif /* __LINUX_KVM_H */
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 4309b60..0c6fc38 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -166,21 +166,36 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
return cval <= now;
}
+/*
+ * Synchronize the timer IRQ state with the interrupt controller.
+ */
static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
{
int ret;
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
- BUG_ON(!vgic_initialized(vcpu->kvm));
-
timer->active_cleared_last = false;
timer->irq.level = new_level;
- trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
+ trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
timer->irq.level);
- ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
- timer->irq.irq,
- timer->irq.level);
- WARN_ON(ret);
+
+ if (irqchip_in_kernel(vcpu->kvm)) {
+ BUG_ON(!vgic_initialized(vcpu->kvm));
+
+ /* Fire the timer in the VGIC */
+ ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
+ timer->irq.irq,
+ timer->irq.level);
+
+ WARN_ON(ret);
+ } else {
+ struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+ /* Populate the timer bitmap for user space */
+ regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
+ if (new_level)
+ regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;
+ }
}
/*
@@ -197,7 +212,8 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
* because the guest would never see the interrupt. Instead wait
* until we call this function from kvm_timer_flush_hwstate.
*/
- if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
+ if ((irqchip_in_kernel(vcpu->kvm) && !vgic_initialized(vcpu->kvm)) ||
+ !timer->enabled)
return -ENODEV;
if (kvm_timer_should_fire(vcpu) != timer->irq.level)
@@ -248,15 +264,20 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
*
* Check if the virtual timer has expired while we were running in the host,
* and inject an interrupt if that was the case.
+ *
+ * Returns:
+ *
+ * 0 - success
+ * 1 - need exit to user space
*/
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
+int kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
bool phys_active;
int ret;
if (kvm_timer_update_state(vcpu))
- return;
+ return 0;
/*
* If we enter the guest with the virtual input level to the VGIC
@@ -275,38 +296,61 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
* to ensure that hardware interrupts from the timer triggers a guest
* exit.
*/
- phys_active = timer->irq.level ||
- kvm_vgic_map_is_active(vcpu, timer->irq.irq);
-
- /*
- * We want to avoid hitting the (re)distributor as much as
- * possible, as this is a potentially expensive MMIO access
- * (not to mention locks in the irq layer), and a solution for
- * this is to cache the "active" state in memory.
- *
- * Things to consider: we cannot cache an "active set" state,
- * because the HW can change this behind our back (it becomes
- * "clear" in the HW). We must then restrict the caching to
- * the "clear" state.
- *
- * The cache is invalidated on:
- * - vcpu put, indicating that the HW cannot be trusted to be
- * in a sane state on the next vcpu load,
- * - any change in the interrupt state
- *
- * Usage conditions:
- * - cached value is "active clear"
- * - value to be programmed is "active clear"
- */
- if (timer->active_cleared_last && !phys_active)
- return;
-
- ret = irq_set_irqchip_state(host_vtimer_irq,
- IRQCHIP_STATE_ACTIVE,
- phys_active);
- WARN_ON(ret);
+ if (irqchip_in_kernel(vcpu->kvm)) {
+ phys_active = timer->irq.level ||
+ kvm_vgic_map_is_active(vcpu, timer->irq.irq);
+
+ /*
+ * We want to avoid hitting the (re)distributor as much as
+ * possible, as this is a potentially expensive MMIO access
+ * (not to mention locks in the irq layer), and a solution for
+ * this is to cache the "active" state in memory.
+ *
+ * Things to consider: we cannot cache an "active set" state,
+ * because the HW can change this behind our back (it becomes
+ * "clear" in the HW). We must then restrict the caching to
+ * the "clear" state.
+ *
+ * The cache is invalidated on:
+ * - vcpu put, indicating that the HW cannot be trusted to be
+ * in a sane state on the next vcpu load,
+ * - any change in the interrupt state
+ *
+ * Usage conditions:
+ * - cached value is "active clear"
+ * - value to be programmed is "active clear"
+ */
+ if (timer->active_cleared_last && !phys_active)
+ return 0;
+
+ ret = irq_set_irqchip_state(host_vtimer_irq,
+ IRQCHIP_STATE_ACTIVE,
+ phys_active);
+ WARN_ON(ret);
+ } else {
+ struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+ /*
+ * User space handles timer events, so we need to check whether
+ * its view of the world is in sync with ours.
+ */
+ if (regs->kernel_timer_pending != regs->user_timer_pending) {
+ /* Return to user space */
+ return 1;
+ }
+
+ /*
+ * As long as user space is aware that the timer is pending,
+ * we do not need to get new host timer events.
+ */
+ if (timer->irq.level)
+ disable_percpu_irq(host_vtimer_irq);
+ else
+ enable_percpu_irq(host_vtimer_irq, 0);
+ }
timer->active_cleared_last = !phys_active;
+ return 0;
}
/**
@@ -479,6 +523,10 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
if (timer->enabled)
return 0;
+ /* No need to route physical IRQs when we don't use the vgic */
+ if (!irqchip_in_kernel(vcpu->kvm))
+ goto no_vgic;
+
/*
* Find the physical IRQ number corresponding to the host_vtimer_irq
*/
@@ -502,6 +550,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
if (ret)
return ret;
+no_vgic:
/*
* There is a potential race here between VCPUs starting for the first
--
1.8.5.6
^ permalink raw reply related
* [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case
From: Lorenzo Pieralisi @ 2016-09-22 12:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F8815A9@lhreml507-mbx>
On Thu, Sep 22, 2016 at 11:10:13AM +0000, Gabriele Paoloni wrote:
> Hi Lorenzo, Bjorn
>
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > Sent: 22 September 2016 10:50
> > To: Bjorn Helgaas
> > Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> > Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya;
> > Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin
> > Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> > pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Linaro ACPI
> > Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> > (C); Gabriele Paoloni; Jeff Hugo; linux-acpi at vger.kernel.org; linux-
> > kernel at vger.kernel.org; Rafael J. Wysocki
> > Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> > specific register range for ACPI case
> >
> > On Wed, Sep 21, 2016 at 01:04:57PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:
> > > > On Tue, Sep 20, 2016 at 02:17:44PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:
> > > >
> > > > [...]
> > > >
> > > > > > None of these platforms can be fixed entirely in software, and
> > given
> > > > > > that we will not be adding quirks for new broken hardware, we
> > should
> > > > > > ask ourselves whether having two versions of a quirk, i.e., one
> > for
> > > > > > broken hardware + currently shipping firmware, and one for the
> > same
> > > > > > broken hardware with fixed firmware is really an improvement
> > over what
> > > > > > has been proposed here.
> > > > >
> > > > > We're talking about two completely different types of quirks:
> > > > >
> > > > > 1) MCFG quirks to use memory-mapped config space that doesn't
> > quite
> > > > > conform to the ECAM model in the PCIe spec, and
> > > > >
> > > > > 2) Some yet-to-be-determined method to describe address space
> > > > > consumed by a bridge.
> > > > >
> > > > > The first two patches of this series are a nice implementation
> > for 1).
> > > > > The third patch (ThunderX-specific) is one possibility for 2),
> > but I
> > > > > don't like it because there's no way for generic software like
> > the
> > > > > ACPI core to discover these resources.
> > > >
> > > > Ok, so basically this means that to implement (2) we need to assign
> > > > some sort of _HID to these quirky PCI bridges (so that we know what
> > > > device they represent and we can retrieve their _CRS). I take from
> > > > this discussion that the goal is to make sure that all non-config
> > > > resources have to be declared through _CRS device objects, which is
> > > > fine but that requires a FW update (unless we can fabricate ACPI
> > > > devices and corresponding _CRS in the kernel whenever we match a
> > > > given MCFG table signature).
> > >
> > > All resources consumed by ACPI devices should be declared through
> > > _CRS. If you want to fabricate ACPI devices or _CRS via kernel
> > > quirks, that's fine with me. This could be triggered via MCFG
> > > signature, DMI info, host bridge _HID, etc.
> >
> > I think the PNP quirk approach + PNP0c02 resource put forward by Gab
> > is enough.
>
> Great thanks as we take a final decision I will ask Dogndgong to submit
> another RFC based on this approach
>
> >
> > > > We discussed this already and I think we should make a decision:
> > > >
> > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-
> > March/414722.html
> > > >
> > > > > > > I'd like to step back and come up with some understanding of
> > how
> > > > > > > non-broken firmware *should* deal with this issue. Then, if
> > we *do*
> > > > > > > work around this particular broken firmware in the kernel, it
> > would be
> > > > > > > nice to do it in a way that fits in with that understanding.
> > > > > > >
> > > > > > > For example, if a companion ACPI device is the preferred
> > solution, an
> > > > > > > ACPI quirk could fabricate a device with the required
> > resources. That
> > > > > > > would address the problem closer to the source and make it
> > more likely
> > > > > > > that the rest of the system will work correctly: /proc/iomem
> > could
> > > > > > > make sense, things that look at _CRS generically would work
> > (e.g,
> > > > > > > /sys/, an admittedly hypothetical "lsacpi", etc.)
> > > > > > >
> > > > > > > Hard-coding stuff in drivers is a point solution that doesn't
> > provide
> > > > > > > any guidance for future platforms and makes it likely that
> > the hack
> > > > > > > will get copied into even more drivers.
> > > > > > >
> > > > > >
> > > > > > OK, I see. But the guidance for future platforms should be 'do
> > not
> > > > > > rely on quirks', and what I am arguing here is that the more we
> > polish
> > > > > > up this code and make it clean and reusable, the more likely it
> > is
> > > > > > that will end up getting abused by new broken hardware that we
> > set out
> > > > > > to reject entirely in the first place.
> > > > > >
> > > > > > So of course, if the quirk involves claiming resources, let's
> > make
> > > > > > sure that this occurs in the cleanest and most compliant way
> > possible.
> > > > > > But any factoring/reuse concerns other than for the current
> > crop of
> > > > > > broken hardware should be avoided imo.
> > > > >
> > > > > If future hardware is completely ECAM-compliant and we don't need
> > any
> > > > > more MCFG quirks, that would be great.
> > > >
> > > > Yes.
> > > >
> > > > > But we'll still need to describe that memory-mapped config space
> > > > > somewhere. If that's done with PNP0C02 or similar devices (as is
> > done
> > > > > on my x86 laptop), we'd be all set.
> > > >
> > > > I am not sure I understand what you mean here. Are you referring
> > > > to MCFG regions reported as PNP0c02 resources through its _CRS ?
> > >
> > > Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges
> > > reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02
> > > device.
> >
> > Ok, that's agreed. It goes without saying that since you are quoting
> > the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device
> > _CRS I will consider that a FW bug.
> >
> > > > IIUC PNP0C02 is a reservation mechanism, but it does not help us
> > > > associate its _CRS to a specific PCI host bridge instance, right ?
> > >
> > > Gab proposed a hierarchy that *would* associate a PNP0C02 device with
> > > a PCI bridge:
> > >
> > > Device (PCI1) {
> > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > Method (_CRS, 0, Serialized) { // Root complex resources
> > (windows) }
> > > Device (RES0) {
> > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > Name (_CRS, ResourceTemplate () { ... }
> > > }
> > > }
> > >
> > > That's a possibility. The PCI Firmware Spec suggests putting RES0 at
> > > the root (under \_SB), but I don't know why.
> > >
> > > Putting it at the root means we couldn't generically associate it
> > with
> > > a bridge, although I could imagine something like this:
> > >
> > > Device (RES1) {
> > > Name (_HID, "HISI0081") // HiSi PCIe RC config base address
> > > Name (_CID, "PNP0C02") // Motherboard reserved resource
> > > Name (_CRS, ResourceTemplate () { ... }
> > > Method (BRDG) { "PCI1" } // hand-wavy ASL
> > > }
> > > Device (PCI1) {
> > > Name (_HID, "HISI0080") // PCI Express Root Bridge
> > > Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
> > > Method (_CRS, 0, Serialized) { // Root complex resources
> > (windows) }
> > > }
> > >
> > > Where you could search PNP0C02 devices for a cookie that matched the
> > > host bridge.o
> >
> > Ok, I am fine with both and I think we are converging, but the way
> > to solve this problem has to be uniform for all ARM partners (and
> > not only ARM). Two points here:
> >
> > 1) Adding a device/subdevice allows people to add a _CRS reporting the
> > non-window bridge resources. Fine. It also allows people to chuck in
> > there all sorts of _DSD properties to describe their PCI host bridge
> > as it is done with DT properties (those _DSD can contain eg clocks
> > etc.), this may be tempting (so that they can reuse the same DT
> > driver and do not have to update their firmware) but I want to be
> > clear here: that must not happen. So, a subdevice with a _CRS to
> > report resources, yes, but it will stop there.
> > 2) It is unclear to me how to formalize the above. People should not
> > write FW by reading the PCI mailing list, so these guidelines have
> > to
> > be written, somehow. I do not want to standardize quirks, I want
> > to prevent random ACPI table content, which is different.
> > Should I report this to the ACPI spec working group ? If we do
> > not do that everyone will go solve this problem as they deem fit.
> >
>
> Do we really need to formalize this?
>
> As we discussed in the Linaro call at the moment we have few vendors
> that need quirks and we want to avoid promoting/accepting quirks for
> the future.
>
> At the time of the call I think we decided to informally accept a set
> of quirks for the current platforms and reject any other quirk coming
> after a certain date/kernel version (this to be decided).
>
> I am not sure if there is a way to document/formalize a temporary
> exception from the rule...
- (1) will be enforced.
- We do not know whether PNP0c02 can be used in non-root devices _CRS
- Are we sure (given that we are implementing this to make sure we are
able to validate resources) that it is valid to have a subdevice with
a _CRS whose resources are not contained in its parent _CRS address
space (because that's exactly the case for these quirks) ?
That's what I mean by formalizing, I want to know how PNP0c02 should
be used. We all want platforms with quirks to be enabled asap but only
if we stick to the ACPI specifications. On top of that, with the
bindings above, the kernel would end up creating a platform device for
the "fake" device with a _CRS approach, which is questionable.
Lorenzo
>
> Thanks
>
> Gab
>
>
> > [...]
> >
> > > > For FW that is immutable I really do not see what we can do apart
> > > > from hardcoding the non-config resources (consumed by a bridge),
> > > > somehow.
> > >
> > > Right. Well, I assume you mean we should hard-code "non-window
> > > resources consumed directly by a bridge". If firmware in the field
> > is
> > > broken, we should work around it, and that may mean hard-coding some
> > > resources.
> > >
> > > My point is that the hard-coding should not be buried in a driver
> > > where it's invisible to the rest of the kernel. If we hard-code it
> > in
> > > a quirk that adds _CRS entries, then the kernel will work just like
> > it
> > > would if the firmware had been correct in the first place. The
> > > resource will appear in /sys/devices/pnp*/*/resources and
> > /proc/iomem,
> > > and if we ever used _SRS to assign or move ACPI devices, we would
> > know
> > > to avoid the bridge resource.
> >
> > We are in complete agreement here.
> >
> > Thanks,
> > Lorenzo
>
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Marc Zyngier @ 2016-09-22 12:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E3CF5A.3050608@suse.de>
On 22/09/16 13:32, Alexander Graf wrote:
> On 09/20/2016 11:21 AM, Marc Zyngier wrote:
>> On 19/09/16 18:39, Alexander Graf wrote:
>>>
>>> On 19.09.16 16:48, Marc Zyngier wrote:
>>>
>>>>> +
>>>>> + /*
>>>>> + * So we can just explicitly mask or unmask the IRQ, gaining
>>>>> + * more compatibility with oddball irq controllers.
>>>>> + */
>>>>> + if (phys_active)
>>>>> + disable_percpu_irq(host_vtimer_irq);
>>>>> + else
>>>>> + enable_percpu_irq(host_vtimer_irq, 0);
>>>> Since you are now targeting random irqchips (as opposed to a GIC
>>>> specifically), what guarantees that the timer is a per-cpu IRQ?
>>> This is the host interrupt controller - and we're already using percpu
>>> irqs on it :). Also as it happens the RPi has them percpu (anything else
>>> wouldn't make sense...).
>> Not really. The RPi is faking percpu interrupts just to have some level
>> of compatibility with the host arch timer driver. But nonetheless, if
>> you're opening the code to something else than a GIC, then you should
>> check that the interrupt you're getting is percpu.
>
> This should already be covered by request_percpu_irq() in
> kvm_timer_hyp_init(), no?
Ah, true. Ignore me, then.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-22 12:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E0FF95.7040305@arm.com>
On 09/20/2016 11:21 AM, Marc Zyngier wrote:
> On 19/09/16 18:39, Alexander Graf wrote:
>>
>> On 19.09.16 16:48, Marc Zyngier wrote:
>>
>>>> +
>>>> + /*
>>>> + * So we can just explicitly mask or unmask the IRQ, gaining
>>>> + * more compatibility with oddball irq controllers.
>>>> + */
>>>> + if (phys_active)
>>>> + disable_percpu_irq(host_vtimer_irq);
>>>> + else
>>>> + enable_percpu_irq(host_vtimer_irq, 0);
>>> Since you are now targeting random irqchips (as opposed to a GIC
>>> specifically), what guarantees that the timer is a per-cpu IRQ?
>> This is the host interrupt controller - and we're already using percpu
>> irqs on it :). Also as it happens the RPi has them percpu (anything else
>> wouldn't make sense...).
> Not really. The RPi is faking percpu interrupts just to have some level
> of compatibility with the host arch timer driver. But nonetheless, if
> you're opening the code to something else than a GIC, then you should
> check that the interrupt you're getting is percpu.
This should already be covered by request_percpu_irq() in
kvm_timer_hyp_init(), no?
Alex
^ permalink raw reply
* [PATCH 0/2] BQL support and fix for a regression issue
From: David Miller @ 2016-09-22 12:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474535121-13958-1-git-send-email-sunil.kovvuri@gmail.com>
From: sunil.kovvuri@gmail.com
Date: Thu, 22 Sep 2016 14:35:19 +0530
> From: Sunil Goutham <sgoutham@cavium.com>
>
> These patches add byte queue limit support and also fixes a regression
> issue introduced by commit
> 'net: thunderx: Use netdev's name for naming VF's interrupts'
The correct way to refer to commits when you fix a bug is
to use the "Fixes: " tag right before the signoffs and ACKs.
The correct form is:
Fixes: $SHA1_ID ("Commit header line.")
You'll see this in many other netdev commits.
^ permalink raw reply
* [PATCH v6 6/7] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
From: Marc Zyngier @ 2016-09-22 12:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474351965-11586-7-git-send-email-vijay.kilari@gmail.com>
On 20/09/16 07:12, 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 version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> arch/arm64/include/uapi/asm/kvm.h | 3 +
> arch/arm64/kvm/Makefile | 1 +
> include/kvm/arm_vgic.h | 9 +
> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 327 ++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic-v3.c | 7 +
> virt/kvm/arm/vgic/vgic.h | 4 +
> 8 files changed, 397 insertions(+)
>
> 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..1a14e29 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -32,5 +32,6 @@ 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-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..b986c25 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -71,6 +71,15 @@ struct vgic_global {
>
> /* GIC system register CPU interface */
> struct static_key_false gicv3_cpuif;
> +
> + /* Cache ICH_VTR_EL2 reg value */
> + u32 ich_vtr_el2;
> +
> + /* Cache guest priority bits */
> + u32 num_pri_bits;
> +
> + /* Cache guest interrupt ID bits */
> + u32 num_id_bits;
I'm going to cry now... Have you noticed that you are caching
guest-dependent values in a global structure? What happens when you have
more than one?
Caching the *host* values are fine, but the guest's? Come on...
> };
>
> extern struct vgic_global kvm_vgic_global_state;
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 6c7d30c..da532d1 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -504,6 +504,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;
> @@ -537,6 +545,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> reg = tmp32;
> return vgic_v3_attr_regs_access(dev, attr, ®, 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, ®, true);
> + }
> }
> return -ENXIO;
> }
> @@ -563,6 +580,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, ®, false);
> + if (ret)
> + return ret;
> + return put_user(reg, uaddr);
> + }
> }
>
> return -ENXIO;
> @@ -581,6 +607,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 b35fb83..8f46e61 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -23,6 +23,7 @@
>
> #include "vgic.h"
> #include "vgic-mmio.h"
> +#include "sys_regs.h"
>
> /* extract @num bytes at @offset bytes offset in data */
> unsigned long extract_bytes(u64 data, unsigned int offset,
> @@ -639,6 +640,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(mpidr_reg);
> +
> + 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, ®);
> + }
> default:
> return -ENXIO;
> }
> 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..769920f
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> @@ -0,0 +1,327 @@
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <kvm/iodev.h>
> +#include <kvm/arm_vgic.h>
> +#include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +#include "sys_regs.h"
> +
> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> + u64 val;
> + u32 hw_pri_bits, num_pri_bits, hw_id_bits, num_id_bits;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + val = p->regval;
> +
> + /*
> + * Does not allow update of ICC_CTLR_EL1 if HW does not support
> + * guest programmed ID and PRI bits
> + */
> + hw_pri_bits = ((kvm_vgic_global_state.ich_vtr_el2 &
> + ICH_VTR_PRI_BITS_MASK) >>
> + ICH_VTR_PRI_BITS_SHIFT) + 1;
> + num_pri_bits = ((val & ICC_CTLR_EL1_PRI_BITS_MASK) >>
> + ICC_CTLR_EL1_PRI_BITS_SHIFT) + 1;
> + if (num_pri_bits > hw_pri_bits)
> + return false;
> +
> + kvm_vgic_global_state.num_pri_bits = num_pri_bits;
And here we go...
I've stopped reading here, because you can't possibly have tested this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
From: Arnd Bergmann @ 2016-09-22 12:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F881744@lhreml507-mbx>
On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
> > > I think extending of_empty_ranges_quirk() may be a reasonable
> > solution.
> > > What do you think Arnd?
> >
> > I don't really like that idea, that quirk is meant to work around
> > broken DTs, but we can just make the DT valid and implement the
> > code properly.
>
> Ok I understand your point where it is not right to use of_empty_ranges_quirk()
> As a quirk is used to work around broken HW or broken FW (as in this case)
> rather than to fix code
>
> What about the following? I think adding the check you suggested next to
> of_empty_ranges_quirk() is adding the case we need in the right point (thus
> avoiding any duplication)
>
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
> return NULL;
> }
>
> +static inline int of_isa_indirect_io(struct device_node *np)
> +{
> + /*
> + * check if the current node is an isa bus and if indirectio operation
> + * are registered
> + */
> + return (of_bus_isa_match(np) && arm64_extio_ops);
> +}
> +
> static int of_empty_ranges_quirk(struct device_node *np)
> {
> if (IS_ENABLED(CONFIG_PPC)) {
> @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> * This code is only enabled on powerpc. --gcl
> */
> ranges = of_get_property(parent, rprop, &rlen);
> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> + if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
> pr_debug("OF: no ranges; cannot translate\n");
> return 1;
> }
I don't see what effect that would have. What do you want to
achieve with this?
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node *dev,
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
memset(r, 0, sizeof(struct resource));
+
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+
+ if (taddr == OF_BAD_ADDR)
+ port = arch_of_address_to_pio(dev, addrp)
+ else
+ port = pci_address_to_pio(taddr);
+
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
} else {
+ if (taddr == OF_BAD_ADDR)
+ return -EINVAL;
+
r->start = taddr;
r->end = taddr + size - 1;
}
Arnd
^ permalink raw reply related
* [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
From: Marc Zyngier @ 2016-09-22 12:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474351965-11586-2-git-send-email-vijay.kilari@gmail.com>
On 20/09/16 07:12, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Read and write of some registers like ISPENDR and ICPENDR
> from userspace requires special handling when compared to
> guest access for these registers.
>
> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> for handling of ISPENDR, ICPENDR registers handling.
>
> Add infrastructure to support guest and userspace read
> and write for the required registers
> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ----------
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 ++++++++++++++++++++++++++++++++--------
> virt/kvm/arm/vgic/vgic-mmio.c | 78 ++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++
> 4 files changed, 169 insertions(+), 51 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..0b32f40 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> return -ENXIO;
> }
>
> -/*
> - * When userland tries to access the VGIC register handlers, we need to
> - * create a usable struct vgic_io_device to be passed to the handlers and we
> - * have to set up a buffer similar to what would have happened if a guest MMIO
> - * access occurred, including doing endian conversions on BE systems.
> - */
> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> - bool is_write, int offset, u32 *val)
> -{
> - unsigned int len = 4;
> - u8 buf[4];
> - int ret;
> -
> - if (is_write) {
> - vgic_data_host_to_mmio_bus(buf, len, *val);
> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
> - } else {
> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
> - if (!ret)
> - *val = vgic_data_mmio_bus_to_host(buf, len);
> - }
> -
> - return ret;
> -}
> -
> int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val)
> {
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 0d3c76a..ce2708d 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 value = 0;
> + int i;
> +
> + /*
> + * A level triggerred interrupt pending state is latched in both
> + * "soft_pending" and "line_level" variables. Userspace will save
> + * and restore soft_pending and line_level separately.
> + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> + * handling of ISPENDR and ICPENDR.
> + */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> + value |= (1U << i);
> + if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
> + value |= (1U << i);
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +
> + return value;
> +}
> +
> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + int i;
> +
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + spin_lock(&irq->irq_lock);
> + if (test_bit(i, &val)) {
> + irq->pending = true;
> + irq->soft_pending = true;
> + vgic_queue_irq_unlock(vcpu->kvm, irq);
> + } else {
> + irq->soft_pending = false;
> + if (irq->config == VGIC_CONFIG_EDGE ||
> + (irq->config == VGIC_CONFIG_LEVEL &&
> + !irq->line_level))
> + irq->pending = false;
> + spin_unlock(&irq->irq_lock);
> + }
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +}
> +
> /* We want to avoid outer shareable. */
> u64 vgic_sanitise_shareability(u64 field)
> {
> @@ -358,7 +414,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> * We take some special care here to fix the calculation of the register
> * offset.
> */
> -#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) \
> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, acc) \
> { \
> .reg_offset = off, \
> .bits_per_irq = bpi, \
> @@ -373,6 +429,8 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> .access_flags = acc, \
> .read = rd, \
> .write = wr, \
> + .uaccess_read = ur, \
> + .uaccess_write = uw, \
> }
>
> static const struct vgic_register_region vgic_v3_dist_registers[] = {
> @@ -380,40 +438,42 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> - vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> + vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
> - vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
> - vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> - vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
> + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER,
> - vgic_mmio_read_active, vgic_mmio_write_cactive, 1,
> + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR,
> - vgic_mmio_read_priority, vgic_mmio_write_priority, 8,
> - VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> + vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
> + 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ITARGETSR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICFGR,
> - vgic_mmio_read_config, vgic_mmio_write_config, 2,
> + vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGRPMODR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IROUTER,
> - vgic_mmio_read_irouter, vgic_mmio_write_irouter, 64,
> + vgic_mmio_read_irouter, vgic_mmio_write_irouter, NULL, NULL, 64,
> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICD_IDREGS,
> vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
> @@ -451,11 +511,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index e18b30d..31f85df 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -468,6 +468,73 @@ static bool check_region(const struct vgic_register_region *region,
> return false;
> }
>
> +static const struct vgic_register_region *
> +vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
> +{
> + const struct vgic_register_region *region;
> +
> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> + addr - iodev->base_addr);
> + if (!region || !check_region(region, addr, len))
> + return NULL;
> +
> + return region;
> +}
> +
> +static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, u32 *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> +
> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
> + if (!region) {
> + *val = 0;
> + return 0;
This is not the previous semantic of vgic_uaccess, and I cannot see why
blindly ignoring an access to an undefined region would be acceptable.
What am I missing?
> + }
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_read)
> + *val = region->uaccess_read(r_vcpu, addr, sizeof(u32));
> + else
> + *val = region->read(r_vcpu, addr, sizeof(u32));
> +
> + return 0;
> +}
> +
> +static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, const u32 *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> +
> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32));
> + if (!region)
> + return 0;
Same here.
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_write)
> + region->uaccess_write(r_vcpu, addr, sizeof(u32), *val);
> + else
> + region->write(r_vcpu, addr, sizeof(u32), *val);
> +
> + return 0;
> +}
> +
> +/*
> + * Userland access to VGIC registers.
> + */
> +int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val)
> +{
> + if (is_write)
> + return vgic_uaccess_write(vcpu, &dev->dev, offset, val);
> + else
> + return vgic_uaccess_read(vcpu, &dev->dev, offset, val);
> +}
> +
> static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> gpa_t addr, int len, void *val)
> {
> @@ -475,9 +542,8 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = 0;
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> - if (!region || !check_region(region, addr, len)) {
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region) {
> memset(val, 0, len);
> return 0;
> }
> @@ -508,14 +574,10 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = vgic_data_mmio_bus_to_host(val, len);
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> + region = vgic_get_mmio_region(iodev, addr, len);
> if (!region)
> return 0;
>
> - if (!check_region(region, addr, len))
> - return 0;
> -
> switch (iodev->iodev_type) {
> case IODEV_CPUIF:
> region->write(vcpu, addr, len, data);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 4c34d39..97e6df7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -34,6 +34,10 @@ struct vgic_register_region {
> gpa_t addr, unsigned int len,
> unsigned long val);
> };
> + unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len);
> + void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len, unsigned long val);
> };
>
> extern struct kvm_io_device_ops kvm_io_gic_ops;
> @@ -86,6 +90,18 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;
> .write = wr, \
> }
>
> +#define REGISTER_DESC_WITH_LENGTH_UACCESS(off, rd, wr, urd, uwr, length, acc) \
> + { \
> + .reg_offset = off, \
> + .bits_per_irq = 0, \
> + .len = length, \
> + .access_flags = acc, \
> + .read = rd, \
> + .write = wr, \
> + .uaccess_read = urd, \
> + .uaccess_write = uwr, \
> + }
> +
> int kvm_vgic_register_mmio_region(struct kvm *kvm, struct kvm_vcpu *vcpu,
> struct vgic_register_region *reg_desc,
> struct vgic_io_device *region,
> @@ -158,6 +174,9 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
> +int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val);
> +
> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>
> unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
From: Gabriele Paoloni @ 2016-09-22 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3538381.vOsx75UXVU@wuerfel>
Hi Arnd
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: 21 September 2016 21:18
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-kernel at lists.infradead.org;
> devicetree at vger.kernel.org; lorenzo.pieralisi at arm.com; minyard at acm.org;
> linux-pci at vger.kernel.org; gregkh at linuxfoundation.org; John Garry;
> will.deacon at arm.com; linux-kernel at vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial at vger.kernel.org;
> benh at kernel.crashing.org; zourongrong at gmail.com; liviu.dudau at arm.com;
> kantyzc at 163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni
> wrote:
> > > -----Original Message-----
> > > From: zhichang [mailto:zhichang.yuan02 at gmail.com]
> > > On 2016?09?15? 20:24, Arnd Bergmann wrote:
> > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni
> > > wrote:
> > > >>> -----Original Message-----
> > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele
> Paoloni
> > > wrote:
> > > >> I think that maybe having the 1:1 range mapping doesn't
> > > >> reflect well the reality but it is the less painful
> > > >> solution...
> > > >>
> > > >> What's your view?
> > > >
> > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> > > > and that should be enough to translate the I/O port number.
> > > >
> > > > The only part we need to change here is to not go through
> > > > the crazy conversion all the way from PCI I/O space to a
> > > > physical address and back to a (logical) port number
> > > > that we do today with of_translate_address/pci_address_to_pio.
> > > >
> > > Sorry for the late response! Several days' leave....
> > > Do you want to bypass of_translate_address and pci_address_to_pio
> for
> > > the registered specific PIO?
> > > I think the bypass for of_translate_address is ok, but worry some
> new
> > > issues will emerge without the
> > > conversion between physical address and logical/linux port number.
>
> The same function that handles the non-translated region would
> do that conversion.
>
> > > When PCI host bridge which support IO operations is configured and
> > > enabled, the pci_address_to_pio will
> > > populate the logical IO range from ZERO for the first host bridge.
> Our
> > > LPC will also use part of the IO range
> > > started from ZERO. It will make in/out enter the wrong branch
> possibly.
> > >
> > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use
> only.
> > > But it seems not so good. In this way,
> > > PCI has no chance to use low 4K IO range(logical).
> > >
> > > So, in V3, applying the conversion from physical/cpu address to
> > > logical/linux IO port for any IO ranges,
> > > including the LPC, but recorded the logical IO range for LPC. When
> > > calling in/out with a logical port address,
> > > we can check this port fall into LPC logical IO range and get back
> the
> > > real IO.
>
> Right, and the same translation can be used in
> __of_address_to_resource()
> going the opposite way.
>
> > > Do you have further comments about this??
> >
> > I think there are two separate issues to be discussed:
> >
> > The first issue is about having of_translate_address failing due to
> > "range" missing. About this Arnd suggested that it is not appropriate
> > to have a range describing a bridge 1:1 mapping and this was
> discussed
> > before in this thread. Arnd had a suggestion about this (see below)
> > however (looking twice at the code) it seems to me that such solution
> > would lead to quite some duplication from __of_translate_address()
> > in order to retrieve the actual addr from dt...
>
> I don't think we need to duplicate much, we can probably safely
> assume that there are no nontrivial ranges in devices below the LPC
> node, so we just walk up the bus to see if the node is a child
> (or possibly grandchild etc) of the LPC bus, and treat any IO port
> number under there as a physical port number, which has a known
> offset from the Linux I/O port number.
>
> > I think extending of_empty_ranges_quirk() may be a reasonable
> solution.
> > What do you think Arnd?
>
> I don't really like that idea, that quirk is meant to work around
> broken DTs, but we can just make the DT valid and implement the
> code properly.
Ok I understand your point where it is not right to use of_empty_ranges_quirk()
As a quirk is used to work around broken HW or broken FW (as in this case)
rather than to fix code
What about the following? I think adding the check you suggested next to
of_empty_ranges_quirk() is adding the case we need in the right point (thus
avoiding any duplication)
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
return NULL;
}
+static inline int of_isa_indirect_io(struct device_node *np)
+{
+ /*
+ * check if the current node is an isa bus and if indirectio operation
+ * are registered
+ */
+ return (of_bus_isa_match(np) && arm64_extio_ops);
+}
+
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, &rlen);
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
>
> > The second issue is a conflict between cpu addresses used by the LPC
> > controller and i/o tokens from pci endpoints.
> >
> > About this what if we modify armn64_extio_ops to have a list of
> ranges
> > rather than only one range (now we have just start/end); then in the
> > LPC driver we can scan the LPC child devices and
> > 1) populate such list of ranges
> > 2) call pci_register_io_range for such ranges
>
> Scanning the child devices sounds really wrong, please register just
> one range that covers the bus to keep the workaround as simple
> as possible.
>
> > Then when calling __of_address_to_resource we retrieve I/O tokens
> > for the devices on top of the LPC driver and in the I/O accessors
> > we call pci_pio_to_address to figure out the cpu address and compare
> > it to the list of ranges in armn64_extio_ops.
> >
> > What about this?
>
> That seems really complex for something that can be quite simple.
> The only thing we need to worry about is that the io_range_list
> contains an entry for the LPC bus so we don't conflict with the
> PCI buses.
Thanks
I discussed with Zhichang and we agreed to use only one LPC range
to be registered with pci_register_io_range.
We'll rework the accessors to check if the retrieved I/O tokens
belong to LPC or PCI IO range...
Cheers
Gab
>
> Arnd
>
> Arnd
^ permalink raw reply
* [RFC PATCH 3/3] efi: implement MMC proxy support for the UEFI variable store
From: Ard Biesheuvel @ 2016-09-22 11:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543806-19210-1-git-send-email-ard.biesheuvel@linaro.org>
---
drivers/firmware/efi/Kconfig | 9 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/arm-init.c | 2 +
drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
include/linux/efi.h | 1 +
5 files changed, 235 insertions(+)
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index c981be17d3c0..1933e186a9c2 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -129,6 +129,15 @@ config EFI_TEST
Say Y here to enable the runtime services support via /dev/efi_test.
If unsure, say N.
+config EFI_MMC_PROXY
+ bool "Expose Embedded MMC host protocol to the firmware"
+ depends on EFI && (ARM || ARM64)
+ help
+ This driver exposes the MMC host whose DT node is annotated with the
+ "linux,uefi-varstore" property to the firmware via a UEFI compatible
+ protocol. This allows the EFI variable store to reside on an MMC
+ volume that is owned by the OS at runtime.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index c8a439f6d715..88fdf0e13d6b 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -26,3 +26,4 @@ arm-obj-$(CONFIG_EFI) := arm-init.o arm-runtime.o
obj-$(CONFIG_ARM) += $(arm-obj-y)
obj-$(CONFIG_ARM64) += $(arm-obj-y)
obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
+obj-$(CONFIG_EFI_MMC_PROXY) += mmc-proxy.o
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 8efe13075c92..7a57253975a5 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -56,9 +56,11 @@ static phys_addr_t efi_to_phys(unsigned long addr)
}
static __initdata unsigned long screen_info_table = EFI_INVALID_TABLE_ADDR;
+unsigned long __initdata mmc_fv_emulation_table = EFI_INVALID_TABLE_ADDR;
static __initdata efi_config_table_type_t arch_tables[] = {
{LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID, NULL, &screen_info_table},
+ {LINUX_MMC_FV_EMULATION_TABLE_GUID, NULL, &mmc_fv_emulation_table},
{NULL_GUID, NULL, NULL}
};
diff --git a/drivers/firmware/efi/mmc-proxy.c b/drivers/firmware/efi/mmc-proxy.c
new file mode 100644
index 000000000000..3702bb2a5af0
--- /dev/null
+++ b/drivers/firmware/efi/mmc-proxy.c
@@ -0,0 +1,222 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/efi.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+
+#include <asm/efi.h>
+
+extern unsigned long mmc_fv_emulation_table;
+
+enum emmc_state {
+ mmc_invalid_state = 0,
+ mmc_hw_initialization_state,
+ mmc_idle_state,
+ mmc_ready_state,
+ mmc_identification_state,
+ mmc_standby_state,
+ mmc_transfer_state,
+ mmc_sending_data_state,
+ mmc_receive_data_state,
+ mmc_programming_state,
+ mmc_disconnect_state,
+};
+
+static char const emmc_state_string[][28] = {
+ "mmc_invalid_state",
+ "mmc_hw_initialization_state",
+ "mmc_idle_state",
+ "mmc_ready_state",
+ "mmc_identification_state",
+ "mmc_standby_state",
+ "mmc_transfer_state",
+ "mmc_sending_data_state",
+ "mmc_receive_data_state",
+ "mmc_programming_state",
+ "mmc_disconnect_state",
+};
+
+/*
+ * This MMC host protocol version is not in the UEFI spec, and lives under
+ * EmbeddedPkg/ in Tianocore/EDK2.
+ */
+struct emmc_host_protocol
+{
+ u32 revision;
+ bool (*is_card_present)(struct emmc_host_protocol*);
+ bool (*is_read_only)(struct emmc_host_protocol*);
+ efi_status_t (*build_device_path)(struct emmc_host_protocol*,
+ void*);
+ efi_status_t (*notify_state)(struct emmc_host_protocol*,
+ enum emmc_state);
+ efi_status_t (*send_command)(struct emmc_host_protocol*,
+ u32, u32);
+ efi_status_t (*receive_response)(struct emmc_host_protocol*,
+ u32, u32*);
+ efi_status_t (*read_block_data)(struct emmc_host_protocol*,
+ u64, unsigned long, u32*);
+ efi_status_t (*write_block_data)(struct emmc_host_protocol*,
+ u64, unsigned long, u32*);
+};
+
+static bool mmc_is_card_present(struct emmc_host_protocol *this)
+{
+ return true;
+}
+
+static bool mmc_is_read_only(struct emmc_host_protocol *this)
+{
+ return false;
+}
+
+static efi_status_t mmc_build_device_path(struct emmc_host_protocol *this,
+ void *dpp)
+{
+ return EFI_UNSUPPORTED;
+}
+
+static efi_status_t mmc_notify_state(struct emmc_host_protocol *this,
+ enum emmc_state state)
+{
+ struct efi_simd_reg_stash simd_stash;
+
+ arch_efi_stash_simd_regs(&simd_stash);
+ arch_efi_call_virt_teardown();
+
+ pr_warn("mmc-proxy: entered %s state\n", emmc_state_string[state]);
+ might_sleep();
+
+ arch_efi_call_virt_setup();
+ arch_efi_unstash_simd_regs(&simd_stash);
+
+ return EFI_SUCCESS;
+}
+
+static efi_status_t mmc_send_command(struct emmc_host_protocol *this, u32 cmd,
+ u32 arg)
+{
+ struct efi_simd_reg_stash simd_stash;
+
+ arch_efi_stash_simd_regs(&simd_stash);
+ arch_efi_call_virt_teardown();
+
+ pr_warn("mmc-proxy: send command %d (0x%x)\n", cmd, arg);
+ might_sleep();
+
+ arch_efi_call_virt_setup();
+ arch_efi_unstash_simd_regs(&simd_stash);
+ return EFI_NOT_FOUND;
+}
+
+static efi_status_t mmc_receive_response(struct emmc_host_protocol *this,
+ u32 mmc_response_type, u32 *buffer)
+{
+ struct efi_simd_reg_stash simd_stash;
+ u32 response[4] = {};
+
+ arch_efi_stash_simd_regs(&simd_stash);
+ arch_efi_call_virt_teardown();
+
+ pr_warn("mmc-proxy: receive response %d\n", mmc_response_type);
+ might_sleep();
+
+ arch_efi_call_virt_setup();
+ arch_efi_unstash_simd_regs(&simd_stash);
+ memcpy(buffer, response, sizeof(response));
+
+ return EFI_NOT_FOUND;
+}
+
+static efi_status_t mmc_read_block_data(struct emmc_host_protocol *this,
+ u64 lba, unsigned long len,
+ u32 *buffer)
+{
+ struct efi_simd_reg_stash simd_stash;
+ void *bounce;
+
+ arch_efi_stash_simd_regs(&simd_stash);
+ arch_efi_call_virt_teardown();
+
+ bounce = kmalloc(len, GFP_KERNEL);
+
+ pr_warn("mmc-proxy: read block data lba==%lld, len==%ld\n", lba, len);
+ might_sleep();
+
+ arch_efi_call_virt_setup();
+ arch_efi_unstash_simd_regs(&simd_stash);
+ memcpy(buffer, bounce, len);
+ kfree(bounce);
+
+ return EFI_NOT_FOUND;
+}
+
+static efi_status_t mmc_write_block_data(struct emmc_host_protocol *this,
+ u64 lba, unsigned long len,
+ u32 *buffer)
+{
+ struct efi_simd_reg_stash simd_stash;
+ void *bounce;
+
+ /*
+ * 'buffer' may point to memory that is mapped via the EFI page tables,
+ * so copy the data before switching back to the ordinary mappings.
+ */
+ bounce = kmalloc(len, GFP_ATOMIC);
+ if (!bounce)
+ return EFI_OUT_OF_RESOURCES;
+ memcpy(bounce, buffer, len);
+
+ arch_efi_stash_simd_regs(&simd_stash);
+ arch_efi_call_virt_teardown();
+
+ pr_warn("mmc-proxy: write block data lba==%lld, len==%ld\n", lba, len);
+ might_sleep();
+
+ kfree(bounce);
+ arch_efi_call_virt_setup();
+ arch_efi_unstash_simd_regs(&simd_stash);
+ return EFI_NOT_FOUND;
+}
+
+static const struct emmc_host_protocol emmc_proxy =
+{
+ 0x00010001, // revision 1.1
+ mmc_is_card_present,
+ mmc_is_read_only,
+ mmc_build_device_path,
+ mmc_notify_state,
+ mmc_send_command,
+ mmc_receive_response,
+ mmc_read_block_data,
+ mmc_write_block_data
+};
+
+static int __init emmc_proxy_install(void)
+{
+ unsigned long *p;
+
+ if (mmc_fv_emulation_table == EFI_INVALID_TABLE_ADDR)
+ return 0;
+
+ p = memremap(mmc_fv_emulation_table, sizeof(*p), MEMREMAP_WB);
+ if (!p)
+ return -ENOMEM;
+
+ /*
+ * We expect a NULL value here, since the firmware should have cleared
+ * this value in its ExitBootServices() handler.
+ */
+ if (!WARN_ON(*p != 0))
+ *p = (unsigned long)&emmc_proxy;
+
+ memunmap(p);
+ return 0;
+}
+late_initcall(emmc_proxy_install);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2d089487d2da..25a4ae648b6f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -599,6 +599,7 @@ void efi_native_runtime_setup(void);
*/
#define LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID EFI_GUID(0xe03fc20a, 0x85dc, 0x406e, 0xb9, 0x0e, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95)
#define LINUX_EFI_LOADER_ENTRY_GUID EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf, 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
+#define LINUX_MMC_FV_EMULATION_TABLE_GUID EFI_GUID(0x44cd9912, 0xfda8, 0x4b7d, 0xa6, 0x13, 0x1b, 0x61, 0x34, 0xfc, 0xdc, 0x4a)
typedef struct {
efi_guid_t guid;
--
2.7.4
^ permalink raw reply related
* [RFC PATCH 2/3] efi/arm: add SIMD stash/unstash operations
From: Ard Biesheuvel @ 2016-09-22 11:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543806-19210-1-git-send-email-ard.biesheuvel@linaro.org>
This adds the SIMD stash/unstash operations that we need to allow UEFI
runtime services calls to call back into the kernel. In the ARM case,
these are actually NOPs, since UEFI on ARM is not allowed to use the
FP/NEON register file.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/arm/include/asm/efi.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 766bf9b78160..c71ea8c21d0d 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -83,4 +83,15 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
#define MIN_ZIMAGE_OFFSET MAX_UNCOMP_KERNEL_SIZE
#define MAX_FDT_OFFSET ZIMAGE_OFFSET_LIMIT
+struct efi_simd_reg_stash {
+};
+
+static inline void arch_efi_stash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+}
+
+static inline void arch_efi_unstash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+}
+
#endif /* _ASM_ARM_EFI_H */
--
2.7.4
^ permalink raw reply related
* [RFC PATCH 1/3] efi/arm64: add SIMD stash/unstash operations
From: Ard Biesheuvel @ 2016-09-22 11:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543806-19210-1-git-send-email-ard.biesheuvel@linaro.org>
This adds the SIMD stash/unstash operations that we need to allow UEFI
runtime services calls to call back into the kernel.
The UEFI bindings for arm64 stipulate that the ordinary AAPCS rules apply,
which means that code may use FP/NEON registers, and may also expect that
the values of the callee saved registers q8 - q15 are preserved across
function calls.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/arm64/include/asm/efi.h | 33 ++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index a9e54aad15ef..d6bbbcba9820 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -81,4 +81,37 @@ static inline void efi_set_pgd(struct mm_struct *mm)
void efi_virtmap_load(void);
void efi_virtmap_unload(void);
+struct efi_simd_reg_stash {
+ u8 q8[16];
+ u8 q9[16];
+ u8 q10[16];
+ u8 q11[16];
+ u8 q12[16];
+ u8 q13[16];
+ u8 q14[16];
+ u8 q15[16];
+};
+
+static inline void arch_efi_stash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+ asm( "stp q8, q9, [%1];"
+ "stp q10, q11, [%1, #32];"
+ "stp q12, q13, [%1, #64];"
+ "stp q14, q15, [%1, #96];"
+
+ : "=m"(*stash)
+ : "r"(stash));
+}
+
+static inline void arch_efi_unstash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+ asm( "ldp q8, q9, [%1];"
+ "ldp q10, q11, [%1, #32];"
+ "ldp q12, q13, [%1, #64];"
+ "ldp q14, q15, [%1, #96];"
+
+ :
+ : "m"(*stash), "r"(stash));
+}
+
#endif /* _ASM_EFI_H */
--
2.7.4
^ permalink raw reply related
* [RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore
From: Ard Biesheuvel @ 2016-09-22 11:30 UTC (permalink / raw)
To: linux-arm-kernel
================================================================================
NOTE: this is a work in progress, and not fully functional yet. In particular,
the actual MMC host protocol methods are stubbed out at the moment, and need to
be wired up to the Linux device drivers.
================================================================================
On mobile and embedded systems, there is usually only a single MMC device for
non-volatile storage, which sits behind a controller that is owned by the OS at
runtime. This makes it difficult to host the UEFI variable store on MMC as well,
since the UEFI runtime services routines expect ownership of the underlying
device as well.
This series proposes an approach to work around this. It implements the UEFI
MMC host protocol in the kernel, in a way that makes it possible to expose it
to the firmware. At the same time, the firmware needs be set up for this, i.e.,
it needs to expose its MMC host protocol pointer via a UEFI configuration table,
so that the kernel can override it if it decides to expose this functionality
to the firmware.
Note that these patches are based on patches in the EFI tree that are queued
for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
allows us to sleep in the firmware callbacks.
Prerequisites for using these patches:
* qemu-system-aarch64 built from this branch:
https://git.linaro.org/people/ard.biesheuvel/qemu.git/shortlog/refs/heads/mach-virt-pl181
which adds a PL181 SD/MMC controller to the mach-virt model, and exposes it
via the device tree. It also sets the 'linux,uefi-varstore' property on this
node.
* UEFI firmware built from this branch:
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/mmc-proxy
Build using the following command
build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuMmcVars.dsc
Run using
qemu-system-aarch64 \
-M virt -cpu cortex-a57 -m 2048 \
-device virtio-blk-device,drive=boot \
-drive if=none,id=boot,file=fat:<path-of-Image>,format=raw \
-kernel <edk2-dir>/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd \
-sd <edk2-dir>/build/edk2/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_VARS.fd \
-nographic $@
This will give you an UEFI environment which keeps its UEFI variables in the
emulated MMC volume, and exposes its MMC host protocol in a way that allows
these patches to hook into it.
Patches #1 and #2 implement the arch specific hooks to preserve/restore the NEON
registers that the firmware may expect to be preserved across function calls.
Patch #3 implements the plumbing to call back into the kernel from the firmware.
Please comment on whether this approach seems feasible, and in particular, how
on earth I should wire this up to the actual MMC code.
Thanks,
Ard.
Ard Biesheuvel (3):
efi/arm64: add SIMD stash/unstash operations
efi/arm: add SIMD stash/unstash operations
efi: implement MMC proxy support for the UEFI variable store
arch/arm/include/asm/efi.h | 11 +
arch/arm64/include/asm/efi.h | 33 +++
drivers/firmware/efi/Kconfig | 9 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/arm-init.c | 2 +
drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
include/linux/efi.h | 1 +
7 files changed, 279 insertions(+)
create mode 100644 drivers/firmware/efi/mmc-proxy.c
--
2.7.4
^ permalink raw reply
* [PATCH 2/2] i2c: rk3x: Fix variable 'min_total_ns' unused warning
From: David Wu @ 2016-09-22 11:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543764-51135-1-git-send-email-david.wu@rock-chips.com>
This patch fixs the following warning:
drivers/i2c/busses/i2c-rk3x.c: In function 'rk3x_i2c_v1_calc_timings':
drivers/i2c/busses/i2c-rk3x.c:745:41: warning: variable 'min_total_ns' set but not used [-Wunused-but-set-variable]
Signed-off-by: David Wu <david.wu@rock-chips.com>
---
drivers/i2c/busses/i2c-rk3x.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 965477e..50702c7 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -742,7 +742,7 @@ static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
struct i2c_timings *t,
struct rk3x_i2c_calced_timings *t_calc)
{
- unsigned long min_low_ns, min_high_ns, min_total_ns;
+ unsigned long min_low_ns, min_high_ns;
unsigned long min_setup_start_ns, min_setup_data_ns;
unsigned long min_setup_stop_ns, max_hold_data_ns;
@@ -793,7 +793,6 @@ static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
/* These are the min dividers needed for min hold times. */
min_div_for_hold = (min_low_div + min_high_div);
- min_total_ns = min_low_ns + min_high_ns;
/*
* This is the maximum divider so we don't go over the maximum.
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] i2c: rk3x: Fix sparse warning
From: David Wu @ 2016-09-22 11:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474543764-51135-1-git-send-email-david.wu@rock-chips.com>
This patch fixes the following sparse warning:
drivers/i2c/busses/i2c-rk3x.c:888:17: warning: cast truncates bits from constant value (ffffffffff00 becomes ffffff00)
Signed-off-by: David Wu <david.wu@rock-chips.com>
---
drivers/i2c/busses/i2c-rk3x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index f2d0148..965477e 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -58,7 +58,7 @@ enum {
#define REG_CON_LASTACK BIT(5) /* 1: send NACK after last received byte */
#define REG_CON_ACTACK BIT(6) /* 1: stop if NACK is received */
-#define REG_CON_TUNING_MASK GENMASK(15, 8)
+#define REG_CON_TUNING_MASK GENMASK_ULL(15, 8)
#define REG_CON_SDA_CFG(cfg) ((cfg) << 8)
#define REG_CON_STA_CFG(cfg) ((cfg) << 12)
--
1.9.1
^ permalink raw reply related
* [PATCH 0/2] Fix warnings for i2c-rk3x.c
From: David Wu @ 2016-09-22 11:29 UTC (permalink / raw)
To: linux-arm-kernel
David Wu (2):
i2c: rk3x: Fix sparse warning
i2c: rk3x: fix variable 'min_total_ns' unused warning
drivers/i2c/busses/i2c-rk3x.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v4] devicetree: bindings: uart: Add new compatible string for ZynqMP
From: Nava kishore Manne @ 2016-09-22 11:26 UTC (permalink / raw)
To: linux-arm-kernel
From: Nava kishore Manne <nava.manne@xilinx.com>
This patch Adds the new compatible string for ZynqMP.
Signed-off-by: Nava kishore Manne <navam@xilinx.com>
---
Changes for v4:
-Modified the ChangeLog comment.
Changes for v3:
-Added changeLog comment.
Changes for v2:
-None
Documentation/devicetree/bindings/serial/cdns,uart.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.txt b/Documentation/devicetree/bindings/serial/cdns,uart.txt
index a3eb154..4fe7aae 100644
--- a/Documentation/devicetree/bindings/serial/cdns,uart.txt
+++ b/Documentation/devicetree/bindings/serial/cdns,uart.txt
@@ -1,7 +1,8 @@
Binding for Cadence UART Controller
Required properties:
-- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps"
+- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps" for Zynq and
+ "cdns,uart-r1p12" for Zynq Ultrascale+ MPSoC uart controller.
- reg: Should contain UART controller registers location and length.
- interrupts: Should contain UART controller interrupts.
- clocks: Must contain phandles to the UART clocks
--
2.1.2
^ permalink raw reply related
* [PATCH v3] devicetree: bindings: uart: Add new compatible string for ZynqMP
From: Nava kishore Manne @ 2016-09-22 11:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Nava kishore Manne <nava.manne@xilinx.com>
This patch Adds the new compatiable string for ZynqMP.
Signed-off-by: Nava kishore Manne <navam@xilinx.com>
---
Changes for v3:
-Added changeLog comment.
Changes for v2:
-None
Documentation/devicetree/bindings/serial/cdns,uart.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.txt b/Documentation/devicetree/bindings/serial/cdns,uart.txt
index a3eb154..4fe7aae 100644
--- a/Documentation/devicetree/bindings/serial/cdns,uart.txt
+++ b/Documentation/devicetree/bindings/serial/cdns,uart.txt
@@ -1,7 +1,8 @@
Binding for Cadence UART Controller
Required properties:
-- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps"
+- compatible : should be "cdns,uart-r1p8", or "xlnx,xuartps" for Zynq and
+ "cdns,uart-r1p12" for Zynq Ultrascale+ MPSoC uart controller.
- reg: Should contain UART controller registers location and length.
- interrupts: Should contain UART controller interrupts.
- clocks: Must contain phandles to the UART clocks
--
2.1.2
^ permalink raw reply related
* [PATCH v2 3/3] musb: sunxi: Force session end on babble errors in host-mode
From: Hans de Goede @ 2016-09-22 11:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922111901.15337-1-hdegoede@redhat.com>
The sunxi musb has a bug where sometimes it will generate a babble
error on device disconnect instead of a disconnect irq. When this
happens the musb-controller switches from host mode to device mode
(it clears MUSB_DEVCTL_SESSION and sets MUSB_DEVCTL_BDEVICE) and
gets stuck in this state.
Clearing this requires reporting Vbus low for 200 or more ms, but
on some devices Vbus is simply always high (host-only mode, no Vbus
control).
This commit adds a sunxi_musb_recover() callback which makes
sunxi_musb_work call phy_set_mode with the current mode, which
will force end the current session.
This fixes the musb controller getting stuck in this state on systems
without Vbus control; and also fixes the need to unplug the usb-b ->
usb-a cable to get out of this state on systems with Vbus control.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use musb_platform_recover callback instead of using DYI code in the
interrupt handler
-Call phy_set_mode with the current mode instead of adding a new custom
sunxi phy callback
---
drivers/usb/musb/sunxi.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index 82eba92..d0be0ea 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -380,6 +380,20 @@ static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
return 0;
}
+static int sunxi_musb_recover(struct musb *musb)
+{
+ struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
+
+ /*
+ * Schedule a phy_set_mode with the current glue->phy_mode value,
+ * this will force end the current session.
+ */
+ set_bit(SUNXI_MUSB_FL_PHY_MODE_PEND, &glue->flags);
+ schedule_work(&glue->work);
+
+ return 0;
+}
+
/*
* sunxi musb register layout
* 0x00 - 0x17 fifo regs, 1 long per fifo
@@ -608,6 +622,7 @@ static const struct musb_platform_ops sunxi_musb_ops = {
.dma_init = sunxi_musb_dma_controller_create,
.dma_exit = sunxi_musb_dma_controller_destroy,
.set_mode = sunxi_musb_set_mode,
+ .recover = sunxi_musb_recover,
.set_vbus = sunxi_musb_set_vbus,
.pre_root_reset_end = sunxi_musb_pre_root_reset_end,
.post_root_reset_end = sunxi_musb_post_root_reset_end,
--
2.9.3
^ permalink raw reply related
* [PATCH v2 2/3] musb: sunxi: Remove custom babble handling
From: Hans de Goede @ 2016-09-22 11:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160922111901.15337-1-hdegoede@redhat.com>
The musb-core now a days always treats babble errors in host mode
as disconnects, so there is no need for the sunxi specific handling
of this anymore.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch series
---
drivers/usb/musb/sunxi.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index 1408245..82eba92 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -186,16 +186,6 @@ static irqreturn_t sunxi_musb_interrupt(int irq, void *__hci)
if (musb->int_usb)
writeb(musb->int_usb, musb->mregs + SUNXI_MUSB_INTRUSB);
- /*
- * sunxi musb often signals babble on low / full speed device
- * disconnect, without ever raising MUSB_INTR_DISCONNECT, since
- * normally babble never happens treat it as disconnect.
- */
- if ((musb->int_usb & MUSB_INTR_BABBLE) && is_host_active(musb)) {
- musb->int_usb &= ~MUSB_INTR_BABBLE;
- musb->int_usb |= MUSB_INTR_DISCONNECT;
- }
-
if ((musb->int_usb & MUSB_INTR_RESET) && !is_host_active(musb)) {
/* ep0 FADDR must be 0 when (re)entering peripheral mode */
musb_ep_select(musb->mregs, 0);
--
2.9.3
^ permalink raw reply related
* [PATCH v2 1/3] phy_sun4i_usb: set_mode: Allow using set_mode to force end the current session
From: Hans de Goede @ 2016-09-22 11:18 UTC (permalink / raw)
To: linux-arm-kernel
The sunxi musb has a bug where sometimes it will generate a babble
error on device disconnect instead of a disconnect irq. When this
happens the musb-controller switches from host mode to device mode
(it clears MUSB_DEVCTL_SESSION and sets MUSB_DEVCTL_BDEVICE) and
gets stuck in this state.
Clearing this requires reporting Vbus low for 200 or more ms, but
on some devices Vbus is simply always high (host-only mode, no Vbus
control).
This commit modifies sun4i_usb_phy_set_mode so that it will force
end the current session when called with the current mode, before this
commit calling set_mode with the current mode was a nop since id_det
would stay the same resulting in the detect_work not doing anything.
This allows the sunxi-musb glue to use sun4i_usb_phy_set_mode to force
end the current session without changing the mode, to fixup the stuck
state after a babble error.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-New patch in v2 of this series replacing the
"phy-sun4i-usb: Add sun4i_usb_phy_force_session_end() function"
from v1
---
drivers/phy/phy-sun4i-usb.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 43c0d98..d76538c 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -437,25 +437,32 @@ static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode)
{
struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+ enum usb_dr_mode new_mode;
if (phy->index != 0)
return -EINVAL;
switch (mode) {
case PHY_MODE_USB_HOST:
- data->dr_mode = USB_DR_MODE_HOST;
+ new_mode = USB_DR_MODE_HOST;
break;
case PHY_MODE_USB_DEVICE:
- data->dr_mode = USB_DR_MODE_PERIPHERAL;
+ new_mode = USB_DR_MODE_PERIPHERAL;
break;
case PHY_MODE_USB_OTG:
- data->dr_mode = USB_DR_MODE_OTG;
+ new_mode = USB_DR_MODE_OTG;
break;
default:
return -EINVAL;
}
- dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode);
+ if (new_mode != data->dr_mode) {
+ dev_info(&_phy->dev, "Changing dr_mode to %d\n",
+ (int)data->dr_mode);
+ data->dr_mode = new_mode;
+ }
+
+ data->id_det = -1; /* Force reprocessing of id */
data->force_session_end = true;
queue_delayed_work(system_wq, &data->detect, 0);
--
2.9.3
^ permalink raw reply related
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-22 11:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E166F9.8080109@arm.com>
On 09/20/2016 06:42 PM, Marc Zyngier wrote:
> On 20/09/16 15:31, Alexander Graf wrote:
>> On 09/20/2016 02:37 PM, Marc Zyngier wrote:
> [...]
>
>>>>>> We also need to know "timer line low + timer line masked", as otherwise
>>>>>> we might get spurious interrupts in the guest, no?
>>>>> Yes. Though you can't really know about this one, and you'll have to
>>>>> wait until the next natural exit to find out. As long as the spurious is
>>>> We can provoke a special exit for it, no?
>>> How? The guest decides to disable its timer. That doesn't trigger any
>>> exit whatsoever. You'll have to wait until the next exit from the guest
>>> to notice it.
>> Before we inject a timer interrupt, we can check whether the pending
>> semantics of user space / kernel space match. If they don't match, we
>> can exit before we inject the interrupt and allow user space to disable
>> the pending state again.
> Let's rewind a bit, because I've long lost track of what you're trying
> to do to handle what.
>
> You need two signals:
>
> (1) TIMER_LEVEL: the output of the timer line, having accounted for the
> IMASK bit. This is conveniently the value of timer->irq.level.
>
> (2) TIMER_IRQ_MASK: an indication from userspace that a timer interrupt
> is pending, and that the physical line should be masked.
>
> You need a number of rules:
>
> (a) On exit to userspace, the kernel always exposes the value of
> TIMER_LEVEL.
>
> (b) On kernel entry, userspace always exposes the required
> TIMER_IRQ_MASK, depending on what has been exposed to it by TIMER_LEVEL.
>
> (c) If on guest exit, TIMER_LEVEL==1 and TIMER_IRQ_MASK==0, perform a
> userspace exit, because the emulated GIC needs to make the interrupt
> pending.
This should be "before guest entry", because the timer might have
expired in between.
> (d) If on guest exit, TIMER_LEVEL==0 and TIMER_IRQ_MASK==1, perform a
> userspace exit, because the guest has disabled its timer before taking
> the interrupt, and the emulated GIC needs to retire the pending state.
>
> and that's it. Nothing else. The kernel tells userspace the state of the
> timer, and userspace drives the masking of the physical interrupt.
> Conveniently, this matches what the current code does.
Yup. It seems to work. It also does feel slower than the previous code,
but maybe that's just me. It definitely is way more correct.
I'll trace around a bit more to see whether I can spot any obviously low
hanging performance fruits, then prettify the patches and send them out :).
Alex
^ permalink raw reply
* [PATCH 2/4] drivers: iio: ti_am335x_adc: add dma support
From: Peter Ujfalusi @ 2016-09-22 11:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <0cd3dfcc-2525-c2fb-35ab-1b260c5973a8@ti.com>
On 09/22/16 13:45, Mugunthan V N wrote:
>>> + if (!dma->chan)
>>> + return -ENODEV;
>>> +
>>> + /* RX buffer */
>>> + dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>>> + &dma->addr, GFP_KERNEL);
>>> + if (!dma->buf)
>>> + goto err;
>>> +
>>> + dev_dbg_ratelimited(adc_dev->mfd_tscadc->dev, "got dma channel\n");
>>
>> Do we need _ratelimited? AFAICS, this print is called only once.
>
> True, will change to dev_dbg.
It would be better to remove it. It gives no useful debuggin information apart
from the fact that the driver did not failed to probe.
--
P?ter
^ permalink raw reply
* [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case
From: Gabriele Paoloni @ 2016-09-22 11:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160921185920.GC20006@localhost>
Hi Bjorn
> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas at kernel.org]
> Sent: 21 September 2016 19:59
> To: Gabriele Paoloni
> Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin
> Marinas; Rafael Wysocki; Lorenzo Pieralisi; Arnd Bergmann; Hanjun Guo;
> Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert
> Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
> pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Linaro ACPI
> Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong
> (C); Jeff Hugo; linux-acpi at vger.kernel.org; linux-
> kernel at vger.kernel.org; Rafael J. Wysocki
> Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-
> specific register range for ACPI case
>
> On Wed, Sep 21, 2016 at 02:10:55PM +0000, Gabriele Paoloni wrote:
> > Hi Bjorn
> >
> > [...]
> >
> >
> > >
> > > If future hardware is completely ECAM-compliant and we don't need
> any
> > > more MCFG quirks, that would be great.
> > >
> > > But we'll still need to describe that memory-mapped config space
> > > somewhere. If that's done with PNP0C02 or similar devices (as is
> done
> > > on my x86 laptop), we'd be all set.
> > >
> > > If we need to work around firmware in the field that doesn't do
> that,
> > > one possibility is a PNP quirk along the lines of
> > > quirk_amd_mmconfig_area().
> >
> > So, if my understanding is correct, for platforms that have not been
> > shipped yet you propose to use PNP0C02 in the ACPI table in order to
> > declare a motherboard reserved resource whereas for shipped platforms
> > you propose to have a quirk along pnp_fixups in order to track the
> > resource usage even if values are hardcoded...correct?
>
> Yes. I'm open to alternate proposals, but x86 uses PNP0C02, and
> following existing practice seems reasonable.
>
> > Before Tomasz came up with this patchset we had a call between the
> vendors
> > involved in this PCI quirks saga and other guys from Linaro and ARM.
> >
> > Lorenzo summarized the outcome as in the following link
> > http://lkml.iu.edu/hypermail/linux/kernel/1606.2/03344.html
> >
> > Since this quirks mechanism has been discussed for quite a long time
> now
> > IMHO it would be good to have a last call including also you (Bjorn)
> so
> > that we can all agree on what to do and we avoid changing our drivers
> again
> > and again...
>
> I think we're converging pretty fast. As far as I'm concerned, the
> v6 ECAM quirks implementation is perfect. The only remaining issue is
> reporting the ECAM resources, and I haven't seen objections to using
> PNP0C02 + PNP quirks for broken firmware.
>
> There is the question of how or whether to associate a PNP0A03 PCI
> bridge with resources from a different PNP0C02 device, but that's not
> super important. If the hard-coded resources appear both in a quirk
> and in the PCI bridge driver, it's ugly but not the end of the world.
> We've still achieved the objective of avoiding landmines in the
> address space.
Ok got it many thanks
Gab
>
> Bjorn
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox