vector status when vlen doesn't match

vector status when vlen doesn't match

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2289 bytes --]

Yo Andy,

I was wondering if you could clarify something that came up today on
IRC.

When we enable vector we check vlenb and if it doesn't match the kernel
is suppose to not support vector. We do this in a few places, the first
is in riscv_fill_hwcap():

	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
		riscv_v_setup_vsize();
		/*
		 * ISA string in device tree might have 'v' flag, but
		 * CONFIG_RISCV_ISA_V is disabled in kernel.
		 * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
		 */
		if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
	}

Why does this riscv_v_setup_vsize() failing have no impact? Because this
is called only once on the boot CPU, right? I feel like it deserves a
comment as to why.

But the main thing I was wondering about was the other time it is called,
in smp_callin():
	if (has_vector()) {
		if (riscv_v_setup_vsize())
			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
	}

If this fails, we clear the HWCAP bit, but I am not immediately grasping
how this affects the rest of the kernel.
has_vector() just checks if the extension is supported by all cores on
the cpu using either an alternative or the extension support bitmap.
There are lots of sites in the kernel that use has_vector() as the
gating check (as far as I can tell), so if vlen does not match between
CPUs should we not be returning an error for has_vector()?

The alternative that has_vector() relies on is patched before the non-boot
CPUs are enabled, so we cannot modify the result once the non-boot CPUs
are in their callin functions and detect a vlen mismatch while at at the
same time using it in smp_calling(), so should this code be changed to
something along the lines of:

	if (riscv_has_extension_unlikely(v)) {
		if (riscv_v_setup_vsize()) {
			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
			riscv_v_vlen_mismatch = true;
		}
	}
and then has_vector() becomes something like
static __always_inline bool has_vector(void)
{
	return riscv_has_extension_unlikely(RISCV_ISA_EXT_v) && unlikely(riscv_v_vlen_mismatch);
}
Probably there's value to be gained in static branches etc here, but I
was just trying to explain what I was getting at. Maybe I am missing
something though? I do remember talking about this back when the vector
patches were still in review.

Thanks,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: vector status when vlen doesn't match

[Palmer Dabbelt]

On Wed, 28 Feb 2024 08:02:47 PST (-0800), Conor Dooley wrote:
> Yo Andy,
>
> I was wondering if you could clarify something that came up today on
> IRC.
>
> When we enable vector we check vlenb and if it doesn't match the kernel
> is suppose to not support vector. We do this in a few places, the first
> is in riscv_fill_hwcap():
>
> 	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> 		riscv_v_setup_vsize();
> 		/*
> 		 * ISA string in device tree might have 'v' flag, but
> 		 * CONFIG_RISCV_ISA_V is disabled in kernel.
> 		 * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
> 		 */
> 		if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> 	}
>
> Why does this riscv_v_setup_vsize() failing have no impact? Because this
> is called only once on the boot CPU, right? I feel like it deserves a
> comment as to why.

Unless I'm missing something, that first call can't fail: there's no 
previous riscv_v_size set, so there's nothing to trigger the failure.

Might warrant a comment, though...

> But the main thing I was wondering about was the other time it is called,
> in smp_callin():
> 	if (has_vector()) {
> 		if (riscv_v_setup_vsize())
> 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> 	}
>
> If this fails, we clear the HWCAP bit, but I am not immediately grasping
> how this affects the rest of the kernel.

I think it's actually a bug: we're not clearing the bit in riscv_isa, so 
this only ends up disabling the V bit in AT_HWCAP and thus doesn't do 
anything in the kernel.

> has_vector() just checks if the extension is supported by all cores on
> the cpu using either an alternative or the extension support bitmap.
> There are lots of sites in the kernel that use has_vector() as the
> gating check (as far as I can tell), so if vlen does not match between
> CPUs should we not be returning an error for has_vector()?
>
> The alternative that has_vector() relies on is patched before the non-boot
> CPUs are enabled, so we cannot modify the result once the non-boot CPUs
> are in their callin functions and detect a vlen mismatch while at at the
> same time using it in smp_calling(), so should this code be changed to
> something along the lines of:
>
> 	if (riscv_has_extension_unlikely(v)) {
> 		if (riscv_v_setup_vsize()) {
> 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> 			riscv_v_vlen_mismatch = true;
> 		}
> 	}
> and then has_vector() becomes something like
> static __always_inline bool has_vector(void)
> {
> 	return riscv_has_extension_unlikely(RISCV_ISA_EXT_v) && unlikely(riscv_v_vlen_mismatch);
> }
>
> Probably there's value to be gained in static branches etc here, but I
> was just trying to explain what I was getting at. Maybe I am missing
> something though? I do remember talking about this back when the vector
> patches were still in review.

