Re: [Qemu-devel] [PATCH qemu v10 03/14] spapr_pci: Convert finish_realize() to dma_capabilities_update()+dma_init_window()

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 07/07/2015 02:41 AM, Laurent Vivier wrote:
>
>
> On 06/07/2015 04:10, Alexey Kardashevskiy wrote:
>> This reworks finish_realize() which used to finalize DMA setup with
>> an assumption that it will not change later.
>>
>> New callbacks supports various window parameters such as page and
>> windows sizes. The new callback return error code rather than Error**.
>>
>> This is a mechanical change so no change in behaviour is expected.
>> This is a part of getting rid of spapr-pci-vfio-host-bridge type.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>> Changes:
>> v8:
>> * moved spapr_phb_dma_capabilities_update() higher to avoid forward
>> declaration in following patches and keep DMA code together (i.e. next
>> to spapr_pci_dma_iommu())
>> ---
>>   hw/ppc/spapr_pci.c          | 59 ++++++++++++++++++++++++++-------------------
>>   hw/ppc/spapr_pci_vfio.c     | 53 ++++++++++++++++------------------------
>>   include/hw/pci-host/spapr.h |  8 +++++-
>>   3 files changed, 62 insertions(+), 58 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index a8f79d8..c1ca13d 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -808,6 +808,28 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
>>       return buf;
>>   }
>>
>> +static int spapr_phb_dma_capabilities_update(sPAPRPHBState *sphb)
>> +{
>> +    sphb->dma32_window_start = 0;
>> +    sphb->dma32_window_size = SPAPR_PCI_DMA32_SIZE;
>> +
>> +    return 0;
>> +}
>> +
>> +static int spapr_phb_dma_init_window(sPAPRPHBState *sphb,
>> +                                     uint32_t liobn, uint32_t page_shift,
>> +                                     uint64_t window_size)
>> +{
>> +    uint64_t bus_offset = sphb->dma32_window_start;
>> +    sPAPRTCETable *tcet;
>> +
>> +    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift,
>> +                               window_size >> page_shift,
>> +                               false);
>> +
>> +    return tcet ? 0 : -1;
>> +}
>> +
>>   /* Macros to operate with address in OF binding to PCI */
>>   #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
>>   #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
>> @@ -1220,6 +1242,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>       int i;
>>       PCIBus *bus;
>>       uint64_t msi_window_size = 4096;
>> +    sPAPRTCETable *tcet;
>>
>>       if (sphb->index != (uint32_t)-1) {
>>           hwaddr windows_base;
>> @@ -1369,33 +1392,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>           }
>>       }
>>
>> -    if (!info->finish_realize) {
>> -        error_setg(errp, "finish_realize not defined");
>> -        return;
>> -    }
>> -
>> -    info->finish_realize(sphb, errp);
>> -
>> -    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
>> -}
>> -
>> -static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>> -{
>> -    sPAPRTCETable *tcet;
>> -    uint32_t nb_table;
>> -
>> -    nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
>> -    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
>> -                               0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
>> +    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, "Unable to create TCE table for %s",
>> -                   sphb->dtbusname);
>> -        return ;
>> +        error_setg(errp, "failed to create TCE table");
>> +        return;
>>       }
>> -
>> -    /* Register default 32bit DMA window */
>> -    memory_region_add_subregion(&sphb->iommu_root, 0,
>> +    memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
>>                                   spapr_tce_get_iommu(tcet));
>> +
>> +    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
>>   }
>>
>>   static int spapr_phb_children_reset(Object *child, void *opaque)
>> @@ -1543,9 +1551,10 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>>       dc->vmsd = &vmstate_spapr_pci;
>>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>       dc->cannot_instantiate_with_device_add_yet = false;
>> -    spc->finish_realize = spapr_phb_finish_realize;
>>       hp->plug = spapr_phb_hot_plug_child;
>>       hp->unplug = spapr_phb_hot_unplug_child;
>> +    spc->dma_capabilities_update = spapr_phb_dma_capabilities_update;
>> +    spc->dma_init_window = spapr_phb_dma_init_window;
>>   }
>>
>>   static const TypeInfo spapr_phb_info = {
>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>> index cca45ed..6e3e17b 100644
>> --- a/hw/ppc/spapr_pci_vfio.c
>> +++ b/hw/ppc/spapr_pci_vfio.c
>> @@ -28,48 +28,36 @@ static Property spapr_phb_vfio_properties[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
>> -static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>> +static int spapr_phb_vfio_dma_capabilities_update(sPAPRPHBState *sphb)
>>   {
>>       sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>>       struct vfio_iommu_spapr_tce_info info = { .argsz = sizeof(info) };
>>       int ret;
>> -    sPAPRTCETable *tcet;
>> -    uint32_t liobn = svphb->phb.dma_liobn;
>>
>> -    if (svphb->iommugroupid == -1) {
>> -        error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid);
>> -        return;
>> -    }
>> -
>> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> -                               VFIO_CHECK_EXTENSION,
>> -                               (void *) VFIO_SPAPR_TCE_IOMMU);
>
>
> This ioctl() disappears completely, was it useless ?


It is called in hw/vfio/common.c and if it fails there, we never get here 
so there is no point in calling it here too.





-- 
Alexey