Re: [Qemu-devel] [PATCH qemu v6 11/15] spapr_pci: Do complete reset of DMA config when resetting PHB

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

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

On Sat, Apr 11, 2015 at 01:24:40AM +1000, Alexey Kardashevskiy wrote:
> On a system reset, DMA configuration has to reset too. At the moment
> it clears the table content. This is enough for the single table case
> but with DDW, we will also have to disable all DMA windows except
> the default one. Furthermore according to sPAPR, if the guest removed
> the default window and created a huge one at the same zero offset on
> a PCI bus, the reset handler has to recreate the default window with
> the default properties (2GB big, 4K pages).
> 
> This reworks SPAPR PHB code to disable the existing DMA window on reset
> and then configure and enable the default window.
> Without DDW that means that the same window will be disabled and then
> enabled with no other change in behaviour.
> 
> This changes the table creation to do it in one place in PHB (VFIO PHB
> just inherits the behaviour from PHB). The actual table allocation is
> done from the reset handler and this is where finish_realize() is called.

Looks like your commit message needs an update - you already got rid
of finish_realize() at this point in the series.

> This disables all DMA windows on a PHB reset. It does not make any
> difference now as there is just one DMA window but it will later with DDW
> patches.
> 
> spapr_phb_dma_reset() and spapr_phb_dma_remove_window() will be used
> in following patch so make it public.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/ppc/spapr_pci.c          | 45 ++++++++++++++++++++++++++++++++++++---------
>  hw/ppc/spapr_pci_vfio.c     |  6 ------
>  include/hw/pci-host/spapr.h |  3 +++
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 664687c..3d40f5b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -736,7 +736,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
>      char *namebuf;
>      int i;
>      PCIBus *bus;
> @@ -887,14 +886,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              return;
>      }
>  
> -    info->dma_capabilities_update(sphb);
> -    info->dma_init_window(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT,
> -                          sphb->dma32_window_size);
> -    tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
> -    if (!tcet) {
> -        error_setg(errp, "failed to create TCE table");
> -        return;
> -    }
>      memory_region_add_subregion(&sphb->iommu_root, 0,
>                                  spapr_tce_get_iommu(tcet));
>  
> @@ -923,6 +914,40 @@ static int spapr_phb_dma_init_window(sPAPRPHBState *sphb,
>      return 0;
>  }
>  
> +int spapr_phb_dma_remove_window(sPAPRPHBState *sphb,
> +                                sPAPRTCETable *tcet)

The only caller of this is just below, why is it not static?

> +{
> +    spapr_tce_table_disable(tcet);
> +
> +    return 0;
> +}
> +
> +static int spapr_phb_disable_dma_windows(Object *child, void *opaque)
> +{
> +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(opaque);
> +    sPAPRTCETable *tcet = (sPAPRTCETable *)
> +        object_dynamic_cast(child, TYPE_SPAPR_TCE_TABLE);
> +
> +    if (tcet) {
> +        spapr_phb_dma_remove_window(sphb, tcet);
> +    }
> +
> +    return 0;
> +}
> +
> +int spapr_phb_dma_reset(sPAPRPHBState *sphb)
> +{
> +    const uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0);
> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +
> +    spc->dma_capabilities_update(sphb); /* Refresh @has_vfio status */
> +    object_child_foreach(OBJECT(sphb), spapr_phb_disable_dma_windows, sphb);
> +    spc->dma_init_window(sphb, liobn, SPAPR_TCE_PAGE_SHIFT,
> +                         sphb->dma32_window_size);
> +
> +    return 0;
> +}
> +
>  static int spapr_phb_children_reset(Object *child, void *opaque)
>  {
>      DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
> @@ -936,6 +961,8 @@ static int spapr_phb_children_reset(Object *child, void *opaque)
>  
>  static void spapr_phb_reset(DeviceState *qdev)
>  {
> +    spapr_phb_dma_reset(SPAPR_PCI_HOST_BRIDGE(qdev));
> +
>      /* Reset the IOMMU state */
>      object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL);
>  }
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index a5b97d0..f89e053 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -58,11 +58,6 @@ static int spapr_phb_vfio_dma_init_window(sPAPRPHBState *sphb,
>      return 0;
>  }
>  
> -static void spapr_phb_vfio_reset(DeviceState *qdev)
> -{
> -    /* Do nothing */
> -}
> -
>  static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>                                           unsigned int addr, int option)
>  {
> @@ -176,7 +171,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>      sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
>  
>      dc->props = spapr_phb_vfio_properties;
> -    dc->reset = spapr_phb_vfio_reset;
>      spc->dma_capabilities_update = spapr_phb_vfio_dma_capabilities_update;
>      spc->dma_init_window = spapr_phb_vfio_dma_init_window;
>      spc->eeh_set_option = spapr_phb_vfio_eeh_set_option;
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 3074145..7fda78e 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -140,5 +140,8 @@ void spapr_pci_rtas_init(void);
>  sPAPRPHBState *spapr_pci_find_phb(sPAPREnvironment *spapr, uint64_t buid);
>  PCIDevice *spapr_pci_find_dev(sPAPREnvironment *spapr, uint64_t buid,
>                                uint32_t config_addr);
> +int spapr_phb_dma_remove_window(sPAPRPHBState *sphb,
> +                                sPAPRTCETable *tcet);
> +int spapr_phb_dma_reset(sPAPRPHBState *sphb);
>  
>  #endif /* __HW_SPAPR_PCI_H__ */

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