Re: [PATCH] hw/ppc: Improve build for PPC VFIO

[Cédric Le Goater]

On 11/23/23 11:19, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
>
> On 23/11/23 08:33, Cédric Le Goater wrote:
> > On 11/23/23 07:01, Zhenzhong Duan wrote:
> > > VFIO is not a required subsystem for the pseries machine but it's
> > > force enabled currently. When --without-default-devices is used
> > > to drop some default devices including vfio-pci, vfio core code
> > > is still kept which is unnecessary.
> > >
> > > Introduce a stub file to hold stub functions of VFIO EEH hooks,
> > > then vfio core could be compiled out.
> > >
> > > Suggested-by: Cédric Le Goater <clg@redhat.com>
> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> >
> >
> > Nick,
> >
> > I will take this patch through the vfio tree if that's OK for you.
> >
> > > ---
> > > Based on vfio-next/vfio-8.2
> > >
> > >   hw/ppc/spapr_pci_vfio_stub.c | 33 +++++++++++++++++++++++++++++++++
> > >   hw/ppc/Kconfig               |  2 +-
> > >   hw/ppc/meson.build           |  6 +++---
> > >   3 files changed, 37 insertions(+), 4 deletions(-)
> > >   create mode 100644 hw/ppc/spapr_pci_vfio_stub.c
>
> We are trying to remove stubs: instead of checking late in the callee,
> we shouldn't let the caller call functions depending on an unavailable
> feature. So I'm a bit reluctant with this patch.
>
> Can we add a simple 'bool pci_vfio_available(void);' helper? Or rework a
> bit. For example looking quickly, we already have:
>
>      #ifdef CONFIG_LINUX
>      int spapr_phb_vfio_eeh_set_option(SpaprPhbState *sphb,
>      #else
>      static inline bool spapr_phb_eeh_available(SpaprPhbState *sphb)
>      {
>          return false;
>      }
>      #endif
>
> This should be enough to protect the other calls.

The problem is that CONFIG_VFIO_PCI is not a target option and you can't
use the define as we do with CONFIG_LINUX. The define poisoning does its
job there.


> Maybe we just need:> 
> -- >8 --
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -122,41 +122,20 @@ int spapr_pci_dt_populate(SpaprDrc *drc, 
> SpaprMachineState *spapr,
>                             void *fdt, int *fdt_start_offset, Error **errp);
>
>   /* VFIO EEH hooks */
> -#ifdef CONFIG_LINUX
> +#if defined(CONFIG_LINUX) && defined(CONFIG_VFIO_PCI)
>   bool spapr_phb_eeh_available(SpaprPhbState *sphb);
> +#else
> +static inline bool spapr_phb_eeh_available(SpaprPhbState *sphb)
> +{
> +    return false;
> +}
> +#endif
>   int spapr_phb_vfio_eeh_set_option(SpaprPhbState *sphb,
>                                     unsigned int addr, int option);
>   int spapr_phb_vfio_eeh_get_state(SpaprPhbState *sphb, int *state);
>   int spapr_phb_vfio_eeh_reset(SpaprPhbState *sphb, int option);
>   int spapr_phb_vfio_eeh_configure(SpaprPhbState *sphb);
>   void spapr_phb_vfio_reset(DeviceState *qdev);
> -#else
> -static inline bool spapr_phb_eeh_available(SpaprPhbState *sphb)
> -{
> -    return false;
> -}
> -static inline int spapr_phb_vfio_eeh_set_option(SpaprPhbState *sphb,
> -                                                unsigned int addr, int option)
> -{
> -    return RTAS_OUT_HW_ERROR;
> -}
> -static inline int spapr_phb_vfio_eeh_get_state(SpaprPhbState *sphb,
> -                                               int *state)
> -{
> -    return RTAS_OUT_HW_ERROR;
> -}
> -static inline int spapr_phb_vfio_eeh_reset(SpaprPhbState *sphb, int option)
> -{
> -    return RTAS_OUT_HW_ERROR;
> -}
> -static inline int spapr_phb_vfio_eeh_configure(SpaprPhbState *sphb)
> -{
> -    return RTAS_OUT_HW_ERROR;
> -}
> -static inline void spapr_phb_vfio_reset(DeviceState *qdev)
> -{
> -}
> -#endif
>
>   void spapr_phb_dma_reset(SpaprPhbState *sphb);
> ---
>
> and massage a bit the calls not protected by spapr_phb_eeh_available().

How about what's below  instead ?


Thanks,

C.



--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -26,10 +26,12 @@
 #include "hw/pci/pci_device.h"
 #include "hw/vfio/vfio-common.h"
 #include "qemu/error-report.h"
+#include CONFIG_DEVICES /* CONFIG_VFIO_PCI */
 
 /*
  * Interfaces for IBM EEH (Enhanced Error Handling)
  */
+#ifdef CONFIG_VFIO_PCI
 static bool vfio_eeh_container_ok(VFIOContainer *container)
 {
     /*
@@ -314,3 +316,37 @@ int spapr_phb_vfio_eeh_configure(SpaprPhbState *sphb)
 
     return RTAS_OUT_SUCCESS;
 }
+
+#else
+
+bool spapr_phb_eeh_available(SpaprPhbState *sphb)
+{
+    return false;
+}
+
+void spapr_phb_vfio_reset(DeviceState *qdev)
+{
+}
+
+int spapr_phb_vfio_eeh_set_option(SpaprPhbState *sphb,
+                                  unsigned int addr, int option)
+{
+    return RTAS_OUT_NOT_SUPPORTED;
+}
+
+int spapr_phb_vfio_eeh_get_state(SpaprPhbState *sphb, int *state)
+{
+    return RTAS_OUT_NOT_SUPPORTED;
+}
+
+int spapr_phb_vfio_eeh_reset(SpaprPhbState *sphb, int option)
+{
+    return RTAS_OUT_NOT_SUPPORTED;
+}
+
+int spapr_phb_vfio_eeh_configure(SpaprPhbState *sphb)
+{
+    return RTAS_OUT_NOT_SUPPORTED;
+}
+
+#endif /* CONFIG_VFIO_PCI */