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

[Alexander Graf]

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.

> I tried patching the guest driver, that did not work so I did not dig further.

Which driver did you try it with?


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


Alex