Re: [PATCH 2/9] configure: Add sys/timex.h to probe clk_adjtime

Re: [PATCH 2/9] configure: Add sys/timex.h to probe clk_adjtime

[Thomas Huth]

In the subject:

s/clk_adjtime/clock_adjtime/

On 21/12/2020 01.53, Jiaxun Yang wrote:
> It is not a part of standard time.h. Glibc put it under
> time.h however musl treat it as a sys timex extension.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   configure | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index c228f7c21e..990f37e123 100755
> --- a/configure
> +++ b/configure
> @@ -4374,6 +4374,7 @@ fi
>   clock_adjtime=no
>   cat > $TMPC <<EOF
>   #include <time.h>
> +#include <sys/timex.h>
>   
>   int main(void)
>   {
> 

According to the man page:

  http://www.tin.org/bin/man.cgi?section=2&topic=clock_adjtime

sys/timex.h is indeed the right header here.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/9] configure/meson: Only check sys/signal.h on non-Linux

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:
> signal.h is equlevant of sys/signal.h on Linux, musl would complain
> wrong usage of sys/signal.h.
> 
> In file included from /builds/FlyGoat/qemu/include/qemu/osdep.h:108,
>                   from ../tests/qemu-iotests/socket_scm_helper.c:13:
> /usr/include/sys/signal.h:1:2: error: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Werror=cpp]
>      1 | #warning redirecting incorrect #include <sys/signal.h> to <signal.h>
>        |  ^~~~~~~
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   meson.build | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 372576f82c..1ef8722b3a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -841,7 +841,10 @@ config_host_data.set('HAVE_DRM_H', cc.has_header('libdrm/drm.h'))
>   config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
>   config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
>   config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
> -config_host_data.set('HAVE_SYS_SIGNAL_H', cc.has_header('sys/signal.h'))
> +if targetos != 'linux'
> +  # signal.h is equlevant of sys/signal.h on Linux
> +  config_host_data.set('HAVE_SYS_SIGNAL_H', cc.has_header('sys/signal.h'))
> +endif

Seems like it sys/signal.h was introduced for OpenBSD once (see commit 
128ab2ff50a), so this new check should be fine.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/9] configure/meson: Only check sys/signal.h on non-Linux

[Peter Maydell]

On Wed, 13 Jan 2021 at 07:06, Thomas Huth <thuth@redhat.com> wrote:
>
> On 21/12/2020 01.53, Jiaxun Yang wrote:
> > signal.h is equlevant of sys/signal.h on Linux, musl would complain
> > wrong usage of sys/signal.h.
> >
> > In file included from /builds/FlyGoat/qemu/include/qemu/osdep.h:108,
> >                   from ../tests/qemu-iotests/socket_scm_helper.c:13:
> > /usr/include/sys/signal.h:1:2: error: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Werror=cpp]
> >      1 | #warning redirecting incorrect #include <sys/signal.h> to <signal.h>
> >        |  ^~~~~~~
> >
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > ---
> >   meson.build | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 372576f82c..1ef8722b3a 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -841,7 +841,10 @@ config_host_data.set('HAVE_DRM_H', cc.has_header('libdrm/drm.h'))
> >   config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
> >   config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
> >   config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
> > -config_host_data.set('HAVE_SYS_SIGNAL_H', cc.has_header('sys/signal.h'))
> > +if targetos != 'linux'
> > +  # signal.h is equlevant of sys/signal.h on Linux
> > +  config_host_data.set('HAVE_SYS_SIGNAL_H', cc.has_header('sys/signal.h'))
> > +endif
>
> Seems like it sys/signal.h was introduced for OpenBSD once (see commit
> 128ab2ff50a), so this new check should be fine.

The right way to fix this would be to apply
https://patchew.org/QEMU/20201027003848.10920-1-mforney@mforney.org/
which got reviewed but never applied (it might need a respin to work
with meson) which simply removes the sys/signal.h include and the
"does the header exist" test entirely -- there is no platform which
needs sys/signal.h.

thanks
-- PMM

