| Message ID | 1420765371-25342-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On 09/01/2015 02:02, Alexey Kardashevskiy wrote: > @@ -100,7 +98,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { > VMSTATE_UINT32_EQUAL(nb_table, sPAPRTCETable), > > /* IOMMU state */ > - VMSTATE_BOOL(bypass, sPAPRTCETable), > + VMSTATE_UNUSED(sizeof(bool)), sizeof(bool) can change across hosts, this needs to be the size that is written by put_bool in migration/vmstate.c, i.e. VMSTATE_UNUSED(1). > VMSTATE_VARRAY_UINT32(table, sPAPRTCETable, nb_table, 0, vmstate_info_uint64, uint64_t), > > VMSTATE_END_OF_LIST() > @@ -131,7 +129,8 @@ static int spapr_tce_table_realize(DeviceState *dev) > trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd); > > memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops, > - "iommu-spapr", ram_size); > + "iommu-spapr", > + (uint64_t)tcet->nb_table << tcet->page_shift); > > QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); > > @@ -191,17 +190,11 @@ MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet) > return &tcet->iommu; > } > > -void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass) > -{ > - tcet->bypass = bypass; > -} > - > static void spapr_tce_reset(DeviceState *dev) > { > sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); > size_t table_size = tcet->nb_table * sizeof(uint64_t); > > - tcet->bypass = false; > memset(tcet->table, 0, table_size); > } > > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index dc9e46a..a1f2316 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -322,6 +322,18 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev) > free_crq(dev); > } > > +static void spapr_vio_set_bypass(VIOsPAPRDevice *dev, bool bypass) > +{ > + if (dev->bypass == bypass) { > + return; > + } > + > + memory_region_set_enabled(&dev->mrbypass, bypass); > + memory_region_set_enabled(spapr_tce_get_iommu(dev->tcet), !bypass); > + > + dev->bypass = bypass; > +} > + > static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr, > uint32_t token, > uint32_t nargs, target_ulong args, > @@ -348,7 +360,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr, > return; > } > > - spapr_tce_set_bypass(dev->tcet, !!enable); > + spapr_vio_set_bypass(dev, !!enable); > > rtas_st(rets, 0, RTAS_OUT_SUCCESS); > } > @@ -407,6 +419,7 @@ static void spapr_vio_busdev_reset(DeviceState *qdev) > > dev->signal_state = 0; > > + spapr_vio_set_bypass(dev, false); > if (pc->reset) { > pc->reset(dev); > } > @@ -456,14 +469,25 @@ static int spapr_vio_busdev_init(DeviceState *qdev) > > if (pc->rtce_window_size) { > uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg; > + > + memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root", > + ram_size); > + memory_region_init_alias(&dev->mrbypass, OBJECT(dev), > + "iommu-spapr-bypass", get_system_memory(), > + 0, ram_size); > + memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1); > + address_space_init(&dev->as, &dev->mrroot, qdev->id); > + > dev->tcet = spapr_tce_new_table(qdev, liobn, > 0, > SPAPR_TCE_PAGE_SHIFT, > pc->rtce_window_size >> > SPAPR_TCE_PAGE_SHIFT, false); > - address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id); > + memory_region_add_subregion_overlap(&dev->mrroot, 0, > + spapr_tce_get_iommu(dev->tcet), 2); > } > > + > return pc->init(dev); > } > > @@ -541,6 +565,15 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data) > k->init = spapr_vio_bridge_init; > } > > +static int spapr_vio_post_load(void *opaque, int version_id) > +{ > + VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(opaque); > + > + spapr_vio_set_bypass(dev, dev->bypass); > + > + return 0; > +} > + > static const TypeInfo spapr_vio_bridge_info = { > .name = "spapr-vio-bridge", > .parent = TYPE_SYS_BUS_DEVICE, > @@ -550,8 +583,9 @@ static const TypeInfo spapr_vio_bridge_info = { > > const VMStateDescription vmstate_spapr_vio = { > .name = "spapr_vio", > - .version_id = 1, > + .version_id = 2, > .minimum_version_id = 1, I think the minimum version should be 2 as well, because migrating from version 1 will not set the bypass field correctly. Paolo > + .post_load = spapr_vio_post_load, > .fields = (VMStateField[]) { > /* Sanity check */ > VMSTATE_UINT32_EQUAL(reg, VIOsPAPRDevice), > @@ -563,6 +597,8 @@ const VMStateDescription vmstate_spapr_vio = { > VMSTATE_UINT32(crq.qsize, VIOsPAPRDevice), > VMSTATE_UINT32(crq.qnext, VIOsPAPRDevice), > > + VMSTATE_BOOL_V(bypass, VIOsPAPRDevice, 2), > + > VMSTATE_END_OF_LIST() > },
On 01/09/2015 09:05 PM, Paolo Bonzini wrote: > > > On 09/01/2015 02:02, Alexey Kardashevskiy wrote: >> @@ -100,7 +98,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { >> VMSTATE_UINT32_EQUAL(nb_table, sPAPRTCETable), >> >> /* IOMMU state */ >> - VMSTATE_BOOL(bypass, sPAPRTCETable), >> + VMSTATE_UNUSED(sizeof(bool)), > > sizeof(bool) can change across hosts, this needs to be the size that is > written by put_bool in migration/vmstate.c, i.e. VMSTATE_UNUSED(1). > >> VMSTATE_VARRAY_UINT32(table, sPAPRTCETable, nb_table, 0, vmstate_info_uint64, uint64_t), >> >> VMSTATE_END_OF_LIST() >> @@ -131,7 +129,8 @@ static int spapr_tce_table_realize(DeviceState *dev) >> trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd); >> >> memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops, >> - "iommu-spapr", ram_size); >> + "iommu-spapr", >> + (uint64_t)tcet->nb_table << tcet->page_shift); >> >> QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); >> >> @@ -191,17 +190,11 @@ MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet) >> return &tcet->iommu; >> } >> >> -void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass) >> -{ >> - tcet->bypass = bypass; >> -} >> - >> static void spapr_tce_reset(DeviceState *dev) >> { >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); >> size_t table_size = tcet->nb_table * sizeof(uint64_t); >> >> - tcet->bypass = false; >> memset(tcet->table, 0, table_size); >> } >> >> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c >> index dc9e46a..a1f2316 100644 >> --- a/hw/ppc/spapr_vio.c >> +++ b/hw/ppc/spapr_vio.c >> @@ -322,6 +322,18 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev) >> free_crq(dev); >> } >> >> +static void spapr_vio_set_bypass(VIOsPAPRDevice *dev, bool bypass) >> +{ >> + if (dev->bypass == bypass) { >> + return; >> + } >> + >> + memory_region_set_enabled(&dev->mrbypass, bypass); >> + memory_region_set_enabled(spapr_tce_get_iommu(dev->tcet), !bypass); >> + >> + dev->bypass = bypass; >> +} >> + >> static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> uint32_t token, >> uint32_t nargs, target_ulong args, >> @@ -348,7 +360,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> return; >> } >> >> - spapr_tce_set_bypass(dev->tcet, !!enable); >> + spapr_vio_set_bypass(dev, !!enable); >> >> rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> } >> @@ -407,6 +419,7 @@ static void spapr_vio_busdev_reset(DeviceState *qdev) >> >> dev->signal_state = 0; >> >> + spapr_vio_set_bypass(dev, false); >> if (pc->reset) { >> pc->reset(dev); >> } >> @@ -456,14 +469,25 @@ static int spapr_vio_busdev_init(DeviceState *qdev) >> >> if (pc->rtce_window_size) { >> uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg; >> + >> + memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root", >> + ram_size); >> + memory_region_init_alias(&dev->mrbypass, OBJECT(dev), >> + "iommu-spapr-bypass", get_system_memory(), >> + 0, ram_size); >> + memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1); >> + address_space_init(&dev->as, &dev->mrroot, qdev->id); >> + >> dev->tcet = spapr_tce_new_table(qdev, liobn, >> 0, >> SPAPR_TCE_PAGE_SHIFT, >> pc->rtce_window_size >> >> SPAPR_TCE_PAGE_SHIFT, false); >> - address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id); >> + memory_region_add_subregion_overlap(&dev->mrroot, 0, >> + spapr_tce_get_iommu(dev->tcet), 2); >> } >> >> + >> return pc->init(dev); >> } >> >> @@ -541,6 +565,15 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data) >> k->init = spapr_vio_bridge_init; >> } >> >> +static int spapr_vio_post_load(void *opaque, int version_id) >> +{ >> + VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(opaque); >> + >> + spapr_vio_set_bypass(dev, dev->bypass); >> + >> + return 0; >> +} >> + >> static const TypeInfo spapr_vio_bridge_info = { >> .name = "spapr-vio-bridge", >> .parent = TYPE_SYS_BUS_DEVICE, >> @@ -550,8 +583,9 @@ static const TypeInfo spapr_vio_bridge_info = { >> >> const VMStateDescription vmstate_spapr_vio = { >> .name = "spapr_vio", >> - .version_id = 1, >> + .version_id = 2, >> .minimum_version_id = 1, > > I think the minimum version should be 2 as well, because migrating from > version 1 will not set the bypass field correctly. This why the patch is "RFC" :) I can keep the flag in TCETable which would be a bit ugly but won't break migration. Is there any better way to keep compatibility? > > Paolo > >> + .post_load = spapr_vio_post_load, >> .fields = (VMStateField[]) { >> /* Sanity check */ >> VMSTATE_UINT32_EQUAL(reg, VIOsPAPRDevice), >> @@ -563,6 +597,8 @@ const VMStateDescription vmstate_spapr_vio = { >> VMSTATE_UINT32(crq.qsize, VIOsPAPRDevice), >> VMSTATE_UINT32(crq.qnext, VIOsPAPRDevice), >> >> + VMSTATE_BOOL_V(bypass, VIOsPAPRDevice, 2), >> + >> VMSTATE_END_OF_LIST() >> },
On 09/01/2015 13:53, Alexey Kardashevskiy wrote: >> > I think the minimum version should be 2 as well, because migrating from >> > version 1 will not set the bypass field correctly. > This why the patch is "RFC" :) > > I can keep the flag in TCETable which would be a bit ugly but won't break > migration. Is there any better way to keep compatibility? No, I don't think so. Paolo
On Fri, Jan 09, 2015 at 01:59:25PM +0100, Paolo Bonzini wrote: > > > On 09/01/2015 13:53, Alexey Kardashevskiy wrote: > >> > I think the minimum version should be 2 as well, because migrating from > >> > version 1 will not set the bypass field correctly. > > This why the patch is "RFC" :) > > > > I can keep the flag in TCETable which would be a bit ugly but won't break > > migration. Is there any better way to keep compatibility? > > No, I don't think so. Alas, there doesn't seem to be. I have the same thing with my RTC patches, where I have to keep the rtc_offset field in the sPAPREnvironment just to grab it on migration from an older version and stuff it into the new device instead.
On Fri, Jan 09, 2015 at 12:02:51PM +1100, Alexey Kardashevskiy wrote: > Instead of tweaking a TCE table device by adding there a bypass flag, > let's add an alias to RAM and IOMMU memory region, and enable/disable > those according to the selected bypass mode. > This way IOMMU memory region can have size of the actual window rather > than ram_size which is essential for upcoming DDW support. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > The main reason for this patch is DDW and the fact that sPAPRTCETable > used for DMA windows for VFIO. My latest approach removes all DMA windows > on the guest reset (and creates a new 32bit one) which means than VFIO > unmaps everything and this fails as normally sPAPRTCETable MemoryRegion is > ram_size big (to support bypass) while it should be 1-2GB. Paolo already mentioned the only significant problem I see with this, which is migration backwards compat. Otherwise it looks good, sorry I messed it up in the first place :). [snip] > @@ -456,14 +469,25 @@ static int spapr_vio_busdev_init(DeviceState *qdev) > > if (pc->rtce_window_size) { > uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg; > + > + memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root", > + ram_size); > + memory_region_init_alias(&dev->mrbypass, OBJECT(dev), > + "iommu-spapr-bypass", get_system_memory(), > + 0, ram_size); > + memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1); > + address_space_init(&dev->as, &dev->mrroot, qdev->id); > + > dev->tcet = spapr_tce_new_table(qdev, liobn, > 0, > SPAPR_TCE_PAGE_SHIFT, > pc->rtce_window_size >> > SPAPR_TCE_PAGE_SHIFT, false); > - address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id); > + memory_region_add_subregion_overlap(&dev->mrroot, 0, > + spapr_tce_get_iommu(dev->tcet), 2); > } > > + ^^ Tiny nit, extraneous whitespace change.
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 6c91d8e..cb01f8a 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -72,9 +72,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, .perm = IOMMU_NONE, }; - if (tcet->bypass) { - ret.perm = IOMMU_RW; - } else if ((addr >> tcet->page_shift) < tcet->nb_table) { + if ((addr >> tcet->page_shift) < tcet->nb_table) { /* Check if we are in bound */ hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); @@ -100,7 +98,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { VMSTATE_UINT32_EQUAL(nb_table, sPAPRTCETable), /* IOMMU state */ - VMSTATE_BOOL(bypass, sPAPRTCETable), + VMSTATE_UNUSED(sizeof(bool)), VMSTATE_VARRAY_UINT32(table, sPAPRTCETable, nb_table, 0, vmstate_info_uint64, uint64_t), VMSTATE_END_OF_LIST() @@ -131,7 +129,8 @@ static int spapr_tce_table_realize(DeviceState *dev) trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd); memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops, - "iommu-spapr", ram_size); + "iommu-spapr", + (uint64_t)tcet->nb_table << tcet->page_shift); QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list); @@ -191,17 +190,11 @@ MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet) return &tcet->iommu; } -void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass) -{ - tcet->bypass = bypass; -} - static void spapr_tce_reset(DeviceState *dev) { sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); size_t table_size = tcet->nb_table * sizeof(uint64_t); - tcet->bypass = false; memset(tcet->table, 0, table_size); } diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c index dc9e46a..a1f2316 100644 --- a/hw/ppc/spapr_vio.c +++ b/hw/ppc/spapr_vio.c @@ -322,6 +322,18 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev) free_crq(dev); } +static void spapr_vio_set_bypass(VIOsPAPRDevice *dev, bool bypass) +{ + if (dev->bypass == bypass) { + return; + } + + memory_region_set_enabled(&dev->mrbypass, bypass); + memory_region_set_enabled(spapr_tce_get_iommu(dev->tcet), !bypass); + + dev->bypass = bypass; +} + static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr, uint32_t token, uint32_t nargs, target_ulong args, @@ -348,7 +360,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr, return; } - spapr_tce_set_bypass(dev->tcet, !!enable); + spapr_vio_set_bypass(dev, !!enable); rtas_st(rets, 0, RTAS_OUT_SUCCESS); } @@ -407,6 +419,7 @@ static void spapr_vio_busdev_reset(DeviceState *qdev) dev->signal_state = 0; + spapr_vio_set_bypass(dev, false); if (pc->reset) { pc->reset(dev); } @@ -456,14 +469,25 @@ static int spapr_vio_busdev_init(DeviceState *qdev) if (pc->rtce_window_size) { uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg; + + memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root", + ram_size); + memory_region_init_alias(&dev->mrbypass, OBJECT(dev), + "iommu-spapr-bypass", get_system_memory(), + 0, ram_size); + memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1); + address_space_init(&dev->as, &dev->mrroot, qdev->id); + dev->tcet = spapr_tce_new_table(qdev, liobn, 0, SPAPR_TCE_PAGE_SHIFT, pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT, false); - address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id); + memory_region_add_subregion_overlap(&dev->mrroot, 0, + spapr_tce_get_iommu(dev->tcet), 2); } + return pc->init(dev); } @@ -541,6 +565,15 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data) k->init = spapr_vio_bridge_init; } +static int spapr_vio_post_load(void *opaque, int version_id) +{ + VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(opaque); + + spapr_vio_set_bypass(dev, dev->bypass); + + return 0; +} + static const TypeInfo spapr_vio_bridge_info = { .name = "spapr-vio-bridge", .parent = TYPE_SYS_BUS_DEVICE, @@ -550,8 +583,9 @@ static const TypeInfo spapr_vio_bridge_info = { const VMStateDescription vmstate_spapr_vio = { .name = "spapr_vio", - .version_id = 1, + .version_id = 2, .minimum_version_id = 1, + .post_load = spapr_vio_post_load, .fields = (VMStateField[]) { /* Sanity check */ VMSTATE_UINT32_EQUAL(reg, VIOsPAPRDevice), @@ -563,6 +597,8 @@ const VMStateDescription vmstate_spapr_vio = { VMSTATE_UINT32(crq.qsize, VIOsPAPRDevice), VMSTATE_UINT32(crq.qnext, VIOsPAPRDevice), + VMSTATE_BOOL_V(bypass, VIOsPAPRDevice, 2), + VMSTATE_END_OF_LIST() }, }; diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 749daf4..92f6fc1 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -458,7 +458,6 @@ struct sPAPRTCETable { uint64_t bus_offset; uint32_t page_shift; uint64_t *table; - bool bypass; bool vfio_accel; int fd; MemoryRegion iommu; @@ -474,7 +473,6 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, uint32_t nb_table, bool vfio_accel); MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); -void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass); int spapr_dma_dt(void *fdt, int node_off, const char *propname, uint32_t liobn, uint64_t window, uint32_t size); int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname, diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h index 46edc2a..90e3965 100644 --- a/include/hw/ppc/spapr_vio.h +++ b/include/hw/ppc/spapr_vio.h @@ -64,6 +64,9 @@ struct VIOsPAPRDevice { target_ulong signal_state; VIOsPAPR_CRQ crq; AddressSpace as; + bool bypass; + MemoryRegion mrroot; + MemoryRegion mrbypass; sPAPRTCETable *tcet; };
Instead of tweaking a TCE table device by adding there a bypass flag, let's add an alias to RAM and IOMMU memory region, and enable/disable those according to the selected bypass mode. This way IOMMU memory region can have size of the actual window rather than ram_size which is essential for upcoming DDW support. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- The main reason for this patch is DDW and the fact that sPAPRTCETable used for DMA windows for VFIO. My latest approach removes all DMA windows on the guest reset (and creates a new 32bit one) which means than VFIO unmaps everything and this fails as normally sPAPRTCETable MemoryRegion is ram_size big (to support bypass) while it should be 1-2GB. --- hw/ppc/spapr_iommu.c | 15 ++++----------- hw/ppc/spapr_vio.c | 42 +++++++++++++++++++++++++++++++++++++++--- include/hw/ppc/spapr.h | 2 -- include/hw/ppc/spapr_vio.h | 3 +++ 4 files changed, 46 insertions(+), 16 deletions(-)