I actually don't think that alone fixes it: even if we make has_vector() 
respect the changes from the new CPU (whether we re-apply the 
alternatives or make a more dynamic check), we'd need to make sure the 
code that has already looked at has_vector() doesn't get scheduled on 
one of the shorter-VLENB CPUs.

So maybe the right answer here is to flip things around, and just refuse 
to enable CPUs that come in with a VLENB different than the boot CPU?  
That way we could avoid being broken by systems that behave this way, if 
someone ends up building one then they can figure out how to make it 
work better.

> Thanks,
> Conor.
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: vector status when vlen doesn't match

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 5108 bytes --]

On Wed, Feb 28, 2024 at 01:56:51PM -0800, Palmer Dabbelt wrote:
> On Wed, 28 Feb 2024 08:02:47 PST (-0800), Conor Dooley wrote:
> > Yo Andy,
> > 
> > I was wondering if you could clarify something that came up today on
> > IRC.
> > 
> > When we enable vector we check vlenb and if it doesn't match the kernel
> > is suppose to not support vector. We do this in a few places, the first
> > is in riscv_fill_hwcap():
> > 
> > 	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> > 		riscv_v_setup_vsize();
> > 		/*
> > 		 * ISA string in device tree might have 'v' flag, but
> > 		 * CONFIG_RISCV_ISA_V is disabled in kernel.
> > 		 * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
> > 		 */
> > 		if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> > 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > 	}
> > 
> > Why does this riscv_v_setup_vsize() failing have no impact? Because this
> > is called only once on the boot CPU, right? I feel like it deserves a
> > comment as to why.
> 
> Unless I'm missing something, that first call can't fail: there's no
> previous riscv_v_size set, so there's nothing to trigger the failure.
> 
> Might warrant a comment, though...

Yeah. I did write (or maybe moreso remodel) that function so I know it
only gets called on the boot CPU, I was just pointing out what is not
immediately obvious about that particular callsite.

> > But the main thing I was wondering about was the other time it is called,
> > in smp_callin():
> > 	if (has_vector()) {
> > 		if (riscv_v_setup_vsize())
> > 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > 	}
> > 
> > If this fails, we clear the HWCAP bit, but I am not immediately grasping
> > how this affects the rest of the kernel.
> 
> I think it's actually a bug: we're not clearing the bit in riscv_isa, so
> this only ends up disabling the V bit in AT_HWCAP and thus doesn't do
> anything in the kernel.

Yeah, that's what I was getting at - I could not see how disabling it in
hwcap would make a difference either in the kernel or in hwprobe. I was
not sure if I was missing something in terms of the
userspace-using-vector side of things, but I was pretty sure that
hwprobe and in-kernel vector had this bug.

> > has_vector() just checks if the extension is supported by all cores on
> > the cpu using either an alternative or the extension support bitmap.
> > There are lots of sites in the kernel that use has_vector() as the
> > gating check (as far as I can tell), so if vlen does not match between
> > CPUs should we not be returning an error for has_vector()?
> > 
> > The alternative that has_vector() relies on is patched before the non-boot
> > CPUs are enabled, so we cannot modify the result once the non-boot CPUs
> > are in their callin functions and detect a vlen mismatch while at at the
> > same time using it in smp_calling(), so should this code be changed to
> > something along the lines of:
> > 
> > 	if (riscv_has_extension_unlikely(v)) {
> > 		if (riscv_v_setup_vsize()) {
> > 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > 			riscv_v_vlen_mismatch = true;
> > 		}
> > 	}
> > and then has_vector() becomes something like
> > static __always_inline bool has_vector(void)
> > {
> > 	return riscv_has_extension_unlikely(RISCV_ISA_EXT_v) && unlikely(riscv_v_vlen_mismatch);
> > }
> > 
> > Probably there's value to be gained in static branches etc here, but I
> > was just trying to explain what I was getting at. Maybe I am missing
> > something though? I do remember talking about this back when the vector
> > patches were still in review.
> 
> I actually don't think that alone fixes it: even if we make has_vector()
> respect the changes from the new CPU (whether we re-apply the alternatives
> or make a more dynamic check), we'd need to make sure the code that has
> already looked at has_vector() doesn't get scheduled on one of the
> shorter-VLENB CPUs.

