Re: [PATCH v3 00/66] KVM: arm64: ARMv8.3/8.4 Nested Virtualization support
Re-send in case the previous email was blocked for the inlined hyper-link.
Hi Marc,
I have tried to enable the NV support in Qemu, and now I can
successfully boot a L2 guest
in Qemu KVM mode.
This patch series looks good from the Qemu side except for two minor
requirements:
(1) Qemu will check whether a feature was supported by the KVM cap
when the user tries
to enable it in the command line, so a new capability was
prefered for the NV(KVM_CAP_ARM_NV?).
(2) According to the Documentation/virt/kvm/api.rst, userspace can
call KVM_ARM_VCPU_INIT
multiple times for a given vcpu, but the kvm_vcpu_init_nested()
do have some issue when
called multiple times(please refer to the detailed comments in patch 63)
Regards,
Haibo
On Fri, 11 Dec 2020 at 00:00, Marc Zyngier wrote:
>
> This is a rework of the NV series that I posted 10 months ago[1], as a
> lot of the KVM code has changed since, and the series apply anymore
> (not that anybody really cares as the the HW is, as usual, made of
> unobtainium...).
>
> From the previous version:
>
> - Integration with the new page-table code
> - New exception injection code
> - No more messing with the nVHE code
> - No AArch32
> - Rebased on v5.10-rc4 + kvmarm/next for 5.11
>
> From a functionality perspective, you can expect a L2 guest to work,
> but don't even think of L3, as we only partially emulate the
> ARMv8.{3,4}-NV extensions themselves. Same thing for vgic, debug, PMU,
> as well as anything that would require a Stage-1 PTW. What we want to
> achieve is that with NV disabled, there is no performance overhead and
> no regression.
>
> The series is roughly divided in 5 parts: exception handling, memory
> virtualization, interrupts and timers for ARMv8.3, followed by the
> ARMv8.4 support. There are of course some dependencies, but you'll
> hopefully get the gist of it.
>
> For the most courageous of you, I've put out a branch[2]. Of course,
> you'll need some userspace. Andre maintains a hacked version of
> kvmtool[3] that takes a --nested option, allowing the guest to be
> started at EL2. You can run the whole stack in the Foundation
> model. Don't be in a hurry ;-).
>
> And to be clear: although Jintack and Christoffer have written tons of
> the stuff originaly, I'm the one responsible for breaking it!
>
> [1] https://lore.kernel.org/r/20200211174938.27809-1-...@kernel.org
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> kvm-arm64/nv-5.11.-WIP
> [3] git://linux-arm.org/kvmtool.git nv/nv-wip-5.2-rc5
>
> Andre Przywara (1):
> KVM: arm64: nv: vgic: Allow userland to set VGIC maintenance IRQ
>
> Christoffer Dall (15):
> KVM: arm64: nv: Introduce nested virtualization VCPU feature
> KVM: arm64: nv: Reset VCPU to EL2 registers if VCPU nested virt is set
> KVM: arm64: nv: Allow userspace to set PSR_MODE_EL2x
> KVM: arm64: nv: Add nested virt VCPU primitives for vEL2 VCPU state
> KVM: arm64: nv: Reset VMPIDR_EL2 and VPIDR_EL2 to sane values
> KVM: arm64: nv: Handle trapped ERET from virtual EL2
> KVM: arm64: nv: Emulate PSTATE.M for a guest hypervisor
> KVM: arm64: nv: Trap EL1 VM register accesses in virtual EL2
> KVM: arm64: nv: Only toggle cache for virtual EL2 when SCTLR_EL2
> changes
> KVM: arm64: nv: Implement nested Stage-2 page table walk logic
> KVM: arm64: nv: Unmap/flush shadow stage 2 page tables
> KVM: arm64: nv: arch_timer: Support hyp timer emulation
> KVM: arm64: nv: vgic: Emulate the HW bit in software
> KVM: arm64: nv: Add nested GICv3 tracepoints
> KVM: arm64: nv: Sync nested timer state with ARMv8.4
>
> Jintack Lim (19):
> arm64: Add ARM64_HAS_NESTED_VIRT cpufeature
> KVM: arm64: nv: Handle HCR_EL2.NV system register traps
> KVM: arm64: nv: Support virtual EL2 exceptions
> KVM: arm64: nv: Inject HVC exceptions to the virtual EL2
> KVM: arm64: nv: Trap SPSR_EL1, ELR_EL1 and VBAR_EL1 from virtual EL2
> KVM: arm64: nv: Trap CPACR_EL1 access in virtual EL2
> KVM: arm64: nv: Handle PSCI call via smc from the guest
> KVM: arm64: nv: Respect virtual HCR_EL2.TWX setting
> KVM: arm64: nv: Respect virtual CPTR_EL2.{TFP,FPEN} settings
> KVM: arm64: nv: Respect the virtual HCR_EL2.NV bit setting
> KVM: arm64: nv: Respect virtual HCR_EL2.TVM and TRVM settings
> KVM: arm64: nv: Respect the virtual HCR_EL2.NV1 bit setting
> KVM: arm64: nv: Emulate EL12 register accesses from the virtual EL2
> KVM: arm64: nv: Configure HCR_EL2 for nested virtualization
> KVM: arm64: nv: Introduce sys_reg_desc.forward_trap
> KVM: arm64: nv: Set a handler for the system instruction traps
> KVM: arm64: nv: Trap and emulate AT instructions from virtual EL2
> KVM: arm64: nv: Trap and emulate TLBI instructions from virtual EL2
> KVM: arm64: nv: Nested GICv3 Support
>
> Marc Zyngier (31):
> KVM: arm64: nv: Add EL2 system registers to vcpu context
> KVM: arm64: nv: Add non-VHE-EL2->EL1 translation helpers
> KVM: arm64: nv: Handle virtual EL2 registers in
>
Re: [PATCH v3 33/66] KVM: arm64: nv: Support multiple nested Stage-2 mmu structures
On Fri, 11 Dec 2020 at 00:04, Marc Zyngier wrote:
>
> Add Stage-2 mmu data structures for virtual EL2 and for nested guests.
> We don't yet populate shadow Stage-2 page tables, but we now have a
> framework for getting to a shadow Stage-2 pgd.
>
> We allocate twice the number of vcpus as Stage-2 mmu structures because
> that's sufficient for each vcpu running two translation regimes without
> having to flush the Stage-2 page tables.
>
> Co-developed-by: Christoffer Dall
> Signed-off-by: Christoffer Dall
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_host.h | 29 +
> arch/arm64/include/asm/kvm_mmu.h| 8 ++
> arch/arm64/include/asm/kvm_nested.h | 7 ++
> arch/arm64/kvm/arm.c| 16 ++-
> arch/arm64/kvm/mmu.c| 18 ++-
> arch/arm64/kvm/nested.c | 183
> 6 files changed, 250 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index d731cf7a56cb..d99e51e7cbee 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -95,14 +95,43 @@ struct kvm_s2_mmu {
> int __percpu *last_vcpu_ran;
>
> struct kvm *kvm;
> +
> + /*
> +* For a shadow stage-2 MMU, the virtual vttbr programmed by the guest
> +* hypervisor. Unused for kvm_arch->mmu. Set to 1 when the structure
> +* contains no valid information.
> +*/
> + u64 vttbr;
> +
> + /* true when this represents a nested context where virtual
> HCR_EL2.VM == 1 */
> + boolnested_stage2_enabled;
> +
> + /*
> +* 0: Nobody is currently using this, check vttbr for validity
> +* >0: Somebody is actively using this.
> +*/
> + atomic_t refcnt;
> };
>
> +static inline bool kvm_s2_mmu_valid(struct kvm_s2_mmu *mmu)
> +{
> + return !(mmu->vttbr & 1);
> +}
> +
> struct kvm_arch_memory_slot {
> };
>
> struct kvm_arch {
> struct kvm_s2_mmu mmu;
>
> + /*
> +* Stage 2 paging stage for VMs with nested virtual using a virtual
> +* VMID.
> +*/
> + struct kvm_s2_mmu *nested_mmus;
> + size_t nested_mmus_size;
> + int nested_mmus_next;
> +
> /* VTCR_EL2 value for this VM */
> u64vtcr;
>
> diff --git a/arch/arm64/include/asm/kvm_mmu.h
> b/arch/arm64/include/asm/kvm_mmu.h
> index 76a8a0ca45b8..ec39015bb2a6 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -126,6 +126,7 @@ alternative_cb_end
> #include
> #include
> #include
> +#include
>
> void kvm_update_va_mask(struct alt_instr *alt,
> __le32 *origptr, __le32 *updptr, int nr_inst);
> @@ -184,6 +185,7 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr,
> size_t size,
> void **haddr);
> void free_hyp_pgds(void);
>
> +void kvm_unmap_stage2_range(struct kvm_s2_mmu *mmu, phys_addr_t start, u64
> size);
> void stage2_unmap_vm(struct kvm *kvm);
> int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu);
> void kvm_free_stage2_pgd(struct kvm_s2_mmu *mmu);
> @@ -306,5 +308,11 @@ static __always_inline void __load_guest_stage2(struct
> kvm_s2_mmu *mmu)
> asm(ALTERNATIVE("nop", "isb", ARM64_WORKAROUND_SPECULATIVE_AT));
> }
>
> +static inline u64 get_vmid(u64 vttbr)
> +{
> + return (vttbr & VTTBR_VMID_MASK(kvm_get_vmid_bits())) >>
> + VTTBR_VMID_SHIFT;
> +}
> +
> #endif /* __ASSEMBLY__ */
> #endif /* __ARM64_KVM_MMU_H__ */
> diff --git a/arch/arm64/include/asm/kvm_nested.h
> b/arch/arm64/include/asm/kvm_nested.h
> index 026ddaad972c..473ecd1d60d0 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -61,6 +61,13 @@ static inline u64 translate_cnthctl_el2_to_cntkctl_el1(u64
> cnthctl)
> (cnthctl & (CNTHCTL_EVNTI | CNTHCTL_EVNTDIR |
> CNTHCTL_EVNTEN)));
> }
>
> +extern void kvm_init_nested(struct kvm *kvm);
> +extern int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu);
> +extern void kvm_init_nested_s2_mmu(struct kvm_s2_mmu *mmu);
> +extern struct kvm_s2_mmu *lookup_s2_mmu(struct kvm *kvm, u64 vttbr, u64 hcr);
> +extern void kvm_vcpu_load_hw_mmu(struct kvm_vcpu *vcpu);
> +extern void kvm_vcpu_put_hw_mmu(struct kvm_vcpu *vcpu);
> +
> int handle_wfx_nested(struct kvm_vcpu *vcpu, bool is_wfe);
> extern bool __forward_traps(struct kvm_vcpu *vcpu, unsigned int reg,
> u64 control_bit);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6e637d2b4cfb..1656dd80bbc4 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -142,6 +143,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> if (ret)
> return ret;
>
> + kvm_init_nested(kvm);
> +
>
Re: [PATCH v3 63/66] KVM: arm64: nv: Allocate VNCR page when required
On Fri, 11 Dec 2020 at 00:04, Marc Zyngier wrote:
>
> If running a NV guest on an ARMv8.4-NV capable system, let's
> allocate an additional page that will be used by the hypervisor
> to fulfill system register accesses.
>
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_host.h | 3 ++-
> arch/arm64/kvm/nested.c | 8
> arch/arm64/kvm/reset.c| 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 78630bd5124d..dada0678c28e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -523,7 +523,8 @@ struct kvm_vcpu_arch {
> */
> static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> {
> - if (unlikely(r >= __VNCR_START__ && ctxt->vncr_array))
> + if (unlikely(cpus_have_final_cap(ARM64_HAS_ENHANCED_NESTED_VIRT) &&
> +r >= __VNCR_START__ && ctxt->vncr_array))
> return >vncr_array[r - __VNCR_START__];
>
> return (u64 *)>sys_regs[r];
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index eef8f9873814..88147ec99755 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -47,6 +47,12 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> if (!cpus_have_final_cap(ARM64_HAS_NESTED_VIRT))
> return -EINVAL;
>
> + if (cpus_have_final_cap(ARM64_HAS_ENHANCED_NESTED_VIRT)) {
> + vcpu->arch.ctxt.vncr_array = (u64
> *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
> + if (!vcpu->arch.ctxt.vncr_array)
> + return -ENOMEM;
> + }
> +
If KVM_ARM_VCPU_INIT was called multiple times, the above codes would
try to allocate a new page
without free-ing the previous one. Besides that, the following
kvm_free_stage2_pgd() call would fail in the
second call with the error message "kvm_arch already initialized?".
I think a possible fix is to add a new flag to indicate whether the NV
related meta data have been initialized,
and only initialize them for the first call.
> mutex_lock(>lock);
>
> /*
> @@ -64,6 +70,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> kvm_init_stage2_mmu(kvm, [num_mmus - 2])) {
> kvm_free_stage2_pgd([num_mmus - 1]);
> kvm_free_stage2_pgd([num_mmus - 2]);
> + free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> + vcpu->arch.ctxt.vncr_array = NULL;
> } else {
> kvm->arch.nested_mmus_size = num_mmus;
> ret = 0;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 2d2c780e6c69..d281eb39036f 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -150,6 +150,7 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
> void kvm_arm_vcpu_destroy(struct kvm_vcpu *vcpu)
> {
> kfree(vcpu->arch.sve_state);
> + free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> }
>
> static void kvm_vcpu_reset_sve(struct kvm_vcpu *vcpu)
> --
> 2.29.2
>
> ___
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 00/66] KVM: arm64: ARMv8.3/8.4 Nested Virtualization support
On Fri, 11 Dec 2020 at 00:00, Marc Zyngier wrote:
>
> This is a rework of the NV series that I posted 10 months ago[1], as a
> lot of the KVM code has changed since, and the series apply anymore
> (not that anybody really cares as the the HW is, as usual, made of
> unobtainium...).
>
> From the previous version:
>
> - Integration with the new page-table code
> - New exception injection code
> - No more messing with the nVHE code
> - No AArch32
> - Rebased on v5.10-rc4 + kvmarm/next for 5.11
>
> From a functionality perspective, you can expect a L2 guest to work,
> but don't even think of L3, as we only partially emulate the
> ARMv8.{3,4}-NV extensions themselves. Same thing for vgic, debug, PMU,
> as well as anything that would require a Stage-1 PTW. What we want to
> achieve is that with NV disabled, there is no performance overhead and
> no regression.
>
> The series is roughly divided in 5 parts: exception handling, memory
> virtualization, interrupts and timers for ARMv8.3, followed by the
> ARMv8.4 support. There are of course some dependencies, but you'll
> hopefully get the gist of it.
>
> For the most courageous of you, I've put out a branch[2]. Of course,
> you'll need some userspace. Andre maintains a hacked version of
> kvmtool[3] that takes a --nested option, allowing the guest to be
> started at EL2. You can run the whole stack in the Foundation
> model. Don't be in a hurry ;-).
>
> And to be clear: although Jintack and Christoffer have written tons of
> the stuff originaly, I'm the one responsible for breaking it!
>
> [1] https://lore.kernel.org/r/20200211174938.27809-1-...@kernel.org
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
kvm-arm64/nv-5.11.-WIP
> [3] git://linux-arm.org/kvmtool.git nv/nv-wip-5.2-rc5
Hi Marc,
I have tried to enable the NV support in Qemu, and now I can successfully
boot a L2 guest
in Qemu KVM mode.
This patch series looks good from the Qemu side except for two minor
requirements:
(1) Qemu will check whether a feature was supported by the KVM cap when the
user tries
to enable it in the command line, so a new capability was prefered for
the NV(KVM_CAP_ARM_NV?).
(2) According to the Documentation/virt/kvm/api.rst
,
userspace can call KVM_ARM_VCPU_INIT
multiple times for a given vcpu, but the kvm_vcpu_init_nested() do
have some issue when
called multiple times(please refer to the detailed comments in patch
63)
Regards,
Haibo
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 08/21] arm64: Move SCTLR_EL1 initialisation to EL-agnostic code
On Mon, Jan 18, 2021 at 09:45:20AM +, Marc Zyngier wrote: > We can now move the initial SCTLR_EL1 setup to be used for both > EL1 and EL2 setup. > > Signed-off-by: Marc Zyngier Acked-by: Catalin Marinas ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 03/21] arm64: Turn the MMU-on sequence into a macro
On Mon, Jan 18, 2021 at 09:45:15AM +, Marc Zyngier wrote: > Turning the MMU on is a popular sport in the arm64 kernel, and > we do it more than once, or even twice. As we are about to add > even more, let's turn it into a macro. > > No expected functional change. > > Signed-off-by: Marc Zyngier Acked-by: Catalin Marinas ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 02/21] arm64: Fix outdated TCR setup comment
On Mon, Jan 18, 2021 at 09:45:14AM +, Marc Zyngier wrote: > The arm64 kernel has long be able to use more than 39bit VAs. > Since day one, actually. Let's rewrite the offending comment. > > Signed-off-by: Marc Zyngier > --- > arch/arm64/mm/proc.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > index 1f7ee8c8b7b8..ece785477bdc 100644 > --- a/arch/arm64/mm/proc.S > +++ b/arch/arm64/mm/proc.S > @@ -464,8 +464,8 @@ SYM_FUNC_START(__cpu_setup) > #endif > msr mair_el1, x5 > /* > - * Set/prepare TCR and TTBR. We use 512GB (39-bit) address range for > - * both user and kernel. > + * Set/prepare TCR and TTBR. TCR_EL1.T1SZ gets further > + * adjusted if the kernel is compiled with 52bit VA support. I think both T0SZ and T1SZ get updated based on a mismatch between the kernel configuration and the hardware support. Anyway, I'm not asking for a detailed comment here, so: Acked-by: Catalin Marinas ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 8/9] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
Hi Eric,
On 1/14/21 10:16 AM, Auger Eric wrote:
> Hi Alexandru,
>
> On 1/12/21 6:02 PM, Alexandru Elisei wrote:
>> Hi Eric,
>>
>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>> Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the
>>> reporting of GICR_TYPER.Last for userspace") temporarily fixed
>>> a bug identified when attempting to access the GICR_TYPER
>>> register before the redistributor region setting but dropped
>>> the support of the LAST bit. This patch restores its
>>> support (if the redistributor region was set) while keeping the
>>> code safe.
>> I suppose the reason for emulating GICR_TYPER.Last is for architecture
>> compliance,
>> right? I think that should be in the commit message.
> OK added this in the commit msg.
>>> Signed-off-by: Eric Auger
>>> ---
>>> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++-
>>> include/kvm/arm_vgic.h | 1 +
>>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> index 581f0f49..2f9ef6058f6e 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> @@ -277,6 +277,8 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct
>>> kvm_vcpu *vcpu,
>>> gpa_t addr, unsigned int len)
>>> {
>>> unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
>>> + struct vgic_cpu *vgic_cpu = >arch.vgic_cpu;
>>> + struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>>> int target_vcpu_id = vcpu->vcpu_id;
>>> u64 value;
>>>
>>> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct
>>> kvm_vcpu *vcpu,
>>> if (vgic_has_its(vcpu->kvm))
>>> value |= GICR_TYPER_PLPIS;
>>>
>>> - /* reporting of the Last bit is not supported for userspace */
>>> + if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
>>> + value |= GICR_TYPER_LAST;
>>> +
>>> return extract_bytes(value, addr & 7, len);
>>> }
>>>
>>> @@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>> return -EINVAL;
>>>
>>> vgic_cpu->rdreg = rdreg;
>>> + vgic_cpu->rdreg_index = rdreg->free_index;
>> What happens if the next redistributor region we register has the base
>> address
>> adjacent to this one?
>>
>> I'm really not familiar with the code, but is it not possible to create two
>> Redistributor regions (via
>> KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST)) where the second
>> Redistributor region start address is immediately after the last
>> Redistributor in
>> the preceding region?
> KVM_VGIC_V3_ADDR_TYPE_REDIST only allows to create a single rdist
> region. Only KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION allows to register
> several of them.
>
> with KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, it is possible to register
> adjacent rdist regions. vgic_v3_rdist_free_slot() previously returned
> the 1st rdist region where enough space remains for inserting the new
> reg. We put the rdist at the free index there.
>
> But maybe I misunderstood your question?
Yes, I think you did a good job at answering my poorly worded question.
This is the case I am concerned about:
1. Userspace sets first redistributor base address to 0x0 via
KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION(count = 1, base = 0x0, flags = 0, index =
0).
2. Userspace sets first redistributor base address to 0x0 + 128K, immediately
following the previous Redistributor.
In that case the two Redistributors will be represented by two separate struct
vgic_redist_region, but they are adjacent to one another and represent one
contiguous memory region.
>From what I understand from your patch, GICR_TYPER.Last will be set for both
Redistributors, when it should be set only for the second Redistributor. Does
any
of that make sense?
Thanks,
Alex
>
> Thanks
>
> Eric
>> Thanks,
>> Alex
>>>
>>> rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE;
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index a8d8fdcd3723..596c069263a7 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -322,6 +322,7 @@ struct vgic_cpu {
>>> */
>>> struct vgic_io_device rd_iodev;
>>> struct vgic_redist_region *rdreg;
>>> + u32 rdreg_index;
>>>
>>> /* Contains the attributes and gpa of the LPI pending tables. */
>>> u64 pendbaser;
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 5/9] KVM: arm: move has_run_once after the map_resources
Hi Eric,
On 1/14/21 10:02 AM, Auger Eric wrote:
> Hi Alexandru,
>
> On 1/12/21 3:55 PM, Alexandru Elisei wrote:
>> Hi Eric,
>>
>> On 12/12/20 6:50 PM, Eric Auger wrote:
>>> has_run_once is set to true at the beginning of
>>> kvm_vcpu_first_run_init(). This generally is not an issue
>>> except when exercising the code with KVM selftests. Indeed,
>>> if kvm_vgic_map_resources() fails due to erroneous user settings,
>>> has_run_once is set and this prevents from continuing
>>> executing the test. This patch moves the assignment after the
>>> kvm_vgic_map_resources().
>>>
>>> Signed-off-by: Eric Auger
>>> ---
>>> arch/arm64/kvm/arm.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>> index c0ffb019ca8b..331fae6bff31 100644
>>> --- a/arch/arm64/kvm/arm.c
>>> +++ b/arch/arm64/kvm/arm.c
>>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu
>>> *vcpu)
>>> if (!kvm_arm_vcpu_is_finalized(vcpu))
>>> return -EPERM;
>>>
>>> - vcpu->arch.has_run_once = true;
>>> -
>>> if (likely(irqchip_in_kernel(kvm))) {
>>> /*
>>> * Map the VGIC hardware resources before running a vcpu the
>>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu
>>> *vcpu)
>>> static_branch_inc(_irqchip_in_use);
>>> }
>>>
>>> + vcpu->arch.has_run_once = true;
>> I have a few concerns regarding this:
>>
>> 1. Moving has_run_once = true here seems very arbitrary to me -
>> kvm_timer_enable()
>> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous
>> user
>> values. If there's a reason why the assignment cannot be moved at the end of
>> the
>> function, I think it should be clearly stated in a comment for the people who
>> might be tempted to write similar tests for the timer or pmu.
> Setting has_run_once = true at the entry of the function looks to me
> even more arbitrary. I agree with you that eventually has_run_once may
Or it could be it's there to prevent the user from calling
kvm_vgic_map_resources() a second time after it failed. This is what I'm
concerned
about and I think deserves more investigation.
Thanks,
Alex
> be moved at the very end but maybe this can be done later once timer,
> pmu tests haven ben written
>> 2. There are many ways that kvm_vgic_map_resources() can fail, other than
>> incorrect user settings. I started digging into how
>> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and
>> this
>> is what I managed to find before I gave up:
>>
>> * vgic_init() can fail in:
>> - kvm_vgic_dist_init()
>> - vgic_v3_init()
>> - kvm_vgic_setup_default_irq_routing()
>> * vgic_register_dist_iodev() can fail in:
>> - vgic_v3_init_dist_iodev()
>> - kvm_io_bus_register_dev()(*)
>> * kvm_phys_addr_ioremap() can fail in:
>> - kvm_mmu_topup_memory_cache()
>> - kvm_pgtable_stage2_map()
> I changed the commit msg so that "incorrect user settings" sounds as an
> example.
>> So if any of the functions below fail, are we 100% sure it is safe to allow
>> the
>> user to execute kvm_vgic_map_resources() again?
> I think additional tests will confirm this. However at the moment,
> moving the assignment, which does not look wrong to me, allows to
> greatly simplify the tests so I would tend to say that it is worth.
>> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a
>> caller that tries to register the same device address range and it will
>> create
>> another identical range. Is this intentional? Is it a bug that should be
>> fixed? Or
>> am I misunderstanding the function?
> doesn't kvm_io_bus_cmp() do the check?
>
> Thanks
>
> Eric
>> Thanks,
>> Alex
>>> +
>>> ret = kvm_timer_enable(vcpu);
>>> if (ret)
>>> return ret;
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 0/5] ARM: arm64: Add SMCCC TRNG entropy service
On 2021-01-20 13:49, Will Deacon wrote: On Wed, Jan 20, 2021 at 01:45:24PM +, Andre Przywara wrote: On Wed, 20 Jan 2021 13:26:26 + Marc Zyngier wrote: Hi, > On 2021-01-20 13:01, Will Deacon wrote: > > On Wed, 6 Jan 2021 10:34:48 +, Andre Przywara wrote: > >> a fix to v5, now *really* fixing the wrong priority of SMCCC vs. > >> RNDR in arch_get_random_seed_long_early(). Apologies for messing > >> this up in v5 and thanks to broonie for being on the watch! > >> > >> Will, Catalin: it would be much appreciated if you could consider > >> taking > >> patch 1/5. This contains the common definitions, and is a > >> prerequisite for every other patch, although they are somewhat > >> independent and likely > >> will need to go through different subsystems. > >> > >> [...] > > > > Applied the first patch only to arm64 (for-next/rng), thanks! > > > > [1/5] firmware: smccc: Add SMCCC TRNG function call IDs > > https://git.kernel.org/arm64/c/67c6bb56b649 > > I can't see how the rest of the patches can go via any other tree > if all the definitions are in the first one. > > Andre, can you explain what your plan is? Well, I don't really have a great solution for that, other than hoping that 1/5 makes it into Linus' master at some point. I see that it's a stretch, but pulling 1/5 into 5.11 now would prepare the stage for the others to go via any tree, into 5.12-rc1? Or you could maybe take both 1/5 and 5/5 into your kvm-arm tree, and would hope that a git rebase later would sort this out for you? But I think you are much more experienced in those kind of issues, so happy to hear about any other solutions. for-next/rng is a stable branch, so anybody who wants the first patch can just pull it (without anything I queue on top). OK. I'll pull that branch and stash the KVM stuff on top. 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
Re: [PATCH v6 0/5] ARM: arm64: Add SMCCC TRNG entropy service
On Wed, Jan 20, 2021 at 01:45:24PM +, Andre Przywara wrote: > On Wed, 20 Jan 2021 13:26:26 + > Marc Zyngier wrote: > > Hi, > > > On 2021-01-20 13:01, Will Deacon wrote: > > > On Wed, 6 Jan 2021 10:34:48 +, Andre Przywara wrote: > > >> a fix to v5, now *really* fixing the wrong priority of SMCCC vs. > > >> RNDR in arch_get_random_seed_long_early(). Apologies for messing > > >> this up in v5 and thanks to broonie for being on the watch! > > >> > > >> Will, Catalin: it would be much appreciated if you could consider > > >> taking > > >> patch 1/5. This contains the common definitions, and is a > > >> prerequisite for every other patch, although they are somewhat > > >> independent and likely > > >> will need to go through different subsystems. > > >> > > >> [...] > > > > > > Applied the first patch only to arm64 (for-next/rng), thanks! > > > > > > [1/5] firmware: smccc: Add SMCCC TRNG function call IDs > > > https://git.kernel.org/arm64/c/67c6bb56b649 > > > > I can't see how the rest of the patches can go via any other tree > > if all the definitions are in the first one. > > > > Andre, can you explain what your plan is? > > Well, I don't really have a great solution for that, other than hoping > that 1/5 makes it into Linus' master at some point. > > I see that it's a stretch, but pulling 1/5 into 5.11 now would > prepare the stage for the others to go via any tree, into 5.12-rc1? > > Or you could maybe take both 1/5 and 5/5 into your kvm-arm tree, and > would hope that a git rebase later would sort this out for you? > > But I think you are much more experienced in those kind of issues, so > happy to hear about any other solutions. for-next/rng is a stable branch, so anybody who wants the first patch can just pull it (without anything I queue on top). Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 0/5] ARM: arm64: Add SMCCC TRNG entropy service
On Wed, 20 Jan 2021 13:26:26 + Marc Zyngier wrote: Hi, > On 2021-01-20 13:01, Will Deacon wrote: > > On Wed, 6 Jan 2021 10:34:48 +, Andre Przywara wrote: > >> a fix to v5, now *really* fixing the wrong priority of SMCCC vs. > >> RNDR in arch_get_random_seed_long_early(). Apologies for messing > >> this up in v5 and thanks to broonie for being on the watch! > >> > >> Will, Catalin: it would be much appreciated if you could consider > >> taking > >> patch 1/5. This contains the common definitions, and is a > >> prerequisite for every other patch, although they are somewhat > >> independent and likely > >> will need to go through different subsystems. > >> > >> [...] > > > > Applied the first patch only to arm64 (for-next/rng), thanks! > > > > [1/5] firmware: smccc: Add SMCCC TRNG function call IDs > > https://git.kernel.org/arm64/c/67c6bb56b649 > > I can't see how the rest of the patches can go via any other tree > if all the definitions are in the first one. > > Andre, can you explain what your plan is? Well, I don't really have a great solution for that, other than hoping that 1/5 makes it into Linus' master at some point. I see that it's a stretch, but pulling 1/5 into 5.11 now would prepare the stage for the others to go via any tree, into 5.12-rc1? Or you could maybe take both 1/5 and 5/5 into your kvm-arm tree, and would hope that a git rebase later would sort this out for you? But I think you are much more experienced in those kind of issues, so happy to hear about any other solutions. Thanks, Andre ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 0/5] ARM: arm64: Add SMCCC TRNG entropy service
On 2021-01-20 13:01, Will Deacon wrote: On Wed, 6 Jan 2021 10:34:48 +, Andre Przywara wrote: a fix to v5, now *really* fixing the wrong priority of SMCCC vs. RNDR in arch_get_random_seed_long_early(). Apologies for messing this up in v5 and thanks to broonie for being on the watch! Will, Catalin: it would be much appreciated if you could consider taking patch 1/5. This contains the common definitions, and is a prerequisite for every other patch, although they are somewhat independent and likely will need to go through different subsystems. [...] Applied the first patch only to arm64 (for-next/rng), thanks! [1/5] firmware: smccc: Add SMCCC TRNG function call IDs https://git.kernel.org/arm64/c/67c6bb56b649 I can't see how the rest of the patches can go via any other tree if all the definitions are in the first one. Andre, can you explain what your plan is? 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
Re: [PATCH v6 0/5] ARM: arm64: Add SMCCC TRNG entropy service
On Wed, 20 Jan 2021 at 14:01, Will Deacon wrote: > > On Wed, 6 Jan 2021 10:34:48 +, Andre Przywara wrote: > > a fix to v5, now *really* fixing the wrong priority of SMCCC vs. RNDR > > in arch_get_random_seed_long_early(). Apologies for messing this up > > in v5 and thanks to broonie for being on the watch! > > > > Will, Catalin: it would be much appreciated if you could consider taking > > patch 1/5. This contains the common definitions, and is a prerequisite > > for every other patch, although they are somewhat independent and likely > > will need to go through different subsystems. > > > > [...] > > Applied the first patch only to arm64 (for-next/rng), thanks! > > [1/5] firmware: smccc: Add SMCCC TRNG function call IDs > https://git.kernel.org/arm64/c/67c6bb56b649 > > What's the plan for the rest of the series, and I think the related > change over at [1]? > Given that Ted seems to have lost interest in /dev/random patches, I was hoping [1] could be taken via the arm64 tree instead. Without this patch, I don't think we should expose the SMCCC RNG interface via arch_get_random_seed(), given how insanely often it will be called in that case. Note that the KVM patch implements the opposite end of this interface, and is not affected by [1] at all, so that can be taken at any time. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm64: Drop workaround for broken 'S' constraint with GCC 4.9
On Mon, 18 Jan 2021 13:01:29 +, Marc Zyngier wrote:
> Since GCC < 5.1 has been shown to be unsuitable for the arm64 kernel,
> let's drop the workaround for the 'S' asm constraint that GCC 4.9
> doesn't always grok.
>
> This is effectively a revert of 9fd339a45be5 ("arm64: Work around
> broken GCC 4.9 handling of "S" constraint").
Applied to arm64 (for-next/misc), thanks!
[1/1] arm64: Drop workaround for broken 'S' constraint with GCC 4.9
https://git.kernel.org/arm64/c/7001d4af926b
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 0/5] ARM: arm64: Add SMCCC TRNG entropy service
On Wed, 6 Jan 2021 10:34:48 +, Andre Przywara wrote: > a fix to v5, now *really* fixing the wrong priority of SMCCC vs. RNDR > in arch_get_random_seed_long_early(). Apologies for messing this up > in v5 and thanks to broonie for being on the watch! > > Will, Catalin: it would be much appreciated if you could consider taking > patch 1/5. This contains the common definitions, and is a prerequisite > for every other patch, although they are somewhat independent and likely > will need to go through different subsystems. > > [...] Applied the first patch only to arm64 (for-next/rng), thanks! [1/5] firmware: smccc: Add SMCCC TRNG function call IDs https://git.kernel.org/arm64/c/67c6bb56b649 What's the plan for the rest of the series, and I think the related change over at [1]? Cheers, -- Will [1] https://lore.kernel.org/linux-arm-kernel/20201105152944.16953-1-a...@kernel.org/ https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
