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

[Alexey Kardashevskiy]

On 08/12/2014 10:13 AM, Alexey Kardashevskiy wrote:
> On 08/12/2014 03:29 AM, Alexander Graf wrote:
>>
>> On 11.08.14 17:26, Alexey Kardashevskiy wrote:
>>> 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 <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?
>>> Sorry, I am not following you here. What bit in config space can enable
>>> 64bit DMA?
>>
>> Apparently there's nothing at all required. The igb driver simply tries to
>> use 64bit DMA masks.
> 
> A driver should use 64bit addresses (unsigned long, u64) for DMA, not 32bit
> (unsigned, u32).
> 
> 
>>
>>>
>>> I tried patching the guest driver, that did not work so I did not dig
>>> further.
>>
>> Which driver did you try it with?
> 
> 
> drivers/net/ethernet/intel/e1000/e1000_main.c
> 
> I looked again, the driver uses 64bit DMA if it is PCI-X-capable adapter
> which e1000 form QEMU is not.
> 
> 
>>
>>
>>>
>>>> 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.
>>
>> What I'm asking is that rather than having
>>
>>   if (machine->ddw_enabled && phb->ddw_supported)
>>
>> to instead only have
>>
>>   if (phb->ddw_enabled)
>>
>> which gets set by the machine to true if machine->ddw_enabled. If you make
>> it a qom property you can control the setter, so you can at the point when
>> the machine wants to set it also ignore the set to true if your vfio
>> implementation doesn't support ddw, leaving ddw_enabled as false.
>>
>>>
>>>
>>>> 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?
>>
>> Different device tree? Different return values on rtas calls? These are
>> guest visible changes, so in theory we would have to make sure we don't
>> change any of them.
>>
>> Of course we can always consciously declare them as unimportant enough that
>> they in reality shouldn't have side effects we care about for hot and live
>> migration, but there'd have to be a good reasoning on why we shouldn't have
>> it disabled rather than why we should have backwards compatibility.
> 
> "hot" migration? What is that? :)
> 
> There is a machine option to disable it and migrate to older guest (which
> we do not support afair or do we?). If we migrate to newer QEMU, these DDW
> tokens will be missing in the destination guest's tree and DDW won't be
> used, everybody is happy. I really fail to see a scenario when I would not
> use DDW...

Ok, Paul explained. So by default "ddw" must be off for the pseries-2.0
machine and on for pseries-2.2 and we'll be fine, right?



-- 
Alexey