On 24/07/17 15:53, David Gibson wrote: > On Thu, Jul 20, 2017 at 05:22:29PM +1000, Alexey Kardashevskiy wrote: >> This replaces g_malloc() with spapr_tce_alloc_table() as this is >> the standard way of allocating tables and this allows moving the table >> back to KVM when unplugging a VFIO PCI device and VFIO TCE acceleration >> support is not present in the KVM. > > So, I like the idea here, and I think it will work for now, but one > thing concerns me. > > AIUI, your future plans include allowing in-kernel accelerated TCE > tables even with VFIO devices. When that happens, we might have an > in-kernel table both before and after a change in the "need_vfio" > state.
The in-kernel table just stays there, mappings will be replayed but in the
end of the replay only the hardware table will change.
>
> But you won't be able to have two in-kernel tables allocated at once,
> whereas this code relies on having both the old and new tables at once
> so it can copy the contents over.
>
> How do you intend to handle that?
I intend to make this function no-op when cap_spapr_vfio is present.
>
>> Although spapr_tce_alloc_table() is expected to fail with EBUSY
>> if called when previous fd is not closed yet, in practice we will not
>> see it because cap_spapr_vfio is false at the moment.
>
> I don't follow this. How would cap_spapr_vfio be changing at any
> point?
It depends on the version of the host kernel.
>
>>
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>> ---
>> hw/ppc/spapr_iommu.c | 35 ++++++++++++++---------------------
>> 1 file changed, 14 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index e614621a83..307dc3021e 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -275,33 +275,26 @@ static int spapr_tce_table_realize(DeviceState *dev)
>> void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio)
>> {
>> size_t table_size = tcet->nb_table * sizeof(uint64_t);
>> - void *newtable;
>> + uint64_t *oldtable;
>> + int newfd = -1;
>>
>> - if (need_vfio == tcet->need_vfio) {
>> - /* Nothing to do */
>> - return;
>> - }
>> + g_assert(need_vfio != tcet->need_vfio);
>>
>> - if (!need_vfio) {
>> - /* FIXME: We don't support transition back to KVM accelerated
>> - * TCEs yet */
>> - return;
>> - }
>> + tcet->need_vfio = need_vfio;
>>
>> - tcet->need_vfio = true;
>> + oldtable = tcet->table;
>>
>> - if (tcet->fd < 0) {
>> - /* Table is already in userspace, nothing to be do */
>> - return;
>> - }
>> + tcet->table = spapr_tce_alloc_table(tcet->liobn,
>> + tcet->page_shift,
>> + tcet->bus_offset,
>> + tcet->nb_table,
>> + &newfd,
>> + need_vfio);
>> + memcpy(tcet->table, oldtable, table_size);
>>
>> - newtable = g_malloc(table_size);
>> - memcpy(newtable, tcet->table, table_size);
>> + spapr_tce_free_table(oldtable, tcet->fd, tcet->nb_table);
>>
>> - kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table);
>> -
>> - tcet->fd = -1;
>> - tcet->table = newtable;
>> + tcet->fd = newfd;
>> }
>>
>> sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
>
--
Alexey
signature.asc
Description: OpenPGP digital signature
