From: "Alex Bennée" <alex.bennee@linaro.org> To: Dave Martin <Dave.Martin@arm.com> Cc: Okamoto Takayuki <tokamoto@jp.fujitsu.com>, Christoffer Dall <cdall@kernel.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Marc Zyngier <marc.zyngier@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Zhang Lei <zhang.lei@jp.fujitsu.com>, Julien Grall <julien.grall@arm.com>, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 07/14] KVM: arm64/sve: Make register ioctl access errors more consistent Date: Thu, 25 Apr 2019 16:04:36 +0100 [thread overview] Message-ID: <87pnpaunwb.fsf@zen.linaroharston> (raw) In-Reply-To: <20190425130424.GA3567@e103592.cambridge.arm.com> Dave Martin <Dave.Martin@arm.com> writes: > On Thu, Apr 25, 2019 at 01:30:29PM +0100, Alex Bennée wrote: >> >> Dave Martin <Dave.Martin@arm.com> writes: >> >> > Currently, the way error codes are generated when processing the >> > SVE register access ioctls in a bit haphazard. >> > >> > This patch refactors the code so that the behaviour is more >> > consistent: now, -EINVAL should be returned only for unrecognised >> > register IDs or when some other runtime error occurs. -ENOENT is >> > returned for register IDs that are recognised, but whose >> > corresponding register (or slice) does not exist for the vcpu. >> > >> > To this end, in {get,set}_sve_reg() we now delegate the >> > vcpu_has_sve() check down into {get,set}_sve_vls() and >> > sve_reg_to_region(). The KVM_REG_ARM64_SVE_VLS special case is >> > picked off first, then sve_reg_to_region() plays the role of >> > exhaustively validating or rejecting the register ID and (where >> > accepted) computing the applicable register region as before. >> > >> > sve_reg_to_region() is rearranged so that -ENOENT or -EPERM is not >> > returned prematurely, before checking whether reg->id is in a >> > recognised range. >> > >> > -EPERM is now only returned when an attempt is made to access an >> > actually existing register slice on an unfinalized vcpu. >> > >> > Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface") >> > Fixes: 9033bba4b535 ("KVM: arm64/sve: Add pseudo-register for the guest's vector lengths") >> > Suggested-by: Andrew Jones <drjones@redhat.com> >> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> > Reviewed-by: Andrew Jones <drjones@redhat.com> > > [...] > >> > @@ -335,25 +344,30 @@ static int sve_reg_to_region(struct sve_state_reg_region *region, >> > /* Verify that we match the UAPI header: */ >> > BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES); >> > >> > - if ((reg->id & SVE_REG_SLICE_MASK) > 0) >> > - return -ENOENT; >> > - >> > - vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> > - >> > reg_num = (reg->id & SVE_REG_ID_MASK) >> SVE_REG_ID_SHIFT; >> > >> > if (reg->id >= zreg_id_min && reg->id <= zreg_id_max) { >> > + if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0) >> > + return -ENOENT; >> > + >> > + vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> > + >> > reqoffset = SVE_SIG_ZREG_OFFSET(vq, reg_num) - >> > SVE_SIG_REGS_OFFSET; >> > reqlen = KVM_SVE_ZREG_SIZE; >> > maxlen = SVE_SIG_ZREG_SIZE(vq); >> > } else if (reg->id >= preg_id_min && reg->id <= preg_id_max) { >> > + if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0) >> > + return -ENOENT; >> > + >> > + vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> > + >> >> I suppose you could argue for a: >> >> if (reg->id >= zreg_id_min && reg->id <= preg_id_max) { >> if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0) >> return -ENOENT; >> >> vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> >> if (reg->id <= zreg_id_max) { >> reqoffset = SVE_SIG_ZREG_OFFSET(vq, reg_num) - >> SVE_SIG_REGS_OFFSET; >> reqlen = KVM_SVE_ZREG_SIZE; >> maxlen = SVE_SIG_ZREG_SIZE(vq); >> } else { >> reqoffset = SVE_SIG_PREG_OFFSET(vq, reg_num) - >> SVE_SIG_REGS_OFFSET; >> reqlen = KVM_SVE_PREG_SIZE; >> maxlen = SVE_SIG_PREG_SIZE(vq); >> } >> } else { >> return -EINVAL; >> } >> >> but only for minimal DRY reasons. > > Agreed, but that bakes in another assumption: that the ZREG and PREG ID > ranges are contiguous. Ahh I'd misread: /* reg ID ranges for P- registers and FFR (which are contiguous) */ However these are defined in the UABI: /* Z- and P-regs occupy blocks at the following offsets within this range: */ #define KVM_REG_ARM64_SVE_ZREG_BASE 0 #define KVM_REG_ARM64_SVE_PREG_BASE 0x400 #define KVM_REG_ARM64_SVE_FFR_BASE 0x600 so there position is pretty fixed now right? > I preferred to keep the number of assumptions down. > > Althoug the resulting code wasn't ideal, the actual amount of > duplication that I ended up with here seemed low enough as to be > acceptable (though opinions can differ on that). It's no biggie ;-) -- Alex Bennée _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: "Alex Bennée" <alex.bennee@linaro.org> To: Dave Martin <Dave.Martin@arm.com> Cc: Okamoto Takayuki <tokamoto@jp.fujitsu.com>, Christoffer Dall <cdall@kernel.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Marc Zyngier <marc.zyngier@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Zhang Lei <zhang.lei@jp.fujitsu.com>, Julien Grall <julien.grall@arm.com>, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 07/14] KVM: arm64/sve: Make register ioctl access errors more consistent Date: Thu, 25 Apr 2019 16:04:36 +0100 [thread overview] Message-ID: <87pnpaunwb.fsf@zen.linaroharston> (raw) Message-ID: <20190425150436.l00shXZ046-WxSdNmbLq4JyFjstLVTjiBqFZsOXIAXg@z> (raw) In-Reply-To: <20190425130424.GA3567@e103592.cambridge.arm.com> Dave Martin <Dave.Martin@arm.com> writes: > On Thu, Apr 25, 2019 at 01:30:29PM +0100, Alex Bennée wrote: >> >> Dave Martin <Dave.Martin@arm.com> writes: >> >> > Currently, the way error codes are generated when processing the >> > SVE register access ioctls in a bit haphazard. >> > >> > This patch refactors the code so that the behaviour is more >> > consistent: now, -EINVAL should be returned only for unrecognised >> > register IDs or when some other runtime error occurs. -ENOENT is >> > returned for register IDs that are recognised, but whose >> > corresponding register (or slice) does not exist for the vcpu. >> > >> > To this end, in {get,set}_sve_reg() we now delegate the >> > vcpu_has_sve() check down into {get,set}_sve_vls() and >> > sve_reg_to_region(). The KVM_REG_ARM64_SVE_VLS special case is >> > picked off first, then sve_reg_to_region() plays the role of >> > exhaustively validating or rejecting the register ID and (where >> > accepted) computing the applicable register region as before. >> > >> > sve_reg_to_region() is rearranged so that -ENOENT or -EPERM is not >> > returned prematurely, before checking whether reg->id is in a >> > recognised range. >> > >> > -EPERM is now only returned when an attempt is made to access an >> > actually existing register slice on an unfinalized vcpu. >> > >> > Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface") >> > Fixes: 9033bba4b535 ("KVM: arm64/sve: Add pseudo-register for the guest's vector lengths") >> > Suggested-by: Andrew Jones <drjones@redhat.com> >> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> > Reviewed-by: Andrew Jones <drjones@redhat.com> > > [...] > >> > @@ -335,25 +344,30 @@ static int sve_reg_to_region(struct sve_state_reg_region *region, >> > /* Verify that we match the UAPI header: */ >> > BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES); >> > >> > - if ((reg->id & SVE_REG_SLICE_MASK) > 0) >> > - return -ENOENT; >> > - >> > - vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> > - >> > reg_num = (reg->id & SVE_REG_ID_MASK) >> SVE_REG_ID_SHIFT; >> > >> > if (reg->id >= zreg_id_min && reg->id <= zreg_id_max) { >> > + if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0) >> > + return -ENOENT; >> > + >> > + vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> > + >> > reqoffset = SVE_SIG_ZREG_OFFSET(vq, reg_num) - >> > SVE_SIG_REGS_OFFSET; >> > reqlen = KVM_SVE_ZREG_SIZE; >> > maxlen = SVE_SIG_ZREG_SIZE(vq); >> > } else if (reg->id >= preg_id_min && reg->id <= preg_id_max) { >> > + if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0) >> > + return -ENOENT; >> > + >> > + vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> > + >> >> I suppose you could argue for a: >> >> if (reg->id >= zreg_id_min && reg->id <= preg_id_max) { >> if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0) >> return -ENOENT; >> >> vq = sve_vq_from_vl(vcpu->arch.sve_max_vl); >> >> if (reg->id <= zreg_id_max) { >> reqoffset = SVE_SIG_ZREG_OFFSET(vq, reg_num) - >> SVE_SIG_REGS_OFFSET; >> reqlen = KVM_SVE_ZREG_SIZE; >> maxlen = SVE_SIG_ZREG_SIZE(vq); >> } else { >> reqoffset = SVE_SIG_PREG_OFFSET(vq, reg_num) - >> SVE_SIG_REGS_OFFSET; >> reqlen = KVM_SVE_PREG_SIZE; >> maxlen = SVE_SIG_PREG_SIZE(vq); >> } >> } else { >> return -EINVAL; >> } >> >> but only for minimal DRY reasons. > > Agreed, but that bakes in another assumption: that the ZREG and PREG ID > ranges are contiguous. Ahh I'd misread: /* reg ID ranges for P- registers and FFR (which are contiguous) */ However these are defined in the UABI: /* Z- and P-regs occupy blocks at the following offsets within this range: */ #define KVM_REG_ARM64_SVE_ZREG_BASE 0 #define KVM_REG_ARM64_SVE_PREG_BASE 0x400 #define KVM_REG_ARM64_SVE_FFR_BASE 0x600 so there position is pretty fixed now right? > I preferred to keep the number of assumptions down. > > Althoug the resulting code wasn't ideal, the actual amount of > duplication that I ended up with here seemed low enough as to be > acceptable (though opinions can differ on that). It's no biggie ;-) -- Alex Bennée _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2019-04-25 15:04 UTC|newest] Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-18 16:06 [PATCH v2 00/14] KVM: arm64: SVE cleanups Dave Martin 2019-04-18 16:06 ` Dave Martin 2019-04-18 16:06 ` Dave Martin 2019-04-18 16:06 ` [PATCH v2 01/14] arm64/sve: Clarify vq map semantics Dave Martin 2019-04-18 16:06 ` Dave Martin 2019-04-18 16:06 ` Dave Martin 2019-04-18 16:06 ` [PATCH v2 02/14] KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up SVE Dave Martin 2019-04-18 16:06 ` Dave Martin 2019-04-18 16:06 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 03/14] KVM: arm: Make vcpu finalization stubs into inline functions Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 04/14] KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to WARNs Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 06/14] KVM: arm64/sve: Miscellaneous tidyups in guest.c Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 07/14] KVM: arm64/sve: Make register ioctl access errors more consistent Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-25 12:30 ` Alex Bennée 2019-04-25 12:30 ` Alex Bennée 2019-04-25 13:04 ` Dave Martin 2019-04-25 13:04 ` Dave Martin 2019-04-25 13:04 ` Dave Martin 2019-04-25 15:04 ` Alex Bennée [this message] 2019-04-25 15:04 ` Alex Bennée 2019-04-25 15:27 ` Dave Martin 2019-04-25 15:27 ` Dave Martin 2019-04-25 15:27 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 08/14] KVM: arm64/sve: WARN when avoiding divide-by-zero in sve_reg_to_region() Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 10/14] KVM: arm64/sve: Explain validity checks in set_sve_vls() Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 11/14] KVM: arm/arm64: Clean up vcpu finalization function parameter naming Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 12/14] KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 13/14] KVM: Clarify KVM_{SET, GET}_ONE_REG error code documentation Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` [PATCH v2 14/14] KVM: arm64: Clarify access behaviour for out-of-range SVE register slice IDs Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-18 16:07 ` Dave Martin 2019-04-24 9:21 ` [PATCH v2 00/14] KVM: arm64: SVE cleanups Alex Bennée 2019-04-24 9:21 ` Alex Bennée 2019-04-24 9:38 ` Marc Zyngier 2019-04-24 9:38 ` Marc Zyngier 2019-04-25 12:35 ` Alex Bennée 2019-04-25 12:35 ` Alex Bennée 2019-04-25 13:05 ` Dave Martin 2019-04-25 13:05 ` Dave Martin 2019-04-25 13:05 ` Dave Martin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87pnpaunwb.fsf@zen.linaroharston \ --to=alex.bennee@linaro.org \ --cc=Dave.Martin@arm.com \ --cc=ard.biesheuvel@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=cdall@kernel.org \ --cc=julien.grall@arm.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=marc.zyngier@arm.com \ --cc=tokamoto@jp.fujitsu.com \ --cc=will.deacon@arm.com \ --cc=zhang.lei@jp.fujitsu.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.