| Message ID | 20190621163422.6127-6-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm/kvm: enable SVE in guests | expand |
On Fri, Jun 21, 2019 at 05:34:13PM +0100, Andrew Jones wrote: The purpose of this check should probably at least be described in a comment -- i.e., what actually depends on this? Cheers ---Dave > Suggested-by: Dave Martin <Dave.Martin@arm.com> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target/arm/helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index df4276f5f6ca..edba94004e0b 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > int new_len; > > /* Bits other than [3:0] are RAZ/WI. */ > + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); > raw_write(env, ri, value & 0xf); > > /* > -- > 2.20.1 >
On Mon, Jun 24, 2019 at 12:05:07PM +0100, Dave Martin wrote: > On Fri, Jun 21, 2019 at 05:34:13PM +0100, Andrew Jones wrote: > > The purpose of this check should probably at least be described in a > comment -- i.e., what actually depends on this? I was thinking the already present "Bits other than [3:0] are RAZ/WI." explained that, but how about this for an improvement? /* * Only the lowest 4 bits of ZCR_ELx may be used to constrain the vector * length, the rest of the bits are RAZ/WI. Since the vector length of * 128-bits (1 in quadwords) is represented as zero in ZCR_ELx, and all * vector lengths are represented as their length in quadwords minus 1, * then four bits allow up to quadword 16 to be selected. */ Thanks, drew > > Cheers > ---Dave > > > Suggested-by: Dave Martin <Dave.Martin@arm.com> > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > target/arm/helper.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > index df4276f5f6ca..edba94004e0b 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > > int new_len; > > > > /* Bits other than [3:0] are RAZ/WI. */ > > + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); > > raw_write(env, ri, value & 0xf); > > > > /* > > -- > > 2.20.1 > > >
On Mon, Jun 24, 2019 at 12:30:37PM +0100, Andrew Jones wrote: > On Mon, Jun 24, 2019 at 12:05:07PM +0100, Dave Martin wrote: > > On Fri, Jun 21, 2019 at 05:34:13PM +0100, Andrew Jones wrote: > > > > The purpose of this check should probably at least be described in a > > comment -- i.e., what actually depends on this? > > I was thinking the already present "Bits other than [3:0] are RAZ/WI." > explained that, but how about this for an improvement? > > /* > * Only the lowest 4 bits of ZCR_ELx may be used to constrain the vector > * length, the rest of the bits are RAZ/WI. Since the vector length of > * 128-bits (1 in quadwords) is represented as zero in ZCR_ELx, and all > * vector lengths are represented as their length in quadwords minus 1, > * then four bits allow up to quadword 16 to be selected. > */ No, maybe the existing comment is enough. I thought there might be more code elsewhere that assumes that checks sve_max_vq <= ARM_MAX_VQ then then assumes that sve_max_vq <= 16. But if not, we probably don't need an additional comment here. I haven't tried to understand all the code in the series beyond the user/kernel interactions, so maybe I was just paranoid. [...] Cheers ---Dave
On Mon, Jun 24, 2019 at 05:03:08PM +0100, Dave Martin wrote: > On Mon, Jun 24, 2019 at 12:30:37PM +0100, Andrew Jones wrote: > > On Mon, Jun 24, 2019 at 12:05:07PM +0100, Dave Martin wrote: > > > On Fri, Jun 21, 2019 at 05:34:13PM +0100, Andrew Jones wrote: > > > > > > The purpose of this check should probably at least be described in a > > > comment -- i.e., what actually depends on this? > > > > I was thinking the already present "Bits other than [3:0] are RAZ/WI." > > explained that, but how about this for an improvement? > > > > /* > > * Only the lowest 4 bits of ZCR_ELx may be used to constrain the vector > > * length, the rest of the bits are RAZ/WI. Since the vector length of > > * 128-bits (1 in quadwords) is represented as zero in ZCR_ELx, and all > > * vector lengths are represented as their length in quadwords minus 1, > > * then four bits allow up to quadword 16 to be selected. > > */ > > No, maybe the existing comment is enough. > > I thought there might be more code elsewhere that assumes that checks > sve_max_vq <= ARM_MAX_VQ then then assumes that sve_max_vq <= 16. But > if not, we probably don't need an additional comment here. I suppose there is some assumption that if sve_max_vq > 0 then it is also <= ARM_MAX_VQ elsewhere in QEMU. However here in zcr_write I don't think that assumption is being used. Here we're simply enforcing a limit of 16 within the emulation, without checking sve_max_vq at all. So I like the suggestion for a build bug like the one this patch adds, because otherwise we have 16 in two separate places; the ARM_MAX_VQ definition and the '& 0xf'. > > I haven't tried to understand all the code in the series beyond the > user/kernel interactions, so maybe I was just paranoid. Paranoia is good for the soul. Or something like that... Thanks, drew
On Tue, Jun 25, 2019 at 08:11:27AM +0200, Andrew Jones wrote: > On Mon, Jun 24, 2019 at 05:03:08PM +0100, Dave Martin wrote: > > On Mon, Jun 24, 2019 at 12:30:37PM +0100, Andrew Jones wrote: > > > On Mon, Jun 24, 2019 at 12:05:07PM +0100, Dave Martin wrote: > > > > On Fri, Jun 21, 2019 at 05:34:13PM +0100, Andrew Jones wrote: > > > > > > > > The purpose of this check should probably at least be described in a > > > > comment -- i.e., what actually depends on this? > > > > > > I was thinking the already present "Bits other than [3:0] are RAZ/WI." > > > explained that, but how about this for an improvement? > > > > > > /* > > > * Only the lowest 4 bits of ZCR_ELx may be used to constrain the vector > > > * length, the rest of the bits are RAZ/WI. Since the vector length of > > > * 128-bits (1 in quadwords) is represented as zero in ZCR_ELx, and all > > > * vector lengths are represented as their length in quadwords minus 1, > > > * then four bits allow up to quadword 16 to be selected. > > > */ > > > > No, maybe the existing comment is enough. > > > > I thought there might be more code elsewhere that assumes that checks > > sve_max_vq <= ARM_MAX_VQ then then assumes that sve_max_vq <= 16. But > > if not, we probably don't need an additional comment here. > > I suppose there is some assumption that if sve_max_vq > 0 then it is > also <= ARM_MAX_VQ elsewhere in QEMU. However here in zcr_write I don't > think that assumption is being used. Here we're simply enforcing a limit > of 16 within the emulation, without checking sve_max_vq at all. So I like > the suggestion for a build bug like the one this patch adds, because > otherwise we have 16 in two separate places; the ARM_MAX_VQ definition > and the '& 0xf'. I suppose we could also write the 0xf in terms of ARM_MAX_VQ, with a (ARM_MAX_VQ - 1), but that's getting into emulation implementation preferences, which I don't know anything about. So I'd leave that to Richard and Peter. > > > > > I haven't tried to understand all the code in the series beyond the > > user/kernel interactions, so maybe I was just paranoid. > > Paranoia is good for the soul. Or something like that... > > Thanks, > drew
Hi Drew, On 6/21/19 6:34 PM, Andrew Jones wrote: > Suggested-by: Dave Martin <Dave.Martin@arm.com> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target/arm/helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index df4276f5f6ca..edba94004e0b 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > int new_len; > > /* Bits other than [3:0] are RAZ/WI. */ > + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); Can you document in the commit message why this check is critical? Thanks Eric > raw_write(env, ri, value & 0xf); > > /* >
On 6/21/19 6:34 PM, Andrew Jones wrote: > Suggested-by: Dave Martin <Dave.Martin@arm.com> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target/arm/helper.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > diff --git a/target/arm/helper.c b/target/arm/helper.c > index df4276f5f6ca..edba94004e0b 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > int new_len; > > /* Bits other than [3:0] are RAZ/WI. */ > + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); > raw_write(env, ri, value & 0xf); > Re down-thread conversation, I think this is the nice easy way to make sure that the 0xf is modified if we ever decide to support larger vectors. I *think* that we could write ARM_MAX_VQ - 1 here, but I'm pretty sure there are a few other places where we assume that we only need 4 bits to store this value. Anyway, we'd definitely need to audit the code to allow ARM_MAX_VQ to change. r~
On Wed, Jun 26, 2019 at 12:01:10PM +0200, Auger Eric wrote: > Hi Drew, > > On 6/21/19 6:34 PM, Andrew Jones wrote: > > Suggested-by: Dave Martin <Dave.Martin@arm.com> > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > target/arm/helper.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > index df4276f5f6ca..edba94004e0b 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > > int new_len; > > > > /* Bits other than [3:0] are RAZ/WI. */ > > + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); > Can you document in the commit message why this check is critical? Sure. I can copy+paste the email subject into the commit message :-) drew > > Thanks > > Eric > > raw_write(env, ri, value & 0xf); > > > > /* > > >
Hi, On 6/26/19 3:28 PM, Andrew Jones wrote: > On Wed, Jun 26, 2019 at 12:01:10PM +0200, Auger Eric wrote: >> Hi Drew, >> >> On 6/21/19 6:34 PM, Andrew Jones wrote: >>> Suggested-by: Dave Martin <Dave.Martin@arm.com> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> target/arm/helper.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/target/arm/helper.c b/target/arm/helper.c >>> index df4276f5f6ca..edba94004e0b 100644 >>> --- a/target/arm/helper.c >>> +++ b/target/arm/helper.c >>> @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, >>> int new_len; >>> >>> /* Bits other than [3:0] are RAZ/WI. */ >>> + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); >> Can you document in the commit message why this check is critical? > > Sure. I can copy+paste the email subject into the commit message :-) Well that's not what I asked for. Are you enforcing an architectural maximum of 2048 bits or is the limitation due to some data structs in the existing code, ... For a non expert reviewer as I am it is not totally obvious. Thanks Eric > > drew > >> >> Thanks >> >> Eric >>> raw_write(env, ri, value & 0xf); >>> >>> /* >>> >>
On Wed, Jun 26, 2019 at 03:40:11PM +0200, Auger Eric wrote: > Hi, > > On 6/26/19 3:28 PM, Andrew Jones wrote: > > On Wed, Jun 26, 2019 at 12:01:10PM +0200, Auger Eric wrote: > >> Hi Drew, > >> > >> On 6/21/19 6:34 PM, Andrew Jones wrote: > >>> Suggested-by: Dave Martin <Dave.Martin@arm.com> > >>> Signed-off-by: Andrew Jones <drjones@redhat.com> > >>> --- > >>> target/arm/helper.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/target/arm/helper.c b/target/arm/helper.c > >>> index df4276f5f6ca..edba94004e0b 100644 > >>> --- a/target/arm/helper.c > >>> +++ b/target/arm/helper.c > >>> @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > >>> int new_len; > >>> > >>> /* Bits other than [3:0] are RAZ/WI. */ > >>> + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); > >> Can you document in the commit message why this check is critical? > > > > Sure. I can copy+paste the email subject into the commit message :-) > Well that's not what I asked for. Are you enforcing an architectural > maximum of 2048 bits or is the limitation due to some data structs in > the existing code, ... For a non expert reviewer as I am it is not > totally obvious. How's this for the commit message The current implementation of ZCR_ELx matches the architecture, only implementing the lower four bits, with the rest RAZ/WI. This puts a strict limit on ARM_MAX_VQ of 16. Make sure we don't let ARM_MAX_VQ grow without a corresponding update here. Thanks, drew > > Thanks > > Eric > > > > drew > > > >> > >> Thanks > >> > >> Eric > >>> raw_write(env, ri, value & 0xf); > >>> > >>> /* > >>> > >>
Hi, On 6/26/19 3:58 PM, Andrew Jones wrote: > On Wed, Jun 26, 2019 at 03:40:11PM +0200, Auger Eric wrote: >> Hi, >> >> On 6/26/19 3:28 PM, Andrew Jones wrote: >>> On Wed, Jun 26, 2019 at 12:01:10PM +0200, Auger Eric wrote: >>>> Hi Drew, >>>> >>>> On 6/21/19 6:34 PM, Andrew Jones wrote: >>>>> Suggested-by: Dave Martin <Dave.Martin@arm.com> >>>>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>>>> --- >>>>> target/arm/helper.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c >>>>> index df4276f5f6ca..edba94004e0b 100644 >>>>> --- a/target/arm/helper.c >>>>> +++ b/target/arm/helper.c >>>>> @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, >>>>> int new_len; >>>>> >>>>> /* Bits other than [3:0] are RAZ/WI. */ >>>>> + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); >>>> Can you document in the commit message why this check is critical? >>> >>> Sure. I can copy+paste the email subject into the commit message :-) >> Well that's not what I asked for. Are you enforcing an architectural >> maximum of 2048 bits or is the limitation due to some data structs in >> the existing code, ... For a non expert reviewer as I am it is not >> totally obvious. > > How's this for the commit message > > The current implementation of ZCR_ELx matches the architecture, only > implementing the lower four bits, with the rest RAZ/WI. This puts > a strict limit on ARM_MAX_VQ of 16. Make sure we don't let ARM_MAX_VQ > grow without a corresponding update here. Yep perfect. Thanks Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > > Thanks, > drew > >> >> Thanks >> >> Eric >>> >>> drew >>> >>>> >>>> Thanks >>>> >>>> Eric >>>>> raw_write(env, ri, value & 0xf); >>>>> >>>>> /* >>>>> >>>> >
diff --git a/target/arm/helper.c b/target/arm/helper.c index df4276f5f6ca..edba94004e0b 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5319,6 +5319,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri, int new_len; /* Bits other than [3:0] are RAZ/WI. */ + QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16); raw_write(env, ri, value & 0xf); /*
Suggested-by: Dave Martin <Dave.Martin@arm.com> Signed-off-by: Andrew Jones <drjones@redhat.com> --- target/arm/helper.c | 1 + 1 file changed, 1 insertion(+)