From: Alexander Graf <agraf@suse•de>
To: Alex Williamson <alex.williamson@redhat•com>
Cc: aik@ozlabs•ru, Gavin Shan <gwshan@linux•vnet.ibm.com>,
kvm-ppc@vger•kernel.org, qiudayu@linux•vnet.ibm.com,
linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
Date: Thu, 29 May 2014 00:40:26 +0200 [thread overview]
Message-ID: <538665DA.8090000@suse.de> (raw)
In-Reply-To: <1401293853.2412.36.camel@ul30vt.home>
On 28.05.14 18:17, Alex Williamson wrote:
> On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
>> On 28.05.14 02:57, Alex Williamson wrote:
>>> On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
>>>> On 28.05.14 02:39, Alex Williamson wrote:
>>>>> On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
>>>>>> On 27.05.14 20:15, 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
>>>>>> [...]
>>>>>>
>>>>>>>> +
>>>>>>>> + return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>> unsigned int cmd, unsigned long arg)
>>>>>>>> {
>>>>>>>> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>> tce_iommu_disable(container);
>>>>>>>> mutex_unlock(&container->lock);
>>>>>>>> return 0;
>>>>>>>> + case VFIO_EEH_PE_SET_OPTION:
>>>>>>>> + case VFIO_EEH_PE_GET_STATE:
>>>>>>>> + case VFIO_EEH_PE_RESET:
>>>>>>>> + case VFIO_EEH_PE_CONFIGURE:
>>>>>>>> + return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
>>>>>>> This is where it would have really made sense to have a single
>>>>>>> VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
>>>>>>> AlexG, are you really attached to splitting these out into separate
>>>>>>> ioctls?
>>>>>> I don't see the problem. We need to forward 4 ioctls to a separate piece
>>>>>> of code, so we forward 4 ioctls to a separate piece of code :). Putting
>>>>>> them into one ioctl just moves the switch() into another function.
>>>>> And uses an extra 3 ioctl numbers and gives us extra things to update if
>>>>> we ever need to add more ioctls, etc. ioctl numbers are an address
>>>>> space, how much address space do we really want to give to EEH? It's
>>>>> not a big difference, but I don't think it's completely even either.
>>>>> Thanks,
>>>> Yes, that's the point. I by far prefer to have you push back on anyone
>>>> who introduces useless ioctls rather than have a separate EEH number
>>>> space that people can just throw anything in they like ;).
>>> Well, I appreciate that, but having them as separate ioctls doesn't
>>> really prevent that either. Any one of these 4 could be set to take a
>>> sub-option to extend and contort the EEH interface. The only way to
>>> prevent that would be to avoid the argsz+flags hack that make the ioctl
>>> extendable. Thanks,
>> Sure, that's what patch review is about. I'm really more concerned about
>> whose court the number space is in - you or Gavin. If we're talking
>> about top level ioctls you will care a lot more.
>>
>> But I'm not religious about this. You're the VFIO maintainer, so it's
>> your call. I just personally cringe when I see an ioctl that gets an
>> "opcode" and a "parameter" argument where the "parameter" argument is a
>> union with one struct for each opcode.
> Well, what would it look like...
>
> struct vfio_eeh_pe_op {
> __u32 argsz;
> __u32 flags;
> __u32 op;
> };
>
> Couldn't every single one of these be a separate "op"? Are there any
> cases where we can't use the ioctl return value?
>
> VFIO_EEH_PE_DISABLE
> VFIO_EEH_PE_ENABLE
> VFIO_EEH_PE_UNFREEZE_IO
> VFIO_EEH_PE_UNFREEZE_DMA
> VFIO_EEH_PE_GET_MODE
> VFIO_EEH_PE_RESET_DEACTIVATE
> VFIO_EEH_PE_RESET_HOT
> VFIO_EEH_PE_RESET_FUNDAMENTAL
> VFIO_EEH_PE_CONFIGURE
>
> It doesn't look that bad to me, what am I missing? Thanks,
Yup, that looks well to me as well :)
Alex
next prev parent reply other threads:[~2014-05-28 22:40 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-27 8:40 [PATCH v7 0/3] EEH Support for VFIO PCI Device Gavin Shan
2014-05-27 8:40 ` [PATCH v7 1/3] powerpc/eeh: Avoid event on passed PE Gavin Shan
2014-05-27 8:40 ` [PATCH v7 2/3] powerpc/eeh: EEH support for VFIO PCI device Gavin Shan
2014-05-27 8:40 ` [PATCH v7 3/3] drivers/vfio: " Gavin Shan
2014-05-27 18:15 ` Alex Williamson
2014-05-27 20:30 ` Benjamin Herrenschmidt
2014-05-27 20:37 ` Alex Williamson
2014-05-27 20:41 ` Benjamin Herrenschmidt
2014-05-27 22:49 ` Alexander Graf
2014-05-28 0:39 ` Alex Williamson
2014-05-28 0:44 ` Alexander Graf
2014-05-28 0:57 ` Alex Williamson
2014-05-28 11:37 ` Alexander Graf
2014-05-28 16:17 ` Alex Williamson
2014-05-28 22:40 ` Alexander Graf [this message]
2014-05-28 23:37 ` Gavin Shan
2014-05-28 23:38 ` Alexander Graf
2014-05-28 23:41 ` Gavin Shan
2014-05-28 0:55 ` Gavin Shan
2014-05-28 11:41 ` Alexander Graf
2014-05-28 12:49 ` Gavin Shan
2014-05-28 13:12 ` Alexander Graf
2014-05-28 23:13 ` Gavin Shan
2014-05-28 21:58 ` Benjamin Herrenschmidt
2014-05-28 22:46 ` Alexander Graf
2014-05-28 23:18 ` Benjamin Herrenschmidt
2014-05-30 3:44 ` Alexey Kardashevskiy
2014-05-30 3:49 ` Benjamin Herrenschmidt
2014-05-28 16:32 ` Alex Williamson
2014-05-29 0:05 ` Gavin Shan
2014-05-29 0:44 ` Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=538665DA.8090000@suse.de \
--to=agraf@suse$(echo .)de \
--cc=aik@ozlabs$(echo .)ru \
--cc=alex.williamson@redhat$(echo .)com \
--cc=gwshan@linux$(echo .)vnet.ibm.com \
--cc=kvm-ppc@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=qiudayu@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox