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

[Philippe Mathieu-Daudé]

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.

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().

> > diff --git a/hw/ppc/spapr_pci_vfio_stub.c b/hw/ppc/spapr_pci_vfio_stub.c
> > new file mode 100644
> > index 0000000000..adb3fb5e35
> > --- /dev/null
> > +++ b/hw/ppc/spapr_pci_vfio_stub.c
> > @@ -0,0 +1,33 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/pci-host/spapr.h"
> > +
> > +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;
> > +}
> > +
> > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> > index edc6d2d139..b8dabdbfbe 100644
> > --- a/hw/ppc/Kconfig
> > +++ b/hw/ppc/Kconfig
> > @@ -3,11 +3,11 @@ config PSERIES
> >       imply PCI_DEVICES
> >       imply TEST_DEVICES
> >       imply VIRTIO_VGA
> > +    imply VFIO if LINUX   # needed by spapr_pci_vfio.c
>
> Zhenzhong,
>
> I changed VFIO to VFIO_PCI because PPC only supports this type
> of passthrough devices.
>
> With that,
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>
> Thanks,
>
> C.
>
>
>
> >       select NVDIMM
> >       select DIMM
> >       select PCI
> >       select SPAPR_VSCSI
> > -    select VFIO_PCI if LINUX   # needed by spapr_pci_vfio.c
> >       select XICS
> >       select XIVE
> >       select MSI_NONBROKEN
> > diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> > index ea44856d43..2df5db2eef 100644
> > --- a/hw/ppc/meson.build
> > +++ b/hw/ppc/meson.build
> > @@ -34,9 +34,9 @@ ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], 
> > if_true: files(
> >     'spapr_softmmu.c',
> >   ))
> >   ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c'))
> > -ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files(
> > -  'spapr_pci_vfio.c',
> > -))
> > +ppc_ss.add(when: [ 'CONFIG_VFIO_PCI', 'CONFIG_PSERIES', 'CONFIG_LINUX'],
> > +           if_true: files('spapr_pci_vfio.c'),
> > +           if_false: files('spapr_pci_vfio_stub.c'))
> >   # IBM PowerNV
> >   ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(