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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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


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