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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/11/2014 09:59 PM, Alexander Graf wrote:
> 
> 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 <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.
> 
> Can't you just add the pci config space bit for it to the e1000 emulation?

Sorry, I am not following you here. What bit in config space can enable
64bit DMA?

I tried patching the guest driver, that did not work so I did not dig further.

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


Yes. Why not? I cannot think of any case when we would not want this. In
practice there is very little chance it will ever be used anyway :) There
is still a machine option to disable it completely.


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

Uff. Very confusing review comments today :)

For VFIO, ddw_supported comes from the host kernel and totally depends on
hardware.

For emulated, there is just one emulated PHB (yes, can be many but noone
seems to be using more in reality) and what you suggest seems to be too
complicated.

This DDW thing - it is not really dynamic in the way it is used by the
existing linux guest. At the boot time the guest driver looks at DMA mask
and only if it is >32bit, it creates DDW, once, and after that the windows
remains active while the guest is running.


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


Where exactly does the default setting "on" break in 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, "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) &&
>> +        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);
> 


-- 
Alexey