Re: [Qemu-devel] [PATCH 2/4] Memory: Enable writeback for given memory region

[Beata Michalska]

On Tue, 24 Sep 2019 at 17:28, Richard Henderson
<address@hidden> wrote:
>
> On 9/10/19 2:56 AM, Beata Michalska wrote:
> > +int main(void) {
> > +#if defined(_POSIX_MAPPED_FILES) && _POSIX_MAPPED_FILES > 0 \
> > +&& defined(_POSIX_SYNCHRONIZED_IO) && _POSIX_SYNCHRONIZED_IO > 0
> > +return msync(NULL,0, MS_SYNC);
> > +#else
> > +#error Not supported
> > +#endif
> > +}
>
> Is there any particular reason to check _POSIX_MAPPED_FILES &
> _POSIX_SYNCHRONIZED_IO?  IIRC, you can use those to "safely" use MS_SYNC.  But
> this is a configure test, and an error is in fact our defined failure case, so
> "safely" doesn't seem particularly relevant.
>
> Alternately, do we even support any systems (besides perhaps windows) that do
> not provide POSIX-2001 support, and so include msync + MS_SYNC?  My first 
> guess
> is that we don't.
>

Both flags are there to verify support for msync itself.
The check there is for posix systems , where if both set to value
greater than '0'
the msync call is available.
AFAIK Windows is the only posix non-compliant system being supported . Though
I might be wrong (?)
I might just drop the check here and use CONFIG_POSIX to handle the
msync call instead.

> > +        msync((void *)((uintptr_t)addr & qemu_host_page_mask),
> > +               HOST_PAGE_ALIGN(length), MS_SYNC);
>
> This isn't quite right.  If you move addr down to a lower address via this 
> page
> mask, you must also increase length by the same amount, and only afterward
> increase length to the host page size.
>
> Consider addr == 0xffffff, length = 2.  This covers two pages, so you'd expect
> the final parameters to be, for 4k page size, 0xfff000, 0x2000.
>
Thanks for catching this - guess I was too focused on the cache line
size, which would not cross page boundaries. Will fix that in the next version.

> r~

Thank you.

BR
Beata