Re: [Qemu-devel] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 03/23/2016 05:03 PM, David Gibson wrote:
> On Wed, Mar 23, 2016 at 02:06:36PM +1100, Alexey Kardashevskiy wrote:
>> On 03/23/2016 01:53 PM, David Gibson wrote:
>>> On Wed, Mar 23, 2016 at 01:12:59PM +1100, Alexey Kardashevskiy wrote:
>>>> On 03/23/2016 12:08 PM, David Gibson wrote:
>>>>> On Tue, Mar 22, 2016 at 04:54:07PM +1100, Alexey Kardashevskiy wrote:
>>>>>> On 03/22/2016 04:14 PM, David Gibson wrote:
>>>>>>> On Mon, Mar 21, 2016 at 06:47:05PM +1100, Alexey Kardashevskiy wrote:
>>>>>>>> New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management.
>>>>>>>> This adds ability to VFIO common code to dynamically allocate/remove
>>>>>>>> DMA windows in the host kernel when new VFIO container is added/removed.
>>>>>>>>
>>>>>>>> This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add
>>>>>>>> and adds just created IOMMU into the host IOMMU list; the opposite
>>>>>>>> action is taken in vfio_listener_region_del.
>>>>>>>>
>>>>>>>> When creating a new window, this uses euristic to decide on the TCE table
>>>>>>>> levels number.
>>>>>>>>
>>>>>>>> This should cause no guest visible change in behavior.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>>> ---
>>>>>>>> Changes:
>>>>>>>> v14:
>>>>>>>> * new to the series
>>>>>>>>
>>>>>>>> ---
>>>>>>>> TODO:
>>>>>>>> * export levels to PHB
>>>>>>>> ---
>>>>>>>>   hw/vfio/common.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>   trace-events     |   2 ++
>>>>>>>>   2 files changed, 105 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>>>>>> index 4e873b7..421d6eb 100644
>>>>>>>> --- a/hw/vfio/common.c
>>>>>>>> +++ b/hw/vfio/common.c
>>>>>>>> @@ -279,6 +279,14 @@ static int vfio_host_iommu_add(VFIOContainer *container,
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +static void vfio_host_iommu_del(VFIOContainer *container, hwaddr min_iova)
>>>>>>>> +{
>>>>>>>> +    VFIOHostIOMMU *hiommu = vfio_host_iommu_lookup(container, min_iova, 0x1000);
>>>>>>>
>>>>>>> The hard-coded 0x1000 looks dubious..
>>>>>>
>>>>>> Well, that's the minimal page size...
>>>>>
>>>>> Really?  Some BookE CPUs support 1KiB page size..
>>>>
>>>> Hm. For IOMMU? Ok. s/0x1000/1/ should do then :)
>>>
>>> Uh.. actually I don't think those CPUs generally had an IOMMU.  But if
>>> it's been done for CPU MMU I wouldn't count on it not being done for
>>> IOMMU.
>>>
>>> 1 is a safer choice.
>>>
>>>>
>>>>
>>>>>
>>>>>>>> +    g_assert(hiommu);
>>>>>>>> +    QLIST_REMOVE(hiommu, hiommu_next);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>>>>>>>>   {
>>>>>>>>       return (!memory_region_is_ram(section->mr) &&
>>>>>>>> @@ -392,6 +400,61 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>>>>>>>       }
>>>>>>>>       end = int128_get64(llend);
>>>>>>>>
>>>>>>>> +    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>>>>>>>
>>>>>>> I think this would be clearer split out into a helper function,
>>>>>>> vfio_create_host_window() or something.
>>>>>>
>>>>>>
>>>>>> It is rather vfio_spapr_create_host_window() and we were avoiding
>>>>>> xxx_spapr_xxx so far. I'd cut-n-paste the SPAPR PCI AS listener to a
>>>>>> separate file but this usually triggers more discussion and never ends well.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>> +        unsigned entries, pages;
>>>>>>>> +        struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
>>>>>>>> +
>>>>>>>> +        g_assert(section->mr->iommu_ops);
>>>>>>>> +        g_assert(memory_region_is_iommu(section->mr));
>>>>>>>
>>>>>>> I don't think you need these asserts.  AFAICT the same logic should
>>>>>>> work if a RAM MR was added directly to PCI address space - this would
>>>>>>> create the new host window, then the existing code for adding a RAM MR
>>>>>>> would map that block of RAM statically into the new window.
>>>>>>
>>>>>> In what configuration/machine can we do that on SPAPR?
>>>>>
>>>>> spapr guests won't ever do that.  But you can run an x86 guest on a
>>>>> powernv host and this situation could come up.
>>>>
>>>>
>>>> I am pretty sure VFIO won't work in this case anyway.
>>>
>>> I'm not.  There's no fundamental reason VFIO shouldn't work with TCG.
>>
>> This is not about TCG (pseries TCG guest works with VFIO on powernv host),
>> this is about things like VFIO_IOMMU_GET_INFO vs.
>> VFIO_IOMMU_SPAPR_TCE_GET_INFO ioctls but yes, fundamentally, it can work.
>>
>> Should I add such support in this patchset?
>
> Unless adding the generality is really complex, and so far I haven't
> seen a reason for it to be.

Seriously? :(



-- 
Alexey