diff mbox series

[for-4.17?] test/vpci: enable by default

Message ID 20221020102706.29267-1-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series [for-4.17?] test/vpci: enable by default | expand

Commit Message

Roger Pau Monné Oct. 20, 2022, 10:27 a.m. UTC
CONFIG_HAS_PCI is not defined for the tools build, and as a result the
vpci harness would never get build.  Fix this by building it
unconditionally, there's nothing arch specific in it.

Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
While not strictly a bugfix, I think it's worth adding this change to the
release in order to always build the vpci test hardness and prevent it
from bitrotting.
---
 tools/tests/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Henry Wang Oct. 20, 2022, 10:30 a.m. UTC | #1
Hi Roger,

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Subject: [PATCH for-4.17?] test/vpci: enable by default
> 
> CONFIG_HAS_PCI is not defined for the tools build, and as a result the
> vpci harness would never get build.  Fix this by building it
> unconditionally, there's nothing arch specific in it.
> 
> Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> While not strictly a bugfix, I think it's worth adding this change to the
> release in order to always build the vpci test hardness and prevent it
> from bitrotting.

Good point.

No problem from my side, but I think you need also Anthony's opinion
as he is the toolstack maintainer. If he also likes this idea, feel free to
add my:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> ---
>  tools/tests/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/tests/Makefile b/tools/tests/Makefile
> index 33e32730c4..d99146d56a 100644
> --- a/tools/tests/Makefile
> +++ b/tools/tests/Makefile
> @@ -10,7 +10,7 @@ SUBDIRS-$(CONFIG_X86) += x86_emulator
>  endif
>  SUBDIRS-y += xenstore
>  SUBDIRS-y += depriv
> -SUBDIRS-$(CONFIG_HAS_PCI) += vpci
> +SUBDIRS-y += vpci
> 
>  .PHONY: all clean install distclean uninstall
>  all clean distclean install uninstall: %: subdirs-%
> --
> 2.37.3
Anthony PERARD Oct. 20, 2022, 1:29 p.m. UTC | #2
On Thu, Oct 20, 2022 at 10:30:26AM +0000, Henry Wang wrote:
> Hi Roger,
> 
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Subject: [PATCH for-4.17?] test/vpci: enable by default
> > 
> > CONFIG_HAS_PCI is not defined for the tools build, and as a result the
> > vpci harness would never get build.  Fix this by building it
> > unconditionally, there's nothing arch specific in it.
> > 
> > Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > While not strictly a bugfix, I think it's worth adding this change to the
> > release in order to always build the vpci test hardness and prevent it
> > from bitrotting.
> 
> Good point.
> 
> No problem from my side, but I think you need also Anthony's opinion
> as he is the toolstack maintainer.

This sounds fine to me, the risk is that the build could fail. But we
can easily revert the patch and reapply it at the next development
cycle.

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,
Andrew Cooper Oct. 21, 2022, 7:01 p.m. UTC | #3
On 20/10/2022 11:27, Roger Pau Monne wrote:
> CONFIG_HAS_PCI is not defined for the tools build, and as a result the
> vpci harness would never get build.  Fix this by building it
> unconditionally, there's nothing arch specific in it.
>
> Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> While not strictly a bugfix, I think it's worth adding this change to the
> release in order to always build the vpci test hardness and prevent it
> from bitrotting.
> ---
>  tools/tests/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/tests/Makefile b/tools/tests/Makefile
> index 33e32730c4..d99146d56a 100644
> --- a/tools/tests/Makefile
> +++ b/tools/tests/Makefile
> @@ -10,7 +10,7 @@ SUBDIRS-$(CONFIG_X86) += x86_emulator
>  endif
>  SUBDIRS-y += xenstore
>  SUBDIRS-y += depriv
> -SUBDIRS-$(CONFIG_HAS_PCI) += vpci
> +SUBDIRS-y += vpci

I'm afraid this is only half the fix.  The other half is:

diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
index 5075bc2be28c..336904958f6a 100644
--- a/tools/tests/vpci/Makefile
+++ b/tools/tests/vpci/Makefile
@@ -22,6 +22,8 @@ distclean: clean
 
 .PHONY: install
 install:
+       $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+       $(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
 
 vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c
        # Remove includes and add the test harness header

so it can actually get deployed somewhere useful.

~Andrew
Roger Pau Monné Oct. 24, 2022, 9:15 a.m. UTC | #4
On Fri, Oct 21, 2022 at 07:01:01PM +0000, Andrew Cooper wrote:
> On 20/10/2022 11:27, Roger Pau Monne wrote:
> > CONFIG_HAS_PCI is not defined for the tools build, and as a result the
> > vpci harness would never get build.  Fix this by building it
> > unconditionally, there's nothing arch specific in it.
> >
> > Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > While not strictly a bugfix, I think it's worth adding this change to the
> > release in order to always build the vpci test hardness and prevent it
> > from bitrotting.
> > ---
> >  tools/tests/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/tests/Makefile b/tools/tests/Makefile
> > index 33e32730c4..d99146d56a 100644
> > --- a/tools/tests/Makefile
> > +++ b/tools/tests/Makefile
> > @@ -10,7 +10,7 @@ SUBDIRS-$(CONFIG_X86) += x86_emulator
> >  endif
> >  SUBDIRS-y += xenstore
> >  SUBDIRS-y += depriv
> > -SUBDIRS-$(CONFIG_HAS_PCI) += vpci
> > +SUBDIRS-y += vpci
> 
> I'm afraid this is only half the fix.  The other half is:
> 
> diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
> index 5075bc2be28c..336904958f6a 100644
> --- a/tools/tests/vpci/Makefile
> +++ b/tools/tests/vpci/Makefile
> @@ -22,6 +22,8 @@ distclean: clean
>  
>  .PHONY: install
>  install:
> +       $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> +       $(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
>  
>  vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c
>         # Remove includes and add the test harness header
> 
> so it can actually get deployed somewhere useful.

For now I just wanted to get it to be built by default.  It wasn't
clear to me we want this installed, as it's a standalone unit test
that could be executed as part of the build phase (doesn't require
interaction with any hypercalls).

It's also currently built using HOSTCC, so installing on the target
would be wrong for cross-builds.

Thanks, Roger.
diff mbox series

Patch

diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 33e32730c4..d99146d56a 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -10,7 +10,7 @@  SUBDIRS-$(CONFIG_X86) += x86_emulator
 endif
 SUBDIRS-y += xenstore
 SUBDIRS-y += depriv
-SUBDIRS-$(CONFIG_HAS_PCI) += vpci
+SUBDIRS-y += vpci
 
 .PHONY: all clean install distclean uninstall
 all clean distclean install uninstall: %: subdirs-%