Re: [PATCH] ARM: KVM: Correctly order SGI register entries in the cp15 array
Le 10/04/18 à 02:29, Marc Zyngier a écrit :
> The ICC_ASGI1R and ICC_SGI0R register entries in the cp15 array
> are not correctly ordered, leading to a BUG() at boot time.
>
> Move them to their natural location.
>
> Fixes: 3e8a8a50c7ef ("KVM: arm: vgic-v3: Add support for ICC_SGI0R and
> ICC_ASGI1R accesses")
> Reported-by: Florian Fainelli
> Signed-off-by: Marc Zyngier
Tested-by: Florian Fainelli
Thanks!
--
Florian
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 05/18] ACPI / APEI: Make estatus queue a Kconfig symbol
On Wed, Oct 03, 2018 at 06:50:36PM +0100, James Morse wrote: > I'm all in favour of letting the compiler work it out, but the existing ghes > code has #ifdef/#else all over the place. This is 'keeping the style'. Yeah, but this "style" is not the optimal one and we should simplify/clean up and fix this thing. Swapping the order of your statements here: > The ACPI spec has four ~NMI notifications, so far the support for > these in Linux has been selectable separately. Yes, but: distro kernels end up enabling all those options anyway and distro kernels are 90-ish% of the setups. Which means, this will get enabled anyway and this additional Kconfig symbol is simply going to be one automatic reply "Yes". So let's build it in by default and if someone complains about it, we can always carve it out. But right now I don't see the need for the unnecessary separation... Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] ARM: KVM: Correctly order SGI register entries in the cp15 array
On 10/04/2018 02:29 AM, Marc Zyngier wrote:
> The ICC_ASGI1R and ICC_SGI0R register entries in the cp15 array
> are not correctly ordered, leading to a BUG() at boot time.
>
> Move them to their natural location.
>
> Fixes: 3e8a8a50c7ef ("KVM: arm: vgic-v3: Add support for ICC_SGI0R and
> ICC_ASGI1R accesses")
> Reported-by: Florian Fainelli
> Signed-off-by: Marc Zyngier
> ---
> Paolo, Radim,
>
> Could you please send this patch directly to Greg so that it makes it
> into 4.19? I thought I had it fixed long before the merge window, and
> obviously didn't...
Thanks for the quick fix! I probably won't be able to test this until 8h
from now but this looks obviously correct.
>
> Thanks,
>
> M.
>
> arch/arm/kvm/coproc.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 450c7a4fbc8a..cb094e55dc5f 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -478,15 +478,15 @@ static const struct coproc_reg cp15_regs[] = {
>
> /* ICC_SGI1R */
> { CRm64(12), Op1( 0), is64, access_gic_sgi},
> - /* ICC_ASGI1R */
> - { CRm64(12), Op1( 1), is64, access_gic_sgi},
> - /* ICC_SGI0R */
> - { CRm64(12), Op1( 2), is64, access_gic_sgi},
>
> /* VBAR: swapped by interrupt.S. */
> { CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
> NULL, reset_val, c12_VBAR, 0x },
>
> + /* ICC_ASGI1R */
> + { CRm64(12), Op1( 1), is64, access_gic_sgi},
> + /* ICC_SGI0R */
> + { CRm64(12), Op1( 2), is64, access_gic_sgi},
> /* ICC_SRE */
> { CRn(12), CRm(12), Op1( 0), Op2(5), is32, access_gic_sre },
>
>
--
Florian
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 00/18] APEI in_nmi() rework
On Wed, Oct 03, 2018 at 06:50:38PM +0100, James Morse wrote: ... > The non-ghes HEST entries have a "number of records to pre-allocate" too, we > could make this memory pool something hest.c looks after, but I can't see if > the > other error sources use those values. Thanks for the detailed analysis! > Hmmm, The size is capped to 64K, we could ignore the firmware description of > the > memory requirements, and allocate SZ_64K each time. Doing it per-GHES is still > the only way to avoid allocating nmi-safe memory for irqs. Right, so I'm thinking a lot simpler: allocate a pool which should be large enough to handle all situations and drop all that logic which recomputes and reallocates pool size. Just a static thing which JustWorks(tm). For a couple of reasons: - you state it above: all those synchronization issues are gone with a prellocated pool - 64K per-GHES pool is nothing if you consider the machines this thing runs on - fat servers with lotsa memory. And RAS there *is* important. And TBH 64K is nothing even on a small client sporting gigabytes of memory. - code is a lot simpler and cleaner - you don't need all that pool expanding and shrinking. I mean, I'm all for smarter solutions if they have any clear advantages warranting the complication but this is a lot of machinery just so that we can save a couple of KBs. Which, as a whole, sounds just too much to me. But this is just me. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm/arm64: Check memslot bounds before mapping hugepages
Hi Lukas,
Lukas Braun writes:
> Userspace can create a memslot with memory backed by (transparent)
> hugepages, but with bounds that do not align with hugepages.
> In that case, we cannot map the entire region in the guest as hugepages
> without exposing additional host memory to the guest and potentially
> interfering with other memslots.
> Consequently, this patch adds a bounds check when populating guest page
> tables and forces the creation of regular PTEs if mapping an entire
> hugepage would violate the memslots bounds.
>
> Signed-off-by: Lukas Braun
> ---
>
> Hi everyone,
>
> for v2, in addition to writing the condition the way Marc suggested, I
> moved the whole check so it also catches the problem when the hugepage
> was allocated explicitly, not only for THPs.
Ok, that makes sense. Memslot bounds could exceed for hugetlbfs pages as
well.
> The second line is quite long, but splitting it up would make things
> rather ugly IMO, so I left it as it is.
Let's try to do better - user_mem_abort() is quite hard to follow as it
is.
>
>
> Regards,
> Lukas
>
>
> virt/kvm/arm/mmu.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index ed162a6c57c5..ba77339e23ec 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -1500,7 +1500,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> phys_addr_t fault_ipa,
> return -EFAULT;
> }
>
> - if (vma_kernel_pagesize(vma) == PMD_SIZE && !logging_active) {
> + if ((fault_ipa & S2_PMD_MASK) < (memslot->base_gfn << PAGE_SHIFT) ||
> + ALIGN(fault_ipa, S2_PMD_SIZE) >= ((memslot->base_gfn +
> memslot->npages) << PAGE_SHIFT)) {
> + /* PMD entry would map something outside of the memslot */
> + force_pte = true;
> + } else if (vma_kernel_pagesize(vma) == PMD_SIZE && !logging_active) {
> hugetlb = true;
> gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
> } else {
For the purpose of this fix, using a helper to check whether the mapping
fits in the memslot makes things clearer (imo) (untested patch below) -
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index ed162a6c57c5..8bca141eb45e 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -1466,6 +1466,18 @@ static void kvm_send_hwpoison_signal(unsigned long
address,
send_sig_info(SIGBUS, , current);
}
+static bool mapping_in_memslot(struct kvm_memory_slot *memslot,
+phys_addr_t fault_ipa, unsigned long mapping_size)
+{
+ gfn_t start_gfn = (fault_ipa & ~(mapping_size - 1)) >> PAGE_SHIFT;
+ gfn_t end_gfn = ALIGN(fault_ipa, mapping_size) >> PAGE_SHIFT;
+
+ WARN_ON(!is_power_of_2(mapping_size));
+
+ return memslot->base_gfn <= start_gfn &&
+ end_gfn < memslot->base_gfn + memslot->npages;
+}
+
static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
struct kvm_memory_slot *memslot, unsigned long hva,
unsigned long fault_status)
@@ -1480,7 +1492,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
kvm_pfn_t pfn;
pgprot_t mem_type = PAGE_S2;
bool logging_active = memslot_is_logging(memslot);
- unsigned long flags = 0;
+ unsigned long vma_pagesize, flags = 0;
write_fault = kvm_is_write_fault(vcpu);
exec_fault = kvm_vcpu_trap_is_iabt(vcpu);
@@ -1500,7 +1512,15 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
return -EFAULT;
}
- if (vma_kernel_pagesize(vma) == PMD_SIZE && !logging_active) {
+ vma_pagesize = vma_kernel_pagesize(vma);
+ /* Is the mapping contained in the memslot? */
+ if (!mapping_in_memslot(memslot, fault_ipa, vma_pagesize)) {
+ /* memslot should be aligned to page size */
+ vma_pagesize = PAGE_SIZE;
+ force_pte = true;
+ }
+
+ if (vma_pagesize == PMD_SIZE && !logging_active) {
hugetlb = true;
gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
} else {
Thoughts?
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] ARM: KVM: Correctly order SGI register entries in the cp15 array
The ICC_ASGI1R and ICC_SGI0R register entries in the cp15 array
are not correctly ordered, leading to a BUG() at boot time.
Move them to their natural location.
Fixes: 3e8a8a50c7ef ("KVM: arm: vgic-v3: Add support for ICC_SGI0R and
ICC_ASGI1R accesses")
Reported-by: Florian Fainelli
Signed-off-by: Marc Zyngier
---
Paolo, Radim,
Could you please send this patch directly to Greg so that it makes it
into 4.19? I thought I had it fixed long before the merge window, and
obviously didn't...
Thanks,
M.
arch/arm/kvm/coproc.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 450c7a4fbc8a..cb094e55dc5f 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -478,15 +478,15 @@ static const struct coproc_reg cp15_regs[] = {
/* ICC_SGI1R */
{ CRm64(12), Op1( 0), is64, access_gic_sgi},
- /* ICC_ASGI1R */
- { CRm64(12), Op1( 1), is64, access_gic_sgi},
- /* ICC_SGI0R */
- { CRm64(12), Op1( 2), is64, access_gic_sgi},
/* VBAR: swapped by interrupt.S. */
{ CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
NULL, reset_val, c12_VBAR, 0x },
+ /* ICC_ASGI1R */
+ { CRm64(12), Op1( 1), is64, access_gic_sgi},
+ /* ICC_SGI0R */
+ { CRm64(12), Op1( 2), is64, access_gic_sgi},
/* ICC_SRE */
{ CRn(12), CRm(12), Op1( 0), Op2(5), is32, access_gic_sre },
--
2.19.0
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 00/18] kvm: arm64: Dynamic IPA and 52bit IPA
Hi Suzuki,
On 9/26/18 6:32 PM, Suzuki K Poulose wrote:
>
> The physical address space size for a VM (IPA size) on arm/arm64 is
> limited to a static limit of 40bits. This series adds support for
> using an IPA size specific to a VM, allowing to use a size supported
> by the host (based on the host kernel configuration and CPU support).
> The default size is fixed to 40bits. On arm64, we can allow the limit
> to be lowered (limiting the number of levels in stage2 to 2, to prevent
> splitting the host PMD huge pages at stage2). We also add support for
> handling 52bit IPA addresses (where supported) added by Arm v8.2
> extensions.
>
> We need to set the IPA limit as early as the VM creation to keep the
> code simpler to avoid sprinkling checks everywhere to ensure that the
> IPA is configured. We encode the IPA size in the machine_type
> argument to KVM_CREATE_VM ioctl. Bits [7-0] of the type are reserved
> for the IPA size. The availability of this feature is advertised by a
> new cap KVM_CAP_ARM_VM_IPA_SIZE. When supported, this capability
> returns the maximum IPA shift supported by the host. The supported IPA
> size on a host could be different from the system's PARange indicated
> by the CPUs (e.g, kernel limit on the PA size).
>
> Supporting different IPA size requires modification to the stage2 page
> table code. The arm64 page table level helpers are defined based on the
> page table levels used by the host VA. So, the accessors may not work
> if the guest uses more number of levels in stage2 than the stage1
> of the host. The previous versions (v1 & v2) of this series refactored
> the stage1 page table accessors to reuse the low-level accessors for an
> independent stage2 table. However, due to the level folding in the
> generic code, the types are redefined as well. i.e, if the PUD is
> folded, the pud_t could be defined as :
>
> typedef struct { pgd_t pgd; } pud_t;
>
> similarly for pmd_t. So, without stage1 independent page table entry
> types for stage2, we could be dealing with a different type for level
> 0-2 entries. This is practically fine on arm/arm64 as the entries
> have similar format and size and we always use the appropriate
> accessors to get the raw value (i.e, pud_val/pmd_val etc). But not
> ideal for a solution upstream. So, this version caps the stage2 page
> table levels to that of the stage1. This has the following impact on
> the IPA support for various pagesize/host-va combinations :
>
>
> x-x
> | host\ipa| 40bit | 42bit | 44bit | 48bit | 52bit |
> ---
> | 39bit-4K| y| y | n| n | n/a |
> ---
> | 48bit-4K| y| y | y| y | n/a |
> ---
> | 36bit-16K | y| n | n| n | n/a |
> ---
> | 47bit-16K | y| y | y| y | n/a |
> ---
> | 48bit-4K| y| y | y| y | n/a |
> ---
> | 42bit-64K | y| y | y| n | n|
> ---
> | 48bit-64K | y| y | y| y | y|
> x-x
>
> Or the following list shows what cannot be supported :
>
> 39bit-4K host | [44 - 48]
> 36bit-16K host | [41 - 48]
> 42bit-64K host | [47 - 52]
>
> which is not really bad. We can pursue the independent stage2
> page table support and lift the restriction once we get there.
> Given there is a proposal for new generic page table walker [0],
> it would make sense to make our efforts in sync with it to avoid
> diverting from a common API.
>
> 52bit support is added for VGIC (including ITS emulation) and handling
> of PAR, HPFAR registers.
>
> The series applies on 4.19-rc4. A tree is available here:
>
>git://linux-arm.org/linux-skp.git ipa52/v6
>
> Tested with
> - Modified kvmtool, which can only be used for (patches included in
> the series for reference / testing):
> * with virtio-pci upto 44bit PA (Due to 4K page size for virtio-pci
> legacy implemented by kvmtool)
> * Upto 48bit PA with virtio-mmio, due to 32bit PFN limitation.
> - Hacked Qemu (boot loader support for highmem, IPA size support)
> * with virtio-pci GIC-v3 ITS & MSI upto 52bit on Foundation model.
> Also see [1] for Qemu support.
>
> [0] https://lkml.org/lkml/2018/4/24/777
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05759.html
>
> Change since v5:
> - Don't raise the IPA Limit to 40bits on systems with lower PA size.
>Doesn't break backward compatibility, we still allow KVM_CREATE_VM
>to succeed with "0" as the IPA size (40bits). But prevent specifying
>40bit
Re: kernel BUG at arch/arm/kvm/coproc.c:1406!
Hi Florian,
On 04/10/18 04:02, Florian Fainelli wrote:
> Hi,
>
> Seeing the following BUG_ON() being triggered on a Lamobo R1
> (SUN7I/Cortex-A7), did not have the time to run a bisection since I was
> chasing another regression. My guess would be that
> 3e8a8a50c7ef1a4f71921731f0e1748a7f70ddaa ("KVM: arm: vgic-v3: Add
> support for ICC_SGI0R and ICC_ASGI1R accesses") is the culprit since it
> does add new entries to cp15_regs[].
OK, this is quite embarrassing. I remember fixing that exact bug before
the 4.19 merge window, and was 100% convinced that I had merged the fix.
Turns out I didn't.
Can you verify that the following patch works for you?
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 450c7a4fbc8a..cb094e55dc5f 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -478,15 +478,15 @@ static const struct coproc_reg cp15_regs[] = {
/* ICC_SGI1R */
{ CRm64(12), Op1( 0), is64, access_gic_sgi},
- /* ICC_ASGI1R */
- { CRm64(12), Op1( 1), is64, access_gic_sgi},
- /* ICC_SGI0R */
- { CRm64(12), Op1( 2), is64, access_gic_sgi},
/* VBAR: swapped by interrupt.S. */
{ CRn(12), CRm( 0), Op1( 0), Op2( 0), is32,
NULL, reset_val, c12_VBAR, 0x },
+ /* ICC_ASGI1R */
+ { CRm64(12), Op1( 1), is64, access_gic_sgi},
+ /* ICC_SGI0R */
+ { CRm64(12), Op1( 2), is64, access_gic_sgi},
/* ICC_SRE */
{ CRn(12), CRm(12), Op1( 0), Op2(5), is32, access_gic_sre },
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
