[v4,2/3] tests: make pc_alloc_init/init_flags/uninit generic

Commit Message

Laurent Vivier Sept. 6, 2016, 1:17 p.m. UTC

And add support for ppc64.

Signed-off-by: Laurent Vivier <[e-mail]>
---
v2:
- remove useless parenthesis, inline

 tests/Makefile.include      |  3 ++-
 tests/libqos/libqos.h       |  2 +-
 tests/libqos/malloc-ppc64.c | 38 ++++++++++++++++++++++++++++++++++++++
 tests/libqos/malloc-ppc64.h | 17 +++++++++++++++++
 tests/libqos/malloc.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
 tests/libqos/malloc.h       |  3 +++
 6 files changed, 103 insertions(+), 2 deletions(-)
 create mode 100644 tests/libqos/malloc-ppc64.c
 create mode 100644 tests/libqos/malloc-ppc64.h

Comments

Thomas Huth Sept. 6, 2016, 7:46 p.m. UTC | #1

On 06.09.2016 15:17, Laurent Vivier wrote:
> And add support for ppc64.
> 
> Signed-off-by: Laurent Vivier <[e-mail]>
> ---
> v2:
> - remove useless parenthesis, inline
> 
>  tests/Makefile.include      |  3 ++-
>  tests/libqos/libqos.h       |  2 +-
>  tests/libqos/malloc-ppc64.c | 38 ++++++++++++++++++++++++++++++++++++++
>  tests/libqos/malloc-ppc64.h | 17 +++++++++++++++++
>  tests/libqos/malloc.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/malloc.h       |  3 +++
>  6 files changed, 103 insertions(+), 2 deletions(-)
>  create mode 100644 tests/libqos/malloc-ppc64.c
>  create mode 100644 tests/libqos/malloc-ppc64.h
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 14be491..a286848 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -557,8 +557,9 @@ tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
>  
>  libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
>  libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
> +libqos-obj-y += tests/libqos/malloc-ppc64.o tests/libqos/malloc-pc.o
>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> -libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> +libqos-pc-obj-y += tests/libqos/libqos-pc.o
>  libqos-pc-obj-y += tests/libqos/ahci.o
>  libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
>  libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
> index 604980d..7b71607 100644
> --- a/tests/libqos/libqos.h
> +++ b/tests/libqos/libqos.h
> @@ -3,7 +3,7 @@
>  
>  #include "libqtest.h"
>  #include "libqos/pci.h"
> -#include "libqos/malloc-pc.h"
> +#include "libqos/malloc.h"
>  
>  typedef struct QOSOps {
>      QGuestAllocator *(*init_allocator)(QAllocOpts);
> diff --git a/tests/libqos/malloc-ppc64.c b/tests/libqos/malloc-ppc64.c
> new file mode 100644
> index 0000000..1b31e33
> --- /dev/null
> +++ b/tests/libqos/malloc-ppc64.c
> @@ -0,0 +1,38 @@
> +/*
> + * libqos malloc support for PPC64
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/malloc-ppc64.h"
> +
> +#include "qemu-common.h"
> +
> +#define PAGE_SIZE 4096
> +
> +/* Memory must be a multiple of 256 MB,
> + * so we have at least 256MB
> + */
> +#define PPC64_MIN_SIZE 0x10000000
> +
> +void ppc64_alloc_uninit(QGuestAllocator *allocator)
> +{
> +    alloc_uninit(allocator);
> +}
> +
> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags)
> +{
> +    QGuestAllocator *s;
> +
> +    s = alloc_init_flags(flags, 1 << 20, PPC64_MIN_SIZE);
> +    alloc_set_page_size(s, PAGE_SIZE);
> +
> +    return s;
> +}
> +
> +QGuestAllocator *ppc64_alloc_init(void)
> +{
> +    return ppc64_alloc_init_flags(ALLOC_NO_FLAGS);
> +}
> diff --git a/tests/libqos/malloc-ppc64.h b/tests/libqos/malloc-ppc64.h
> new file mode 100644
> index 0000000..c2b2dff
> --- /dev/null
> +++ b/tests/libqos/malloc-ppc64.h
> @@ -0,0 +1,17 @@
> +/*
> + * libqos malloc support for PPC64
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_MALLOC_PPC64_H
> +#define LIBQOS_MALLOC_PPC64_H
> +
> +#include "libqos/malloc.h"
> +
> +QGuestAllocator *ppc64_alloc_init(void);
> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags);
> +void ppc64_alloc_uninit(QGuestAllocator *allocator);
> +
> +#endif
> diff --git a/tests/libqos/malloc.c b/tests/libqos/malloc.c
> index b8eff5f..6a02345 100644
> --- a/tests/libqos/malloc.c
> +++ b/tests/libqos/malloc.c
> @@ -12,6 +12,9 @@
>  
>  #include "qemu/osdep.h"
>  #include "libqos/malloc.h"
> +#include "libqos/malloc-pc.h"
> +#include "libqos/malloc-ppc64.h"
> +#include "libqtest.h"
>  #include "qemu-common.h"
>  #include "qemu/host-utils.h"
>  
> @@ -375,3 +378,42 @@ void migrate_allocator(QGuestAllocator *src,
>      QTAILQ_INSERT_HEAD(src->free, node, MLIST_ENTNAME);
>      return;
>  }
> +
> +QGuestAllocator *machine_alloc_init(void)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        return pc_alloc_init();
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        return ppc64_alloc_init();
> +    }
> +    return NULL;
> +}
> +
> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        return pc_alloc_init_flags(flags);
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        return ppc64_alloc_init_flags(flags);
> +    }
> +    return NULL;
> +}
> +
> +void machine_alloc_uninit(QGuestAllocator *allocator)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        pc_alloc_uninit(allocator);
> +        return;
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        ppc64_alloc_uninit(allocator);
> +    }
> +}
> diff --git a/tests/libqos/malloc.h b/tests/libqos/malloc.h
> index ae9dac8..a5f4c63 100644
> --- a/tests/libqos/malloc.h
> +++ b/tests/libqos/malloc.h
> @@ -37,4 +37,7 @@ QGuestAllocator *alloc_init_flags(QAllocOpts flags,
>  void alloc_set_page_size(QGuestAllocator *allocator, size_t page_size);
>  void alloc_set_flags(QGuestAllocator *allocator, QAllocOpts opts);
>  
> +QGuestAllocator *machine_alloc_init(void);
> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags);
> +void machine_alloc_uninit(QGuestAllocator *allocator);
>  #endif
> 

Looks fine to me now!

Reviewed-by: Thomas Huth <[e-mail]>

Greg Kurz Sept. 6, 2016, 9:41 p.m. UTC | #2

On Tue,  6 Sep 2016 15:17:56 +0200
Laurent Vivier <[e-mail]> wrote:

> And add support for ppc64.
> 
> Signed-off-by: Laurent Vivier <[e-mail]>
> ---
> v2:
> - remove useless parenthesis, inline
> 

This works indeed but I'm just feeling curious about the QOSOps type introduced
by the following commit:

commit 90e5add6f2fa0b0bd9a4c1d5a4de2304b5f3e466
Author: John Snow <[e-mail]>
Date:   Mon Jan 19 15:15:55 2015 -0500

    libqos: add pc specific interface

Wouldn't this be better to implement something similar for ppc64 instead of
relying on strcmp() ?

Cheers.

--
Greg

>  tests/Makefile.include      |  3 ++-
>  tests/libqos/libqos.h       |  2 +-
>  tests/libqos/malloc-ppc64.c | 38 ++++++++++++++++++++++++++++++++++++++
>  tests/libqos/malloc-ppc64.h | 17 +++++++++++++++++
>  tests/libqos/malloc.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/malloc.h       |  3 +++
>  6 files changed, 103 insertions(+), 2 deletions(-)
>  create mode 100644 tests/libqos/malloc-ppc64.c
>  create mode 100644 tests/libqos/malloc-ppc64.h
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 14be491..a286848 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -557,8 +557,9 @@ tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
>  
>  libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
>  libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
> +libqos-obj-y += tests/libqos/malloc-ppc64.o tests/libqos/malloc-pc.o
>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> -libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> +libqos-pc-obj-y += tests/libqos/libqos-pc.o
>  libqos-pc-obj-y += tests/libqos/ahci.o
>  libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
>  libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
> index 604980d..7b71607 100644
> --- a/tests/libqos/libqos.h
> +++ b/tests/libqos/libqos.h
> @@ -3,7 +3,7 @@
>  
>  #include "libqtest.h"
>  #include "libqos/pci.h"
> -#include "libqos/malloc-pc.h"
> +#include "libqos/malloc.h"
>  
>  typedef struct QOSOps {
>      QGuestAllocator *(*init_allocator)(QAllocOpts);
> diff --git a/tests/libqos/malloc-ppc64.c b/tests/libqos/malloc-ppc64.c
> new file mode 100644
> index 0000000..1b31e33
> --- /dev/null
> +++ b/tests/libqos/malloc-ppc64.c
> @@ -0,0 +1,38 @@
> +/*
> + * libqos malloc support for PPC64
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/malloc-ppc64.h"
> +
> +#include "qemu-common.h"
> +
> +#define PAGE_SIZE 4096
> +
> +/* Memory must be a multiple of 256 MB,
> + * so we have at least 256MB
> + */
> +#define PPC64_MIN_SIZE 0x10000000
> +
> +void ppc64_alloc_uninit(QGuestAllocator *allocator)
> +{
> +    alloc_uninit(allocator);
> +}
> +
> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags)
> +{
> +    QGuestAllocator *s;
> +
> +    s = alloc_init_flags(flags, 1 << 20, PPC64_MIN_SIZE);
> +    alloc_set_page_size(s, PAGE_SIZE);
> +
> +    return s;
> +}
> +
> +QGuestAllocator *ppc64_alloc_init(void)
> +{
> +    return ppc64_alloc_init_flags(ALLOC_NO_FLAGS);
> +}
> diff --git a/tests/libqos/malloc-ppc64.h b/tests/libqos/malloc-ppc64.h
> new file mode 100644
> index 0000000..c2b2dff
> --- /dev/null
> +++ b/tests/libqos/malloc-ppc64.h
> @@ -0,0 +1,17 @@
> +/*
> + * libqos malloc support for PPC64
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_MALLOC_PPC64_H
> +#define LIBQOS_MALLOC_PPC64_H
> +
> +#include "libqos/malloc.h"
> +
> +QGuestAllocator *ppc64_alloc_init(void);
> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags);
> +void ppc64_alloc_uninit(QGuestAllocator *allocator);
> +
> +#endif
> diff --git a/tests/libqos/malloc.c b/tests/libqos/malloc.c
> index b8eff5f..6a02345 100644
> --- a/tests/libqos/malloc.c
> +++ b/tests/libqos/malloc.c
> @@ -12,6 +12,9 @@
>  
>  #include "qemu/osdep.h"
>  #include "libqos/malloc.h"
> +#include "libqos/malloc-pc.h"
> +#include "libqos/malloc-ppc64.h"
> +#include "libqtest.h"
>  #include "qemu-common.h"
>  #include "qemu/host-utils.h"
>  
> @@ -375,3 +378,42 @@ void migrate_allocator(QGuestAllocator *src,
>      QTAILQ_INSERT_HEAD(src->free, node, MLIST_ENTNAME);
>      return;
>  }
> +
> +QGuestAllocator *machine_alloc_init(void)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        return pc_alloc_init();
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        return ppc64_alloc_init();
> +    }
> +    return NULL;
> +}
> +
> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        return pc_alloc_init_flags(flags);
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        return ppc64_alloc_init_flags(flags);
> +    }
> +    return NULL;
> +}
> +
> +void machine_alloc_uninit(QGuestAllocator *allocator)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        pc_alloc_uninit(allocator);
> +        return;
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        ppc64_alloc_uninit(allocator);
> +    }
> +}
> diff --git a/tests/libqos/malloc.h b/tests/libqos/malloc.h
> index ae9dac8..a5f4c63 100644
> --- a/tests/libqos/malloc.h
> +++ b/tests/libqos/malloc.h
> @@ -37,4 +37,7 @@ QGuestAllocator *alloc_init_flags(QAllocOpts flags,
>  void alloc_set_page_size(QGuestAllocator *allocator, size_t page_size);
>  void alloc_set_flags(QGuestAllocator *allocator, QAllocOpts opts);
>  
> +QGuestAllocator *machine_alloc_init(void);
> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags);
> +void machine_alloc_uninit(QGuestAllocator *allocator);
>  #endif

Laurent Vivier Sept. 7, 2016, 9:36 a.m. UTC | #3

On 06/09/2016 23:41, Greg Kurz wrote:
> On Tue,  6 Sep 2016 15:17:56 +0200
> Laurent Vivier <[e-mail]> wrote:
> 
>> And add support for ppc64.
>>
>> Signed-off-by: Laurent Vivier <[e-mail]>
>> ---
>> v2:
>> - remove useless parenthesis, inline
>>
> 
> This works indeed but I'm just feeling curious about the QOSOps type introduced
> by the following commit:
> 
> commit 90e5add6f2fa0b0bd9a4c1d5a4de2304b5f3e466
> Author: John Snow <[e-mail]>
> Date:   Mon Jan 19 15:15:55 2015 -0500
> 
>     libqos: add pc specific interface
> 
> Wouldn't this be better to implement something similar for ppc64 instead of
> relying on strcmp() ?

Tests can be generic and to be run on several archs: we need the
strcmp() to check the guest arch [1], it can't be hardcoded in the test.

Thanks,
Laurent

[1]
const char *qtest_get_arch(void)
{
    const char *qemu = getenv("QTEST_QEMU_BINARY");
    g_assert(qemu != NULL);
    const char *end = strrchr(qemu, '/');

    return end + strlen("/qemu-system-");
}

Greg Kurz Sept. 7, 2016, 12:44 p.m. UTC | #4

On Wed, 7 Sep 2016 11:36:21 +0200
Laurent Vivier <[e-mail]> wrote:

> On 06/09/2016 23:41, Greg Kurz wrote:
> > On Tue,  6 Sep 2016 15:17:56 +0200
> > Laurent Vivier <[e-mail]> wrote:
> >   
> >> And add support for ppc64.
> >>
> >> Signed-off-by: Laurent Vivier <[e-mail]>
> >> ---
> >> v2:
> >> - remove useless parenthesis, inline
> >>  
> > 
> > This works indeed but I'm just feeling curious about the QOSOps type introduced
> > by the following commit:
> > 
> > commit 90e5add6f2fa0b0bd9a4c1d5a4de2304b5f3e466
> > Author: John Snow <[e-mail]>
> > Date:   Mon Jan 19 15:15:55 2015 -0500
> > 
> >     libqos: add pc specific interface
> > 
> > Wouldn't this be better to implement something similar for ppc64 instead of
> > relying on strcmp() ?  
> 
> Tests can be generic and to be run on several archs: we need the
> strcmp() to check the guest arch [1], it can't be hardcoded in the test.
> 

I agree for truely platform agnostic tests, but this is obviously not the case
for RTAS, which is the goal of this series.

My suggestion is basically to:
- keep malloc-ppc64.[ch] from your series
- introduce libqos-ppc64.[ch] like the existing libqos-pc.[ch]
- add qtest_ppc64_[start|end]() wrappers to pass global_qtest to
  qtest_ppc64_[boot|shutdown]()
- adapt the final RTAS test patch to use these wrappers and q[malloc|free]()

BTW, maybe s/ppc64/ppc to match hw/ppc, since libqos is about HW platforms,
not target archs.

This is more work, but I guess in the end it maybe useful in the long term.

And, of course, I'm volunteering to participate, with patches/reviewing/testing.

Makes sense ?

Cheers.

--
Greg

> Thanks,
> Laurent
> 
> [1]
> const char *qtest_get_arch(void)
> {
>     const char *qemu = getenv("QTEST_QEMU_BINARY");
>     g_assert(qemu != NULL);
>     const char *end = strrchr(qemu, '/');
> 
>     return end + strlen("/qemu-system-");
> }

Laurent Vivier Sept. 7, 2016, 1:10 p.m. UTC | #5

On 07/09/2016 14:44, Greg Kurz wrote:
> On Wed, 7 Sep 2016 11:36:21 +0200
> Laurent Vivier <[e-mail]> wrote:
> 
>> On 06/09/2016 23:41, Greg Kurz wrote:
>>> On Tue,  6 Sep 2016 15:17:56 +0200
>>> Laurent Vivier <[e-mail]> wrote:
>>>   
>>>> And add support for ppc64.
>>>>
>>>> Signed-off-by: Laurent Vivier <[e-mail]>
>>>> ---
>>>> v2:
>>>> - remove useless parenthesis, inline
>>>>  
>>>
>>> This works indeed but I'm just feeling curious about the QOSOps type introduced
>>> by the following commit:
>>>
>>> commit 90e5add6f2fa0b0bd9a4c1d5a4de2304b5f3e466
>>> Author: John Snow <[e-mail]>
>>> Date:   Mon Jan 19 15:15:55 2015 -0500
>>>
>>>     libqos: add pc specific interface
>>>
>>> Wouldn't this be better to implement something similar for ppc64 instead of
>>> relying on strcmp() ?  
>>
>> Tests can be generic and to be run on several archs: we need the
>> strcmp() to check the guest arch [1], it can't be hardcoded in the test.
>>
> 
> I agree for truely platform agnostic tests, but this is obviously not the case
> for RTAS, which is the goal of this series.
> 
> My suggestion is basically to:
> - keep malloc-ppc64.[ch] from your series
> - introduce libqos-ppc64.[ch] like the existing libqos-pc.[ch]
> - add qtest_ppc64_[start|end]() wrappers to pass global_qtest to
>   qtest_ppc64_[boot|shutdown]()
> - adapt the final RTAS test patch to use these wrappers and q[malloc|free]()

You're right it's the good way to implement guest memory allocation...
I'm going to add this part in the series.

> BTW, maybe s/ppc64/ppc to match hw/ppc, since libqos is about HW platforms,
> not target archs.

I use ppc64 because we guess guest memory is at least 256MB, and this is
true only with ppc64/pseries. With ppc, I think we should use qfw_cfg()
as for PC.

Thanks,
Laurent
> This is more work, but I guess in the end it maybe useful in the long term.
> 
> And, of course, I'm volunteering to participate, with patches/reviewing/testing.
> 
> Makes sense ?
> 
> Cheers.
> 
> --
> Greg
> 
>> Thanks,
>> Laurent
>>
>> [1]
>> const char *qtest_get_arch(void)
>> {
>>     const char *qemu = getenv("QTEST_QEMU_BINARY");
>>     g_assert(qemu != NULL);
>>     const char *end = strrchr(qemu, '/');
>>
>>     return end + strlen("/qemu-system-");
>> }
>

Greg Kurz Sept. 7, 2016, 1:42 p.m. UTC | #6

On Wed, 7 Sep 2016 15:10:59 +0200
Laurent Vivier <[e-mail]> wrote:

> On 07/09/2016 14:44, Greg Kurz wrote:
> > On Wed, 7 Sep 2016 11:36:21 +0200
> > Laurent Vivier <[e-mail]> wrote:
> >   
> >> On 06/09/2016 23:41, Greg Kurz wrote:  
> >>> On Tue,  6 Sep 2016 15:17:56 +0200
> >>> Laurent Vivier <[e-mail]> wrote:
> >>>     
> >>>> And add support for ppc64.
> >>>>
> >>>> Signed-off-by: Laurent Vivier <[e-mail]>
> >>>> ---
> >>>> v2:
> >>>> - remove useless parenthesis, inline
> >>>>    
> >>>
> >>> This works indeed but I'm just feeling curious about the QOSOps type introduced
> >>> by the following commit:
> >>>
> >>> commit 90e5add6f2fa0b0bd9a4c1d5a4de2304b5f3e466
> >>> Author: John Snow <[e-mail]>
> >>> Date:   Mon Jan 19 15:15:55 2015 -0500
> >>>
> >>>     libqos: add pc specific interface
> >>>
> >>> Wouldn't this be better to implement something similar for ppc64 instead of
> >>> relying on strcmp() ?    
> >>
> >> Tests can be generic and to be run on several archs: we need the
> >> strcmp() to check the guest arch [1], it can't be hardcoded in the test.
> >>  
> > 
> > I agree for truely platform agnostic tests, but this is obviously not the case
> > for RTAS, which is the goal of this series.
> > 
> > My suggestion is basically to:
> > - keep malloc-ppc64.[ch] from your series
> > - introduce libqos-ppc64.[ch] like the existing libqos-pc.[ch]
> > - add qtest_ppc64_[start|end]() wrappers to pass global_qtest to
> >   qtest_ppc64_[boot|shutdown]()
> > - adapt the final RTAS test patch to use these wrappers and q[malloc|free]()  
> 
> You're right it's the good way to implement guest memory allocation...
> I'm going to add this part in the series.
> 
> > BTW, maybe s/ppc64/ppc to match hw/ppc, since libqos is about HW platforms,
> > not target archs.  
> 
> I use ppc64 because we guess guest memory is at least 256MB, and this is
> true only with ppc64/pseries. With ppc, I think we should use qfw_cfg()
> as for PC.
> 

True. In this case, maybe you should even use spapr, since RTAS is for pseries
only, and so will be the PCI bits you mention in the cover letter.

Cheers.

--
Greg

> Thanks,
> Laurent
> > This is more work, but I guess in the end it maybe useful in the long term.
> > 
> > And, of course, I'm volunteering to participate, with patches/reviewing/testing.
> > 
> > Makes sense ?
> > 
> > Cheers.
> > 
> > --
> > Greg
> >   
> >> Thanks,
> >> Laurent
> >>
> >> [1]
> >> const char *qtest_get_arch(void)
> >> {
> >>     const char *qemu = getenv("QTEST_QEMU_BINARY");
> >>     g_assert(qemu != NULL);
> >>     const char *end = strrchr(qemu, '/');
> >>
> >>     return end + strlen("/qemu-system-");
> >> }  
> >

David Gibson Sept. 8, 2016, 2:04 a.m. UTC | #7

On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:
> And add support for ppc64.
> 
> Signed-off-by: Laurent Vivier <[e-mail]>

Some of my coments may be obsoleted by the discussion with Greg.

> ---
> v2:
> - remove useless parenthesis, inline
> 
>  tests/Makefile.include      |  3 ++-
>  tests/libqos/libqos.h       |  2 +-
>  tests/libqos/malloc-ppc64.c | 38 ++++++++++++++++++++++++++++++++++++++
>  tests/libqos/malloc-ppc64.h | 17 +++++++++++++++++
>  tests/libqos/malloc.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/malloc.h       |  3 +++
>  6 files changed, 103 insertions(+), 2 deletions(-)
>  create mode 100644 tests/libqos/malloc-ppc64.c
>  create mode 100644 tests/libqos/malloc-ppc64.h
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 14be491..a286848 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -557,8 +557,9 @@ tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
>  
>  libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
>  libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
> +libqos-obj-y += tests/libqos/malloc-ppc64.o tests/libqos/malloc-pc.o
>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> -libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> +libqos-pc-obj-y += tests/libqos/libqos-pc.o
>  libqos-pc-obj-y += tests/libqos/ahci.o
>  libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
>  libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
> index 604980d..7b71607 100644
> --- a/tests/libqos/libqos.h
> +++ b/tests/libqos/libqos.h
> @@ -3,7 +3,7 @@
>  
>  #include "libqtest.h"
>  #include "libqos/pci.h"
> -#include "libqos/malloc-pc.h"
> +#include "libqos/malloc.h"
>  
>  typedef struct QOSOps {
>      QGuestAllocator *(*init_allocator)(QAllocOpts);
> diff --git a/tests/libqos/malloc-ppc64.c b/tests/libqos/malloc-ppc64.c
> new file mode 100644
> index 0000000..1b31e33
> --- /dev/null
> +++ b/tests/libqos/malloc-ppc64.c
> @@ -0,0 +1,38 @@
> +/*
> + * libqos malloc support for PPC64
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/malloc-ppc64.h"
> +
> +#include "qemu-common.h"
> +
> +#define PAGE_SIZE 4096
> +
> +/* Memory must be a multiple of 256 MB,
> + * so we have at least 256MB
> + */
> +#define PPC64_MIN_SIZE 0x10000000