Is it possible to have that happen given this is in smp_callin()?
I guess the concern is some sort of hotplug situations, given that a CPU
could be hotplugged into a fully running system with a vlen mismatch.

> So maybe the right answer here is to flip things around, and just refuse to
> enable CPUs that come in with a VLENB different than the boot CPU?  That way
> we could avoid being broken by systems that behave this way,

smp_callin() doesn't return an error to report a failure, but it does
have a completion. I suppose we would just return early from
smp_callin() so that the completion never gets completed, in turn
causing __cpu_up() to fail?

> if someone ends
> up building one then they can figure out how to make it work better.

Apparently the next gen sophgo product does. Not sure if people will wnt
to run across all the cores though, apparently they different by a lot
more than vlen.
A DT property would allow us to get the vlen info before the CPUs have
themselves been brought up. Apparently there are Zvl extensions for
this, but I'd rather just have a property like we have for the cbo
sizes if it comes to DT properties.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: vector status when vlen doesn't match

[Andy Chiu]

On Thu, Feb 29, 2024 at 7:32 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Feb 28, 2024 at 01:56:51PM -0800, Palmer Dabbelt wrote:
> > On Wed, 28 Feb 2024 08:02:47 PST (-0800), Conor Dooley wrote:
> > > Yo Andy,
> > >
> > > I was wondering if you could clarify something that came up today on
> > > IRC.
> > >
> > > When we enable vector we check vlenb and if it doesn't match the kernel
> > > is suppose to not support vector. We do this in a few places, the first
> > > is in riscv_fill_hwcap():
> > >
> > >     if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> > >             riscv_v_setup_vsize();
> > >             /*
> > >              * ISA string in device tree might have 'v' flag, but
> > >              * CONFIG_RISCV_ISA_V is disabled in kernel.
> > >              * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
> > >              */
> > >             if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> > >                     elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > >     }
> > >
> > > Why does this riscv_v_setup_vsize() failing have no impact? Because this
> > > is called only once on the boot CPU, right? I feel like it deserves a
> > > comment as to why.
> >
> > Unless I'm missing something, that first call can't fail: there's no
> > previous riscv_v_size set, so there's nothing to trigger the failure.
> >
> > Might warrant a comment, though...
>
> Yeah. I did write (or maybe moreso remodel) that function so I know it
> only gets called on the boot CPU, I was just pointing out what is not
> immediately obvious about that particular callsite.

Yes, this call can't fall. Ok, I'll update the comment.

>
> > > But the main thing I was wondering about was the other time it is called,
> > > in smp_callin():
> > >     if (has_vector()) {
> > >             if (riscv_v_setup_vsize())
> > >                     elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > >     }
> > >
> > > If this fails, we clear the HWCAP bit, but I am not immediately grasping
> > > how this affects the rest of the kernel.
> >
> > I think it's actually a bug: we're not clearing the bit in riscv_isa, so
> > this only ends up disabling the V bit in AT_HWCAP and thus doesn't do
> > anything in the kernel.
>
> Yeah, that's what I was getting at - I could not see how disabling it in
> hwcap would make a difference either in the kernel or in hwprobe. I was
> not sure if I was missing something in terms of the
> userspace-using-vector side of things, but I was pretty sure that
> hwprobe and in-kernel vector had this bug.

hwprobe still reports the hardware has Vector. Shouldn't this be ok
because hwprobe reflects what is supported by the hardware? I think
we'd need another bit in hwprobe to reflect the value in hwcap though.

Userspace context switch code is gated by VS == 0. I am not sure if we
need something more obvious.

The in-kernel vector part was not well-considered. It sounds ok for
the non-preemptible vector. Userspace won't be able to execute any
user vector on a non-symmetric vector platform. So all in-kernel
vector code doesn't have to save/restore userspace vector registers.
The task executing non-preemptible in-kernel vector code has no chance
to be rescheduled on other cores during vector execution. So it should
be fine as long as multiple parts of kernel_vector_{begin,end}() do
not assume using the same vlen. Most vector code doesn't assume
running on a particular vlen (vlen agnostic), so ideally it should be
safe.

Parts of the kernel blindly assume riscv_v_size != 0. Those should be
changed. However, race conditions may happen if userspace is already
up and running at this stage (cpu hotplug?). We must not reschedule a
userspace process which is actively executing vector to a core that
runs in a different vlen.

