Re: [Qemu-ppc] [PATCH qemu 1/2] spapr_pci_vfio: Remove redundant spapr-pci-vfio-host-bridge

[Alexey Kardashevskiy]

On 09/03/2015 12:05 PM, David Gibson wrote:
> On Wed, Sep 02, 2015 at 06:16:02PM +1000, Alexey Kardashevskiy wrote:
> > sPAPRTCETable is handling 2 TCE tables already:
> >
> > 1) guest view of the TCE table - emulated devices use only this table;
> >
> > 2) hardware IOMMU table - VFIO PCI devices use it for actual work but
> > it does not replace 1) and it is not visible to the guest.
> > The initialization of this table is driven by vfio-pci device,
> > DMA map/unmap requests are handled via MemoryListener so there is very
> > little to do in spapr-pci-vfio-host-bridge.
> >
> > This moves VFIO bits to the generic spapr-pci-host-bridge which allows
> > putting emulated and VFIO devices on the same PHB. It is still possible
> > to create multiple PHBs and avoid sharing PHB resouces for emulated and
> > VFIO devices.
> >
> > If there is no VFIO-PCI device attaches, no special ioctls will be called.
> > If there are some VFIO-PCI devices attached, PHB may refuse to attach
> > another VFIO-PCI device if a VFIO container on the host kernel side
> > does not support container sharing.
> >
> > This makes spapr-pci-vfio-host-bridge type equal to spapr-pci-host-bridge
> > except it has an additional "iommu" property so spapr-pci-vfio-host-bridge
> > still should be used for VFIO devices. The next patch will remove IOMMU ID
> > property and allow putting VFIO-PCI devices onto spapr-pci-host-bridge.
> >
> > This adds a number of VFIO-PCI devices currently attached to a PHB as
> > PHB needs to know whether to do DMA setup for VFIO or not. Since
> > at the moment of the PHB's realize() invocation we cannot tell yet
> > how many VFIO-PCI devices are there (they are not attached yet),
> > this moves DMA setup to the reset handler.
> >
> > This moves PCI device lookup from spapr_phb_vfio_eeh_set_option() to
> > rtas_ibm_set_eeh_option() as we need to know if the device is "vfio-pci"
> > and decide whether to call spapr_phb_vfio_eeh_set_option() or not.
> >
> > This should cause no behavioural change.
> >
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
>
> [snip]
> >   static int spapr_phb_children_reset(Object *child, void *opaque)
> >   {
> >       DeviceState *dev = (DeviceState *) object_dynamic_cast(child, 
> > TYPE_DEVICE);
> > @@ -1413,8 +1401,42 @@ static int spapr_phb_children_reset(Object *child, void 
> > *opaque)
> >
> >   static void spapr_phb_reset(DeviceState *qdev)
> >   {
> > +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
> > +    sPAPRTCETable *tcet;
> > +
> >       /* Reset the IOMMU state */
> >       object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL);
> > +
> > +    if (spapr_phb_dma_capabilities_update(sphb)) {
> > +        return;
> > +    }
> > +
> > +    /* Register default 32bit DMA window */
> > +    tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
> > +    if (!tcet) {
> > +        const unsigned nb = sphb->dma32_window_size >> SPAPR_TCE_PAGE_SHIFT;
> > +        tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
> > +                                   sphb->dma32_window_start,
> > +                                   SPAPR_TCE_PAGE_SHIFT, nb,
> > +                                   sphb->vfio_num > 0);
>
> Could delaying the construction of the TCE table object until reset
> time cause problems with migration?  i.e. can you be sure that the
> destination will have the TCE table object present and in a suitable
> state to accept the incoming table information from the source?


This is a valid concern but the PHB reset handler is called (just checked) 
when QEMU is started with "-incoming tcp:vpl2:33333" so yes, I am sure :)




--
Alexey