[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC PATCH 08/10] spapr_pci: Enable DDW
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-ppc] [RFC PATCH 08/10] spapr_pci: Enable DDW |
Date: |
Tue, 12 Aug 2014 13:59:06 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 |
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