diff mbox series

[RFC,22/31] test: lmb: run the LMB tests only on sandbox

Message ID 20240607185240.1892031-23-sughosh.ganu@linaro.org
State New
Headers show
Series Make U-Boot memory reservations coherent | expand

Commit Message

Sughosh Ganu June 7, 2024, 6:52 p.m. UTC
The LMB memory map is now persistent and global. Running the tests for
the LMB module will result in the memory map getting reset, and this
will have side-effects on the rest of the working of the platform. Run
the LMB tests only on the sandbox platform, which is meant for running
such kinds of tests.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 test/lib/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tom Rini June 10, 2024, 5:44 p.m. UTC | #1
On Sat, Jun 08, 2024 at 12:22:31AM +0530, Sughosh Ganu wrote:

> The LMB memory map is now persistent and global. Running the tests for
> the LMB module will result in the memory map getting reset, and this
> will have side-effects on the rest of the working of the platform. Run
> the LMB tests only on the sandbox platform, which is meant for running
> such kinds of tests.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

I'm not sure about this. We can reset real hardware as often as we need,
too. Did you run in to problems with this test on non-sandbox?
Sughosh Ganu June 11, 2024, 8:55 a.m. UTC | #2
On Mon, 10 Jun 2024 at 23:14, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Jun 08, 2024 at 12:22:31AM +0530, Sughosh Ganu wrote:
>
> > The LMB memory map is now persistent and global. Running the tests for
> > the LMB module will result in the memory map getting reset, and this
> > will have side-effects on the rest of the working of the platform. Run
> > the LMB tests only on the sandbox platform, which is meant for running
> > such kinds of tests.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> I'm not sure about this. We can reset real hardware as often as we need,
> too. Did you run in to problems with this test on non-sandbox?

But do we want to reset the state of the LMB memory map on real
hardware? This was working up until now because of the local nature of
the LMB variables. But if the LMB memory map is to be persistent,
should we allow it to be changed for running some test? That would
have side effects? I think running these tests on sandbox should
suffice. I mean there isn't any aspect of the LMB module that is not
getting tested on sandbox, right?

-sughosh
Heinrich Schuchardt June 11, 2024, 9:56 a.m. UTC | #3
On 11.06.24 10:55, Sughosh Ganu wrote:
> On Mon, 10 Jun 2024 at 23:14, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Sat, Jun 08, 2024 at 12:22:31AM +0530, Sughosh Ganu wrote:
>>
>>> The LMB memory map is now persistent and global. Running the tests for
>>> the LMB module will result in the memory map getting reset, and this
>>> will have side-effects on the rest of the working of the platform. Run
>>> the LMB tests only on the sandbox platform, which is meant for running
>>> such kinds of tests.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>
>> I'm not sure about this. We can reset real hardware as often as we need,
>> too. Did you run in to problems with this test on non-sandbox?
>
> But do we want to reset the state of the LMB memory map on real
> hardware? This was working up until now because of the local nature of
> the LMB variables. But if the LMB memory map is to be persistent,
> should we allow it to be changed for running some test? That would
> have side effects? I think running these tests on sandbox should
> suffice. I mean there isn't any aspect of the LMB module that is not
> getting tested on sandbox, right?
>
> -sughosh

We should run tests on systems with different bitness and endianness.

As the LMB test does not rely on any sandbox driver we should be able to
run it on any system.

If the memory map is not usable anymore after the test, the system
should be rebooted.

Best regards

Heinrich
Sughosh Ganu June 11, 2024, 10:09 a.m. UTC | #4
On Tue, 11 Jun 2024 at 15:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11.06.24 10:55, Sughosh Ganu wrote:
> > On Mon, 10 Jun 2024 at 23:14, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Sat, Jun 08, 2024 at 12:22:31AM +0530, Sughosh Ganu wrote:
> >>
> >>> The LMB memory map is now persistent and global. Running the tests for
> >>> the LMB module will result in the memory map getting reset, and this
> >>> will have side-effects on the rest of the working of the platform. Run
> >>> the LMB tests only on the sandbox platform, which is meant for running
> >>> such kinds of tests.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>
> >> I'm not sure about this. We can reset real hardware as often as we need,
> >> too. Did you run in to problems with this test on non-sandbox?
> >
> > But do we want to reset the state of the LMB memory map on real
> > hardware? This was working up until now because of the local nature of
> > the LMB variables. But if the LMB memory map is to be persistent,
> > should we allow it to be changed for running some test? That would
> > have side effects? I think running these tests on sandbox should
> > suffice. I mean there isn't any aspect of the LMB module that is not
> > getting tested on sandbox, right?
> >
> > -sughosh
>
> We should run tests on systems with different bitness and endianness.
>

