Message ID | 1442495357-26547-10-git-send-email-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
On Thu, 2015-09-17 at 23:09 +1000, David Gibson wrote: > Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not > all TCE tables (guest IOMMU contexts) can support VFIO devices. Currently, > this is decided at creation time. > > To support hotplug of VFIO devices, we need to allow a TCE table which > previously didn't allow VFIO devices to be switched so that it can. This > patch adds an spapr_tce_need_vfio() function to do this, by reallocating > the table in userspace if necessary. > > Currently this doesn't allow the KVM acceleration to be re-enabled if all > the VFIO devices are removed. That's an optimization for another time. Same comment as previous patch, spapr_tce_need_userspace_table() (or something) makes the code much more self documenting. > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_iommu.c | 19 +++++++++++++++++++ > include/hw/ppc/spapr.h | 2 ++ > 2 files changed, 21 insertions(+) > > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 5166cde..109e10d 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -168,6 +168,25 @@ static int spapr_tce_table_realize(DeviceState *dev) > return 0; > } > > +void spapr_tce_need_vfio(sPAPRTCETable *tcet) > +{ > + size_t table_size = tcet->nb_table * sizeof(uint64_t); > + void *newtable; > + > + if (tcet->fd < 0) { > + /* Table is already in userspace, nothing to be done */ > + return; > + } > + > + newtable = g_malloc0(table_size); > + memcpy(newtable, tcet->table, table_size); > + > + kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table); > + > + tcet->fd = -1; > + tcet->table = newtable; > +} > + > sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, > uint64_t bus_offset, > uint32_t page_shift, > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 03a9804..38a29fb 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -588,6 +588,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, > uint32_t page_shift, > uint32_t nb_table, > bool need_vfio); > +void spapr_tce_need_vfio(sPAPRTCETable *tcet); > + > MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); > int spapr_dma_dt(void *fdt, int node_off, const char *propname, > uint32_t liobn, uint64_t window, uint32_t size);
On 17/09/15 18:54, Alex Williamson wrote: > On Thu, 2015-09-17 at 23:09 +1000, David Gibson wrote: >> Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not >> all TCE tables (guest IOMMU contexts) can support VFIO devices. Currently, >> this is decided at creation time. >> >> To support hotplug of VFIO devices, we need to allow a TCE table which >> previously didn't allow VFIO devices to be switched so that it can. This >> patch adds an spapr_tce_need_vfio() function to do this, by reallocating >> the table in userspace if necessary. >> >> Currently this doesn't allow the KVM acceleration to be re-enabled if all >> the VFIO devices are removed. That's an optimization for another time. > > > Same comment as previous patch, spapr_tce_need_userspace_table() (or > something) makes the code much more self documenting. May I also add the using the word "need" in a function name is could be a little bit confusing here? It's maybe just me, but if I read ..._need_...() I first think of a function that just checks something and returns a boolean for yes or no, not of a function that changes something. Could you maybe use something like "..._change_to_..." instead? Thomas
On Wed, Sep 23, 2015 at 01:24:00PM +0200, Thomas Huth wrote: > On 17/09/15 18:54, Alex Williamson wrote: > > On Thu, 2015-09-17 at 23:09 +1000, David Gibson wrote: > >> Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not > >> all TCE tables (guest IOMMU contexts) can support VFIO devices. Currently, > >> this is decided at creation time. > >> > >> To support hotplug of VFIO devices, we need to allow a TCE table which > >> previously didn't allow VFIO devices to be switched so that it can. This > >> patch adds an spapr_tce_need_vfio() function to do this, by reallocating > >> the table in userspace if necessary. > >> > >> Currently this doesn't allow the KVM acceleration to be re-enabled if all > >> the VFIO devices are removed. That's an optimization for another time. > > > > > > Same comment as previous patch, spapr_tce_need_userspace_table() (or > > something) makes the code much more self documenting. > > May I also add the using the word "need" in a function name is could be > a little bit confusing here? It's maybe just me, but if I read > ..._need_...() I first think of a function that just checks something > and returns a boolean for yes or no, not of a function that changes > something. > Could you maybe use something like "..._change_to_..." instead? Yeah, I never did much like that name, just couldn't think of a better one. Hmm.. how about spapr_tce_set_need_vfio(), taking a bool parameter. I won't implement the reverse transition just yet, but that gives us an obvious place to put it when/if we do.
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 5166cde..109e10d 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -168,6 +168,25 @@ static int spapr_tce_table_realize(DeviceState *dev) return 0; } +void spapr_tce_need_vfio(sPAPRTCETable *tcet) +{ + size_t table_size = tcet->nb_table * sizeof(uint64_t); + void *newtable; + + if (tcet->fd < 0) { + /* Table is already in userspace, nothing to be done */ + return; + } + + newtable = g_malloc0(table_size); + memcpy(newtable, tcet->table, table_size); + + kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table); + + tcet->fd = -1; + tcet->table = newtable; +} + sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, uint64_t bus_offset, uint32_t page_shift, diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 03a9804..38a29fb 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -588,6 +588,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, uint32_t page_shift, uint32_t nb_table, bool need_vfio); +void spapr_tce_need_vfio(sPAPRTCETable *tcet); + MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); int spapr_dma_dt(void *fdt, int node_off, const char *propname, uint32_t liobn, uint64_t window, uint32_t size);
Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not all TCE tables (guest IOMMU contexts) can support VFIO devices. Currently, this is decided at creation time. To support hotplug of VFIO devices, we need to allow a TCE table which previously didn't allow VFIO devices to be switched so that it can. This patch adds an spapr_tce_need_vfio() function to do this, by reallocating the table in userspace if necessary. Currently this doesn't allow the KVM acceleration to be re-enabled if all the VFIO devices are removed. That's an optimization for another time. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_iommu.c | 19 +++++++++++++++++++ include/hw/ppc/spapr.h | 2 ++ 2 files changed, 21 insertions(+)