On Wed, Aug 17, 2022 at 08:43:57AM -0700, Reinette Chatre wrote: > Hi Jarkko, > > On 8/17/2022 7:53 AM, Jarkko Sakkinen wrote: > > On Wed, Aug 17, 2022 at 05:44:31PM +0300, Jarkko Sakkinen wrote: > >> On Tue, Aug 16, 2022 at 09:35:27PM -0700, Reinette Chatre wrote: > >>>>>> This portion below was also copied from previous test and by only > >>>>>> testing a write to the first page of the range the purpose is not > >>>>>> clear. Could you please elaborate if the intention is to only test > >>>>>> accessibility of the first page and why that is sufficient? > >>>>> > >>>>> It is sufficient because the test reproduces the bug. It would have to be > >>>>> rather elaborated why you would possibly want to do more than that. > >>> > >>> That is fair. An accurate comment (currently an inaccurate copy&paste) would > >>> help to explain this part of the test. > >> > >> I would simply add something like: > >> > >> /* > >> * Define memory pool size big enough to trigger the reclaimer in the EAUG > >> * path of the page reclaimer. > >> */ > >> > >> Suggestions/edits obviously welcome for the comment. > > The comment seems to better match the code below than the area referred to above: > static const unsigned long edmm_size = 8589934592; //8G > > Even so, I think that raises the point that this is platform specific since > edmm_size of 8GB would not trigger reclaimer on all platforms. > > How about adjusting it to: > /* > * Define memory pool size big enough to trigger the reclaimer in the EAUG > * path of the page reclaimer on some platforms. This constant has been > * successful in triggering a bug on some platforms (independent of the > * platforms where the reclaimer is triggered) and thus considered > * appropriate for general use. > */ > > > Regarding the area referred to above, a comment like below may help: > > /* > * Pool of pages were successfully added to enclave. Perform sanity > * check on first page of the pool only to ensure data can be written > * to and read from a dynamically added enclave page. > */ > > > > > I wonder if we could put .bt files somewhere to make them available. In > > root causing this bug bpftrace scripting was the key so it would nice to > > have them available along with kselftest. > > > > I could imagine that we end up also in future to bugs allocation so > > it would have the script when you clone the kernel tree, and possibly > > more scripts in future. > > > > E.g. add bt/alloc-error.bt under tools/testing/selftests/sgx. > > Thank you very much for helping to debug this issue. I also think > the scripts you created are very valuable and making them easily > accessible sounds great. Yeah, I mean they do no harm there, even if not directly used by the test program. Thanks for the valuable feedback. I will incorporate it to the next version. BR, Jarkko
