Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap

Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap

[Björn Töpel]

Andy Chiu <andy.chiu@sifive.com> writes:

> Hey Björn,
>
> On Tue, Feb 14, 2023 at 2:43 PM Björn Töpel <bjorn@kernel.org> wrote:
>> So, two changes:
>>
>> 1. Disallow V-enablement if the existing altstack does not fit a V-sized
>>    frame.
> This could potentially break old programs (non-V) that load new system
> libraries (with V), If the program sets a small alt stack and takes
> the fault in some libraries that use V. However, existing
> implementation will also kill the process when the signal arrives,
> finding insufficient stack frame in such cases. I'd choose the second
> one if we only have these two options, because there is a chance that
> the signal handler may not even run.

I think we might have different views here. A process has a pre-V, a and
post-V state. Is allowing a process to enter V without the correct
preconditions a good idea? Allow to run with V turned on, but not able
to correctly handle a signal (the stack is too small)?

This was the same argument that the Intel folks had when enabling
AMX. Sure, AMX requires *explicit* enablement, but same rules should
apply, no?

>> 2. Sanitize altstack changes when V is enabled.
> Yes, I'd like to have this. But it may be tricky when it comes to
> deciding whether V is enabled, due to the first-use trap. If V is
> commonly used in system libraries then it is likely that V will be
> enabled before an user set an altstack. Sanitizing this case would be
> easy and straightforward. But what if the user sets an altstack before
> enabling V in the first-use trap? This could happen on a statically
> program that has hand-written V routines. This takes us to the 1st
> question above, should we fail the user program immediately if the
> altstack is set too small?

For me it's obvious to fail (always) "if the altstack is too small to
enable V", because it allows to execute V without proper preconditions.

Personally, I prefer a stricter model. Only enter V if you can, and
after entering it disallow changing the altstack.

Then again, this is *my* opinion and concern. What do other people
think? I don't want to stall the series.

>>
>> Other than the altstack handling, I think the series is a good state! It
>> would great if we could see a v14 land in -next...
> Thanks. I am reforming the v14 patch and hoping the same to happen soon too!

Thank you for your hard work! It would be awesome to *finally* have
vector support in the kernel!


Björn

Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap

[Vineet Gupta]

On 2/14/23 08:50, Björn Töpel wrote:
> Andy Chiu <andy.chiu@sifive.com> writes:
>
>> Hey Björn,
>>
>> On Tue, Feb 14, 2023 at 2:43 PM Björn Töpel <bjorn@kernel.org> wrote:
>>> So, two changes:
>>>
>>> 1. Disallow V-enablement if the existing altstack does not fit a V-sized
>>>     frame.
>> This could potentially break old programs (non-V) that load new system
>> libraries (with V), If the program sets a small alt stack and takes
>> the fault in some libraries that use V. However, existing
>> implementation will also kill the process when the signal arrives,
>> finding insufficient stack frame in such cases. I'd choose the second
>> one if we only have these two options, because there is a chance that
>> the signal handler may not even run.
> I think we might have different views here. A process has a pre-V, a and
> post-V state. Is allowing a process to enter V without the correct
> preconditions a good idea? Allow to run with V turned on, but not able
> to correctly handle a signal (the stack is too small)?

The requirement is sane, but the issue is user experience: User trying 
to bring up some V code has no clue that deep in some startup code some 
alt stack had been setup and causing his process to be terminated on 
first V code.

>
> This was the same argument that the Intel folks had when enabling
> AMX. Sure, AMX requires *explicit* enablement, but same rules should
> apply, no?
>
>>> 2. Sanitize altstack changes when V is enabled.
>> Yes, I'd like to have this. But it may be tricky when it comes to
>> deciding whether V is enabled, due to the first-use trap. If V is
>> commonly used in system libraries then it is likely that V will be
>> enabled before an user set an altstack. Sanitizing this case would be
>> easy and straightforward.

Good. Lets have this in v14 as it seems reasonably easy to implement.

>> But what if the user sets an altstack before
>> enabling V in the first-use trap? This could happen on a statically
>> program that has hand-written V routines. This takes us to the 1st
>> question above, should we fail the user program immediately if the
>> altstack is set too small?

Please lets not cross threads. We discussed this already at top. While 
ideally required, seems tricky so lets start with post-V alt stack check.

> For me it's obvious to fail (always) "if the altstack is too small to
> enable V", because it allows to execute V without proper preconditions.
>
> Personally, I prefer a stricter model. Only enter V if you can, and
> after entering it disallow changing the altstack.
>
> Then again, this is *my* opinion and concern. What do other people
> think? I don't want to stall the series.

I concur that the alt stack checking requirements are sensible in the 
long run. We can add the obvious check for post-V case and see if there 
is a sane way to flag pre-V case to.


>
>>> Other than the altstack handling, I think the series is a good state! It
>>> would great if we could see a v14 land in -next...
>> Thanks. I am reforming the v14 patch and hoping the same to happen soon too!
> Thank you for your hard work! It would be awesome to *finally* have
> vector support in the kernel!

Indeed we've come a long way, lets push the gear so we can use the 
coming cycle to flesh out any changes for a possible 6.4 inclusion.

Thx,
-Vineet

Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap

[Björn Töpel]

Vineet Gupta <vineetg@rivosinc.com> writes:

