[PATCH kernel 2/2] KVM: PPC: Allow backing bigger guest IOMMU pages with smaller physical pages

[Balbir Singh]

On Wed, May 2, 2018 at 4:26 PM, Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
> On 2/5/18 3:53 pm, Balbir Singh wrote:
>> On Wed,  2 May 2018 14:07:23 +1000
>> Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
>>
>>> At the moment we only support in the host the IOMMU page sizes which
>>> the guest is aware of, which is 4KB/64KB/16MB. However P9 does not support
>>> 16MB IOMMU pages, 2MB and 1GB pages are supported instead. We can still
>>> emulate bigger guest pages (for example 16MB) with smaller host pages
>>> (4KB/64KB/2MB).
>>>
>>> This allows the physical IOMMU pages to use a page size smaller or equal
>>> than the guest visible IOMMU page size.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>>> ---
>>>  arch/powerpc/kvm/book3s_64_vio.c    | 66 +++++++++++++++++++++++++++++--------
>>>  arch/powerpc/kvm/book3s_64_vio_hv.c | 52 +++++++++++++++++++++++++----
>>>  2 files changed, 98 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
>>> index 041e54d..e10d6a3 100644
>>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>>> @@ -176,14 +176,12 @@ extern long kvm_spapr_tce_attach_iommu_group(struct kvm *kvm, int tablefd,
>>>
>>>              if (!tbltmp)
>>>                      continue;
>>> -            /*
>>> -             * Make sure hardware table parameters are exactly the same;
>>> -             * this is used in the TCE handlers where boundary checks
>>> -             * use only the first attached table.
>>> -             */
>>> -            if ((tbltmp->it_page_shift == stt->page_shift) &&
>>> -                            (tbltmp->it_offset == stt->offset) &&
>>> -                            (tbltmp->it_size == stt->size)) {
>>> +            /* Make sure hardware table parameters are compatible */
>>> +            if ((tbltmp->it_page_shift <= stt->page_shift) &&
>>> +                            (tbltmp->it_offset << tbltmp->it_page_shift ==
>>> +                             stt->offset << stt->page_shift) &&
>>> +                            (tbltmp->it_size << tbltmp->it_page_shift ==
>>> +                             stt->size << stt->page_shift)) {
>>
>> Very difficult to parse
>>
>> How about -
>>
>> subpages_shift = (stt->page_shift - tbl->it_page_shift)
>>
>> and then
>>
>> matches_offset = !(tbltmp->it_offset - (stt->it_offset << subpages_shift));
>> matches_size = !(tbltmp->it_size - (stt->it_size << subpages_shift));
>>
>> The condition then is just
>>
>>               if ((tbltmp->it_page_shift == stt->page_shift) &&
>>                               matches_offset && matches_size) {
>
> This is harder to parse. My variant is bulky but straight forward otherwise.

OK, I don't read this code as often, but found it_page_shift and page_shift
variants hard to understand, but I guess it means I need to read more code.

>
>
>>
>>
>>
>>>                      /*
>>>                       * Reference the table to avoid races with
>>>                       * add/remove DMA windows.
>>> @@ -396,7 +394,7 @@ static long kvmppc_tce_iommu_mapped_dec(struct kvm *kvm,
>>>      return H_SUCCESS;
>>>  }
>>>
>>> -static long kvmppc_tce_iommu_unmap(struct kvm *kvm,
>>> +static long kvmppc_tce_iommu_do_unmap(struct kvm *kvm,
>>>              struct iommu_table *tbl, unsigned long entry)
>>>  {
>>>      enum dma_data_direction dir = DMA_NONE;
>>> @@ -416,7 +414,25 @@ static long kvmppc_tce_iommu_unmap(struct kvm *kvm,
>>>      return ret;
>>>  }
>>>
>>> -long kvmppc_tce_iommu_map(struct kvm *kvm, struct iommu_table *tbl,
>>> +static long kvmppc_tce_iommu_unmap(struct kvm *kvm,
>>> +            struct kvmppc_spapr_tce_table *stt, struct iommu_table *tbl,
>>> +            unsigned long entry)
>>> +{
>>> +    unsigned long ret = H_SUCCESS;
>>> +    unsigned long subpages = 1ULL << (stt->page_shift - tbl->it_page_shift);
>>> +    unsigned long io_entry = entry * subpages;
>>
>> How do we know this multiplication does not overflow?
>
> Entry, page_shift come from the userspace and checked in appropriate
> ioctl/hcall handlers.
>

Yes, I am not sure how many safety nets we need, but I worry about
such things :)

>>
>>> +    unsigned long subpg;
>>
>> Why not just i?
>
> I can imagine pages so huge so backing them with 4K will overflow 32bit
> anyway. It is very (very) unlikely but it is 64bit arch anyway and there is
> no much point in not-long types anyway.
>

What David said, i is an easy iterator to understand :)

Balbir Singh.