Re: [PATCH 4/9] libvhost-user: Include poll.h instead of sys/poll.h

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:
> Musl libc complains about it's wrong usage.
> 
> In file included from ../subprojects/libvhost-user/libvhost-user.h:20,
>                   from ../subprojects/libvhost-user/libvhost-user-glib.h:19,
>                   from ../subprojects/libvhost-user/libvhost-user-glib.c:15:
> /usr/include/sys/poll.h:1:2: error: #warning redirecting incorrect #include <sys/poll.h> to <poll.h> [-Werror=cpp]
>      1 | #warning redirecting incorrect #include <sys/poll.h> to <poll.h>
>        |  ^~~~~~~
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   subprojects/libvhost-user/libvhost-user.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
> index 7d47f1364a..3d13dfadde 100644
> --- a/subprojects/libvhost-user/libvhost-user.h
> +++ b/subprojects/libvhost-user/libvhost-user.h
> @@ -17,7 +17,7 @@
>   #include <stdint.h>
>   #include <stdbool.h>
>   #include <stddef.h>
> -#include <sys/poll.h>
> +#include <poll.h>
>   #include <linux/vhost.h>
>   #include <pthread.h>
>   #include "standard-headers/linux/virtio_ring.h"
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 5/9] elf2dmp: Rename PAGE_SIZE to ELF2DMP_PAGE_SIZE

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:
> As per POSIX specification of limits.h [1], OS libc may define
> PAGE_SIZE in limits.h.
> 
> To prevent collosion of definition, we rename PAGE_SIZE here.
> 
> [1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   contrib/elf2dmp/addrspace.c |  4 ++--
>   contrib/elf2dmp/addrspace.h |  6 +++---
>   contrib/elf2dmp/main.c      | 18 +++++++++---------
>   3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
> index 8a76069cb5..53ded17061 100644
> --- a/contrib/elf2dmp/addrspace.c
> +++ b/contrib/elf2dmp/addrspace.c
> @@ -207,8 +207,8 @@ int va_space_rw(struct va_space *vs, uint64_t addr,
>           void *buf, size_t size, int is_write)
>   {
>       while (size) {
> -        uint64_t page = addr & PFN_MASK;
> -        size_t s = (page + PAGE_SIZE) - addr;
> +        uint64_t page = addr & ELF2DMP_PFN_MASK;
> +        size_t s = (page + ELF2DMP_PAGE_SIZE) - addr;
>           void *ptr;
>   
>           s = (s > size) ? size : s;
> diff --git a/contrib/elf2dmp/addrspace.h b/contrib/elf2dmp/addrspace.h
> index d87f6a18c6..00b44c1218 100644
> --- a/contrib/elf2dmp/addrspace.h
> +++ b/contrib/elf2dmp/addrspace.h
> @@ -10,9 +10,9 @@
>   
>   #include "qemu_elf.h"
>   
> -#define PAGE_BITS 12
> -#define PAGE_SIZE (1ULL << PAGE_BITS)
> -#define PFN_MASK (~(PAGE_SIZE - 1))
> +#define ELF2DMP_PAGE_BITS 12
> +#define ELF2DMP_PAGE_SIZE (1ULL << ELF2DMP_PAGE_BITS)
> +#define ELF2DMP_PFN_MASK (~(ELF2DMP_PAGE_SIZE - 1))
>   
>   #define INVALID_PA  UINT64_MAX
>   
> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
> index ac746e49e0..20b477d582 100644
> --- a/contrib/elf2dmp/main.c
> +++ b/contrib/elf2dmp/main.c
> @@ -244,8 +244,8 @@ static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
>       WinDumpHeader64 h;
>       size_t i;
>   
> -    QEMU_BUILD_BUG_ON(KUSD_OFFSET_SUITE_MASK >= PAGE_SIZE);
> -    QEMU_BUILD_BUG_ON(KUSD_OFFSET_PRODUCT_TYPE >= PAGE_SIZE);
> +    QEMU_BUILD_BUG_ON(KUSD_OFFSET_SUITE_MASK >= ELF2DMP_PAGE_SIZE);
> +    QEMU_BUILD_BUG_ON(KUSD_OFFSET_PRODUCT_TYPE >= ELF2DMP_PAGE_SIZE);
>   
>       if (!suite_mask || !product_type) {
>           return 1;
> @@ -281,14 +281,14 @@ static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
>       };
>   
>       for (i = 0; i < ps->block_nr; i++) {
> -        h.PhysicalMemoryBlock.NumberOfPages += ps->block[i].size / PAGE_SIZE;
> +        h.PhysicalMemoryBlock.NumberOfPages += ps->block[i].size / ELF2DMP_PAGE_SIZE;
>           h.PhysicalMemoryBlock.Run[i] = (WinDumpPhyMemRun64) {
> -            .BasePage = ps->block[i].paddr / PAGE_SIZE,
> -            .PageCount = ps->block[i].size / PAGE_SIZE,
> +            .BasePage = ps->block[i].paddr / ELF2DMP_PAGE_SIZE,
> +            .PageCount = ps->block[i].size / ELF2DMP_PAGE_SIZE,
>           };
>       }
>   
> -    h.RequiredDumpSpace += h.PhysicalMemoryBlock.NumberOfPages << PAGE_BITS;
> +    h.RequiredDumpSpace += h.PhysicalMemoryBlock.NumberOfPages << ELF2DMP_PAGE_BITS;
>   
>       *hdr = h;
>   
> @@ -379,7 +379,7 @@ static int pe_get_pdb_symstore_hash(uint64_t base, void *start_addr,
>       size_t pdb_name_sz;
>       size_t i;
>   
> -    QEMU_BUILD_BUG_ON(sizeof(*dos_hdr) >= PAGE_SIZE);
> +    QEMU_BUILD_BUG_ON(sizeof(*dos_hdr) >= ELF2DMP_PAGE_SIZE);
>   
>       if (memcmp(&dos_hdr->e_magic, e_magic, sizeof(e_magic))) {
>           return 1;
> @@ -509,10 +509,10 @@ int main(int argc, char *argv[])
>       }
>       printf("CPU #0 IDT[0] -> 0x%016"PRIx64"\n", idt_desc_addr(first_idt_desc));
>   
> -    KernBase = idt_desc_addr(first_idt_desc) & ~(PAGE_SIZE - 1);
> +    KernBase = idt_desc_addr(first_idt_desc) & ~(ELF2DMP_PAGE_SIZE - 1);
>       printf("Searching kernel downwards from 0x%016"PRIx64"...\n", KernBase);
>   
> -    for (; KernBase >= 0xfffff78000000000; KernBase -= PAGE_SIZE) {
> +    for (; KernBase >= 0xfffff78000000000; KernBase -= ELF2DMP_PAGE_SIZE) {
>           nt_start_addr = va_space_resolve(&vs, KernBase);
>           if (!nt_start_addr) {
>               continue;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 6/9] hw/block/nand: Rename PAGE_SIZE to NAND_PAGE_SIZE

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:
> As per POSIX specification of limits.h [1], OS libc may define
> PAGE_SIZE in limits.h.
> 
> To prevent collosion of definition, we rename PAGE_SIZE here.
> 
> [1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   hw/block/nand.c | 40 ++++++++++++++++++++--------------------
>   1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 1d7a48a2ec..17645667d8 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -114,24 +114,24 @@ static void mem_and(uint8_t *dest, const uint8_t *src, size_t n)
>   # define NAND_IO
>   
>   # define PAGE(addr)		((addr) >> ADDR_SHIFT)
> -# define PAGE_START(page)	(PAGE(page) * (PAGE_SIZE + OOB_SIZE))
> +# define PAGE_START(page)	(PAGE(page) * (NAND_PAGE_SIZE + OOB_SIZE))
>   # define PAGE_MASK		((1 << ADDR_SHIFT) - 1)
>   # define OOB_SHIFT		(PAGE_SHIFT - 5)
>   # define OOB_SIZE		(1 << OOB_SHIFT)
>   # define SECTOR(addr)		((addr) >> (9 + ADDR_SHIFT - PAGE_SHIFT))
>   # define SECTOR_OFFSET(addr)	((addr) & ((511 >> PAGE_SHIFT) << 8))
>   
> -# define PAGE_SIZE		256
> +# define NAND_PAGE_SIZE     256
>   # define PAGE_SHIFT		8
>   # define PAGE_SECTORS		1
>   # define ADDR_SHIFT		8
>   # include "nand.c"
> -# define PAGE_SIZE		512
> +# define NAND_PAGE_SIZE     512
>   # define PAGE_SHIFT		9
>   # define PAGE_SECTORS		1
>   # define ADDR_SHIFT		8
>   # include "nand.c"
> -# define PAGE_SIZE		2048
> +# define NAND_PAGE_SIZE		2048
>   # define PAGE_SHIFT		11
>   # define PAGE_SECTORS		4
>   # define ADDR_SHIFT		16
> @@ -661,7 +661,7 @@ type_init(nand_register_types)
>   #else
>   
>   /* Program a single page */
> -static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
> +static void glue(nand_blk_write_, NAND_PAGE_SIZE)(NANDFlashState *s)
>   {
>       uint64_t off, page, sector, soff;
>       uint8_t iobuf[(PAGE_SECTORS + 2) * 0x200];
> @@ -681,11 +681,11 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
>               return;
>           }
>   
> -        mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, PAGE_SIZE - off));
> -        if (off + s->iolen > PAGE_SIZE) {
> +        mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, NAND_PAGE_SIZE - off));
> +        if (off + s->iolen > NAND_PAGE_SIZE) {
>               page = PAGE(s->addr);
> -            mem_and(s->storage + (page << OOB_SHIFT), s->io + PAGE_SIZE - off,
> -                            MIN(OOB_SIZE, off + s->iolen - PAGE_SIZE));
> +            mem_and(s->storage + (page << OOB_SHIFT), s->io + NAND_PAGE_SIZE - off,
> +                            MIN(OOB_SIZE, off + s->iolen - NAND_PAGE_SIZE));
>           }
>   
>           if (blk_pwrite(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> @@ -713,7 +713,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
>   }
>   
>   /* Erase a single block */
> -static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
> +static void glue(nand_blk_erase_, NAND_PAGE_SIZE)(NANDFlashState *s)
>   {
>       uint64_t i, page, addr;
>       uint8_t iobuf[0x200] = { [0 ... 0x1ff] = 0xff, };
> @@ -725,7 +725,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
>   
>       if (!s->blk) {
>           memset(s->storage + PAGE_START(addr),
> -                        0xff, (PAGE_SIZE + OOB_SIZE) << s->erase_shift);
> +                        0xff, (NAND_PAGE_SIZE + OOB_SIZE) << s->erase_shift);
>       } else if (s->mem_oob) {
>           memset(s->storage + (PAGE(addr) << OOB_SHIFT),
>                           0xff, OOB_SIZE << s->erase_shift);
> @@ -751,7 +751,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
>   
>           memset(iobuf, 0xff, 0x200);
>           i = (addr & ~0x1ff) + 0x200;
> -        for (addr += ((PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
> +        for (addr += ((NAND_PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
>                           i < addr; i += 0x200) {
>               if (blk_pwrite(s->blk, i, iobuf, BDRV_SECTOR_SIZE, 0) < 0) {
>                   printf("%s: write error in sector %" PRIu64 "\n",
> @@ -772,7 +772,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
>       }
>   }
>   
> -static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
> +static void glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
>                   uint64_t addr, int offset)
>   {
>       if (PAGE(addr) >= s->pages) {
> @@ -786,7 +786,7 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
>                   printf("%s: read error in sector %" PRIu64 "\n",
>                                   __func__, SECTOR(addr));
>               }
> -            memcpy(s->io + SECTOR_OFFSET(s->addr) + PAGE_SIZE,
> +            memcpy(s->io + SECTOR_OFFSET(s->addr) + NAND_PAGE_SIZE,
>                               s->storage + (PAGE(s->addr) << OOB_SHIFT),
>                               OOB_SIZE);
>               s->ioaddr = s->io + SECTOR_OFFSET(s->addr) + offset;
> @@ -800,23 +800,23 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
>           }
>       } else {
>           memcpy(s->io, s->storage + PAGE_START(s->addr) +
> -                        offset, PAGE_SIZE + OOB_SIZE - offset);
> +                        offset, NAND_PAGE_SIZE + OOB_SIZE - offset);
>           s->ioaddr = s->io;
>       }
>   }
>   
> -static void glue(nand_init_, PAGE_SIZE)(NANDFlashState *s)
> +static void glue(nand_init_, NAND_PAGE_SIZE)(NANDFlashState *s)
>   {
>       s->oob_shift = PAGE_SHIFT - 5;
>       s->pages = s->size >> PAGE_SHIFT;
>       s->addr_shift = ADDR_SHIFT;
>   
> -    s->blk_erase = glue(nand_blk_erase_, PAGE_SIZE);
> -    s->blk_write = glue(nand_blk_write_, PAGE_SIZE);
> -    s->blk_load = glue(nand_blk_load_, PAGE_SIZE);
> +    s->blk_erase = glue(nand_blk_erase_, NAND_PAGE_SIZE);
> +    s->blk_write = glue(nand_blk_write_, NAND_PAGE_SIZE);
> +    s->blk_load = glue(nand_blk_load_, NAND_PAGE_SIZE);
>   }
>   
> -# undef PAGE_SIZE
> +# undef NAND_PAGE_SIZE
>   # undef PAGE_SHIFT
>   # undef PAGE_SECTORS
>   # undef ADDR_SHIFT
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>