> On 2/14/23 08:50, Björn Töpel wrote:
>> Andy Chiu <andy.chiu@sifive.com> writes:
>>
>>> Hey Björn,
>>>
>>> On Tue, Feb 14, 2023 at 2:43 PM Björn Töpel <bjorn@kernel.org> wrote:
>>>> So, two changes:
>>>>
>>>> 1. Disallow V-enablement if the existing altstack does not fit a V-sized
>>>>     frame.
>>> This could potentially break old programs (non-V) that load new system
>>> libraries (with V), If the program sets a small alt stack and takes
>>> the fault in some libraries that use V. However, existing
>>> implementation will also kill the process when the signal arrives,
>>> finding insufficient stack frame in such cases. I'd choose the second
>>> one if we only have these two options, because there is a chance that
>>> the signal handler may not even run.
>> I think we might have different views here. A process has a pre-V, a and
>> post-V state. Is allowing a process to enter V without the correct
>> preconditions a good idea? Allow to run with V turned on, but not able
>> to correctly handle a signal (the stack is too small)?
>
> The requirement is sane, but the issue is user experience: User trying 
> to bring up some V code has no clue that deep in some startup code some 
> alt stack had been setup and causing his process to be terminated on 
> first V code.
>
>>
>> This was the same argument that the Intel folks had when enabling
>> AMX. Sure, AMX requires *explicit* enablement, but same rules should
>> apply, no?
>>
>>>> 2. Sanitize altstack changes when V is enabled.
>>> Yes, I'd like to have this. But it may be tricky when it comes to
>>> deciding whether V is enabled, due to the first-use trap. If V is
>>> commonly used in system libraries then it is likely that V will be
>>> enabled before an user set an altstack. Sanitizing this case would be
>>> easy and straightforward.
>
> Good. Lets have this in v14 as it seems reasonably easy to implement.
>
>>> But what if the user sets an altstack before
>>> enabling V in the first-use trap? This could happen on a statically
>>> program that has hand-written V routines. This takes us to the 1st
>>> question above, should we fail the user program immediately if the
>>> altstack is set too small?
>
> Please lets not cross threads. We discussed this already at top. While 
> ideally required, seems tricky so lets start with post-V alt stack check.
>
>> For me it's obvious to fail (always) "if the altstack is too small to
>> enable V", because it allows to execute V without proper preconditions.
>>
>> Personally, I prefer a stricter model. Only enter V if you can, and
>> after entering it disallow changing the altstack.
>>
>> Then again, this is *my* opinion and concern. What do other people
>> think? I don't want to stall the series.
>
> I concur that the alt stack checking requirements are sensible in the 
> long run. We can add the obvious check for post-V case and see if there 
> is a sane way to flag pre-V case to.

Reasonable. @Andy does this resonate with you as well?


Björn

Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap

[Andy Chiu]

On Wed, Feb 15, 2023 at 3:14 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> Vineet Gupta <vineetg@rivosinc.com> writes:
>
> > On 2/14/23 08:50, Björn Töpel wrote:
> >> Andy Chiu <andy.chiu@sifive.com> writes:
> >>
> >>> Hey Björn,
> >>>
> >>> On Tue, Feb 14, 2023 at 2:43 PM Björn Töpel <bjorn@kernel.org> wrote:
> >>>> So, two changes:
> >>>>
> >>>> 1. Disallow V-enablement if the existing altstack does not fit a V-sized
> >>>>     frame.
> >>> This could potentially break old programs (non-V) that load new system
> >>> libraries (with V), If the program sets a small alt stack and takes
> >>> the fault in some libraries that use V. However, existing
> >>> implementation will also kill the process when the signal arrives,
> >>> finding insufficient stack frame in such cases. I'd choose the second
> >>> one if we only have these two options, because there is a chance that
> >>> the signal handler may not even run.
> >> I think we might have different views here. A process has a pre-V, a and
> >> post-V state. Is allowing a process to enter V without the correct
> >> preconditions a good idea? Allow to run with V turned on, but not able
> >> to correctly handle a signal (the stack is too small)?
> >
> > The requirement is sane, but the issue is user experience: User trying
> > to bring up some V code has no clue that deep in some startup code some
> > alt stack had been setup and causing his process to be terminated on
> > first V code.
> >
> >>
> >> This was the same argument that the Intel folks had when enabling
> >> AMX. Sure, AMX requires *explicit* enablement, but same rules should
> >> apply, no?
> >>
> >>>> 2. Sanitize altstack changes when V is enabled.
> >>> Yes, I'd like to have this. But it may be tricky when it comes to
> >>> deciding whether V is enabled, due to the first-use trap. If V is
> >>> commonly used in system libraries then it is likely that V will be
> >>> enabled before an user set an altstack. Sanitizing this case would be
> >>> easy and straightforward.
> >
> > Good. Lets have this in v14 as it seems reasonably easy to implement.
> >
> >>> But what if the user sets an altstack before
> >>> enabling V in the first-use trap? This could happen on a statically
> >>> program that has hand-written V routines. This takes us to the 1st
> >>> question above, should we fail the user program immediately if the
> >>> altstack is set too small?
> >
> > Please lets not cross threads. We discussed this already at top. While
> > ideally required, seems tricky so lets start with post-V alt stack check.
> >
> >> For me it's obvious to fail (always) "if the altstack is too small to
> >> enable V", because it allows to execute V without proper preconditions.
> >>
> >> Personally, I prefer a stricter model. Only enter V if you can, and
> >> after entering it disallow changing the altstack.
> >>
> >> Then again, this is *my* opinion and concern. What do other people
> >> think? I don't want to stall the series.
> >
> > I concur that the alt stack checking requirements are sensible in the
> > long run. We can add the obvious check for post-V case and see if there
> > is a sane way to flag pre-V case to.
>
> Reasonable. @Andy does this resonate with you as well?
Yes, it makes sense to me. I am making this happen on v14 :)

Thanks,
Andy