On 19.04.21 18:03, Bram Hooimeijer wrote: >> >> On 19/04/2021 17:37, Ralf Ramsauer wrote: >>> Hi Bram, >>> >>> On 19/04/2021 14:17, Bram Hooimeijer wrote: >>>> When creating a new page table, the table should be filled with >>>> zeroes to prevent decoding invalid entries as valid in the future. >>>> Given that in the inmate memory space no assumptions can be made on >>>> the contents of unallocated heapspace, zeroing needs to be done explicitly. >>> >>> Don't we already zero pages when reloading a guest? (I'm not sure) >>> IOW: Did you really experience dirty pages? >> >> Just cross-checked: Looks like we don't zero inmate memory on cell reload. >> Now I wonder why I never experienced a similar bug… But we haven't many >> inmates that make excessive use of the heap. > > I was just writing that I got to the same conclusion. Before sending in the > patch, > I was seriously considering what I overlooked, given that I never ran into > this > before either, even when doing memory latency experiments. This occasion it > was a single MMIO address, and thus I didn't move the heap_pos to an address > beyond the communication region, if that might influence something. I guess > not, > and sheer luck is involved so far. > >> >>> >>> Anyway, if we don't, then we should think if we should zero them. The >>> question is, if it is acceptable to leave artefacts of previously >>> running inmates in memory. >> >> I think we should rather implement a zalloc() wrapper around alloc(), which >> gives us guarantees to return zeroed pages. We have some other spots in >> libinmate where we could substitute alloc/memset-sequences in libinmate, e.g. >> in lib/arm-common/mem.c and lib/x86/smp.c. > > I would also prefer zalloc(), as memset clears the area per byte which seems > inefficient. Though I expect the store-buffer in the CPU to combine the > writes. >
We don't have optimized memset/copy implementations in the inmate lib. If there is a need, someone would have to write them first. Jan > Actually, the alloc/memset sequence in arm-common/mem.c prevents the issue > from arising on Arm platforms, and this patch uses memset just to be > consistent > with the Arm implementation. > > I'll send a new proposal. > > Thanks, Bram > >> >> Ralf >> >>> >>> Thanks >>> Ralf >>> >>>> >>>> Signed-off-by: Bram Hooimeijer >>>> <[email protected]> >>>> --- >>>> inmates/lib/x86/mem.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/inmates/lib/x86/mem.c b/inmates/lib/x86/mem.c index >>>> 7e1c8b83..45424ea1 100644 >>>> --- a/inmates/lib/x86/mem.c >>>> +++ b/inmates/lib/x86/mem.c >>>> @@ -58,6 +58,7 @@ void map_range(void *start, unsigned long size, enum >> map_type map_type) >>>> pt = (unsigned long *)(*pt_entry & PAGE_MASK); >>>> } else { >>>> pt = alloc(PAGE_SIZE, PAGE_SIZE); >>>> + memset(pt, 0, PAGE_SIZE); >>>> *pt_entry = (unsigned long)pt | PAGE_DEFAULT_FLAGS; >>>> } >>>> >>>> @@ -66,6 +67,7 @@ void map_range(void *start, unsigned long size, enum >> map_type map_type) >>>> pt = (unsigned long *)(*pt_entry & PAGE_MASK); >>>> } else { >>>> pt = alloc(PAGE_SIZE, PAGE_SIZE); >>>> + memset(pt, 0, PAGE_SIZE); >>>> *pt_entry = (unsigned long)pt | PAGE_DEFAULT_FLAGS; >>>> } >>>> >>>> >>> > -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/a91a6b11-6bb4-fcc8-fd9a-bcbc36332d29%40siemens.com.