So, this is really a "pseries" machine type fact rather than a ppc64
fact.  Is there a way to make this dependent on machine type rather
than target architecture?  Naming this based on machine type would
seem to better match the "pc" things these are based on ("pc" rather
than "i386" or "x86_64").

> +
> +void ppc64_alloc_uninit(QGuestAllocator *allocator)
> +{
> +    alloc_uninit(allocator);
> +}
> +
> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags)
> +{
> +    QGuestAllocator *s;
> +
> +    s = alloc_init_flags(flags, 1 << 20, PPC64_MIN_SIZE);
> +    alloc_set_page_size(s, PAGE_SIZE);
> +
> +    return s;
> +}
> +
> +QGuestAllocator *ppc64_alloc_init(void)
> +{
> +    return ppc64_alloc_init_flags(ALLOC_NO_FLAGS);
> +}
> diff --git a/tests/libqos/malloc-ppc64.h b/tests/libqos/malloc-ppc64.h
> new file mode 100644
> index 0000000..c2b2dff
> --- /dev/null
> +++ b/tests/libqos/malloc-ppc64.h
> @@ -0,0 +1,17 @@
> +/*
> + * libqos malloc support for PPC64
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_MALLOC_PPC64_H
> +#define LIBQOS_MALLOC_PPC64_H
> +
> +#include "libqos/malloc.h"
> +
> +QGuestAllocator *ppc64_alloc_init(void);
> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags);
> +void ppc64_alloc_uninit(QGuestAllocator *allocator);
> +
> +#endif
> diff --git a/tests/libqos/malloc.c b/tests/libqos/malloc.c
> index b8eff5f..6a02345 100644
> --- a/tests/libqos/malloc.c
> +++ b/tests/libqos/malloc.c
> @@ -12,6 +12,9 @@
>  
>  #include "qemu/osdep.h"
>  #include "libqos/malloc.h"
> +#include "libqos/malloc-pc.h"
> +#include "libqos/malloc-ppc64.h"
> +#include "libqtest.h"
>  #include "qemu-common.h"
>  #include "qemu/host-utils.h"
>  
> @@ -375,3 +378,42 @@ void migrate_allocator(QGuestAllocator *src,
>      QTAILQ_INSERT_HEAD(src->free, node, MLIST_ENTNAME);
>      return;
>  }
> +
> +QGuestAllocator *machine_alloc_init(void)
> +{
> +    const char *arch = qtest_get_arch();

Maybe we need to add a qtest_get_machine_type().

> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        return pc_alloc_init();
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        return ppc64_alloc_init();
> +    }
> +    return NULL;
> +}
> +
> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        return pc_alloc_init_flags(flags);
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        return ppc64_alloc_init_flags(flags);
> +    }
> +    return NULL;
> +}
> +
> +void machine_alloc_uninit(QGuestAllocator *allocator)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        pc_alloc_uninit(allocator);
> +        return;
> +    }
> +    if (strcmp(arch, "ppc64") == 0) {
> +        ppc64_alloc_uninit(allocator);
> +    }
> +}
> diff --git a/tests/libqos/malloc.h b/tests/libqos/malloc.h
> index ae9dac8..a5f4c63 100644
> --- a/tests/libqos/malloc.h
> +++ b/tests/libqos/malloc.h
> @@ -37,4 +37,7 @@ QGuestAllocator *alloc_init_flags(QAllocOpts flags,
>  void alloc_set_page_size(QGuestAllocator *allocator, size_t page_size);
>  void alloc_set_flags(QGuestAllocator *allocator, QAllocOpts opts);
>  
> +QGuestAllocator *machine_alloc_init(void);
> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags);
> +void machine_alloc_uninit(QGuestAllocator *allocator);
>  #endif

Laurent Vivier Sept. 8, 2016, 7:50 a.m. UTC | #8

On 08/09/2016 04:04, David Gibson wrote:
> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:
>> And add support for ppc64.
>>
>> Signed-off-by: Laurent Vivier <[e-mail]>
> 
> Some of my coments may be obsoleted by the discussion with Greg.
> 
>> ---
>> v2:
>> - remove useless parenthesis, inline
>>
>>  tests/Makefile.include      |  3 ++-
>>  tests/libqos/libqos.h       |  2 +-
>>  tests/libqos/malloc-ppc64.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  tests/libqos/malloc-ppc64.h | 17 +++++++++++++++++
>>  tests/libqos/malloc.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
>>  tests/libqos/malloc.h       |  3 +++
>>  6 files changed, 103 insertions(+), 2 deletions(-)
>>  create mode 100644 tests/libqos/malloc-ppc64.c
>>  create mode 100644 tests/libqos/malloc-ppc64.h
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 14be491..a286848 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -557,8 +557,9 @@ tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
>>  
>>  libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
>>  libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
>> +libqos-obj-y += tests/libqos/malloc-ppc64.o tests/libqos/malloc-pc.o
>>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
>> -libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
>> +libqos-pc-obj-y += tests/libqos/libqos-pc.o
>>  libqos-pc-obj-y += tests/libqos/ahci.o
>>  libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
>>  libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
>> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
>> index 604980d..7b71607 100644
>> --- a/tests/libqos/libqos.h
>> +++ b/tests/libqos/libqos.h
>> @@ -3,7 +3,7 @@
>>  
>>  #include "libqtest.h"
>>  #include "libqos/pci.h"
>> -#include "libqos/malloc-pc.h"
>> +#include "libqos/malloc.h"
>>  
>>  typedef struct QOSOps {
>>      QGuestAllocator *(*init_allocator)(QAllocOpts);
>> diff --git a/tests/libqos/malloc-ppc64.c b/tests/libqos/malloc-ppc64.c
>> new file mode 100644
>> index 0000000..1b31e33
>> --- /dev/null
>> +++ b/tests/libqos/malloc-ppc64.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * libqos malloc support for PPC64
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "libqos/malloc-ppc64.h"
>> +
>> +#include "qemu-common.h"
>> +
>> +#define PAGE_SIZE 4096
>> +
>> +/* Memory must be a multiple of 256 MB,
>> + * so we have at least 256MB
>> + */
>> +#define PPC64_MIN_SIZE 0x10000000
> 
> So, this is really a "pseries" machine type fact rather than a ppc64
> fact.  Is there a way to make this dependent on machine type rather
> than target architecture?  Naming this based on machine type would
> seem to better match the "pc" things these are based on ("pc" rather
> than "i386" or "x86_64").

