On Thu, Mar 10, 2016 at 06:39:39PM +1100, Alexey Kardashevskiy wrote:
> On 03/03/2016 02:00 PM, David Gibson wrote:
> >On Tue, Mar 01, 2016 at 08:10:29PM +1100, Alexey Kardashevskiy wrote:
> >>Currently TCE tables are created once at start and their sizes never
> >>change. We are going to change that by introducing a Dynamic DMA windows
> >>support where DMA configuration may change during the guest execution.
> >>
> >>This changes spapr_tce_new_table() to create an empty zero-size IOMMU
> >>memory region (IOMMU MR). Only LIOBN is assigned by the time of creation.
> >>It still will be called once at the owner object (VIO or PHB) creation.
> >>
> >>This introduces an "enabled" state for TCE table objects with two
> >>helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
> >>- spapr_tce_table_enable() receives TCE table parameters, allocates
> >>a guest view of the TCE table (in the user space or KVM) and
> >>sets the correct size on the IOMMU MR.
> >>- spapr_tce_table_disable() disposes the table and resets the IOMMU MR
> >>size.
> >>
> >>This changes the PHB reset handler to do the default DMA initialization
> >>instead of spapr_phb_realize(). This does not make differenct now but
> >>later with more than just one DMA window, we will have to remove them all
> >>and create the default one on a system reset.
> >>
> >>No visible change in behaviour is expected except the actual table
> >>will be reallocated every reset. We might optimize this later.
> >>
> >>The other way to implement this would be dynamically create/remove
> >>the TCE table QOM objects but this would make migration impossible
> >>as the migration code expects all QOM objects to exist at the receiver
> >>so we have to have TCE table objects created when migration begins.
> >>
> >>spapr_tce_table_do_enable() is separated from from spapr_tce_table_enable()
> >>as later it will be called at the sPAPRTCETable post-migration stage when
> >>it already has all the properties set after the migration.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> >
> >Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> >
> >Although there's one nit that could be improved:
> >
> >
> >>---
> >> hw/ppc/spapr_iommu.c | 80
> >> +++++++++++++++++++++++++++++++++++---------------
> >> hw/ppc/spapr_pci.c | 21 +++++++++----
> >> hw/ppc/spapr_vio.c | 9 +++---
> >> include/hw/ppc/spapr.h | 10 +++----
> >> 4 files changed, 80 insertions(+), 40 deletions(-)
> >>
> >>diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> >>index 8132f64..e66e128 100644
> >>--- a/hw/ppc/spapr_iommu.c
> >>+++ b/hw/ppc/spapr_iommu.c
> >>@@ -174,15 +174,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
> >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> >>
> >> tcet->fd = -1;
> >>- tcet->table = spapr_tce_alloc_table(tcet->liobn,
> >>- tcet->page_shift,
> >>- tcet->nb_table,
> >>- &tcet->fd,
> >>- tcet->need_vfio);
> >>-
> >> memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
> >>- "iommu-spapr",
> >>- (uint64_t)tcet->nb_table << tcet->page_shift);
> >>+ "iommu-spapr", 0);
> >>
> >> QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
> >>
> >>@@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet,
> >>bool need_vfio)
> >> tcet->table = newtable;
> >> }
> >>
> >>-sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
> >>- uint64_t bus_offset,
> >>- uint32_t page_shift,
> >>- uint32_t nb_table,
> >>- bool need_vfio)
> >>+sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
> >> {
> >> sPAPRTCETable *tcet;
> >>- char tmp[64];
> >>+ char tmp[32];
> >>
> >> if (spapr_tce_find_by_liobn(liobn)) {
> >> fprintf(stderr, "Attempted to create TCE table with duplicate"
> >>@@ -239,16 +228,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner,
> >>uint32_t liobn,
> >> return NULL;
> >> }
> >>
> >>- if (!nb_table) {
> >>- return NULL;
> >>- }
> >>-
> >> tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
> >> tcet->liobn = liobn;
> >>- tcet->bus_offset = bus_offset;
> >>- tcet->page_shift = page_shift;
> >>- tcet->nb_table = nb_table;
> >>- tcet->need_vfio = need_vfio;
> >>
> >> snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
> >> object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> >>@@ -258,14 +239,65 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState
> >>*owner, uint32_t liobn,
> >> return tcet;
> >> }
> >>
> >>+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
> >>+{
> >>+ if (!tcet->nb_table) {
> >>+ return;
> >>+ }
> >>+
> >>+ tcet->table = spapr_tce_alloc_table(tcet->liobn,
> >>+ tcet->page_shift,
> >>+ tcet->nb_table,
> >>+ &tcet->fd,
> >>+ tcet->need_vfio);
> >>+
> >>+ memory_region_set_size(&tcet->iommu,
> >>+ (uint64_t)tcet->nb_table << tcet->page_shift);
> >>+
> >>+ tcet->enabled = true;
> >>+}
> >>+
> >>+void spapr_tce_table_enable(sPAPRTCETable *tcet,
> >>+ uint32_t page_shift, uint64_t bus_offset,
> >>+ uint32_t nb_table, bool need_vfio)
> >>+{
> >>+ if (tcet->enabled) {
> >>+ return;
> >
> >If the given parameters are different from the current ones, treating
> >this as a no-op is rather misleading. I gather that to resize the
> >window you're expected to disable, then re-enable. In which case I
> >think it would be safer to actually throw some kind of error on a
> >double enable.
>
> I'll add here
>
> error_report("Warning: trying to enable already enabled TCE table");
>
> ...
>
>
>
>
> >
> >
> >>+ }
> >>+
> >>+ tcet->bus_offset = bus_offset;
> >>+ tcet->page_shift = page_shift;
> >>+ tcet->nb_table = nb_table;
> >>+ tcet->need_vfio = need_vfio;
> >>+
> >>+ spapr_tce_table_do_enable(tcet);
> >>+}
> >>+
> >>+static void spapr_tce_table_disable(sPAPRTCETable *tcet)
> >>+{
> >>+ if (!tcet->enabled) {
>
> ...and
>
> error_report("Warning: trying to disable already disabled TCE table");
That sounds good.
> or g_assert()?
Erm.. only if this can't be triggered by user actions.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