It is not possible to execute the preemptible vector in a
non-symmetric vector platform. This should be gated by failing the
allocation of kernel_vstate.datap.

>
> > > has_vector() just checks if the extension is supported by all cores on
> > > the cpu using either an alternative or the extension support bitmap.
> > > There are lots of sites in the kernel that use has_vector() as the
> > > gating check (as far as I can tell), so if vlen does not match between
> > > CPUs should we not be returning an error for has_vector()?

No, has_vector() still returns true because it is backed by
alternative and determined during boot.

> > >
> > > The alternative that has_vector() relies on is patched before the non-boot
> > > CPUs are enabled, so we cannot modify the result once the non-boot CPUs
> > > are in their callin functions and detect a vlen mismatch while at at the
> > > same time using it in smp_calling(), so should this code be changed to
> > > something along the lines of:
> > >
> > >     if (riscv_has_extension_unlikely(v)) {
> > >             if (riscv_v_setup_vsize()) {
> > >                     elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > >                     riscv_v_vlen_mismatch = true;
> > >             }
> > >     }
> > > and then has_vector() becomes something like
> > > static __always_inline bool has_vector(void)
> > > {
> > >     return riscv_has_extension_unlikely(RISCV_ISA_EXT_v) && unlikely(riscv_v_vlen_mismatch);
> > > }
> > >
> > > Probably there's value to be gained in static branches etc here, but I
> > > was just trying to explain what I was getting at. Maybe I am missing
> > > something though? I do remember talking about this back when the vector
> > > patches were still in review.
> >
> > I actually don't think that alone fixes it: even if we make has_vector()
> > respect the changes from the new CPU (whether we re-apply the alternatives
> > or make a more dynamic check), we'd need to make sure the code that has
> > already looked at has_vector() doesn't get scheduled on one of the
> > shorter-VLENB CPUs.

I agree that flipping off the alternative for has_vector() could be
dangerous at this point.

>
> Is it possible to have that happen given this is in smp_callin()?
> I guess the concern is some sort of hotplug situations, given that a CPU
> could be hotplugged into a fully running system with a vlen mismatch.
>
> > So maybe the right answer here is to flip things around, and just refuse to
> > enable CPUs that come in with a VLENB different than the boot CPU?  That way
> > we could avoid being broken by systems that behave this way,
>
> smp_callin() doesn't return an error to report a failure, but it does
> have a completion. I suppose we would just return early from
> smp_callin() so that the completion never gets completed, in turn
> causing __cpu_up() to fail?

I agree disabling the entire vector or fail bringing up the cpu is the
safe way though.

>
> > if someone ends
> > up building one then they can figure out how to make it work better.
>
> Apparently the next gen sophgo product does. Not sure if people will wnt
> to run across all the cores though, apparently they different by a lot
> more than vlen.

The vector patchset assumes running on an SMP system. Supporting
non-symmetric vlen or non-symmetric isa may require some efforts.

> A DT property would allow us to get the vlen info before the CPUs have
> themselves been brought up. Apparently there are Zvl extensions for
> this, but I'd rather just have a property like we have for the cbo
> sizes if it comes to DT properties.

Yes, perhaps having a DT property is the first step to go. Because we
can prevent race set/get-ing riscv_v_vsize in such case.

>
> Cheers,
> Conor.

Thanks,
Andy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: vector status when vlen doesn't match

[Palmer Dabbelt]

On Wed, 28 Feb 2024 15:32:47 PST (-0800), Conor Dooley wrote:
> On Wed, Feb 28, 2024 at 01:56:51PM -0800, Palmer Dabbelt wrote:
>> On Wed, 28 Feb 2024 08:02:47 PST (-0800), Conor Dooley wrote:
>> > Yo Andy,
>> > 
>> > I was wondering if you could clarify something that came up today on
>> > IRC.
>> > 
>> > When we enable vector we check vlenb and if it doesn't match the kernel
>> > is suppose to not support vector. We do this in a few places, the first
>> > is in riscv_fill_hwcap():
>> > 
>> > 	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
>> > 		riscv_v_setup_vsize();
>> > 		/*
>> > 		 * ISA string in device tree might have 'v' flag, but
>> > 		 * CONFIG_RISCV_ISA_V is disabled in kernel.
>> > 		 * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
>> > 		 */
>> > 		if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
>> > 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
>> > 	}
>> > 
>> > Why does this riscv_v_setup_vsize() failing have no impact? Because this
>> > is called only once on the boot CPU, right? I feel like it deserves a
>> > comment as to why.
>> 
>> Unless I'm missing something, that first call can't fail: there's no
>> previous riscv_v_size set, so there's nothing to trigger the failure.
>> 
>> Might warrant a comment, though...
>
> Yeah. I did write (or maybe moreso remodel) that function so I know it
> only gets called on the boot CPU, I was just pointing out what is not
> immediately obvious about that particular callsite.

