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

[Alexey Kardashevskiy]

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


-- 
Alexey