Re: [Qemu-devel] [RFC PATCH 08/10] spapr_pci: Enable DDW

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6588 bytes --]

On Thu, Jul 31, 2014 at 07:34:12PM +1000, Alexey Kardashevskiy wrote:
> This implements DDW for emulated PHB.
> 
> This advertises DDW in device tree.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> The DDW has not been tested as QEMU does not implement any 64bit DMA capable
> device and existing linux guests do not use DDW for 32bit DMA.
> ---
>  hw/ppc/spapr_pci.c          | 65 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/spapr.h |  5 ++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 230b59c..d1f4c86 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -22,6 +22,7 @@
>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>   * THE SOFTWARE.
>   */
> +#include "sysemu/sysemu.h"
>  #include "hw/hw.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/msi.h"
> @@ -650,6 +651,8 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>      /* Register default 32bit DMA window */
>      memory_region_add_subregion(&sphb->iommu_root, 0,
>                                  spapr_tce_get_iommu(tcet));
> +
> +    sphb->ddw_supported = true;
>  }
>  
>  static int spapr_phb_children_reset(Object *child, void *opaque)
> @@ -781,6 +784,42 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
>      return sphb->dtbusname;
>  }
>  
> +static int spapr_pci_ddw_query(sPAPRPHBState *sphb,
> +                               uint32_t *windows_available,
> +                               uint32_t *page_size_mask)
> +{
> +    *windows_available = 1;
> +    *page_size_mask = DDW_PGSIZE_16M;
> +
> +    return 0;
> +}
> +
> +static int spapr_pci_ddw_create(sPAPRPHBState *sphb, uint32_t page_shift,
> +                                uint32_t window_shift, uint32_t liobn,
> +                                sPAPRTCETable **ptcet)
> +{
> +    *ptcet = spapr_tce_new_table(DEVICE(sphb), liobn, SPAPR_PCI_TCE64_START,
> +                                 page_shift, 1 << (window_shift - page_shift),
> +                                 true);
> +    if (!*ptcet) {
> +        return -1;
> +    }
> +    memory_region_add_subregion(&sphb->iommu_root, (*ptcet)->bus_offset,
> +                                spapr_tce_get_iommu(*ptcet));
> +
> +    return 0;
> +}
> +
> +static int spapr_pci_ddw_remove(sPAPRPHBState *sphb, sPAPRTCETable *tcet)
> +{
> +    return 0;
> +}
> +
> +static int spapr_pci_ddw_reset(sPAPRPHBState *sphb)
> +{
> +    return 0;
> +}
> +
>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> @@ -795,6 +834,10 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->cannot_instantiate_with_device_add_yet = false;
>      spc->finish_realize = spapr_phb_finish_realize;
> +    spc->ddw_query = spapr_pci_ddw_query;
> +    spc->ddw_create = spapr_pci_ddw_create;
> +    spc->ddw_remove = spapr_pci_ddw_remove;
> +    spc->ddw_reset = spapr_pci_ddw_reset;
>  }
>  
>  static const TypeInfo spapr_phb_info = {
> @@ -878,6 +921,14 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      uint32_t interrupt_map_mask[] = {
>          cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
>      uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
> +    uint32_t ddw_applicable[] = {
> +        RTAS_IBM_QUERY_PE_DMA_WINDOW,
> +        RTAS_IBM_CREATE_PE_DMA_WINDOW,
> +        RTAS_IBM_REMOVE_PE_DMA_WINDOW
> +    };
> +    uint32_t ddw_extensions[] = { 1, RTAS_IBM_RESET_PE_DMA_WINDOW };
> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(phb);
> +    QemuOpts *machine_opts = qemu_get_machine_opts();
>  
>      /* Start populating the FDT */
>      sprintf(nodename, "pci@%" PRIx64, phb->buid);
> @@ -907,6 +958,20 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pci-config-space-type", 0x1));
>      _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS));
>  
> +    /* Dynamic DMA window */
> +    if (qemu_opt_get_bool(machine_opts, "ddw", true) &&

So, I think this is a rephrasing of agraf's objection.  This feels
like the wrong place to be checking the machine option.

I think the machine option should actually disable the DDW support on
the PHBs (i.e. cause the DDW RTAS calls to fail if attempted), rather
than just turn off advertisement in the device tree.

In fact it should probably stop the RTAS calls being registered at
all, at the moment the RTAS tokens will still be advertised in the
device tree, even if ddw is "disabled"


> +        phb->ddw_supported &&
> +        spc->ddw_query && spc->ddw_create && spc->ddw_remove) {
> +        _FDT(fdt_setprop(fdt, bus_off, "ibm,ddw-applicable", &ddw_applicable,
> +                         sizeof(ddw_applicable)));
> +
> +        if (spc->ddw_reset) {
> +            /* When enabled, the guest will remove the default 32bit window */
> +            _FDT(fdt_setprop(fdt, bus_off, "ibm,ddw-extensions",
> +                             &ddw_extensions, sizeof(ddw_extensions)));
> +        }
> +    }
> +
>      /* Build the interrupt-map, this must matches what is done
>       * in pci_spapr_map_irq
>       */
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 119d326..2046356 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -103,6 +103,8 @@ struct sPAPRPHBState {
>      int32_t msi_devs_num;
>      spapr_pci_msi_mig *msi_devs;
>  
> +    bool ddw_supported;
> +
>      QLIST_ENTRY(sPAPRPHBState) list;
>  };
>  
> @@ -125,6 +127,9 @@ struct sPAPRPHBVFIOState {
>  
>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>  
> +/* Default 64bit dynamic window offset */
> +#define SPAPR_PCI_TCE64_START        0x8000000000000000ULL
> +
>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>  {
>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);

Also, I think there might be a leak on system_reset here.  I can't see
anything that will clean up any secondary DDW TCE tables when the
whole system is reset.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]