Re: [Qemu-devel] [PATCH 8/9] spapr_iommu: Introduce page_shift in sPAPRTCETable

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 05/22/2014 08:11 AM, Alexander Graf wrote:
> 
> On 21.05.14 16:21, Alexey Kardashevskiy wrote:
>> At the moment only 4K pages are supported by sPAPRTCETable. Since sPAPR
>> spec allows other page sizes and we are going to implement them, we need
>> page size to be configrable.
>>
>> This adds @page_shift into sPAPRTCETable and replaces SPAPR_TCE_PAGE_SHIFT
>> with it whereever it is possible.
>>
>> This removes SPAPR_TCE_PAGE_MASK as it is no longer used.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   hw/ppc/spapr_iommu.c   | 54
>> +++++++++++++++++++++++++++++---------------------
>>   hw/ppc/spapr_pci.c     |  1 +
>>   hw/ppc/spapr_vio.c     |  1 +
>>   include/hw/ppc/spapr.h |  3 ++-
>>   4 files changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index 90de3e3..e765a6d 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -70,12 +70,13 @@ static IOMMUTLBEntry
>> spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr)
>>         if (tcet->bypass) {
>>           ret.perm = IOMMU_RW;
>> -    } else if ((addr >> SPAPR_TCE_PAGE_SHIFT) < tcet->nb_table) {
>> +    } else if ((addr >> tcet->page_shift) < tcet->nb_table) {
>>           /* Check if we are in bound */
>> -        tce = tcet->table[addr >> SPAPR_TCE_PAGE_SHIFT];
>> -        ret.iova = addr & ~SPAPR_TCE_PAGE_MASK;
>> -        ret.translated_addr = tce & ~SPAPR_TCE_PAGE_MASK;
>> -        ret.addr_mask = SPAPR_TCE_PAGE_MASK;
>> +        target_ulong mask = ~((1 << tcet->page_shift) - 1);
> 
> Why target_ulong? This should be u64 or hwaddr or something along those
> lines, no?

Should be hwaddr, right, will fix.

> Also, can the mask grow bigger than 31bits? If so you probably
> want 1ULL (below as well).

It cannot now but PAPR allows pages to be 16GB so you are making good point
here, will fix too.

> In fact, we might be better off with a few more fields to tcet. Just add
> page_mask and page_size in addition to the page_shift one and use them
> instead of calculating them over and over again.

This is only used for emulated devices, not even for virtio. How much will
we earn from that optimization? Will it be even measurable? On the other
hand, this creates an opportunity (subtle, yes) to screw things up. Still
worth?



-- 
Alexey