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

[Alexey Kardashevskiy]

On 04/22/2015 04:39 PM, David Gibson wrote:
> 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 <address@hidden>
> > ---
> >   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?


rtas_ibm_remove_pe_dma_window() will call it in "[PATCH qemu v6 14/15] 
spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW)". I do not see 
much sense in making it static first and then making it public in later 
patch, should I fix this anyway (do not care that much) or updated commit 
log will be enough?




> > +{
> > +    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__ */


--
Alexey