date:20211103

[gem5-dev] Change in gem5/gem5[develop]: cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with operator delete.

2021-11-03 Thread Gabe Black (Gerrit) via gem5-dev

Gabe Black has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/52383 )


Change subject: cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with  
operator delete.

..

cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with operator delete.

When allocating memory with operator new(size_t), we should also delete
that memory with operator delete(). Note that this is a generic form of
new and delete which do not construct an object in the allocated space,
or delete the object when freeing the space.

There were a number of places where we were over-allocating a structure
so that there would be room after it for other data, at least sometimes
to allocate C structures which would have a trailing array of some other
structure with an undefined size. Those structures were then being
stored in a std::unique_ptr with the default deleter, which just calls
full blown delete and not operator delete.

It seems that this is often ok, and I was not able to find anything that
spelled out in bold letters that it isn't. I did find this sentence:

"If the pointer passed to the standard library deallocation function was
not obtained from the corresponding standard library allocation function,
the behavior is undefined."

On this webpage:

https://en.cppreference.com/w/cpp/memory/new/operator_delete

This is a *little* vague, since they might mean you can't mix malloc and
delete, or new and free. Strictly interpretting it though, it could mean
you can't mix operator new with regular delete, or any other mismatched
combination.

I also found that exactly how this causes problems depends on what heap
allocator you're using. When I used tcmalloc, gem5 would segfault within
that library. When I disabled tcmalloc to run valgrind, the segfault
went away. I think this may be because sometimes you get lucky and
undefined behavior is what you actually wanted, and sometimes you don't.

To fix this problem, this change overrides the deleter on all of these
unique_ptr-s so that they use operator delete. Also, it refactors some
code in arch/x86/kvm/x86_cpu.cc so that the function that allocates
memory with operator new returns a std::unique_ptr instead of a raw
pointer. This raw pointer was always immediately put into a unique_ptr
anyway, and, in addition to tidying up the call sights slightly, also
avoids having to define a custom deleter in each of those locations
instead of once in the allocation function.

Change-Id: I9ebff430996cf603051f5baa8708424819ed8465
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/52383
Reviewed-by: Andreas Sandberg 
Reviewed-by: Jason Lowe-Power 
Reviewed-by: Bobby R. Bruce 
Maintainer: Gabe Black 
Maintainer: Bobby R. Bruce 
Tested-by: kokoro 
---
M src/arch/x86/kvm/x86_cpu.cc
M src/dev/virtio/fs9p.hh
M src/cpu/kvm/base.cc
M src/arch/arm/kvm/base_cpu.cc
M src/arch/arm/kvm/arm_cpu.cc
M src/cpu/kvm/vm.cc
6 files changed, 88 insertions(+), 23 deletions(-)

Approvals:
  Jason Lowe-Power: Looks good to me, but someone else must approve
  Andreas Sandberg: Looks good to me, approved
  Bobby R. Bruce: Looks good to me, approved; Looks good to me, approved
  Gabe Black: Looks good to me, approved
  kokoro: Regressions pass




