Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/3] VFIO: Clear stale MSIx table during EEH reset

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5269 bytes --]

On Tue, Mar 24, 2015 at 05:24:55PM +1100, Gavin Shan wrote:
> On Tue, Mar 24, 2015 at 04:41:21PM +1100, David Gibson wrote:
> >On Mon, Mar 23, 2015 at 04:25:10PM +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 04:06:56PM +1100, David Gibson wrote:
> >> >On Fri, Mar 20, 2015 at 05:27:29PM +1100, Gavin Shan wrote:
> >> >> On Fri, Mar 20, 2015 at 05:04:01PM +1100, David Gibson wrote:
> >> >> >On Tue, Mar 17, 2015 at 03:31:24AM +1100, Gavin Shan wrote:
> >> >> >> The PCI device MSIx table is cleaned out in hardware after EEH PE
> >> >> >> reset. However, we still hold the stale MSIx entries in QEMU, which
> >> >> >> should be cleared accordingly. Otherwise, we will run into another
> >> >> >> (recursive) EEH error and the PCI devices contained in the PE have
> >> >> >> to be offlined exceptionally.
> >> >> >> 
> >> >> >> The patch clears stale MSIx table before EEH PE reset so that MSIx
> >> >> >> table could be restored properly after EEH PE reset.
> >> >> >> 
> >> >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> >> >> ---
> >> >> >> v2: vfio_container_eeh_event() stub for !CONFIG_PCI and separate
> >> >> >>     error message for this function. Dropped vfio_put_group()
> >> >> >>     on NULL group
> >> >> >> ---
> >> >> >>  hw/vfio/Makefile.objs  |  6 +++++-
> >> >> >>  hw/vfio/common.c       |  7 +++++++
> >> >> >>  hw/vfio/pci-stub.c     | 17 +++++++++++++++++
> >> >> >>  hw/vfio/pci.c          | 38 ++++++++++++++++++++++++++++++++++++++
> >> >> >>  include/hw/vfio/vfio.h |  2 ++
> >> >> >>  5 files changed, 69 insertions(+), 1 deletion(-)
> >> >> >>  create mode 100644 hw/vfio/pci-stub.c
> >> >> >> 
> >> >> >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> >> >> >> index e31f30e..1b8a065 100644
> >> >> >> --- a/hw/vfio/Makefile.objs
> >> >> >> +++ b/hw/vfio/Makefile.objs
> >> >> >> @@ -1,4 +1,8 @@
> >> >> >>  ifeq ($(CONFIG_LINUX), y)
> >> >> >>  obj-$(CONFIG_SOFTMMU) += common.o
> >> >> >> -obj-$(CONFIG_PCI) += pci.o
> >> >> >> +ifeq ($(CONFIG_PCI), y)
> >> >> >> +obj-y += pci.o
> >> >> >> +else
> >> >> >> +obj-y += pci-stub.o
> >> >> >> +endif
> >> >> >>  endif
> >> >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> >> >> index 148eb53..ed07814 100644
> >> >> >> --- a/hw/vfio/common.c
> >> >> >> +++ b/hw/vfio/common.c
> >> >> >> @@ -949,7 +949,14 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >> >> >>      switch (req) {
> >> >> >>      case VFIO_CHECK_EXTENSION:
> >> >> >>      case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
> >> >> >> +        break;
> >> >> >>      case VFIO_EEH_PE_OP:
> >> >> >> +        if (vfio_container_eeh_event(as, groupid, param) != 0) {
> >> >> >
> >> >> >I really dislike the idea of having an arbitrarily complex side effect
> >> >> >from a function whose name suggest's it's just a trivial wrapper
> >> >> >around the ioctl().
> >> >> >
> >> >> 
> >> >> Ok. I guess you would like putting the complex in the callers of
> >> >> vfio_container_ioctl().
> >> >
> >> >Well.. maybe.  I'd also be happy if helper functions were implemeneted
> >> >which both called the ioctl() and did the other necessary pieces.
> >> >They should just be called something that indicates their full
> >> >function, not a name which suggests they're just an ioctl wrapper.
> >> >
> >> 
> >> Indeed, vfio_container_ioctl() isn't indicating what the function is doing.
> >> How about renaming it to vfio_container_event_and_ioctl()? I'm always bad
> >> at giving a good function name :)
> >
> >Well, I don't think your wrapper should be multiplexed.  The multiplex
> >works for the simple ioctl() wrapper, because there really is nothing
> >that varies apart from the exact ioctl number called.
> >
> >But now that you have different operations here, I think you want
> >wrappers for each one - each one will call the ioctl(), then do the
> >specific extra steps necessary for that operation.  So
> >vfio_container_event() will go away as well, split into various other
> >functions.
> >
> 
> It wouldn't a good idea if I understand your proposal correctly. Currnetly,
> the global function vfio_container_ioctl() can be called from sPAPR platform
> for any ioctl commands handled in kernel source file vfio_iommu_spapr_tce.c,
> which means the function isn't called for EEH only. Other sPAPR TCE container
> ioctl commands are also routed by this function. There will be lots if having
> one global function for each ioctl commands, which just improve the cost to
> maintain the code.

I don't really follow your objection.  I'm only suggesting separate
wrappers for things which require extra actions currently implemented
in vfio_container_event().  Things which only ned the plain ioctl()
can still use the simple vfio_container_ioctl() wrapper.

> Alternatively, we might expose another function vfio_container_eeh_ioctl(),
> which calls vfio_container_ioctl() after doing what we did in vfio_container_event()
> if necessary.
> 
> Thanks,
> Gavin
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]