Re: [PATCH v11 18/18] KVM: selftests: guest_memfd mmap() test when mapping is allowed

[Sean Christopherson <seanjc@google.com>]

On Thu, Jun 05, 2025, James Houghton wrote:
> On Thu, Jun 5, 2025 at 8:38 AM Fuad Tabba <tabba@google.com> wrote:
> > @@ -34,12 +36,83 @@ static void test_file_read_write(int fd)
> >                     "pwrite on a guest_mem fd should fail");
> >  }
> >
> > -static void test_mmap(int fd, size_t page_size)
> > +static void test_mmap_supported(int fd, size_t page_size, size_t total_size)
> > +{
> > +       const char val = 0xaa;
> > +       char *mem;
> 
> This must be `volatile char *` to ensure that the compiler doesn't
> elide the accesses you have written.
> 
> > +       size_t i;
> > +       int ret;
> > +
> > +       mem = mmap(NULL, total_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> > +       TEST_ASSERT(mem == MAP_FAILED, "Copy-on-write not allowed by guest_memfd.");
> > +
> > +       mem = mmap(NULL, total_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> > +       TEST_ASSERT(mem != MAP_FAILED, "mmap() for shared guest memory should succeed.");
> > +
> > +       memset(mem, val, total_size);
> 
> Now unfortunately, `memset` and `munmap` will complain about the
> volatile qualification. So...
> 
> memset((char *)mem, val, total_size);
> 
> Eh... wish they just wouldn't complain, but this is a small price to
> pay for correctness. :)
> 
> > +       for (i = 0; i < total_size; i++)
> > +               TEST_ASSERT_EQ(mem[i], val);
> 
> The compiler is allowed to[1] elide the read of `mem[i]` and just
> assume that it is `val`.

I don't think "volatile" is needed.  Won't READ_ONCE(mem[i]) do the trick?  That
in turn will force the compiler to emit the stores as well.

> [1]: https://godbolt.org/z/Wora54bP6
> 
> Feel free to add `volatile` to that snippet to see how the code changes.