Re: [Qemu-devel] [PATCH qemu v10 09/14] spapr_vfio_pci: Remove redundant spapr-pci-vfio-host-bridge

[Thomas Huth <thuth@redhat.com>]

On Mon,  6 Jul 2015 12:11:05 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> 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 changes spapr-pci-host-bridge to support properties of
> spapr-pci-vfio-host-bridge. This makes spapr-pci-vfio-host-bridge type
> equal to spapr-pci-host-bridge except it has an additional "iommu"
> property for backward compatibility reasons.
> 
> 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.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> Changes:
> v9:
> * s'iommugroupid shall not be used'iommugroupid is deprecated and will be ignored'
> in error log
> 
> v8:
> * call spapr_phb_vfio_eeh_set_option() on vfio-pci devices only (reported by Gavin)
> ---
>  hw/ppc/spapr_pci.c          | 82 +++++++++++++++----------------------------
>  hw/ppc/spapr_pci_vfio.c     | 85 +++++++++------------------------------------
>  include/hw/pci-host/spapr.h | 25 ++++++-------
>  3 files changed, 55 insertions(+), 137 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 00816b3..76c988f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
...
> @@ -841,9 +811,8 @@ int spapr_phb_dma_reset(sPAPRPHBState *sphb)
>  {
>      int i;
>      sPAPRTCETable *tcet;
> -    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>  
> -    spc->dma_capabilities_update(sphb); /* Refresh @has_vfio status */
> +    spapr_phb_dma_capabilities_update(sphb); /* Refresh @has_vfio status */
>  
>      for (i = 0; i < SPAPR_PCI_DMA_MAX_WINDOWS; ++i) {
>          tcet = spapr_tce_find_by_liobn(SPAPR_PCI_LIOBN(sphb->index, i));
> @@ -852,8 +821,8 @@ int spapr_phb_dma_reset(sPAPRPHBState *sphb)
>          }
>      }
>  
> -    spc->dma_init_window(sphb, SPAPR_PCI_LIOBN(sphb->index, 0),
> -                         SPAPR_TCE_PAGE_SHIFT, sphb->dma32_window_size);
> +    spapr_phb_dma_init_window(sphb, SPAPR_PCI_LIOBN(sphb->index, 0),
> +                              SPAPR_TCE_PAGE_SHIFT, sphb->dma32_window_size);
>  
>      return 0;
>  }
> @@ -1271,6 +1240,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      uint64_t msi_window_size = 4096;
>      sPAPRTCETable *tcet;
>  
> +    if ((sphb->iommugroupid != -1) &&

Too many brackets for my taste...
... but apart from that, the patch looks good to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>