OK, so sounds like a comment then ;)

>> > But the main thing I was wondering about was the other time it is called,
>> > in smp_callin():
>> > 	if (has_vector()) {
>> > 		if (riscv_v_setup_vsize())
>> > 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
>> > 	}
>> > 
>> > If this fails, we clear the HWCAP bit, but I am not immediately grasping
>> > how this affects the rest of the kernel.
>> 
>> I think it's actually a bug: we're not clearing the bit in riscv_isa, so
>> this only ends up disabling the V bit in AT_HWCAP and thus doesn't do
>> anything in the kernel.
>
> Yeah, that's what I was getting at - I could not see how disabling it in
> hwcap would make a difference either in the kernel or in hwprobe. I was
> not sure if I was missing something in terms of the
> userspace-using-vector side of things, but I was pretty sure that
> hwprobe and in-kernel vector had this bug.

IIRC we used to have this stuff hooked into elf_hwcap back when it was 
the only user-visible interface.  The V support dates back to around 
then, so we probably just missed the in-flight conflict.

>> > has_vector() just checks if the extension is supported by all cores on
>> > the cpu using either an alternative or the extension support bitmap.
>> > There are lots of sites in the kernel that use has_vector() as the
>> > gating check (as far as I can tell), so if vlen does not match between
>> > CPUs should we not be returning an error for has_vector()?
>> > 
>> > The alternative that has_vector() relies on is patched before the non-boot
>> > CPUs are enabled, so we cannot modify the result once the non-boot CPUs
>> > are in their callin functions and detect a vlen mismatch while at at the
>> > same time using it in smp_calling(), so should this code be changed to
>> > something along the lines of:
>> > 
>> > 	if (riscv_has_extension_unlikely(v)) {
>> > 		if (riscv_v_setup_vsize()) {
>> > 			elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
>> > 			riscv_v_vlen_mismatch = true;
>> > 		}
>> > 	}
>> > and then has_vector() becomes something like
>> > static __always_inline bool has_vector(void)
>> > {
>> > 	return riscv_has_extension_unlikely(RISCV_ISA_EXT_v) && unlikely(riscv_v_vlen_mismatch);
>> > }
>> > 
>> > Probably there's value to be gained in static branches etc here, but I
>> > was just trying to explain what I was getting at. Maybe I am missing
>> > something though? I do remember talking about this back when the vector
>> > patches were still in review.
>> 
>> I actually don't think that alone fixes it: even if we make has_vector()
>> respect the changes from the new CPU (whether we re-apply the alternatives
>> or make a more dynamic check), we'd need to make sure the code that has
>> already looked at has_vector() doesn't get scheduled on one of the
>> shorter-VLENB CPUs.
>
> Is it possible to have that happen given this is in smp_callin()?
> I guess the concern is some sort of hotplug situations, given that a CPU
> could be hotplugged into a fully running system with a vlen mismatch.

Ya, something along those lines.  We essentially need to defer this sort 
of alternative processing until after all CPUs have booted and run their 
V detection, but IIUC that could be arbitrarily late if we're talking 
the hotplug world.

>> So maybe the right answer here is to flip things around, and just refuse to
>> enable CPUs that come in with a VLENB different than the boot CPU?  That way
>> we could avoid being broken by systems that behave this way,
>
> smp_callin() doesn't return an error to report a failure, but it does
> have a completion. I suppose we would just return early from
> smp_callin() so that the completion never gets completed, in turn
> causing __cpu_up() to fail?

Ya, I think something along those lines would do it.  I haven't really 
thought it through so I bet something hairy is lurking around in there.

>> if someone ends
>> up building one then they can figure out how to make it work better.
>
> Apparently the next gen sophgo product does. Not sure if people will wnt
> to run across all the cores though, apparently they different by a lot
> more than vlen.
> A DT property would allow us to get the vlen info before the CPUs have
> themselves been brought up. Apparently there are Zvl extensions for
> this, but I'd rather just have a property like we have for the cbo
> sizes if it comes to DT properties.

