RE: [PATCH] hw/ppc: Improve build for PPC VFIO
>-Original Message- >From: Cédric Le Goater >Sent: Friday, November 24, 2023 3:59 PM >Subject: Re: [PATCH] hw/ppc: Improve build for PPC VFIO > >Zhenzhong, > >> How about what's below instead ? >> >> >> Thanks, >> >> C. > >I will resend the build fix with the proposal below since it addresses >Phil's concerns. Sure, appreciated😊 Thanks Zhenzhong
Re: [PATCH] hw/ppc: Improve build for PPC VFIO
Zhenzhong, How about what's below instead ? Thanks, C. I will resend the build fix with the proposal below since it addresses Phil's concerns. 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 */
Re: [PATCH] hw/ppc: Improve build for PPC VFIO
On Thu Nov 23, 2023 at 5:33 PM AEST, 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 > > Signed-off-by: Zhenzhong Duan > > > Nick, > > I will take this patch through the vfio tree if that's OK for you. That's fine, thank you. Thanks, Nick
Re: [PATCH] hw/ppc: Improve build for PPC VFIO
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 Signed-off-by: Zhenzhong Duan 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 */
Re: [PATCH] hw/ppc: Improve build for PPC VFIO
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 Signed-off-by: Zhenzhong Duan 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 00..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 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', i
RE: [PATCH] hw/ppc: Improve build for PPC VFIO
>-Original Message- >From: Cédric Le Goater >Sent: Thursday, November 23, 2023 3:33 PM >Subject: Re: [PATCH] hw/ppc: Improve build for PPC VFIO > >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 >> Signed-off-by: Zhenzhong Duan > > >Nick, > >I will take this patch through the vfio tree if that's OK for you. Sure. > >> --- >> 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 >> >> diff --git a/hw/ppc/spapr_pci_vfio_stub.c b/hw/ppc/spapr_pci_vfio_stub.c >> new file mode 100644 >> index 00..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. Oh, I see, thanks BRs. Zhenzhong > >With that, > >Reviewed-by: Cédric Le Goater > >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(
Re: [PATCH] hw/ppc: Improve build for PPC VFIO
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 Signed-off-by: Zhenzhong Duan 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 diff --git a/hw/ppc/spapr_pci_vfio_stub.c b/hw/ppc/spapr_pci_vfio_stub.c new file mode 100644 index 00..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 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(