Re: [Qemu-devel] [PATCH 7/8] tests: Use qpci_mem{read, write} in ivshmem-test

2016-10-18 Thread David Gibson
On Tue, Oct 18, 2016 at 06:14:09PM +0200, Laurent Vivier wrote:
> 
> 
> On 18/10/2016 12:52, David Gibson wrote:
> > ivshmem implements a block of shared memory in a PCI BAR.  Currently our
> > test case accesses this using qtest_mem{read,write}.  However, deducing
> > the correct addresses for these requires making assumptions about the
> > internel format returned by qpci_iomap(), along with some ugly casts.
> > 
> > This patch changes the test to use the new qpci_mem{read,write} interfaces
> > which is neater.
> > 
> > Signed-off-by: David Gibson 
> 
> I think your changes break this test.
> 
> There is two guest, so we need to access them through two sockets,
> defined in s1->qtest and s2->qtest.
> 
> But qpci_memXXX() always use memread(...) and thus
> qtest_memread(global_qtest, ...) , so always test the last guest started.
> 
> You must swap global_qtest as it is done in in_reg()/out_reg() before
> your qpci_memXXX() in test_ivshmem_pair().

Ah.. good point.  I'll fix that in the next spin.

> 
> Laurent
> 
> > ---
> >  tests/ivshmem-test.c | 16 
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> > index f36bfe7..97a887e 100644
> > --- a/tests/ivshmem-test.c
> > +++ b/tests/ivshmem-test.c
> > @@ -169,7 +169,7 @@ static void test_ivshmem_single(void)
> >  for (i = 0; i < G_N_ELEMENTS(data); i++) {
> >  data[i] = i;
> >  }
> > -qtest_memwrite(s->qtest, (uintptr_t)s->mem_base, data, sizeof(data));
> > +qpci_memwrite(s->dev, s->mem_base, data, sizeof(data));
> >  
> >  /* verify write */
> >  for (i = 0; i < G_N_ELEMENTS(data); i++) {
> > @@ -178,7 +178,7 @@ static void test_ivshmem_single(void)
> >  
> >  /* read it back and verify read */
> >  memset(data, 0, sizeof(data));
> > -qtest_memread(s->qtest, (uintptr_t)s->mem_base, data, sizeof(data));
> > +qpci_memread(s->dev, s->mem_base, data, sizeof(data));
> >  for (i = 0; i < G_N_ELEMENTS(data); i++) {
> >  g_assert_cmpuint(data[i], ==, i);
> >  }
> > @@ -201,29 +201,29 @@ static void test_ivshmem_pair(void)
> >  
> >  /* host write, guest 1 & 2 read */
> >  memset(tmpshmem, 0x42, TMPSHMSIZE);
> > -qtest_memread(s1->qtest, (uintptr_t)s1->mem_base, data, TMPSHMSIZE);
> > +qpci_memread(s1->dev, s1->mem_base, data, TMPSHMSIZE);
> >  for (i = 0; i < TMPSHMSIZE; i++) {
> >  g_assert_cmpuint(data[i], ==, 0x42);
> >  }
> > -qtest_memread(s2->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> > +qpci_memread(s2->dev, s2->mem_base, data, TMPSHMSIZE);
> >  for (i = 0; i < TMPSHMSIZE; i++) {
> >  g_assert_cmpuint(data[i], ==, 0x42);
> >  }
> >  
> >  /* guest 1 write, guest 2 read */
> >  memset(data, 0x43, TMPSHMSIZE);
> > -qtest_memwrite(s1->qtest, (uintptr_t)s1->mem_base, data, TMPSHMSIZE);
> > +qpci_memwrite(s1->dev, s1->mem_base, data, TMPSHMSIZE);
> >  memset(data, 0, TMPSHMSIZE);
> > -qtest_memread(s2->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> > +qpci_memread(s2->dev, s2->mem_base, data, TMPSHMSIZE);
> >  for (i = 0; i < TMPSHMSIZE; i++) {
> >  g_assert_cmpuint(data[i], ==, 0x43);
> >  }
> >  
> >  /* guest 2 write, guest 1 read */
> >  memset(data, 0x44, TMPSHMSIZE);
> > -qtest_memwrite(s2->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> > +qpci_memwrite(s2->dev, s2->mem_base, data, TMPSHMSIZE);
> >  memset(data, 0, TMPSHMSIZE);
> > -qtest_memread(s1->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> > +qpci_memread(s1->dev, s2->mem_base, data, TMPSHMSIZE);
> >  for (i = 0; i < TMPSHMSIZE; i++) {
> >  g_assert_cmpuint(data[i], ==, 0x44);
> >  }
> > 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature


Re: [Qemu-devel] [PATCH 7/8] tests: Use qpci_mem{read, write} in ivshmem-test

2016-10-18 Thread Laurent Vivier


On 18/10/2016 12:52, David Gibson wrote:
> ivshmem implements a block of shared memory in a PCI BAR.  Currently our
> test case accesses this using qtest_mem{read,write}.  However, deducing
> the correct addresses for these requires making assumptions about the
> internel format returned by qpci_iomap(), along with some ugly casts.
> 
> This patch changes the test to use the new qpci_mem{read,write} interfaces
> which is neater.
> 
> Signed-off-by: David Gibson 

I think your changes break this test.

There is two guest, so we need to access them through two sockets,
defined in s1->qtest and s2->qtest.

But qpci_memXXX() always use memread(...) and thus
qtest_memread(global_qtest, ...) , so always test the last guest started.

You must swap global_qtest as it is done in in_reg()/out_reg() before
your qpci_memXXX() in test_ivshmem_pair().

Laurent

> ---
>  tests/ivshmem-test.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index f36bfe7..97a887e 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -169,7 +169,7 @@ static void test_ivshmem_single(void)
>  for (i = 0; i < G_N_ELEMENTS(data); i++) {
>  data[i] = i;
>  }
> -qtest_memwrite(s->qtest, (uintptr_t)s->mem_base, data, sizeof(data));
> +qpci_memwrite(s->dev, s->mem_base, data, sizeof(data));
>  
>  /* verify write */
>  for (i = 0; i < G_N_ELEMENTS(data); i++) {
> @@ -178,7 +178,7 @@ static void test_ivshmem_single(void)
>  
>  /* read it back and verify read */
>  memset(data, 0, sizeof(data));
> -qtest_memread(s->qtest, (uintptr_t)s->mem_base, data, sizeof(data));
> +qpci_memread(s->dev, s->mem_base, data, sizeof(data));
>  for (i = 0; i < G_N_ELEMENTS(data); i++) {
>  g_assert_cmpuint(data[i], ==, i);
>  }
> @@ -201,29 +201,29 @@ static void test_ivshmem_pair(void)
>  
>  /* host write, guest 1 & 2 read */
>  memset(tmpshmem, 0x42, TMPSHMSIZE);
> -qtest_memread(s1->qtest, (uintptr_t)s1->mem_base, data, TMPSHMSIZE);
> +qpci_memread(s1->dev, s1->mem_base, data, TMPSHMSIZE);
>  for (i = 0; i < TMPSHMSIZE; i++) {
>  g_assert_cmpuint(data[i], ==, 0x42);
>  }
> -qtest_memread(s2->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> +qpci_memread(s2->dev, s2->mem_base, data, TMPSHMSIZE);
>  for (i = 0; i < TMPSHMSIZE; i++) {
>  g_assert_cmpuint(data[i], ==, 0x42);
>  }
>  
>  /* guest 1 write, guest 2 read */
>  memset(data, 0x43, TMPSHMSIZE);
> -qtest_memwrite(s1->qtest, (uintptr_t)s1->mem_base, data, TMPSHMSIZE);
> +qpci_memwrite(s1->dev, s1->mem_base, data, TMPSHMSIZE);
>  memset(data, 0, TMPSHMSIZE);
> -qtest_memread(s2->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> +qpci_memread(s2->dev, s2->mem_base, data, TMPSHMSIZE);
>  for (i = 0; i < TMPSHMSIZE; i++) {
>  g_assert_cmpuint(data[i], ==, 0x43);
>  }
>  
>  /* guest 2 write, guest 1 read */
>  memset(data, 0x44, TMPSHMSIZE);
> -qtest_memwrite(s2->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> +qpci_memwrite(s2->dev, s2->mem_base, data, TMPSHMSIZE);
>  memset(data, 0, TMPSHMSIZE);
> -qtest_memread(s1->qtest, (uintptr_t)s2->mem_base, data, TMPSHMSIZE);
> +qpci_memread(s1->dev, s2->mem_base, data, TMPSHMSIZE);
>  for (i = 0; i < TMPSHMSIZE; i++) {
>  g_assert_cmpuint(data[i], ==, 0x44);
>  }
>