Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 04/16] spapr_iommu: Introduce "enabled" state for TCE table

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 03/03/2016 02:00 PM, David Gibson wrote:
> On Tue, Mar 01, 2016 at 08:10:29PM +1100, Alexey Kardashevskiy wrote:
>> Currently TCE tables are created once at start and their sizes never
>> change. We are going to change that by introducing a Dynamic DMA windows
>> support where DMA configuration may change during the guest execution.
>>
>> This changes spapr_tce_new_table() to create an empty zero-size IOMMU
>> memory region (IOMMU MR). Only LIOBN is assigned by the time of creation.
>> It still will be called once at the owner object (VIO or PHB) creation.
>>
>> This introduces an "enabled" state for TCE table objects with two
>> helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
>> - spapr_tce_table_enable() receives TCE table parameters, allocates
>> a guest view of the TCE table (in the user space or KVM) and
>> sets the correct size on the IOMMU MR.
>> - spapr_tce_table_disable() disposes the table and resets the IOMMU MR
>> size.
>>
>> This changes the PHB reset handler to do the default DMA initialization
>> instead of spapr_phb_realize(). This does not make differenct now but
>> later with more than just one DMA window, we will have to remove them all
>> and create the default one on a system reset.
>>
>> No visible change in behaviour is expected except the actual table
>> will be reallocated every reset. We might optimize this later.
>>
>> The other way to implement this would be dynamically create/remove
>> the TCE table QOM objects but this would make migration impossible
>> as the migration code expects all QOM objects to exist at the receiver
>> so we have to have TCE table objects created when migration begins.
>>
>> spapr_tce_table_do_enable() is separated from from spapr_tce_table_enable()
>> as later it will be called at the sPAPRTCETable post-migration stage when
>> it already has all the properties set after the migration.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> Although there's one nit that could be improved:
>
>
>> ---
>>   hw/ppc/spapr_iommu.c   | 80 +++++++++++++++++++++++++++++++++++---------------
>>   hw/ppc/spapr_pci.c     | 21 +++++++++----
>>   hw/ppc/spapr_vio.c     |  9 +++---
>>   include/hw/ppc/spapr.h | 10 +++----
>>   4 files changed, 80 insertions(+), 40 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index 8132f64..e66e128 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -174,15 +174,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
>>       sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>>
>>       tcet->fd = -1;
>> -    tcet->table = spapr_tce_alloc_table(tcet->liobn,
>> -                                        tcet->page_shift,
>> -                                        tcet->nb_table,
>> -                                        &tcet->fd,
>> -                                        tcet->need_vfio);
>> -
>>       memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
>> -                             "iommu-spapr",
>> -                             (uint64_t)tcet->nb_table << tcet->page_shift);
>> +                             "iommu-spapr", 0);
>>
>>       QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>>
>> @@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio)
>>       tcet->table = newtable;
>>   }
>>
>> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>> -                                   uint64_t bus_offset,
>> -                                   uint32_t page_shift,
>> -                                   uint32_t nb_table,
>> -                                   bool need_vfio)
>> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
>>   {
>>       sPAPRTCETable *tcet;
>> -    char tmp[64];
>> +    char tmp[32];
>>
>>       if (spapr_tce_find_by_liobn(liobn)) {
>>           fprintf(stderr, "Attempted to create TCE table with duplicate"
>> @@ -239,16 +228,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>>           return NULL;
>>       }
>>
>> -    if (!nb_table) {
>> -        return NULL;
>> -    }
>> -
>>       tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
>>       tcet->liobn = liobn;
>> -    tcet->bus_offset = bus_offset;
>> -    tcet->page_shift = page_shift;
>> -    tcet->nb_table = nb_table;
>> -    tcet->need_vfio = need_vfio;
>>
>>       snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
>>       object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
>> @@ -258,14 +239,65 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>>       return tcet;
>>   }
>>
>> +static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
>> +{
>> +    if (!tcet->nb_table) {
>> +        return;
>> +    }
>> +
>> +    tcet->table = spapr_tce_alloc_table(tcet->liobn,
>> +                                        tcet->page_shift,
>> +                                        tcet->nb_table,
>> +                                        &tcet->fd,
>> +                                        tcet->need_vfio);
>> +
>> +    memory_region_set_size(&tcet->iommu,
>> +                           (uint64_t)tcet->nb_table << tcet->page_shift);
>> +
>> +    tcet->enabled = true;
>> +}
>> +
>> +void spapr_tce_table_enable(sPAPRTCETable *tcet,
>> +                            uint32_t page_shift, uint64_t bus_offset,
>> +                            uint32_t nb_table, bool need_vfio)
>> +{
>> +    if (tcet->enabled) {
>> +        return;
>
> If the given parameters are different from the current ones, treating
> this as a no-op is rather misleading.  I gather that to resize the
> window you're expected to disable, then re-enable.  In which case I
> think it would be safer to actually throw some kind of error on a
> double enable.

I'll add here

  error_report("Warning: trying to enable already enabled TCE table");

...




>
>
>> +    }
>> +
>> +    tcet->bus_offset = bus_offset;
>> +    tcet->page_shift = page_shift;
>> +    tcet->nb_table = nb_table;
>> +    tcet->need_vfio = need_vfio;
>> +
>> +    spapr_tce_table_do_enable(tcet);
>> +}
>> +
>> +static void spapr_tce_table_disable(sPAPRTCETable *tcet)
>> +{
>> +    if (!tcet->enabled) {

...and

error_report("Warning: trying to disable already disabled TCE table");

or g_assert()?


>> +        return;
>> +    }
>> +
>> +    memory_region_set_size(&tcet->iommu, 0);
>> +
>> +    spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table);
>> +    tcet->fd = -1;
>> +    tcet->table = NULL;
>> +    tcet->enabled = false;
>> +    tcet->bus_offset = 0;
>> +    tcet->page_shift = 0;
>> +    tcet->nb_table = 0;
>> +    tcet->need_vfio = false;
>> +}
>> +
>>   static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
>>   {
>>       sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>>
>>       QLIST_REMOVE(tcet, list);
>>
>> -    spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table);
>> -    tcet->fd = -1;
>> +    spapr_tce_table_disable(tcet);
>>   }
>>
>>   MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 248f20a..c34a906 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -815,12 +815,13 @@ static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb,
>>           return -1;
>>       }
>>
>> -    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr,
>> -                               page_shift, nb_table, false);
>> +    tcet = spapr_tce_find_by_liobn(liobn);
>>       if (!tcet) {
>>           return -1;
>>       }
>>
>> +    spapr_tce_table_enable(tcet, page_shift, window_addr, nb_table, false);
>> +
>>       memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
>>                                   spapr_tce_get_iommu(tcet));
>>       return 0;
>> @@ -1251,6 +1252,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;
>> @@ -1402,11 +1404,18 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>           }
>>       }
>>
>> +    /* DMA setup */
>> +    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn);
>> +    if (!tcet) {
>> +        error_report("No default TCE table for %s", sphb->dtbusname);
>> +        return;
>> +    }
>> +
>>       /* Register default 32bit DMA window */
>> -    if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT,
>> -                                    sphb->dma_win_addr, sphb->dma_win_size)) {
>> -        error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname);
>> -    }
>> +    spapr_phb_dma_window_enable(sphb, sphb->dma_liobn,
>> +                                SPAPR_TCE_PAGE_SHIFT,
>> +                                sphb->dma_win_addr,
>> +                                sphb->dma_win_size);
>>
>>       sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
>>   }
>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>> index 0f61a55..a745884 100644
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -481,11 +481,10 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>>           memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1);
>>           address_space_init(&dev->as, &dev->mrroot, qdev->id);
>>
>> -        dev->tcet = spapr_tce_new_table(qdev, liobn,
>> -                                        0,
>> -                                        SPAPR_TCE_PAGE_SHIFT,
>> -                                        pc->rtce_window_size >>
>> -                                        SPAPR_TCE_PAGE_SHIFT, false);
>> +        dev->tcet = spapr_tce_new_table(qdev, liobn);
>> +        spapr_tce_table_enable(dev->tcet, SPAPR_TCE_PAGE_SHIFT, 0,
>> +                               pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT,
>> +                               false);
>>           dev->tcet->vdev = dev;
>>           memory_region_add_subregion_overlap(&dev->mrroot, 0,
>>                                               spapr_tce_get_iommu(dev->tcet), 2);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 098d85d..3e6bb84 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -539,6 +539,7 @@ typedef struct sPAPRTCETable sPAPRTCETable;
>>
>>   struct sPAPRTCETable {
>>       DeviceState parent;
>> +    bool enabled;
>>       uint32_t liobn;
>>       uint32_t nb_table;
>>       uint64_t bus_offset;
>> @@ -566,11 +567,10 @@ void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
>>   int spapr_h_cas_compose_response(sPAPRMachineState *sm,
>>                                    target_ulong addr, target_ulong size,
>>                                    bool cpu_update, bool memory_update);
>> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>> -                                   uint64_t bus_offset,
>> -                                   uint32_t page_shift,
>> -                                   uint32_t nb_table,
>> -                                   bool need_vfio);
>> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
>> +void spapr_tce_table_enable(sPAPRTCETable *tcet,
>> +                            uint32_t page_shift, uint64_t bus_offset,
>> +                            uint32_t nb_table, bool vfio_accel);
>>   void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio);
>>
>>   MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
>


-- 
Alexey