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: 3331 bytes --]

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.

> All those callers are implemtend in sPAPR
> platform (spapr_pci_vfio.c). I didn't put the logic there because:
> 
> - VFIOPCIDevice is invisible to sPAPR platform.

It seems kind of silly that VFIOPCIDevice isn't visible to
spapr_pci_vfio.c.

> - We can't tell one particular PCI device attached to sPAPRPHBState
>   is the basement of VFIOPCIDevice, or emulated PCI device.

I'm not sure what you mean by that.


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