Re: [PATCH v15 4/4] qtest/cxl: Add aarch64 virt test for CXL

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 13 Jun 2025 13:32:03 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 12 Jun 2025 at 14:45, Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Add a single complex case for aarch64 virt machine.
> > Given existing much more comprehensive tests for x86 cover the
> > common functionality, a single test should be enough to verify
> > that the aarch64 part continue to work.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > ---
> > v15: Dropped tags due to changes in patches 2 and 3.
> > ---
> >  tests/qtest/cxl-test.c  | 59 ++++++++++++++++++++++++++++++++---------
> >  tests/qtest/meson.build |  1 +
> >  2 files changed, 47 insertions(+), 13 deletions(-)
> >
> > diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
> > index a600331843..c7189d6222 100644
> > --- a/tests/qtest/cxl-test.c
> > +++ b/tests/qtest/cxl-test.c
> > @@ -19,6 +19,12 @@
> >      "-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=53 " \
> >      "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=4G "
> >
> > +#define QEMU_VIRT_2PXB_CMD \
> > +    "-machine virt,cxl=on -cpu max " \
> > +    "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 " \
> > +    "-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=53 " \
> > +    "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=4G "
> > +
> >  #define QEMU_RP \
> >      "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 "
> >
> > @@ -197,25 +203,52 @@ static void cxl_2pxb_4rp_4t3d(void)
> >      qtest_end();
> >      rmdir(tmpfs);
> >  }
> > +
> > +static void cxl_virt_2pxb_4rp_4t3d(void)
> > +{
> > +    g_autoptr(GString) cmdline = g_string_new(NULL);
> > +    char template[] = "/tmp/cxl-test-XXXXXX";
> > +    const char *tmpfs;
> > +
> > +    tmpfs = mkdtemp(template);  
> 
> We prefer g_mkdtemp() or g_dir_make_temp() over raw mkdtemp(),
> I think. Other tests in this file use g_dir_make_tmp().

Ah. I'd failed to update this when various people cleaned up the other
tests.  Your comments reflect that earlier cleanup so I'll update
this to match the other tests.

> 
> Also you aren't checking whether it failed.
> 
> > +
> > +    g_string_printf(cmdline, QEMU_VIRT_2PXB_CMD QEMU_4RP QEMU_4T3D,
> > +                    tmpfs, tmpfs, tmpfs, tmpfs, tmpfs, tmpfs,
> > +                    tmpfs, tmpfs);
> > +
> > +    qtest_start(cmdline->str);  
> 
> We never change the GString and only use its C representation,
> so I think it's simpler to use
>   g_autofree char *cmdline = NULL;
>   ...
>   cmdline = g_strdup_printf(...);
> 
> But I see all the other tests in this file are written this
> way, so I'm ok with staying consistent to that here.

I'll stick to matching the others.

> 
> -- PMM