Re: [Qemu-devel] [PATCH qemu v10 10/14] spapr_pci: Enable vfio-pci hotplug

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 07/07/2015 07:31 AM, Thomas Huth wrote:
> On Mon,  6 Jul 2015 12:11:06 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>> sPAPR IOMMU is managing two copies of an TCE table:
>> 1) a guest view of the table - this is what emulated devices use and
>> this is where H_GET_TCE reads from;
>> 2) a hardware TCE table - only present if there is at least one vfio-pci
>> device on a PHB; it is updated via a memory listener on a PHB address
>> space which forwards map/unmap requests to vfio-pci IOMMU host driver.
>>
>> At the moment presence of vfio-pci devices on a bus affect the way
>> the guest view table is allocated. If there is no vfio-pci on a PHB
>> and the host kernel supports KVM acceleration of H_PUT_TCE, a table
>> is allocated in KVM. However, if there is vfio-pci and we do yet not
>> support KVM acceleration for these, the table has to be allocated
>> by the userspace.
>>
>> When vfio-pci device is hotplugged and there were no vfio-pci devices
>> already, the guest view table could have been allocated by KVM which
>> means that H_PUT_TCE is handled by the host kernel and since we
>> do not support vfio-pci in KVM, the hardware table will not be updated.
>>
>> This reallocates the guest view table in QEMU if the first vfio-pci
>> device has just been plugged. spapr_tce_realloc_userspace() handles this.
>
> I wonder whether it would help to improve the readability of the code
> later if you put the description of the function into the code instead
> of the commit message?


Not sure I understood how much of this commit log you'd like to see in the 
code. The function has some comments already...

>
>> This replays all the mappings to make sure that the tables are in sync.
>> This will not have a visible effect though as for a new device
>> the guest kernel will allocate-and-map new addresses and therefore
>> existing mappings from emulated devices will not be used by vfio-pci
>> devices.
>>
>> This adds calls to spapr_phb_dma_capabilities_update() in PCI hotplug
>> hooks.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
> ...
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 76c988f..d1fa157 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -827,6 +827,43 @@ int spapr_phb_dma_reset(sPAPRPHBState *sphb)
>>       return 0;
>>   }
>>
>> +static int spapr_phb_hotplug_dma_sync(sPAPRPHBState *sphb)
>> +{
>> +    int ret = 0, i;
>> +    bool had_vfio = sphb->has_vfio;
>> +    sPAPRTCETable *tcet;
>> +
>> +    spapr_phb_dma_capabilities_update(sphb);
>> +
>> +    if (!had_vfio && sphb->has_vfio) {
>
>      if (had_vfio || !sphb->has_vfio) {
>          return 0;
>      }
>
> ... and then you can save one level of indentation for the following
> for-loop.

Right. I was going to add another chunk later with "if", "had_vfio" and 
"sphb->has_vfio", this is why this indentation. I'll remove this.


>> +        for (i = 0; i < SPAPR_PCI_DMA_MAX_WINDOWS; ++i) {
>> +            tcet = spapr_tce_find_by_liobn(SPAPR_PCI_LIOBN(sphb->index, i));
>> +            if (!tcet || !tcet->enabled) {
>> +                continue;
>> +            }
>> +            if (tcet->fd >= 0) {
>> +                /*
>> +                 * We got first vfio-pci device on accelerated table.
>> +                 * VFIO acceleration is not possible.
>> +                 * Reallocate table in userspace and replay mappings.
>> +                 */
>> +                ret = spapr_tce_realloc_userspace(tcet, true);
>> +                trace_spapr_pci_dma_realloc_update(tcet->liobn, ret);
>> +            } else {
>> +                /* There was no acceleration, so just replay mappings. */
>> +                ret = spapr_tce_replay(tcet);
>> +                trace_spapr_pci_dma_update(tcet->liobn, ret);
>> +            }
>> +            if (ret) {
>> +                break;
>> +            }
>> +        }
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /* Macros to operate with address in OF binding to PCI */
>>   #define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
>>   #define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
> ...
>> @@ -1130,6 +1174,9 @@ static void spapr_phb_remove_pci_device_cb(DeviceState *dev, void *opaque)
>>        */
>>       pci_device_reset(PCI_DEVICE(dev));
>>       object_unparent(OBJECT(dev));
>> +
>> +    /* Actual VFIO device release happens from RCU so postpone DMA update */
>> +    call_rcu1(&((sPAPRPHBState *)opaque)->rcu, spapr_phb_remove_sync_dma);
>
> Too much brackets again for my taste ;-)


Never too much! ;)


>
>>   }
>>
>
>   Thomas
>
>


-- 
Alexey