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

[David Gibson]

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 <address@hidden>
> >> >> ---
> >> >> 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.

-- 
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

pgpiRC7k5_X_j.pgp
Description: PGP signature