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

[Alexander Graf]

> Am 22.05.2014 um 06:25 schrieb Alexey Kardashevskiy <address@hidden>:
> 
>> 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 <address@hidden>
>>> ---
>>>  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?

I'm not concerned about speed here, but readibility :). Inline calculations are 
just harder to read.

The alternative would be to extract every calculation into a local variable and 
use that instead ;).

Alex

> 
> 
> 
> -- 
> Alexey