That might be the right answer here.  In general the "just run some code 
on the CPU to figure out what it implements" style of detection comes 
with a lot of these long-tail issues, DT just fixes that because we can 
look at every core from early in boot.

> Cheers,
> Conor.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: vector status when vlen doesn't match

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 9256 bytes --]

On Thu, Feb 29, 2024 at 09:32:56PM +0800, Andy Chiu wrote:
> On Thu, Feb 29, 2024 at 7:32 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Feb 28, 2024 at 01:56:51PM -0800, Palmer Dabbelt wrote:
> > > On Wed, 28 Feb 2024 08:02:47 PST (-0800), Conor Dooley wrote:
> > > > Yo Andy,
> > > >
> > > > I was wondering if you could clarify something that came up today on
> > > > IRC.
> > > >
> > > > When we enable vector we check vlenb and if it doesn't match the kernel
> > > > is suppose to not support vector. We do this in a few places, the first
> > > > is in riscv_fill_hwcap():
> > > >
> > > >     if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
> > > >             riscv_v_setup_vsize();
> > > >             /*
> > > >              * ISA string in device tree might have 'v' flag, but
> > > >              * CONFIG_RISCV_ISA_V is disabled in kernel.
> > > >              * Clear V flag in elf_hwcap if CONFIG_RISCV_ISA_V is disabled.
> > > >              */
> > > >             if (!IS_ENABLED(CONFIG_RISCV_ISA_V))
> > > >                     elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > > >     }
> > > >
> > > > Why does this riscv_v_setup_vsize() failing have no impact? Because this
> > > > is called only once on the boot CPU, right? I feel like it deserves a
> > > > comment as to why.
> > >
> > > Unless I'm missing something, that first call can't fail: there's no
> > > previous riscv_v_size set, so there's nothing to trigger the failure.
> > >
> > > Might warrant a comment, though...
> >
> > Yeah. I did write (or maybe moreso remodel) that function so I know it
> > only gets called on the boot CPU, I was just pointing out what is not
> > immediately obvious about that particular callsite.
> 
> Yes, this call can't fall. Ok, I'll update the comment.

Thanks.

> > > > But the main thing I was wondering about was the other time it is called,
> > > > in smp_callin():
> > > >     if (has_vector()) {
> > > >             if (riscv_v_setup_vsize())
> > > >                     elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > > >     }
> > > >
> > > > If this fails, we clear the HWCAP bit, but I am not immediately grasping
> > > > how this affects the rest of the kernel.
> > >
> > > I think it's actually a bug: we're not clearing the bit in riscv_isa, so
> > > this only ends up disabling the V bit in AT_HWCAP and thus doesn't do
> > > anything in the kernel.
> >
> > Yeah, that's what I was getting at - I could not see how disabling it in
> > hwcap would make a difference either in the kernel or in hwprobe. I was
> > not sure if I was missing something in terms of the
> > userspace-using-vector side of things, but I was pretty sure that
> > hwprobe and in-kernel vector had this bug.
> 
> hwprobe still reports the hardware has Vector. Shouldn't this be ok
> because hwprobe reflects what is supported by the hardware? I think
> we'd need another bit in hwprobe to reflect the value in hwcap though.

No. The hwprobe test was never whether or not the hardware has vector,
given has_vector() does not actually represent that. Rather,
has_vector() should only return true when the running kernel on hardware
that supports vector is capable of supporting vector.

> Userspace context switch code is gated by VS == 0. I am not sure if we
> need something more obvious.

Could you point me out where exactly this is happening? Also, what is
"VS" in this context?

> The in-kernel vector part was not well-considered. It sounds ok for
> the non-preemptible vector. Userspace won't be able to execute any
> user vector on a non-symmetric vector platform.

How does the kernel prevent this?

> So all in-kernel
> vector code doesn't have to save/restore userspace vector registers.
> The task executing non-preemptible in-kernel vector code has no chance
> to be rescheduled on other cores during vector execution. So it should
> be fine as long as multiple parts of kernel_vector_{begin,end}() do
> not assume using the same vlen. Most vector code doesn't assume
> running on a particular vlen (vlen agnostic), so ideally it should be
> safe.
> 
> Parts of the kernel blindly assume riscv_v_size != 0. Those should be
> changed. However, race conditions may happen if userspace is already
> up and running at this stage (cpu hotplug?). We must not reschedule a
> userspace process which is actively executing vector to a core that
> runs in a different vlen.

