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

[Alexander Graf <agraf@suse.de>]

On 12.08.14 05:59, Alexey Kardashevskiy wrote:
> 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 <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?
>>> 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.

Does it decide this only based on the pci id? If so, fake it.

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

Yes.


Alex