I've changed all my "ppc64" by "spapr". Is "pseries" better?

>> +
>> +void ppc64_alloc_uninit(QGuestAllocator *allocator)
>> +{
>> +    alloc_uninit(allocator);
>> +}
>> +
>> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags)
>> +{
>> +    QGuestAllocator *s;
>> +
>> +    s = alloc_init_flags(flags, 1 << 20, PPC64_MIN_SIZE);
>> +    alloc_set_page_size(s, PAGE_SIZE);
>> +
>> +    return s;
>> +}
>> +
>> +QGuestAllocator *ppc64_alloc_init(void)
>> +{
>> +    return ppc64_alloc_init_flags(ALLOC_NO_FLAGS);
>> +}
>> diff --git a/tests/libqos/malloc-ppc64.h b/tests/libqos/malloc-ppc64.h
>> new file mode 100644
>> index 0000000..c2b2dff
>> --- /dev/null
>> +++ b/tests/libqos/malloc-ppc64.h
>> @@ -0,0 +1,17 @@
>> +/*
>> + * libqos malloc support for PPC64
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef LIBQOS_MALLOC_PPC64_H
>> +#define LIBQOS_MALLOC_PPC64_H
>> +
>> +#include "libqos/malloc.h"
>> +
>> +QGuestAllocator *ppc64_alloc_init(void);
>> +QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags);
>> +void ppc64_alloc_uninit(QGuestAllocator *allocator);
>> +
>> +#endif
>> diff --git a/tests/libqos/malloc.c b/tests/libqos/malloc.c
>> index b8eff5f..6a02345 100644
>> --- a/tests/libqos/malloc.c
>> +++ b/tests/libqos/malloc.c
>> @@ -12,6 +12,9 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "libqos/malloc.h"
>> +#include "libqos/malloc-pc.h"
>> +#include "libqos/malloc-ppc64.h"
>> +#include "libqtest.h"
>>  #include "qemu-common.h"
>>  #include "qemu/host-utils.h"
>>  
>> @@ -375,3 +378,42 @@ void migrate_allocator(QGuestAllocator *src,
>>      QTAILQ_INSERT_HEAD(src->free, node, MLIST_ENTNAME);
>>      return;
>>  }
>> +
>> +QGuestAllocator *machine_alloc_init(void)
>> +{
>> +    const char *arch = qtest_get_arch();
> 
> Maybe we need to add a qtest_get_machine_type().

I'm working on that...

>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +        return pc_alloc_init();
>> +    }
>> +    if (strcmp(arch, "ppc64") == 0) {
>> +        return ppc64_alloc_init();
>> +    }
>> +    return NULL;
>> +}
>> +
>> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags)
>> +{
>> +    const char *arch = qtest_get_arch();
>> +
>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +        return pc_alloc_init_flags(flags);
>> +    }
>> +    if (strcmp(arch, "ppc64") == 0) {
>> +        return ppc64_alloc_init_flags(flags);
>> +    }
>> +    return NULL;
>> +}
>> +
>> +void machine_alloc_uninit(QGuestAllocator *allocator)
>> +{
>> +    const char *arch = qtest_get_arch();
>> +
>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +        pc_alloc_uninit(allocator);
>> +        return;
>> +    }
>> +    if (strcmp(arch, "ppc64") == 0) {
>> +        ppc64_alloc_uninit(allocator);
>> +    }
>> +}
>> diff --git a/tests/libqos/malloc.h b/tests/libqos/malloc.h
>> index ae9dac8..a5f4c63 100644
>> --- a/tests/libqos/malloc.h
>> +++ b/tests/libqos/malloc.h
>> @@ -37,4 +37,7 @@ QGuestAllocator *alloc_init_flags(QAllocOpts flags,
>>  void alloc_set_page_size(QGuestAllocator *allocator, size_t page_size);
>>  void alloc_set_flags(QGuestAllocator *allocator, QAllocOpts opts);
>>  
>> +QGuestAllocator *machine_alloc_init(void);
>> +QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags);
>> +void machine_alloc_uninit(QGuestAllocator *allocator);
>>  #endif
> 
Thanks,
Laurent

Greg Kurz Sept. 9, 2016, 12:25 p.m. UTC | #9

On Thu, 8 Sep 2016 09:50:31 +0200
Laurent Vivier <[e-mail]> wrote:

> On 08/09/2016 04:04, David Gibson wrote:
> > On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:  
> >> And add support for ppc64.
> >>
> >> Signed-off-by: Laurent Vivier <[e-mail]>  
> > 
> > Some of my coments may be obsoleted by the discussion with Greg.
> >   
> >> ---
> >> v2:
> >> - remove useless parenthesis, inline
> [...]
> >> +
> >> +QGuestAllocator *machine_alloc_init(void)
> >> +{
> >> +    const char *arch = qtest_get_arch();  
> > 
> > Maybe we need to add a qtest_get_machine_type().  
> 
> I'm working on that...
> 

The problem is that qtest only knows about archs, based on $(TARGETS).
Maybe the machine type could be the default one for a given arch ?

Cheers.

--
Greg

Laurent Vivier Sept. 9, 2016, 12:31 p.m. UTC | #10

On 09/09/2016 14:25, Greg Kurz wrote:
> On Thu, 8 Sep 2016 09:50:31 +0200
> Laurent Vivier <[e-mail]> wrote:
> 
>> On 08/09/2016 04:04, David Gibson wrote:
>>> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:  
>>>> And add support for ppc64.
>>>>
>>>> Signed-off-by: Laurent Vivier <[e-mail]>  
>>>
>>> Some of my coments may be obsoleted by the discussion with Greg.
>>>   
>>>> ---
>>>> v2:
>>>> - remove useless parenthesis, inline
>> [...]
>>>> +
>>>> +QGuestAllocator *machine_alloc_init(void)
>>>> +{
>>>> +    const char *arch = qtest_get_arch();  
>>>
>>> Maybe we need to add a qtest_get_machine_type().  
>>
>> I'm working on that...
>>
> 
> The problem is that qtest only knows about archs, based on $(TARGETS).
> Maybe the machine type could be the default one for a given arch ?

Once the machine is started we can use QMP[1] to ask the machine type
(for instance, "pseries-2.7-machine").

So what we could do is a generic qtest_machine_vboot() which ask the
machine type and configure the qtest framework accordingly.

Laurent
[1] { 'execute': 'qom-get', 'arguments': { 'path': '/machine',
'property': 'type' } }

David Gibson Sept. 12, 2016, 1:27 a.m. UTC | #11

On Fri, Sep 09, 2016 at 02:31:55PM +0200, Laurent Vivier wrote:
> 
> 
> On 09/09/2016 14:25, Greg Kurz wrote:
> > On Thu, 8 Sep 2016 09:50:31 +0200
> > Laurent Vivier <[e-mail]> wrote:
> > 
> >> On 08/09/2016 04:04, David Gibson wrote:
> >>> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:  
> >>>> And add support for ppc64.
> >>>>
> >>>> Signed-off-by: Laurent Vivier <[e-mail]>  
> >>>
> >>> Some of my coments may be obsoleted by the discussion with Greg.
> >>>   
> >>>> ---
> >>>> v2:
> >>>> - remove useless parenthesis, inline
> >> [...]
> >>>> +
> >>>> +QGuestAllocator *machine_alloc_init(void)
> >>>> +{
> >>>> +    const char *arch = qtest_get_arch();  
> >>>
> >>> Maybe we need to add a qtest_get_machine_type().  
> >>
> >> I'm working on that...
> >>
> > 
> > The problem is that qtest only knows about archs, based on $(TARGETS).
> > Maybe the machine type could be the default one for a given arch ?
> 
> Once the machine is started we can use QMP[1] to ask the machine type
> (for instance, "pseries-2.7-machine").
> 
> So what we could do is a generic qtest_machine_vboot() which ask the
> machine type and configure the qtest framework accordingly.
> 
> Laurent
> [1] { 'execute': 'qom-get', 'arguments': { 'path': '/machine',
> 'property': 'type' } }

Ok.. doesn't the qtest framework start the machine though?  So it
should already know the machine type, shouldn't it?

Greg Kurz Sept. 12, 2016, 8:27 a.m. UTC | #12

On Mon, 12 Sep 2016 11:27:57 +1000
David Gibson <[e-mail]> wrote:

> On Fri, Sep 09, 2016 at 02:31:55PM +0200, Laurent Vivier wrote:
> > 
> > 
> > On 09/09/2016 14:25, Greg Kurz wrote:  
> > > On Thu, 8 Sep 2016 09:50:31 +0200
> > > Laurent Vivier <[e-mail]> wrote:
> > >   
> > >> On 08/09/2016 04:04, David Gibson wrote:  
> > >>> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:    
> > >>>> And add support for ppc64.
> > >>>>
> > >>>> Signed-off-by: Laurent Vivier <[e-mail]>    
> > >>>
> > >>> Some of my coments may be obsoleted by the discussion with Greg.
> > >>>     
> > >>>> ---
> > >>>> v2:
> > >>>> - remove useless parenthesis, inline  
> > >> [...]  
> > >>>> +
> > >>>> +QGuestAllocator *machine_alloc_init(void)
> > >>>> +{
> > >>>> +    const char *arch = qtest_get_arch();    
> > >>>
> > >>> Maybe we need to add a qtest_get_machine_type().    
> > >>
> > >> I'm working on that...
> > >>  
> > > 
> > > The problem is that qtest only knows about archs, based on $(TARGETS).
> > > Maybe the machine type could be the default one for a given arch ?  
> > 
> > Once the machine is started we can use QMP[1] to ask the machine type
> > (for instance, "pseries-2.7-machine").
> > 
> > So what we could do is a generic qtest_machine_vboot() which ask the
> > machine type and configure the qtest framework accordingly.
> > 
> > Laurent
> > [1] { 'execute': 'qom-get', 'arguments': { 'path': '/machine',
> > 'property': 'type' } }  
> 
> Ok.. doesn't the qtest framework start the machine though?  So it
> should already know the machine type, shouldn't it?
> 

As already mentioned , qtest only knows about archs... the machine type is
either explicitly passed to qtest_start(), either the default one for a
given arch, if any.

For some generic code that would have to choose the right memory allocation
implementation based on the machine type, Laurent's suggestion seems to be
appropriate indeed.

Cheers.

--
Greg

Laurent Vivier Sept. 12, 2016, 3:46 p.m. UTC | #13

On 12/09/2016 03:27, David Gibson wrote:
> On Fri, Sep 09, 2016 at 02:31:55PM +0200, Laurent Vivier wrote:
>>
>>
>> On 09/09/2016 14:25, Greg Kurz wrote:
>>> On Thu, 8 Sep 2016 09:50:31 +0200
>>> Laurent Vivier <[e-mail]> wrote:
>>>
>>>> On 08/09/2016 04:04, David Gibson wrote:
>>>>> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:  
>>>>>> And add support for ppc64.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <[e-mail]>  
>>>>>
>>>>> Some of my coments may be obsoleted by the discussion with Greg.
>>>>>   
>>>>>> ---
>>>>>> v2:
>>>>>> - remove useless parenthesis, inline
>>>> [...]
>>>>>> +
>>>>>> +QGuestAllocator *machine_alloc_init(void)
>>>>>> +{
>>>>>> +    const char *arch = qtest_get_arch();  
>>>>>
>>>>> Maybe we need to add a qtest_get_machine_type().  
>>>>
>>>> I'm working on that...
>>>>
>>>
>>> The problem is that qtest only knows about archs, based on $(TARGETS).
>>> Maybe the machine type could be the default one for a given arch ?
>>
>> Once the machine is started we can use QMP[1] to ask the machine type
>> (for instance, "pseries-2.7-machine").
>>
>> So what we could do is a generic qtest_machine_vboot() which ask the
>> machine type and configure the qtest framework accordingly.
>>
>> Laurent
>> [1] { 'execute': 'qom-get', 'arguments': { 'path': '/machine',
>> 'property': 'type' } }
> 
> Ok.. doesn't the qtest framework start the machine though?  So it
> should already know the machine type, shouldn't it?

In fact qtest starts the machine with the content of QTEST_QEMU_BINARY
and we don't provide "-machine" parameter, so it doesn't know the
machine type. It can guess it according to the machine arch, and by
default, ppc64 (arch) is pseries (machine).

Perhaps we can add "-machine pseries" in qtest_spapr_vboot().

Laurent
Laurent

Greg Kurz Sept. 12, 2016, 5:37 p.m. UTC | #14

On Mon, 12 Sep 2016 17:46:35 +0200
Laurent Vivier <[e-mail]> wrote:

> On 12/09/2016 03:27, David Gibson wrote:
> > On Fri, Sep 09, 2016 at 02:31:55PM +0200, Laurent Vivier wrote:  
> >>
> >>
> >> On 09/09/2016 14:25, Greg Kurz wrote:  
> >>> On Thu, 8 Sep 2016 09:50:31 +0200
> >>> Laurent Vivier <[e-mail]> wrote:
> >>>  
> >>>> On 08/09/2016 04:04, David Gibson wrote:  
> >>>>> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:    
> >>>>>> And add support for ppc64.
> >>>>>>
> >>>>>> Signed-off-by: Laurent Vivier <[e-mail]>    
> >>>>>
> >>>>> Some of my coments may be obsoleted by the discussion with Greg.
> >>>>>     
> >>>>>> ---
> >>>>>> v2:
> >>>>>> - remove useless parenthesis, inline  
> >>>> [...]  
> >>>>>> +
> >>>>>> +QGuestAllocator *machine_alloc_init(void)
> >>>>>> +{
> >>>>>> +    const char *arch = qtest_get_arch();    
> >>>>>
> >>>>> Maybe we need to add a qtest_get_machine_type().    
> >>>>
> >>>> I'm working on that...
> >>>>  
> >>>
> >>> The problem is that qtest only knows about archs, based on $(TARGETS).
> >>> Maybe the machine type could be the default one for a given arch ?  
> >>
> >> Once the machine is started we can use QMP[1] to ask the machine type
> >> (for instance, "pseries-2.7-machine").
> >>
> >> So what we could do is a generic qtest_machine_vboot() which ask the
> >> machine type and configure the qtest framework accordingly.
> >>
> >> Laurent
> >> [1] { 'execute': 'qom-get', 'arguments': { 'path': '/machine',
> >> 'property': 'type' } }  
> > 
> > Ok.. doesn't the qtest framework start the machine though?  So it
> > should already know the machine type, shouldn't it?  
> 
> In fact qtest starts the machine with the content of QTEST_QEMU_BINARY
> and we don't provide "-machine" parameter, so it doesn't know the
> machine type. It can guess it according to the machine arch, and by
> default, ppc64 (arch) is pseries (machine).
> 
> Perhaps we can add "-machine pseries" in qtest_spapr_vboot().
> 

We may want to test something that is only available for a given
version (like CPU hotplug for pseries >= 2.7 for example)... it
could *work* if we can pass "-machine pseries -machine pseries-2.7"
and the latter prevails, but that looks a bit like a hack.

Cheers.

--
Greg

> Laurent
> Laurent

Patch

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 14be491..a286848 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -557,8 +557,9 @@  tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
 
 libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
 libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
+libqos-obj-y += tests/libqos/malloc-ppc64.o tests/libqos/malloc-pc.o
 libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
-libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
+libqos-pc-obj-y += tests/libqos/libqos-pc.o
 libqos-pc-obj-y += tests/libqos/ahci.o
 libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
 libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
index 604980d..7b71607 100644
--- a/tests/libqos/libqos.h
+++ b/tests/libqos/libqos.h
@@ -3,7 +3,7 @@ 
 
 #include "libqtest.h"
 #include "libqos/pci.h"
-#include "libqos/malloc-pc.h"
+#include "libqos/malloc.h"
 
 typedef struct QOSOps {
     QGuestAllocator *(*init_allocator)(QAllocOpts);
diff --git a/tests/libqos/malloc-ppc64.c b/tests/libqos/malloc-ppc64.c
new file mode 100644
index 0000000..1b31e33
--- /dev/null
+++ b/tests/libqos/malloc-ppc64.c
@@ -0,0 +1,38 @@ 
+/*
+ * libqos malloc support for PPC64
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqos/malloc-ppc64.h"
+
+#include "qemu-common.h"
+
+#define PAGE_SIZE 4096
+
+/* Memory must be a multiple of 256 MB,
+ * so we have at least 256MB
+ */
+#define PPC64_MIN_SIZE 0x10000000
+
+void ppc64_alloc_uninit(QGuestAllocator *allocator)
+{
+    alloc_uninit(allocator);
+}
+
+QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags)
+{
+    QGuestAllocator *s;
+
+    s = alloc_init_flags(flags, 1 << 20, PPC64_MIN_SIZE);
+    alloc_set_page_size(s, PAGE_SIZE);
+
+    return s;
+}
+
+QGuestAllocator *ppc64_alloc_init(void)
+{
+    return ppc64_alloc_init_flags(ALLOC_NO_FLAGS);
+}
diff --git a/tests/libqos/malloc-ppc64.h b/tests/libqos/malloc-ppc64.h
new file mode 100644
index 0000000..c2b2dff
--- /dev/null
+++ b/tests/libqos/malloc-ppc64.h
@@ -0,0 +1,17 @@ 
+/*
+ * libqos malloc support for PPC64
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef LIBQOS_MALLOC_PPC64_H
+#define LIBQOS_MALLOC_PPC64_H
+
+#include "libqos/malloc.h"
+
+QGuestAllocator *ppc64_alloc_init(void);
+QGuestAllocator *ppc64_alloc_init_flags(QAllocOpts flags);
+void ppc64_alloc_uninit(QGuestAllocator *allocator);
+
+#endif
diff --git a/tests/libqos/malloc.c b/tests/libqos/malloc.c
index b8eff5f..6a02345 100644
--- a/tests/libqos/malloc.c
+++ b/tests/libqos/malloc.c
@@ -12,6 +12,9 @@ 
 
 #include "qemu/osdep.h"
 #include "libqos/malloc.h"
+#include "libqos/malloc-pc.h"
+#include "libqos/malloc-ppc64.h"
+#include "libqtest.h"
 #include "qemu-common.h"
 #include "qemu/host-utils.h"
 
@@ -375,3 +378,42 @@  void migrate_allocator(QGuestAllocator *src,
     QTAILQ_INSERT_HEAD(src->free, node, MLIST_ENTNAME);
     return;
 }
+
+QGuestAllocator *machine_alloc_init(void)
+{
+    const char *arch = qtest_get_arch();
+
+    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        return pc_alloc_init();
+    }
+    if (strcmp(arch, "ppc64") == 0) {
+        return ppc64_alloc_init();
+    }
+    return NULL;
+}
+
+QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags)
+{
+    const char *arch = qtest_get_arch();
+
+    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        return pc_alloc_init_flags(flags);
+    }
+    if (strcmp(arch, "ppc64") == 0) {
+        return ppc64_alloc_init_flags(flags);
+    }
+    return NULL;
+}
+
+void machine_alloc_uninit(QGuestAllocator *allocator)
+{
+    const char *arch = qtest_get_arch();
+
+    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        pc_alloc_uninit(allocator);
+        return;
+    }
+    if (strcmp(arch, "ppc64") == 0) {
+        ppc64_alloc_uninit(allocator);
+    }
+}
diff --git a/tests/libqos/malloc.h b/tests/libqos/malloc.h
index ae9dac8..a5f4c63 100644
--- a/tests/libqos/malloc.h
+++ b/tests/libqos/malloc.h
@@ -37,4 +37,7 @@  QGuestAllocator *alloc_init_flags(QAllocOpts flags,
 void alloc_set_page_size(QGuestAllocator *allocator, size_t page_size);
 void alloc_set_flags(QGuestAllocator *allocator, QAllocOpts opts);
 
+QGuestAllocator *machine_alloc_init(void);
+QGuestAllocator *machine_alloc_init_flags(QAllocOpts flags);
+void machine_alloc_uninit(QGuestAllocator *allocator);
 #endif