I would think there are platforms in the CI suite that are big-endian,
because if there are, they are surely exercising the LMB memory
reservation functions multiple times. I am only talking about running
the unit tests on sandbox.

> As the LMB test does not rely on any sandbox driver we should be able to
> run it on any system.
>
> If the memory map is not usable anymore after the test, the system
> should be rebooted.

Yes, that will happen when the test is run in CI -- I am working on
changes to have the LMB tests run separately and then reboot the
system. But if the tests are run by a user manually, that might have
side-effects. Btw, even today, the LMB tests only get run on sandbox
as part of the CI run. What happens is that the tests are enabled to
be run on other platforms as well(snow as of now).

But OTOH, an explicit reboot can be called after having run the tests.
This would handle the scenario of a user running these manually from
command-line.

-sughosh

>
> Best regards
>
> Heinrich
Tom Rini June 11, 2024, 2:05 p.m. UTC | #5
On Tue, Jun 11, 2024 at 02:25:05PM +0530, Sughosh Ganu wrote:
> On Mon, 10 Jun 2024 at 23:14, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Jun 08, 2024 at 12:22:31AM +0530, Sughosh Ganu wrote:
> >
> > > The LMB memory map is now persistent and global. Running the tests for
> > > the LMB module will result in the memory map getting reset, and this
> > > will have side-effects on the rest of the working of the platform. Run
> > > the LMB tests only on the sandbox platform, which is meant for running
> > > such kinds of tests.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >
> > I'm not sure about this. We can reset real hardware as often as we need,
> > too. Did you run in to problems with this test on non-sandbox?
> 
> But do we want to reset the state of the LMB memory map on real
> hardware? This was working up until now because of the local nature of
> the LMB variables. But if the LMB memory map is to be persistent,
> should we allow it to be changed for running some test? That would
> have side effects? I think running these tests on sandbox should
> suffice. I mean there isn't any aspect of the LMB module that is not
> getting tested on sandbox, right?

When running our test/ test code on hardware side effects are fine, this
isn't a POST type test that gets used in production. Do changes, reset
platform as needed just like sandbox, etc. And yes, a user running tests
that have side effects manually and then not dealing with them is user
error.
Ilias Apalodimas June 11, 2024, 2:06 p.m. UTC | #6
On Tue, 11 Jun 2024 at 17:05, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jun 11, 2024 at 02:25:05PM +0530, Sughosh Ganu wrote:
> > On Mon, 10 Jun 2024 at 23:14, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Jun 08, 2024 at 12:22:31AM +0530, Sughosh Ganu wrote:
> > >
> > > > The LMB memory map is now persistent and global. Running the tests for
> > > > the LMB module will result in the memory map getting reset, and this
> > > > will have side-effects on the rest of the working of the platform. Run
> > > > the LMB tests only on the sandbox platform, which is meant for running
> > > > such kinds of tests.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > >
> > > I'm not sure about this. We can reset real hardware as often as we need,
> > > too. Did you run in to problems with this test on non-sandbox?
> >
> > But do we want to reset the state of the LMB memory map on real
> > hardware? This was working up until now because of the local nature of
> > the LMB variables. But if the LMB memory map is to be persistent,
> > should we allow it to be changed for running some test? That would
> > have side effects? I think running these tests on sandbox should
> > suffice. I mean there isn't any aspect of the LMB module that is not
> > getting tested on sandbox, right?
>
> When running our test/ test code on hardware side effects are fine, this
> isn't a POST type test that gets used in production. Do changes, reset
> platform as needed just like sandbox, etc. And yes, a user running tests
> that have side effects manually and then not dealing with them is user
> error.
>

So the lmb changes are scary to begin with. I think the tests
shouldn't be limited to sandbox.
Can't we plan c tests in test/dm etc that call functions and test?
IOW we could allocate EFI memory and try to overwrite it with LMB,
vice versa etc, without having to deal with the predefined areas.

Regards
/Ilias
> --
> Tom
diff mbox series

Patch

diff --git a/test/lib/Makefile b/test/lib/Makefile
index e75a263e6a..9154e07993 100644
--- a/test/lib/Makefile
+++ b/test/lib/Makefile
@@ -9,7 +9,7 @@  obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
 obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
 obj-y += hexdump.o
 obj-$(CONFIG_SANDBOX) += kconfig.o
-obj-y += lmb.o
+obj-$(CONFIG_SANDBOX) += lmb.o
 obj-y += longjmp.o
 obj-$(CONFIG_CONSOLE_RECORD) += test_print.o
 obj-$(CONFIG_SSCANF) += sscanf.o