| Message ID | 1471610295-1456-6-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 August 2016 at 13:38, Marc Zyngier <marc.zyngier@arm.com> wrote: > So far, we've been disabling KVM on systems where the GICV region couldn't > be safely given to a guest. Now that we're able to handle this access > safely by emulating it in HYP, we can enable this feature when we detect > an unsafe configuration. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/vgic/vgic-v2.c | 69 +++++++++++++++++++++++++++------------------ > 1 file changed, 42 insertions(+), 27 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index b8da901..d1dcfc76 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -278,12 +278,14 @@ int vgic_v2_map_resources(struct kvm *kvm) > goto out; > } > > - ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, > - kvm_vgic_global_state.vcpu_base, > - KVM_VGIC_V2_CPU_SIZE, true); > - if (ret) { > - kvm_err("Unable to remap VGIC CPU to VCPU\n"); > - goto out; > + if (!static_branch_unlikely(&vgic_v2_cpuif_trap)) { > + ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, > + kvm_vgic_global_state.vcpu_base, > + KVM_VGIC_V2_CPU_SIZE, true); > + if (ret) { > + kvm_err("Unable to remap VGIC CPU to VCPU\n"); > + goto out; > + } > } > > dist->ready = true; > @@ -312,45 +314,51 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > return -ENXIO; > } > > - if (!PAGE_ALIGNED(info->vcpu.start)) { > - kvm_err("GICV physical address 0x%llx not page aligned\n", > - (unsigned long long)info->vcpu.start); > - return -ENXIO; > - } > + if (!PAGE_ALIGNED(info->vcpu.start) || > + !PAGE_ALIGNED(resource_size(&info->vcpu))) { > + kvm_info("GICV region size/alignement is unsafe, using trapping\n"); "alignment". Is it worth specifically saying "performance will be worse", or do we expect this to only happen on systems where the h/w can't permit direct access (as opposed to those with bad dt info) ? thanks -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On 19/08/16 13:53, Peter Maydell wrote: > On 19 August 2016 at 13:38, Marc Zyngier <marc.zyngier@arm.com> wrote: >> So far, we've been disabling KVM on systems where the GICV region couldn't >> be safely given to a guest. Now that we're able to handle this access >> safely by emulating it in HYP, we can enable this feature when we detect >> an unsafe configuration. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> virt/kvm/arm/vgic/vgic-v2.c | 69 +++++++++++++++++++++++++++------------------ >> 1 file changed, 42 insertions(+), 27 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c >> index b8da901..d1dcfc76 100644 >> --- a/virt/kvm/arm/vgic/vgic-v2.c >> +++ b/virt/kvm/arm/vgic/vgic-v2.c >> @@ -278,12 +278,14 @@ int vgic_v2_map_resources(struct kvm *kvm) >> goto out; >> } >> >> - ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, >> - kvm_vgic_global_state.vcpu_base, >> - KVM_VGIC_V2_CPU_SIZE, true); >> - if (ret) { >> - kvm_err("Unable to remap VGIC CPU to VCPU\n"); >> - goto out; >> + if (!static_branch_unlikely(&vgic_v2_cpuif_trap)) { >> + ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, >> + kvm_vgic_global_state.vcpu_base, >> + KVM_VGIC_V2_CPU_SIZE, true); >> + if (ret) { >> + kvm_err("Unable to remap VGIC CPU to VCPU\n"); >> + goto out; >> + } >> } >> >> dist->ready = true; >> @@ -312,45 +314,51 @@ int vgic_v2_probe(const struct gic_kvm_info *info) >> return -ENXIO; >> } >> >> - if (!PAGE_ALIGNED(info->vcpu.start)) { >> - kvm_err("GICV physical address 0x%llx not page aligned\n", >> - (unsigned long long)info->vcpu.start); >> - return -ENXIO; >> - } >> + if (!PAGE_ALIGNED(info->vcpu.start) || >> + !PAGE_ALIGNED(resource_size(&info->vcpu))) { >> + kvm_info("GICV region size/alignement is unsafe, using trapping\n"); > > "alignment". Ah, thanks. There's always a bit of French being stuck somewhere... ;-) > > Is it worth specifically saying "performance will be worse", or do we > expect this to only happen on systems where the h/w can't permit direct > access (as opposed to those with bad dt info) ? We cannot distinguish between the two, unfortunately. Even worse, ACPI only gives us a base address, and not the size of the region. So even if the HW was perfectly compliant with SBSA, we have to assume the worse case. I guess that a slightly more alarming message is in order indeed. Thanks, M.
On 19 August 2016 at 14:05, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 19/08/16 13:53, Peter Maydell wrote: >> Is it worth specifically saying "performance will be worse", or do we >> expect this to only happen on systems where the h/w can't permit direct >> access (as opposed to those with bad dt info) ? > > We cannot distinguish between the two, unfortunately. Even worse, ACPI > only gives us a base address, and not the size of the region. So even if > the HW was perfectly compliant with SBSA, we have to assume the worse case. Right, but if we expect this is mostly going to be "you just have to live with it on this hardware" there's less point in printing an alarming message, whereas if there's a significant subset of "dt is just wrong" cases then the alarm might help in getting them fixed, maybe... thanks -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Aug 2016 14:31:12 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 August 2016 at 14:05, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On 19/08/16 13:53, Peter Maydell wrote: > >> Is it worth specifically saying "performance will be worse", or do we > >> expect this to only happen on systems where the h/w can't permit direct > >> access (as opposed to those with bad dt info) ? > > > > We cannot distinguish between the two, unfortunately. Even worse, ACPI > > only gives us a base address, and not the size of the region. So even if > > the HW was perfectly compliant with SBSA, we have to assume the worse case. > > Right, but if we expect this is mostly going to be "you just have > to live with it on this hardware" there's less point in printing > an alarming message, whereas if there's a significant subset of > "dt is just wrong" cases then the alarm might help in getting them > fixed, maybe... That'd require some more infrastructure from the kernel's GIC driver (which now provides the various base addresses), but I guess that we can have a look as well. Thanks, M.
On Fri, Aug 19, 2016 at 01:38:15PM +0100, Marc Zyngier wrote: > So far, we've been disabling KVM on systems where the GICV region couldn't > be safely given to a guest. Now that we're able to handle this access > safely by emulating it in HYP, we can enable this feature when we detect > an unsafe configuration. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/vgic/vgic-v2.c | 69 +++++++++++++++++++++++++++------------------ > 1 file changed, 42 insertions(+), 27 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index b8da901..d1dcfc76 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -278,12 +278,14 @@ int vgic_v2_map_resources(struct kvm *kvm) > goto out; > } > > - ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, > - kvm_vgic_global_state.vcpu_base, > - KVM_VGIC_V2_CPU_SIZE, true); > - if (ret) { > - kvm_err("Unable to remap VGIC CPU to VCPU\n"); > - goto out; > + if (!static_branch_unlikely(&vgic_v2_cpuif_trap)) { > + ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, > + kvm_vgic_global_state.vcpu_base, > + KVM_VGIC_V2_CPU_SIZE, true); > + if (ret) { > + kvm_err("Unable to remap VGIC CPU to VCPU\n"); > + goto out; > + } > } > > dist->ready = true; > @@ -312,45 +314,51 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > return -ENXIO; > } > > - if (!PAGE_ALIGNED(info->vcpu.start)) { > - kvm_err("GICV physical address 0x%llx not page aligned\n", > - (unsigned long long)info->vcpu.start); > - return -ENXIO; > - } > + if (!PAGE_ALIGNED(info->vcpu.start) || > + !PAGE_ALIGNED(resource_size(&info->vcpu))) { > + kvm_info("GICV region size/alignement is unsafe, using trapping\n"); > + kvm_vgic_global_state.vcpu_base_va = ioremap(info->vcpu.start, > + resource_size(&info->vcpu)); > + if (!kvm_vgic_global_state.vcpu_base_va) { > + kvm_err("Cannot ioremap GICV\n"); > + return -ENOMEM; > + } > > - if (!PAGE_ALIGNED(resource_size(&info->vcpu))) { > - kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n", > - (unsigned long long)resource_size(&info->vcpu), > - PAGE_SIZE); > - return -ENXIO; > + ret = create_hyp_io_mappings(kvm_vgic_global_state.vcpu_base_va, > + kvm_vgic_global_state.vcpu_base_va + resource_size(&info->vcpu), > + info->vcpu.start); > + if (ret) { > + kvm_err("Cannot map GICV into hyp\n"); > + goto out; > + } > + > + static_branch_enable(&vgic_v2_cpuif_trap); > } > > kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start, > resource_size(&info->vctrl)); > if (!kvm_vgic_global_state.vctrl_base) { > kvm_err("Cannot ioremap GICH\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out; > } > > vtr = readl_relaxed(kvm_vgic_global_state.vctrl_base + GICH_VTR); > kvm_vgic_global_state.nr_lr = (vtr & 0x3f) + 1; > > - ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2); > - if (ret) { > - kvm_err("Cannot register GICv2 KVM device\n"); > - iounmap(kvm_vgic_global_state.vctrl_base); > - return ret; > - } > - > ret = create_hyp_io_mappings(kvm_vgic_global_state.vctrl_base, > kvm_vgic_global_state.vctrl_base + > resource_size(&info->vctrl), > info->vctrl.start); > if (ret) { > kvm_err("Cannot map VCTRL into hyp\n"); > - kvm_unregister_device_ops(KVM_DEV_TYPE_ARM_VGIC_V2); > - iounmap(kvm_vgic_global_state.vctrl_base); > - return ret; > + goto out; > + } > + > + ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2); > + if (ret) { > + kvm_err("Cannot register GICv2 KVM device\n"); > + goto out; > } > > kvm_vgic_global_state.can_emulate_gicv2 = true; > @@ -361,4 +369,11 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > kvm_info("vgic-v2@%llx\n", info->vctrl.start); > > return 0; > +out: > + if (kvm_vgic_global_state.vctrl_base) > + iounmap(kvm_vgic_global_state.vctrl_base); > + if (kvm_vgic_global_state.vcpu_base_va) > + iounmap(kvm_vgic_global_state.vcpu_base_va); > + > + return ret; > } > -- > 2.1.4 With the spelling fix from Peter, and the slightly more alarming message (shouldn't this be a kvm_warn("...using trapping") as well?) then: Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index b8da901..d1dcfc76 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -278,12 +278,14 @@ int vgic_v2_map_resources(struct kvm *kvm) goto out; } - ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, - kvm_vgic_global_state.vcpu_base, - KVM_VGIC_V2_CPU_SIZE, true); - if (ret) { - kvm_err("Unable to remap VGIC CPU to VCPU\n"); - goto out; + if (!static_branch_unlikely(&vgic_v2_cpuif_trap)) { + ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base, + kvm_vgic_global_state.vcpu_base, + KVM_VGIC_V2_CPU_SIZE, true); + if (ret) { + kvm_err("Unable to remap VGIC CPU to VCPU\n"); + goto out; + } } dist->ready = true; @@ -312,45 +314,51 @@ int vgic_v2_probe(const struct gic_kvm_info *info) return -ENXIO; } - if (!PAGE_ALIGNED(info->vcpu.start)) { - kvm_err("GICV physical address 0x%llx not page aligned\n", - (unsigned long long)info->vcpu.start); - return -ENXIO; - } + if (!PAGE_ALIGNED(info->vcpu.start) || + !PAGE_ALIGNED(resource_size(&info->vcpu))) { + kvm_info("GICV region size/alignement is unsafe, using trapping\n"); + kvm_vgic_global_state.vcpu_base_va = ioremap(info->vcpu.start, + resource_size(&info->vcpu)); + if (!kvm_vgic_global_state.vcpu_base_va) { + kvm_err("Cannot ioremap GICV\n"); + return -ENOMEM; + } - if (!PAGE_ALIGNED(resource_size(&info->vcpu))) { - kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n", - (unsigned long long)resource_size(&info->vcpu), - PAGE_SIZE); - return -ENXIO; + ret = create_hyp_io_mappings(kvm_vgic_global_state.vcpu_base_va, + kvm_vgic_global_state.vcpu_base_va + resource_size(&info->vcpu), + info->vcpu.start); + if (ret) { + kvm_err("Cannot map GICV into hyp\n"); + goto out; + } + + static_branch_enable(&vgic_v2_cpuif_trap); } kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start, resource_size(&info->vctrl)); if (!kvm_vgic_global_state.vctrl_base) { kvm_err("Cannot ioremap GICH\n"); - return -ENOMEM; + ret = -ENOMEM; + goto out; } vtr = readl_relaxed(kvm_vgic_global_state.vctrl_base + GICH_VTR); kvm_vgic_global_state.nr_lr = (vtr & 0x3f) + 1; - ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2); - if (ret) { - kvm_err("Cannot register GICv2 KVM device\n"); - iounmap(kvm_vgic_global_state.vctrl_base); - return ret; - } - ret = create_hyp_io_mappings(kvm_vgic_global_state.vctrl_base, kvm_vgic_global_state.vctrl_base + resource_size(&info->vctrl), info->vctrl.start); if (ret) { kvm_err("Cannot map VCTRL into hyp\n"); - kvm_unregister_device_ops(KVM_DEV_TYPE_ARM_VGIC_V2); - iounmap(kvm_vgic_global_state.vctrl_base); - return ret; + goto out; + } + + ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2); + if (ret) { + kvm_err("Cannot register GICv2 KVM device\n"); + goto out; } kvm_vgic_global_state.can_emulate_gicv2 = true; @@ -361,4 +369,11 @@ int vgic_v2_probe(const struct gic_kvm_info *info) kvm_info("vgic-v2@%llx\n", info->vctrl.start); return 0; +out: + if (kvm_vgic_global_state.vctrl_base) + iounmap(kvm_vgic_global_state.vctrl_base); + if (kvm_vgic_global_state.vcpu_base_va) + iounmap(kvm_vgic_global_state.vcpu_base_va); + + return ret; }
So far, we've been disabling KVM on systems where the GICV region couldn't be safely given to a guest. Now that we're able to handle this access safely by emulating it in HYP, we can enable this feature when we detect an unsafe configuration. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- virt/kvm/arm/vgic/vgic-v2.c | 69 +++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 27 deletions(-)