[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qemu v14 05/18] spapr_iommu: Introduce "enabled"
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH qemu v14 05/18] spapr_iommu: Introduce "enabled" state for TCE table |
Date: |
Tue, 22 Mar 2016 12:11:51 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Mon, Mar 21, 2016 at 06:46:53PM +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; the same is
> done for spapr_tce_table_disable().
>
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
R-b stands, but I noticed one nit:
> ---
> Changes:
> v14:
> * added spapr_tce_table_do_disable(), will make difference in following
> patch with fully dynamic table migration
> ---
> hw/ppc/spapr_iommu.c | 86
> ++++++++++++++++++++++++++++++++++++--------------
> hw/ppc/spapr_pci.c | 13 ++++++--
> hw/ppc/spapr_vio.c | 8 ++---
> include/hw/ppc/spapr.h | 10 +++---
> 4 files changed, 81 insertions(+), 36 deletions(-)
>
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 8132f64..9bcd3f6 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -17,6 +17,7 @@
> * License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> */
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "hw/hw.h"
> #include "sysemu/kvm.h"
> #include "hw/qdev.h"
> @@ -174,15 +175,9 @@ 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);
> -
> + tcet->need_vfio = false;
> 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 +219,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 +230,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 +241,69 @@ 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)
> +{
> + if (tcet->enabled) {
> + error_report("Warning: trying to enable already enabled TCE table");
> + return;
> + }
> +
> + tcet->bus_offset = bus_offset;
> + tcet->page_shift = page_shift;
> + tcet->nb_table = nb_table;
> +
> + spapr_tce_table_do_enable(tcet);
> +}
> +
> +static void spapr_tce_table_do_disable(sPAPRTCETable *tcet)
> +{
> + memory_region_set_size(&tcet->iommu, 0);
> +
> + spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table);
> + tcet->fd = -1;
> + tcet->table = NULL;
> + tcet->enabled = false;
> + tcet->bus_offset = 0;
> + tcet->page_shift = 0;
> + tcet->nb_table = 0;
> +}
> +
> +static void spapr_tce_table_disable(sPAPRTCETable *tcet)
> +{
> + if (!tcet->enabled) {
> + error_report("Warning: trying to disable already disabled TCE
> table");
> + return;
> + }
> + spapr_tce_table_do_disable(tcet);
> +}
> +
> static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
> {
> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>
> QLIST_REMOVE(tcet, list);
>
> - spapr_tce_free_table(tcet->table, tcet->fd, tcet->nb_table);
> - tcet->fd = -1;
> + spapr_tce_table_disable(tcet);
> }
>
> MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 18332bf..df5f7b9 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -818,14 +818,15 @@ static void spapr_phb_dma_window_enable(sPAPRPHBState
> *sphb,
> return;
> }
>
> - tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr,
> - page_shift, nb_table, false);
> + tcet = spapr_tce_find_by_liobn(liobn);
> if (!tcet) {
> error_setg(errp, "Unable to create TCE table liobn %x for %s",
> liobn, sphb->dtbusname);
> return;
> }
>
> + spapr_tce_table_enable(tcet, page_shift, window_addr, nb_table);
> +
> memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
> spapr_tce_get_iommu(tcet));
> }
> @@ -1335,6 +1336,7 @@ static void spapr_phb_realize(DeviceState *dev, Error
> **errp)
> PCIBus *bus;
> uint64_t msi_window_size = 4096;
> Error *local_err = NULL;
> + sPAPRTCETable *tcet;
>
> if (sphb->index != (uint32_t)-1) {
> hwaddr windows_base;
> @@ -1486,6 +1488,13 @@ static void spapr_phb_realize(DeviceState *dev, Error
> **errp)
> }
> }
>
> + /* DMA setup */
> + tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn);
> + if (!tcet) {
> + error_report("No default TCE table for %s", sphb->dtbusname);
> + return;
You have an errp in this function so you should error_setg(), rather
than just error_report().
> + }
> +
> /* Register default 32bit DMA window */
> spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT,
> sphb->dma_win_addr, sphb->dma_win_size,
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 0f61a55..7f57290 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -481,11 +481,9 @@ static void spapr_vio_busdev_realize(DeviceState *qdev,
> Error **errp)
> 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);
> + dev->tcet = spapr_tce_new_table(qdev, liobn);
> + spapr_tce_table_enable(dev->tcet, SPAPR_TCE_PAGE_SHIFT, 0,
> + pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT);
> dev->tcet->vdev = dev;
> memory_region_add_subregion_overlap(&dev->mrroot, 0,
> spapr_tce_get_iommu(dev->tcet),
> 2);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 098d85d..75b0b55 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -539,6 +539,7 @@ typedef struct sPAPRTCETable sPAPRTCETable;
>
> struct sPAPRTCETable {
> DeviceState parent;
> + bool enabled;
> uint32_t liobn;
> uint32_t nb_table;
> uint64_t bus_offset;
> @@ -566,11 +567,10 @@ void spapr_events_fdt_skel(void *fdt, uint32_t
> epow_irq);
> int spapr_h_cas_compose_response(sPAPRMachineState *sm,
> target_ulong addr, target_ulong size,
> bool cpu_update, bool memory_update);
> -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);
> +void spapr_tce_table_enable(sPAPRTCETable *tcet,
> + uint32_t page_shift, uint64_t bus_offset,
> + uint32_t nb_table);
> void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio);
>
> MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
--
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
- [Qemu-devel] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, (continued)
- [Qemu-devel] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 13/18] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering), Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 11/18] memory: Add reporting of supported page sizes, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 02/18] vmstate: Define VARRAY with VMS_ALLOC, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 09/18] spapr_iommu: Add root memory region, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 05/18] spapr_iommu: Introduce "enabled" state for TCE table, Alexey Kardashevskiy, 2016/03/21
- Re: [Qemu-devel] [PATCH qemu v14 05/18] spapr_iommu: Introduce "enabled" state for TCE table,
David Gibson <=
- [Qemu-devel] [PATCH qemu v14 07/18] spapr_iommu: Realloc table during migration, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 10/18] spapr_pci: Reset DMA config on PHB reset, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 16/18] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping being used by VFIO, Alexey Kardashevskiy, 2016/03/21
- [Qemu-devel] [PATCH qemu v14 01/18] memory: Fix IOMMU replay base address, Alexey Kardashevskiy, 2016/03/21