Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 02/16] spapr_pci: Move DMA window enablement to a helper

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 03/03/2016 12:40 PM, David Gibson wrote:
> On Tue, Mar 01, 2016 at 08:10:27PM +1100, Alexey Kardashevskiy wrote:
>> We are going to have multiple DMA windows soon so let's start preparing.
>>
>> This adds a new helper to create a DMA window and makes use of it in
>> sPAPRPHBState::realize().
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++-------------
>>   1 file changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 3d1145e..248f20a 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
>>       return buf;
>>   }
>>
>> +static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb,
>> +                                       uint32_t liobn, uint32_t page_shift,
>> +                                       uint64_t window_addr,
>> +                                       uint64_t window_size)
>> +{
>> +    sPAPRTCETable *tcet;
>> +    uint32_t nb_table = window_size >> page_shift;
>> +
>> +    if (!nb_table) {
>> +        return -1;
>> +    }
>
> The caller shouldn't do this, so this probably makes more sense as an
> assert() than an error return.


@dma_win_size is a PHB property so the cli can set it to zero - where is it 
supposed to fail? When DMA won't work? This will be not really obvious for 
the user. Check dma_win_size==0 where we check mem_win_addr/...?

>
>> +
>> +    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr,
>> +                               page_shift, nb_table, false);
>> +    if (!tcet) {
>> +        return -1;
>> +    }
>
> Since you're adding error reporting, you might as well make it via the
> error API instead of a return code.  That way if we wasnt to add more
> detailed error API reporting to spapr_tce_new_table() in future,
> there's less to change.

Well, the table allocation is the only real thing which may fail there and 
spapr_phb_realize() does not pass Error to the callers so 
spapr_phb_dma_window_enable() would be the first one to propagate an error 
and it just seems a bit over engineered. Should I still do that?


>> +
>> +    memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
>> +                                spapr_tce_get_iommu(tcet));
>> +    return 0;
>> +}
>> +
>>   /* 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 */
>> @@ -1228,8 +1251,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>       int i;
>>       PCIBus *bus;
>>       uint64_t msi_window_size = 4096;
>> -    sPAPRTCETable *tcet;
>> -    uint32_t nb_table;
>>
>>       if (sphb->index != (uint32_t)-1) {
>>           hwaddr windows_base;
>> @@ -1381,18 +1402,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>           }
>>       }
>>
>> -    nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT;
>> -    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
>> -                               0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
>> -    if (!tcet) {
>> -        error_setg(errp, "Unable to create TCE table for %s",
>> -                   sphb->dtbusname);
>> -        return;
>> -    }
>> -
>>       /* Register default 32bit DMA window */
>> -    memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr,
>> -                                spapr_tce_get_iommu(tcet));
>> +    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);
>> +    }
>>
>>       sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
>>   }
>


-- 
Alexey