> It is not possible to execute the preemptible vector in a
> non-symmetric vector platform. This should be gated by failing the
> allocation of kernel_vstate.datap.

It sounds to me like the kernel does not currently do this?
However, I don't think we should even get as far as a failed allocation,
given we _know_ far before any in-kernel vector users appear whether or
not we have identical vlen across all CPUs.

> > > > has_vector() just checks if the extension is supported by all cores on
> > > > the cpu using either an alternative or the extension support bitmap.
> > > > There are lots of sites in the kernel that use has_vector() as the
> > > > gating check (as far as I can tell), so if vlen does not match between
> > > > CPUs should we not be returning an error for has_vector()?
> 
> No, has_vector() still returns true because it is backed by
> alternative and determined during boot.

In case you misunderstood, I know how has_vector() works - I was
suggesting a change to its behaviour.

> > > > The alternative that has_vector() relies on is patched before the non-boot
> > > > CPUs are enabled, so we cannot modify the result once the non-boot CPUs
> > > > are in their callin functions and detect a vlen mismatch while at at the
> > > > same time using it in smp_calling(), so should this code be changed to
> > > > something along the lines of:
> > > >
> > > >     if (riscv_has_extension_unlikely(v)) {
> > > >             if (riscv_v_setup_vsize()) {
> > > >                     elf_hwcap &= ~COMPAT_HWCAP_ISA_V;
> > > >                     riscv_v_vlen_mismatch = true;
> > > >             }
> > > >     }
> > > > and then has_vector() becomes something like
> > > > static __always_inline bool has_vector(void)
> > > > {
> > > >     return riscv_has_extension_unlikely(RISCV_ISA_EXT_v) && unlikely(riscv_v_vlen_mismatch);
> > > > }
> > > >
> > > > Probably there's value to be gained in static branches etc here, but I
> > > > was just trying to explain what I was getting at. Maybe I am missing
> > > > something though? I do remember talking about this back when the vector
> > > > patches were still in review.
> > >
> > > I actually don't think that alone fixes it: even if we make has_vector()
> > > respect the changes from the new CPU (whether we re-apply the alternatives
> > > or make a more dynamic check), we'd need to make sure the code that has
> > > already looked at has_vector() doesn't get scheduled on one of the
> > > shorter-VLENB CPUs.
> 
> I agree that flipping off the alternative for has_vector() could be
> dangerous at this point.
> 
> >
> > Is it possible to have that happen given this is in smp_callin()?
> > I guess the concern is some sort of hotplug situations, given that a CPU
> > could be hotplugged into a fully running system with a vlen mismatch.
> >
> > > So maybe the right answer here is to flip things around, and just refuse to
> > > enable CPUs that come in with a VLENB different than the boot CPU?  That way
> > > we could avoid being broken by systems that behave this way,
> >
> > smp_callin() doesn't return an error to report a failure, but it does
> > have a completion. I suppose we would just return early from
> > smp_callin() so that the completion never gets completed, in turn
> > causing __cpu_up() to fail?
> 
> I agree disabling the entire vector or fail bringing up the cpu is the
> safe way though.
> 
> >
> > > if someone ends
> > > up building one then they can figure out how to make it work better.
> >
> > Apparently the next gen sophgo product does. Not sure if people will wnt
> > to run across all the cores though, apparently they different by a lot
> > more than vlen.
> 
> The vector patchset assumes running on an SMP system. Supporting
> non-symmetric vlen or non-symmetric isa may require some efforts.

Supporting non symmetric extensions is actually not that hard, depending
on what the variance is - we already support enabling the intersection
of what all CPUs support. Supporting something like that for vlen
though, I agree.

> > A DT property would allow us to get the vlen info before the CPUs have
> > themselves been brought up. Apparently there are Zvl extensions for
> > this, but I'd rather just have a property like we have for the cbo
> > sizes if it comes to DT properties.
> 
> Yes, perhaps having a DT property is the first step to go. Because we
> can prevent race set/get-ing riscv_v_vsize in such case.

I think it is the second step. The first step I think is making
smp_callin() fail if vlen doesn't match, since that can be applied as a
quick fix to avoid issues on systems that have different vlens and would
also prevent an issue on a non-DT system.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv