Re: [Qemu-devel] [PATCH qemu v10 05/14] spapr_iommu: Introduce "enabled" state for TCE table

[Thomas Huth]

On Mon,  6 Jul 2015 12:11:01 +1000
Alexey Kardashevskiy <address@hidden> wrote:

> Currently TCE tables are created once at start and their size never
> changes. 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 stub object. 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 and allocates
> a guest view of the TCE table (in the user space or KVM).
> spapr_tce_table_disable() disposes the table.
> 
> Follow up patches will disable+enable tables on reset (system reset
> or DDW 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 migration 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 has all the properties set after the migration.
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
...
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c1ca13d..3ddd72f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -821,13 +821,12 @@ static int spapr_phb_dma_init_window(sPAPRPHBState 
> *sphb,
>                                       uint64_t window_size)
>  {
>      uint64_t bus_offset = sphb->dma32_window_start;
> -    sPAPRTCETable *tcet;
> +    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
>  
> -    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift,
> -                               window_size >> page_shift,
> -                               false);
> -
> -    return tcet ? 0 : -1;
> +    spapr_tce_table_enable(tcet, bus_offset, page_shift,
> +                           window_size >> page_shift,
> +                           false);
> +    return 0;
>  }

Would it be possible that this function is called with a window_size
where window_size >> page_shift results in 0?
For example triggered by a guest with a bad value for the RTAS call in
rtas_ibm_create_pe_dma_window() ?
In that case, the enablement would fail, but you'd still return 0 for
success here.
==> Maybe add a check for window_size >> page_shift == 0 and return an
error code in that case?

 Thomas