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

[Alexander Graf]

On 31.07.14 11:34, Alexey Kardashevskiy wrote:
> This implements DDW for emulated PHB.
>
> This advertises DDW in device tree.
>
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>
> 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.

Can't you just add the pci config space bit for it to the e1000 
emulation? That one should be pretty safe, no?

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

Unconditionally?

Also, can't you make the ddw enable/disable flow go set-only? Basically 
have the flag in the machine struct if you must, but then on every PHB 
instantiation you set a QOM property that sets ddw_supported respectively?

Also keep in mind that we will have to at least disable ddw by default 
for existing machine types to maintain backwards compatibility.

>   }
>   
>   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, "address@hidden" 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) &&
> +        phb->ddw_supported &&

Yeah, just rename this to ddw_enabled and expose it via QOM. Make it 
unsettable to true for PHBs that don't support ddw.


Alex

> +        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);