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