Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Wed, May 28, 2014 at 03:12:35PM +0200, Alexander Graf wrote:
>
>On 28.05.14 14:49, Gavin Shan wrote:
>>On Wed, May 28, 2014 at 01:41:35PM +0200, Alexander Graf wrote:
>>>On 28.05.14 02:55, Gavin Shan wrote:
>>>>On Tue, May 27, 2014 at 12:15:27PM -0600, Alex Williamson wrote:
>>>>>On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>>>>>>The patch adds new IOCTL commands for sPAPR VFIO container device
>>>>>>to support EEH functionality for PCI devices, which have been passed
>>>>>>through from host to somebody else via VFIO.
>>>>>>
>>>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>>>---
>>>>>>  Documentation/vfio.txt              | 92 ++++++++++++++++++++++++++++++++++++-
>>>>>>  drivers/vfio/pci/Makefile           |  1 +
>>>>>>  drivers/vfio/pci/vfio_pci.c         | 20 +++++---
>>>>>>  drivers/vfio/pci/vfio_pci_eeh.c     | 46 +++++++++++++++++++
>>>>>>  drivers/vfio/pci/vfio_pci_private.h |  5 ++
>>>>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>>>>>>  include/uapi/linux/vfio.h           | 66 ++++++++++++++++++++++++++
>>>>>>  7 files changed, 308 insertions(+), 7 deletions(-)
>>>>>>  create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>>>[...]
>>>
>>>>>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>>>>index cb9023d..c5fac36 100644
>>>>>>--- a/include/uapi/linux/vfio.h
>>>>>>+++ b/include/uapi/linux/vfio.h
>>>>>>@@ -455,6 +455,72 @@ struct vfio_iommu_spapr_tce_info {
>>>>>>  #define VFIO_IOMMU_SPAPR_TCE_GET_INFO	_IO(VFIO_TYPE, VFIO_BASE + 12)
>>>>>>+/*
>>>>>>+ * EEH functionality can be enabled or disabled on one specific device.
>>>>>>+ * Also, the DMA or IO frozen state can be removed from the frozen PE
>>>>>>+ * if required.
>>>>>>+ */
>>>>>>+struct vfio_eeh_pe_set_option {
>>>>>>+	__u32 argsz;
>>>>>>+	__u32 flags;
>>>>>>+	__u32 option;
>>>>>>+#define VFIO_EEH_PE_SET_OPT_DISABLE	0	/* Disable EEH	*/
>>>>>>+#define VFIO_EEH_PE_SET_OPT_ENABLE	1	/* Enable EEH	*/
>>>>>>+#define VFIO_EEH_PE_SET_OPT_IO		2	/* Enable IO	*/
>>>>>>+#define VFIO_EEH_PE_SET_OPT_DMA		3	/* Enable DMA	*/
>>>>>This is more of a "command" than an "option" isn't it?  Each of these
>>>>>probably needs a more significant description.
>>>>>
>>>>Yeah, it would be regarded as "opcode" and I'll add more description about
>>>>them in next revision.
>>>Please just call them commands.
>>>
>>Ok. I guess you want me to change the macro names like this ?
>>
>>#define VFIO_EEH_CMD_DISABLE	0	/* Disable EEH functionality	*/
>>#define VFIO_EEH_CMD_ENABLE	1	/* Enable EEH functionality	*/
>>#define VFIO_EEH_CMD_ENABLE_IO	2	/* Enable IO for frozen PE	*/
>>#define VFIO_EEH_CMD_ENABLE_DMA	3	/* Enable DMA for frozen PE	*/
>
>Yes, the ioctl name too.
>

Ok. Thanks. I will also to rename those "option" / "command" related macros
to "VFIO_EEH_CMD_xxxx" in next revision.

>>
>>>>>>+};
>>>>>>+
>>>>>>+#define VFIO_EEH_PE_SET_OPTION		_IO(VFIO_TYPE, VFIO_BASE + 21)
>>>>>>+
>>>>>>+/*
>>>>>>+ * Each EEH PE should have unique address to be identified. PE's
>>>>>>+ * sharing mode is also useful information as well.
>>>>>>+ */
>>>>>>+#define VFIO_EEH_PE_GET_ADDRESS		0	/* Get address	*/
>>>>>>+#define VFIO_EEH_PE_GET_MODE		1	/* Query mode	*/
>>>>>>+#define VFIO_EEH_PE_MODE_NONE		0	/* Not a PE	*/
>>>>>>+#define VFIO_EEH_PE_MODE_NOT_SHARED	1	/* Exclusive	*/
>>>>>>+#define VFIO_EEH_PE_MODE_SHARED		2	/* Shared mode	*/
>>>>>>+
>>>>>>+/*
>>>>>>+ * EEH PE might have been frozen because of PCI errors. Also, it might
>>>>>>+ * be experiencing reset for error revoery. The following command helps
>>>>>>+ * to get the state.
>>>>>>+ */
>>>>>>+struct vfio_eeh_pe_get_state {
>>>>>>+	__u32 argsz;
>>>>>>+	__u32 flags;
>>>>>>+	__u32 state;
>>>>>>+};
>>>>>Should state be a union to better describe the value returned?  What
>>>>>exactly is the address and why does the user need to know it?  Does this
>>>>>need user input or could we just return the address and mode regardless?
>>>>>
>>>>Ok. I think you want enum (not union) for state. I'll have macros for the
>>>>state in next revision as I did that for other cases.
>>>>
>>>>Those macros defined for "address" just for ABI stuff as Alex.G mentioned.
>>>>There isn't corresponding ioctl command for host to get address any more
>>>>because QEMU (user) will have to figure it out by himself. The "address"
>>>>here means PE address and user has to figure it out according to PE
>>>>segmentation.
>>>Why would the user ever need the address?
>>>
>>I will remove those "address" related macros in next revision because it's
>>user-level bussiness, not related to host kernel any more.
>
>Ok, so the next question is whether there will be any state outside
>of GET_MODE left in the future.
>

That's also user-level business and those macros should be removed as well :-)

Thanks,
Gavin