On Thu, Jun 5, 2025 at 3:12 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Jun 05, 2025, James Houghton wrote:
> > On Thu, Jun 5, 2025 at 8:38 AM Fuad Tabba <tabba@xxxxxxxxxx> 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.
Yeah `volatile` is only needed on the reads. READ_ONCE() implies a
`volatile` read, so if you want to write it that way, that's fine too.
I prefer my original suggestion though; it's less likely for there to
be a bug. :)
> > [1]: https://godbolt.org/z/Wora54bP6
> >
> > Feel free to add `volatile` to that snippet to see how the code changes.