diff --git a/src/arch/arm/kvm/arm_cpu.cc b/src/arch/arm/kvm/arm_cpu.cc
index 28fa036..4f47a15 100644
--- a/src/arch/arm/kvm/arm_cpu.cc
+++ b/src/arch/arm/kvm/arm_cpu.cc
@@ -414,7 +414,8 @@
 ArmKvmCPU::getRegList() const
 {
 if (_regIndexList.size() == 0) {
-std::unique_ptr regs;
+std::unique_ptr
+regs(nullptr, [](void *p) { operator delete(p); });
 uint64_t i = 1;

 do {
diff --git a/src/arch/arm/kvm/base_cpu.cc b/src/arch/arm/kvm/base_cpu.cc
index 524b410..fe922a1 100644
--- a/src/arch/arm/kvm/base_cpu.cc
+++ b/src/arch/arm/kvm/base_cpu.cc
@@ -201,7 +201,8 @@

 // Request the actual register list now that we know how many
 // register we need to allocate space for.
-std::unique_ptr regs;
+std::unique_ptr
+regs(nullptr, [](void *p) { operator delete(p); });
 const size_t size(sizeof(kvm_reg_list) +
   regs_probe.n * sizeof(uint64_t));
 regs.reset((kvm_reg_list *)operator new(size));
diff --git a/src/arch/x86/kvm/x86_cpu.cc b/src/arch/x86/kvm/x86_cpu.cc
index 67cecd4..11c017c 100644
--- a/src/arch/x86/kvm/x86_cpu.cc
+++ b/src/arch/x86/kvm/x86_cpu.cc
@@ -167,10 +167,14 @@
 APPLY_DTABLE(idt, MISCREG_IDTR - MISCREG_SEG_SEL_BASE); \
 } while (0)

-template
-static STRUCT *newVarStruct(size_t entries)
+template
+static auto
+newVarStruct(size_t entries)
 {
-return (STRUCT *)operator new(sizeof(STRUCT) + entries *  
sizeof(ENTRY));

+size_t size = sizeof(Struct) + entries * sizeof(Entry);
+return std::unique_ptr(
+(Struct *)operator new(size),
+[](Struct *p) { operator delete(p); });
 }

 static void
@@ -664,9

[gem5-dev] Change in gem5/gem5[develop]: tests: Removing Atomic CPU with Ruby tests

2021-11-03 Thread Bobby R. Bruce (Gerrit) via gem5-dev

Bobby R. Bruce has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/52384 )


Change subject: tests: Removing Atomic CPU with Ruby tests
..

tests: Removing Atomic CPU with Ruby tests

test_kvm_fork_run.py and test_kvm_cpu_switch.py both contained tests
which attempted to run a Ruby cache with an Atomic CPU. This is not
permitted. As such these tests have been removed.

Change-Id: I7996bda6313f59f76d7f9b73bef8351d72547481
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/52384
Reviewed-by: Jason Lowe-Power 
Maintainer: Jason Lowe-Power 
Tested-by: kokoro 
---
M tests/gem5/kvm-fork-tests/test_kvm_fork_run.py
M tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py
2 files changed, 17 insertions(+), 12 deletions(-)

Approvals:
  Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
  kokoro: Regressions pass




diff --git a/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py  
b/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py

index d12f104..c62211e 100644
--- a/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py
+++ b/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py
@@ -118,12 +118,6 @@
 ### The long (nightly) tests 

 test_kvm_fork_run(
-cpu="atomic",
-num_cpus=1,
-mem_system="mesi_two_level",
-length=constants.long_tag,
-)
-test_kvm_fork_run(
 cpu="timing",
 num_cpus=1,
 mem_system="mesi_two_level",
diff --git a/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py  
b/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py

index 19e83e5..a8cbce1 100644
--- a/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py
+++ b/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py
@@ -116,12 +116,6 @@
 ### The long (nightly) tests 

 test_kvm_switch(
-cpu="atomic",
-num_cpus=1,
-mem_system="mesi_two_level",
-length=constants.long_tag,
-)
-test_kvm_switch(
 cpu="timing",
 num_cpus=1,
 mem_system="mesi_two_level",

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/52384
To unsubscribe, or for help writing mail filters, visit  
https://gem5-review.googlesource.com/settings


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I7996bda6313f59f76d7f9b73bef8351d72547481
Gerrit-Change-Number: 52384
Gerrit-PatchSet: 2
Gerrit-Owner: Bobby R. Bruce 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] Change in gem5/gem5[develop]: python: Remove incorrect usage of typing 'Optional'

2021-11-03 Thread Bobby R. Bruce (Gerrit) via gem5-dev

Bobby R. Bruce has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/52143 )


Change subject: python: Remove incorrect usage of typing 'Optional'
..

python: Remove incorrect usage of typing 'Optional'

There has been some confusion about usage of 'Optional'. In some areas
of the codebase it was assumed this specifies an optional parameter
(i.e., one which may or may not set, as it has a default value). This is
incorrect. 'Optional[]' is shorthand for 'Union[, None]',
i.e., it is used to state the value may be 'None'. This patch corrects
this throughout the gem5 codebase.

Change-Id: I77a6708dee448e8480870d073e128aed3d6ae904
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/52143
Reviewed-by: Jason Lowe-Power 
Maintainer: Jason Lowe-Power 
Tested-by: kokoro 
---
M tests/gem5/x86-boot-tests/test_linux_boot.py
M src/python/gem5/utils/requires.py
M src/python/gem5/resources/downloader.py
M src/python/gem5/components/cachehierarchies/classic/no_cache.py
M src/python/gem5/resources/resource.py
M src/python/gem5/components/cachehierarchies/classic/caches/l1dcache.py
M src/python/gem5/components/cachehierarchies/classic/caches/l1icache.py
M  
src/python/gem5/components/cachehierarchies/classic/private_l1_private_l2_cache_hierarchy.py

M src/python/gem5/components/cachehierarchies/classic/caches/mmu_cache.py
M src/python/gem5/components/boards/x86_board.py
M src/python/gem5/components/cachehierarchies/classic/caches/l2cache.py
M  
src/python/gem5/components/cachehierarchies/classic/private_l1_cache_hierarchy.py

12 files changed, 63 insertions(+), 54 deletions(-)

Approvals:
  Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
  kokoro: Regressions pass




diff --git a/src/python/gem5/components/boards/x86_board.py  
b/src/python/gem5/components/boards/x86_board.py

index 49bf789..df7e7fb 100644
--- a/src/python/gem5/components/boards/x86_board.py
+++ b/src/python/gem5/components/boards/x86_board.py
@@ -271,7 +271,7 @@
 kernel: AbstractResource,
 disk_image: AbstractResource,
 command: Optional[str] = None,
-kernel_args: Optional[List[str]] = [],
+kernel_args: List[str] = [],
 ):
 """Setup the full system files

diff --git  
a/src/python/gem5/components/cachehierarchies/classic/caches/l1dcache.py  
b/src/python/gem5/components/cachehierarchies/classic/caches/l1dcache.py

index 7346e7a..c80032b 100644
--- a/src/python/gem5/components/cachehierarchies/classic/caches/l1dcache.py
+++ b/src/python/gem5/components/cachehierarchies/classic/caches/l1dcache.py
@@ -28,7 +28,7 @@

 from m5.objects import Cache, BasePrefetcher, StridePrefetcher

-from typing import Optional, Type
+from typing import Type


 class L1DCache(Cache):
@@ -39,13 +39,13 @@
 def __init__(
 self,
 size: str,
-assoc: Optional[int] = 8,
-tag_latency: Optional[int] = 1,
-data_latency: Optional[int] = 1,
-response_latency: Optional[int] = 1,
-mshrs: Optional[int] = 16,
-tgts_per_mshr: Optional[int] = 20,
-writeback_clean: Optional[bool] = True,
+assoc: int = 8,
+tag_latency: int = 1,
+data_latency: int = 1,
+response_latency: int = 1,
+mshrs: int = 16,
+tgts_per_mshr: int = 20,
+writeback_clean: bool = True,
 PrefetcherCls: Type[BasePrefetcher] = StridePrefetcher,
 ):
 super(L1DCache, self).__init__()
diff --git  
a/src/python/gem5/components/cachehierarchies/classic/caches/l1icache.py  
b/src/python/gem5/components/cachehierarchies/classic/caches/l1icache.py

index d1bf5aa..8e4ba09 100644
--- a/src/python/gem5/components/cachehierarchies/classic/caches/l1icache.py
+++ b/src/python/gem5/components/cachehierarchies/classic/caches/l1icache.py
@@ -24,7 +24,7 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

-from typing import Optional, Type
+from typing import Type

 from m5.objects import Cache, BasePrefetcher, StridePrefetcher

@@ -39,13 +39,13 @@
 def __init__(
 self,
 size: str,
-assoc: Optional[int] = 8,
-tag_latency: Optional[int] = 1,
-data_latency: Optional[int] = 1,
-response_latency: Optional[int] = 1,
-mshrs: Optional[int] = 16,
-tgts_per_mshr: Optional[int] = 20,
-writeback_clean: Optional[bool] = True,
+assoc: int = 8,
+tag_latency: int = 1,
+data_latency: int = 1,
+response_latency: int = 1,
+mshrs: int = 16,
+tgts_per_mshr: int = 20,
+writeback_clean: bool = True,
 PrefetcherCls: Type[BasePrefetcher] = StridePrefetcher,
 ):
 super(L1ICache, self).__init__()
diff --git  
a/src/python/gem5/components/cachehierarchies/classic/caches/l2cache.py

[gem5-dev] Re: Build failed in Jenkins: nightly #27

2021-11-03 Thread Jason Lowe-Power via gem5-dev

Here's my data:

BAD * 4fe56ff72 - (3 months ago) arch-arm,cpu: Replace rename modes with
split reg/elem register files. - Gabe Black
GOOD * 25138cbb7 - (4 weeks ago) arch: Simplify and tidy up PCState
classes. - Gabe Black
* 930986332 - (7 days ago) mem: Fix whitespace in
mem/ruby/system/Sequencer.py. - Gabe Black
GOOD * 69e6ea485 - (3 months ago) arch-arm: Add walkBits method to
PageTableOps - Giacomo Travaglini
* 1268c6ec3 - (2 weeks ago) arch-arm: Expose LookupLevel enum to the python
world - Giacomo Travaglini

I've tested this a number of times and tested commits before and after
these commits. I have increasing confidence (though no certainty) that
4fe56ff72 is the culprit.

I'm running a UBSAN now to see if that will help at all.

Cheers,
Jason

On Tue, Nov 2, 2021 at 5:28 PM Gabe Black  wrote:

> I'm going to kill it now, and restart it with --track-origins=yes and in a
> separate tree so I can keep working on other things while it runs.
>
> Gabe
>
> On Tue, Nov 2, 2021 at 5:11 PM Gabe Black  wrote:
>
>> It's still running, but here's what it's found so far.
>>
>> Gabe
>>
>> On Tue, Nov 2, 2021 at 7:48 AM Jason Lowe-Power 
>> wrote:
>>
>>> Thanks! I tried a bisect but, tbh, it wasn't helpful since the error
>>> doesn't seem to be deterministic.
>>>
>>> As more evidence that it's a memory issue, the backtrace that I saw with
>>> GDB was something a bit different.
>>>
>>> Cheers,
>>> Jason
>>>
>>> On Tue, Nov 2, 2021 at 5:07 AM Gabe Black  wrote:
>>>
 I'm running it under valgrind to see if that tells me anything, which
 is going to take a while. I'll let you know if/when it finishes.

 Gabe

 On Tue, Nov 2, 2021 at 4:36 AM Gabe Black  wrote:

> Attached is a log of a failing run, and backtrace of the segfault.
>
> Gabe
>
> On Tue, Nov 2, 2021 at 4:17 AM Gabe Black 
> wrote:
>
>> Ok, I reproduced the segfault once, but then running again in gdb it
>> exited normally. I'm pretty confident it's something to do with things
>> getting cleaned up and/or destructed at the end of the simulation, but
>> until I catch something in the act of exploding I won't be able to nail
>> down specifically what's doing it.
>>
>> Gabe
>>
>> On Tue, Nov 2, 2021 at 3:46 AM Gabe Black 
>> wrote:
>>
>>> A clean build seems to have fixed the IdeDisk problem.
>>>
>>> On Tue, Nov 2, 2021 at 3:35 AM Gabe Black 
>>> wrote:
>>>
 Can you get a backtrace from it? Or run it in valgrind? I'm trying
 to use the command line you provided locally, but it's complaining 
 about
 not being able to find IdeDisk which is very strange... I don't think I
 have an account on the machine where this ran, but ideally I'll be 
 able to
 reproduce it locally where it will be easier to work with.

 Gabe

 On Mon, Nov 1, 2021 at 3:03 PM Jason Lowe-Power <
 ja...@lowepower.com> wrote:

> After spending some time on this, there is definitely a segfault
> at the end of execution. It's odd that the testing scripts sometimes
> reports that it works.
>
> If you run the following, you should see a segfault at the end and
> no stats are generated:
> ../gem5/> build/ARM/gem5.opt tests/gem5/fs/linux/arm/run.py
> tests/gem5/configs/realview-o3.py tests/gem5/resources/arm .
>
> I also notice that when it doesn't fail the following is printed:
> "build/ARM/arch/arm/isa.hh:650: warn: User mode does not have SPSR
> build/ARM/arch/arm/isa.hh:650: warn: User mode does not have SPSR"
>
> But it's not printed when it fails.
>
> Cheers,
> Jason
>
> On Mon, Nov 1, 2021 at 8:29 AM Jason Lowe-Power <
> ja...@lowepower.com> wrote:
>
>> I don't think so. The binaries haven't been updated since April
>> ('aarch-system-20210904.tar.bz2'). Well, the blame says April even 
>> if the
>> filename is confusing (DDMM?).
>>
>> Here's the failure:
>> https://jenkins.gem5.org/job/nightly/ws/tests/testing-results/SuiteUID%3Arealview-o3-ARM-x86_64-opt/TestUID%3Arealview-o3-ARM-x86_64-opt/simerr
>>
>> build/ARM/arch/arm/insts/pseudo.cc:173: warn: instruction 'csdb'
>> unimplemented
>> build/ARM/arch/arm/generated/decoder-ns.cc.inc:100643: warn:
>> instruction 'mcr bpiall' unimplemented
>> gem5 has encountered a segmentation fault!
>>
>> Cheers,
>> Jason
>>
>> On Sat, Oct 30, 2021 at 4:11 AM Gabe Black via gem5-dev <
>> gem5-dev@gem5.org> wrote:
>>
>>> Maybe you need to re-download the resources?
>>>
>>> Gabe
>>>
>>> On Sat, Oct 30, 2021 at 3:50 AM jenkins-no-reply--- via gem5-dev
>>>  wrote:

[gem5-dev] Re: failing kvm tests

2021-11-03 Thread Bobby Bruce via gem5-dev

Hey Gabe,

At present our Jenkins doesn't have KVM enabled so I believe no tests
that use KVM were being run regularly. I intend to get KVM enabled on the
Jenkins server over the next few days. I also found the bugs you were
referring to when running the long (nightly) and very-long (weekly) tests
on my local machine (where I have KVM). The long tests are fixed with this
patch: https://gem5-review.googlesource.com/c/public/gem5/+/52384, and I'm
currently looking into the bugs in the very-long tests.

Kind regards,
Bobby
--
Dr. Bobby R. Bruce
Room 3050,
Kemper Hall, UC Davis
Davis,
CA, 95616

web: https://www.bobbybruce.net

On Wed, Nov 3, 2021 at 4:22 AM Gabe Black via gem5-dev 
wrote:

> Hey folks, I recently discovered KVM wasn't set up on my desktop, which I
> just corrected. Now that it's enabled, the KVM tests have started running,
> and they are also failing. This could be sort of weird issue on my machine,
> and I'm currently looking into the failure itself.
>
> The thing I wanted to ask was, do we have any KVM tests that run anywhere?
> Do we have them in the quick regressions but not the long regressions? Do
> we have KVM enabled on the nightly server? I can imagine it (KVM) not being
> enabled on kokoro, but hopefully we can run those on the nightlies?
>
> Gabe
> ___
> gem5-dev mailing list -- gem5-dev@gem5.org
> To unsubscribe send an email to gem5-dev-le...@gem5.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] Change in gem5/gem5[develop]: tests: Removing Atomic CPU with Ruby tests

2021-11-03 Thread Bobby R. Bruce (Gerrit) via gem5-dev

Bobby R. Bruce has uploaded this change for review. (  
https://gem5-review.googlesource.com/c/public/gem5/+/52384 )



Change subject: tests: Removing Atomic CPU with Ruby tests
..

tests: Removing Atomic CPU with Ruby tests

test_kvm_fork_run.py and test_kvm_cpu_switch.py both contained tests
which attempted to run a Ruby cache with an Atomic CPU. This is not
permitted. As such these tests have been removed.

Change-Id: I7996bda6313f59f76d7f9b73bef8351d72547481
---
M tests/gem5/kvm-fork-tests/test_kvm_fork_run.py
M tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py
2 files changed, 13 insertions(+), 12 deletions(-)



diff --git a/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py  
b/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py

index d12f104..c62211e 100644
--- a/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py
+++ b/tests/gem5/kvm-fork-tests/test_kvm_fork_run.py
@@ -118,12 +118,6 @@
 ### The long (nightly) tests 

 test_kvm_fork_run(
-cpu="atomic",
-num_cpus=1,
-mem_system="mesi_two_level",
-length=constants.long_tag,
-)
-test_kvm_fork_run(
 cpu="timing",
 num_cpus=1,
 mem_system="mesi_two_level",
diff --git a/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py  
b/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py

index 19e83e5..a8cbce1 100644
--- a/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py
+++ b/tests/gem5/kvm-switch-tests/test_kvm_cpu_switch.py
@@ -116,12 +116,6 @@
 ### The long (nightly) tests 

 test_kvm_switch(
-cpu="atomic",
-num_cpus=1,
-mem_system="mesi_two_level",
-length=constants.long_tag,
-)
-test_kvm_switch(
 cpu="timing",
 num_cpus=1,
 mem_system="mesi_two_level",

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/52384
To unsubscribe, or for help writing mail filters, visit  
https://gem5-review.googlesource.com/settings


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I7996bda6313f59f76d7f9b73bef8351d72547481
Gerrit-Change-Number: 52384
Gerrit-PatchSet: 1
Gerrit-Owner: Bobby R. Bruce 
Gerrit-MessageType: newchange
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] Change in gem5/gem5[develop]: arch: Fix serialization/deserialization of Vector registers

2021-11-03 Thread Giacomo Travaglini (Gerrit) via gem5-dev

Giacomo Travaglini has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/51947 )


Change subject: arch: Fix serialization/deserialization of Vector registers
..

arch: Fix serialization/deserialization of Vector registers

This bug has been introduced by [1].
Without this fix a vector register is only partially unserialized,  
effectively

breaking checkpoiting for vectored applications. For example if I am
initializing a vector register with the following checkpointed value:

0x___

The ParseParam logic will produce instead

0x___

[1]: https://gem5-review.googlesource.com/c/public/gem5/+/41994

Change-Id: I5010d9f39d57fcee390e7419a64dbcd293e51fa0
Signed-off-by: Giacomo Travaglini 
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/51947
Maintainer: Bobby R. Bruce 
Maintainer: Jason Lowe-Power 
Tested-by: kokoro 
Reviewed-by: Jason Lowe-Power 
---
M src/arch/generic/vec_reg.hh
1 file changed, 29 insertions(+), 1 deletion(-)

Approvals:
  Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
  Bobby R. Bruce: Looks good to me, approved
  kokoro: Regressions pass




diff --git a/src/arch/generic/vec_reg.hh b/src/arch/generic/vec_reg.hh
index 1bc9099..34ba554 100644
--- a/src/arch/generic/vec_reg.hh
+++ b/src/arch/generic/vec_reg.hh
@@ -238,7 +238,7 @@

 for (int i = 0; i < Sz; i++) {
 uint8_t b = 0;
-if (2 * i < value.size())
+if (2 * i < str.size())
 b = stoul(str.substr(i * 2, 2), nullptr, 16);
 value.template as()[i] = b;
 }

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/51947
To unsubscribe, or for help writing mail filters, visit  
https://gem5-review.googlesource.com/settings


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I5010d9f39d57fcee390e7419a64dbcd293e51fa0
Gerrit-Change-Number: 51947
Gerrit-PatchSet: 2
Gerrit-Owner: Giacomo Travaglini 
Gerrit-Reviewer: Andreas Sandberg 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Gabe Black 
Gerrit-Reviewer: Giacomo Travaglini 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] Change in gem5/gem5[develop]: cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with operator delete.

2021-11-03 Thread Gabe Black (Gerrit) via gem5-dev

Gabe Black has uploaded this change for review. (  
https://gem5-review.googlesource.com/c/public/gem5/+/52383 )



Change subject: cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with  
operator delete.

..

cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with operator delete.

When allocating memory with operator new(size_t), we should also delete
that memory with operator delete(). Note that this is a generic form of
new and delete which do not construct an object in the allocated space,
or delete the object when freeing the space.

There were a number of places where we were over-allocating a structure
so that there would be room after it for other data, at least sometimes
to allocate C structures which would have a trailing array of some other
structure with an undefined size. Those structures were then being
stored in a std::unique_ptr with the default deleter, which just calls
full blown delete and not operator delete.

It seems that this is often ok, and I was not able to find anything that
spelled out in bold letters that it isn't. I did find this sentence:

"If the pointer passed to the standard library deallocation function was
not obtained from the corresponding standard library allocation function,
the behavior is undefined."

On this webpage:

https://en.cppreference.com/w/cpp/memory/new/operator_delete

This is a *little* vague, since they might mean you can't mix malloc and
delete, or new and free. Strictly interpretting it though, it could mean
you can't mix operator new with regular delete, or any other mismatched
combination.

I also found that exactly how this causes problems depends on what heap
allocator you're using. When I used tcmalloc, gem5 would segfault within
that library. When I disabled tcmalloc to run valgrind, the segfault
went away. I think this may be because sometimes you get lucky and
undefined behavior is what you actually wanted, and sometimes you don't.

To fix this problem, this change overrides the deleter on all of these
unique_ptr-s so that they use operator delete. Also, it refactors some
code in arch/x86/kvm/x86_cpu.cc so that the function that allocates
memory with operator new returns a std::unique_ptr instead of a raw
pointer. This raw pointer was always immediately put into a unique_ptr
anyway, and, in addition to tidying up the call sights slightly, also
avoids having to define a custom deleter in each of those locations
instead of once in the allocation function.

Change-Id: I9ebff430996cf603051f5baa8708424819ed8465
---
M src/arch/x86/kvm/x86_cpu.cc
M src/dev/virtio/fs9p.hh
M src/cpu/kvm/base.cc
M src/arch/arm/kvm/base_cpu.cc
M src/arch/arm/kvm/arm_cpu.cc
M src/cpu/kvm/vm.cc
6 files changed, 81 insertions(+), 23 deletions(-)



diff --git a/src/arch/arm/kvm/arm_cpu.cc b/src/arch/arm/kvm/arm_cpu.cc
index 28fa036..4f47a15 100644
--- a/src/arch/arm/kvm/arm_cpu.cc
+++ b/src/arch/arm/kvm/arm_cpu.cc
@@ -414,7 +414,8 @@
 ArmKvmCPU::getRegList() const
 {
 if (_regIndexList.size() == 0) {
-std::unique_ptr regs;
+std::unique_ptr
+regs(nullptr, [](void *p) { operator delete(p); });
 uint64_t i = 1;

 do {
diff --git a/src/arch/arm/kvm/base_cpu.cc b/src/arch/arm/kvm/base_cpu.cc
index 524b410..fe922a1 100644
--- a/src/arch/arm/kvm/base_cpu.cc
+++ b/src/arch/arm/kvm/base_cpu.cc
@@ -201,7 +201,8 @@

 // Request the actual register list now that we know how many
 // register we need to allocate space for.
-std::unique_ptr regs;
+std::unique_ptr
+regs(nullptr, [](void *p) { operator delete(p); });
 const size_t size(sizeof(kvm_reg_list) +
   regs_probe.n * sizeof(uint64_t));
 regs.reset((kvm_reg_list *)operator new(size));
diff --git a/src/arch/x86/kvm/x86_cpu.cc b/src/arch/x86/kvm/x86_cpu.cc
index 67cecd4..11c017c 100644
--- a/src/arch/x86/kvm/x86_cpu.cc
+++ b/src/arch/x86/kvm/x86_cpu.cc
@@ -167,10 +167,14 @@
 APPLY_DTABLE(idt, MISCREG_IDTR - MISCREG_SEG_SEL_BASE); \
 } while (0)

-template
-static STRUCT *newVarStruct(size_t entries)
+template
+static auto
+newVarStruct(size_t entries)
 {
-return (STRUCT *)operator new(sizeof(STRUCT) + entries *  
sizeof(ENTRY));

+size_t size = sizeof(Struct) + entries * sizeof(Entry);
+return std::unique_ptr(
+(Struct *)operator new(size),
+[](Struct *p) { operator delete(p); });
 }

 static void
@@ -664,9 +668,8 @@
 X86KvmCPU::dumpMSRs() const
 {
 const Kvm::MSRIndexVector _msrs(vm.kvm->getSupportedMSRs());
-std::unique_ptr msrs(
-newVarStruct(
-supported_msrs.size()));
+auto msrs = newVarStruct(
+supported_msrs.size());

 msrs->nmsrs = supported_msrs.size();
 for (int i = 0; i < supported_msrs.size(); ++i) {
@@ -,8 +1114,8 @@
 {
 const Kvm::MSRIndexVector (getMsrIntersection());

-std::unique_ptr kvm_msrs(
-

[gem5-dev] Jenkins build is back to normal : nightly #31

2021-11-03 Thread jenkins-no-reply--- via gem5-dev

See 
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] failing kvm tests

2021-11-03 Thread Gabe Black via gem5-dev

Hey folks, I recently discovered KVM wasn't set up on my desktop, which I
just corrected. Now that it's enabled, the KVM tests have started running,
and they are also failing. This could be sort of weird issue on my machine,
and I'm currently looking into the failure itself.

The thing I wanted to ask was, do we have any KVM tests that run anywhere?
Do we have them in the quick regressions but not the long regressions? Do
we have KVM enabled on the nightly server? I can imagine it (KVM) not being
enabled on kokoro, but hopefully we can run those on the nightlies?

Gabe
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

[gem5-dev] Change in gem5/gem5[develop]: arch-riscv: Add NMI support

2021-11-03 Thread Jui-min Lee (Gerrit) via gem5-dev

Jui-min Lee has uploaded this change for review. (  
https://gem5-review.googlesource.com/c/public/gem5/+/52363 )



Change subject: arch-riscv: Add NMI support
..

arch-riscv: Add NMI support

NMI is platform dependent according to riscv spec, so we intentionally
propose a very minimal design that doesn't give user any new accessible
register to interact with the NMI mechanism.

In current design, the NMI reset vector is fixed to 0x0 and always set
mcause to zero. mret from NMI handler is still possible, but it's up to
the user to detect whether an M-mode trap handler is interrupted and to
recover from it (if possible at all).

1. Add new fault type to represent NMI fault
2. Add non-standard registers to save the status of NMI
   a. nmivec[63:2] = NMI reset vector address
   b. nmie[0:0] = is NMI enabled = not in NMI handler
   c. nmip[0:0] = is NMI pending
3. Add new function in RiscvInterrupts to raise/clear NMI

Bug: 200169094
Test: None
Change-Id: Ia81e1c9589bc02f0690d094fff5f13412846acbe
---
M src/arch/riscv/isa.cc
M src/arch/riscv/regs/misc.hh
M src/arch/riscv/isa/decoder.isa
M src/arch/riscv/interrupts.hh
M src/arch/riscv/faults.cc
M src/arch/riscv/faults.hh
6 files changed, 100 insertions(+), 12 deletions(-)



diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc
index b7fb509..085e113 100644
--- a/src/arch/riscv/faults.cc
+++ b/src/arch/riscv/faults.cc
@@ -67,8 +67,17 @@
 PrivilegeMode prv = PRV_M;
 STATUS status = tc->readMiscReg(MISCREG_STATUS);

+// According to riscv-privileged-v1.11, if a NMI occurs at the  
middle
+// of a M-mode trap handler, the state (epc/cause) will be  
overwritten
+// and is not necessary recoverable. There's nothing we can do  
here so

+// we'll just warn our user that the CPU state might be broken.
+warn_if(isNonMaskableInterrupt() && pp == PRV_M && status.mie == 0,
+"NMI overwriting M-mode trap handler state");
+
 // Set fault handler privilege mode
-if (isInterrupt()) {
+if (isNonMaskableInterrupt()) {
+prv = PRV_M;
+} else if (isInterrupt()) {
 if (pp != PRV_M &&
 bits(tc->readMiscReg(MISCREG_MIDELEG), _code) != 0) {
 prv = PRV_S;
@@ -113,7 +122,7 @@
   case PRV_M:
 cause = MISCREG_MCAUSE;
 epc = MISCREG_MEPC;
-tvec = MISCREG_MTVEC;
+tvec = isNonMaskableInterrupt() ? MISCREG_NMIVEC :  
MISCREG_MTVEC;

 tval = MISCREG_MTVAL;

 status.mpp = pp;
@@ -136,6 +145,12 @@
 tc->setMiscReg(tval, trap_value());
 tc->setMiscReg(MISCREG_PRV, prv);
 tc->setMiscReg(MISCREG_STATUS, status);
+// Temporarily mask NMI while we're in NMI handler. Otherweise, the
+// checkNonMaskableInterrupt will always return true and we'll be
+// stucked in an infinite loop.
+if (isNonMaskableInterrupt()) {
+tc->setMiscReg(MISCREG_NMIE, 0);
+}

 // Set PC to fault handler address
 Addr addr = mbits(tc->readMiscReg(tvec), 63, 2);
diff --git a/src/arch/riscv/faults.hh b/src/arch/riscv/faults.hh
index a8df3f5..1ea8f70 100644
--- a/src/arch/riscv/faults.hh
+++ b/src/arch/riscv/faults.hh
@@ -96,19 +96,30 @@
 NumInterruptTypes
 };

+enum class FaultType
+{
+INTERRUPT,
+NON_MASKABLE_INTERRUPT,
+OTHERS,
+};
+
 class RiscvFault : public FaultBase
 {
   protected:
 const FaultName _name;
-const bool _interrupt;
+const FaultType _fault_type;
 ExceptionCode _code;

-RiscvFault(FaultName n, bool i, ExceptionCode c)
-: _name(n), _interrupt(i), _code(c)
+RiscvFault(FaultName n, FaultType ft, ExceptionCode c)
+: _name(n), _fault_type(ft), _code(c)
 {}

 FaultName name() const override { return _name; }
-bool isInterrupt() const { return _interrupt; }
+bool isInterrupt() const { return _fault_type == FaultType::INTERRUPT;  
}

+bool isNonMaskableInterrupt() const
+{
+return _fault_type == FaultType::NON_MASKABLE_INTERRUPT;
+}
 ExceptionCode exception() const { return _code; }
 virtual RegVal trap_value() const { return 0; }

@@ -132,10 +143,22 @@
 class InterruptFault : public RiscvFault
 {
   public:
-InterruptFault(ExceptionCode c) : RiscvFault("interrupt", true, c) {}
+InterruptFault(ExceptionCode c)
+: RiscvFault("interrupt", FaultType::INTERRUPT, c)
+{}
 InterruptFault(int c) : InterruptFault(static_cast(c))  
{}

 };

+class NonMaskableInterruptFault : public RiscvFault
+{
+  public:
+NonMaskableInterruptFault()
+: RiscvFault("non_maskable_interrupt",
+ FaultType::NON_MASKABLE_INTERRUPT,
+ static_cast(0))
+{}
+};
+
 class InstFault : public RiscvFault
 {
   protected:
@@ -143,7 +166,7 @@

   public:
 InstFault(FaultName n,

[gem5-dev] Change in gem5/gem5[develop]: cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with operator delete.

[gem5-dev] Change in gem5/gem5[develop]: tests: Removing Atomic CPU with Ruby tests

[gem5-dev] Change in gem5/gem5[develop]: python: Remove incorrect usage of typing 'Optional'

[gem5-dev] Re: Build failed in Jenkins: nightly #27

[gem5-dev] Re: failing kvm tests

[gem5-dev] Change in gem5/gem5[develop]: tests: Removing Atomic CPU with Ruby tests

[gem5-dev] Change in gem5/gem5[develop]: arch: Fix serialization/deserialization of Vector registers

[gem5-dev] Change in gem5/gem5[develop]: cpu-kvm,arch-x86,arch-arm,dev: Pair operator new with operator delete.

[gem5-dev] Jenkins build is back to normal : nightly #31

[gem5-dev] failing kvm tests

[gem5-dev] Change in gem5/gem5[develop]: arch-riscv: Add NMI support

11 matches

Site Navigation

Mail list logo

Footer information