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