Re: [Qemu-devel] [PATCH qemu v2 0/4] spapr_pci, vfio: NVIDIA V100 + POWER9 passthrough

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4505 bytes --]

On Mon, Feb 25, 2019 at 05:39:06PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 15/02/2019 16:21, David Gibson wrote:
> > On Fri, Feb 15, 2019 at 03:34:52PM +1100, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 15/02/2019 14:54, David Gibson wrote:
> >>> On Fri, Feb 15, 2019 at 02:32:14PM +1100, Alexey Kardashevskiy wrote:
> >>>>
> >>>>
> >>>> On 15/02/2019 14:24, David Gibson wrote:
> >>>>> On Fri, Feb 15, 2019 at 11:35:02AM +1100, Alexey Kardashevskiy wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 15/02/2019 10:37, Alex Williamson wrote:
> >>>>>>> On Thu, 14 Feb 2019 16:21:40 +1100
> >>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>>>>>
> >>>>>>>> This is for passing through NVIDIA V100 GPUs on POWER9 systems.
> >>>>>>>>
> >>>>>>>> This implements a subdriver for NVIDIA V100 GPU with coherent memory and
> >>>>>>>> NPU/ATS support available in the POWER9 CPU.
> >>>>>>>>
> >>>>>>>> 1/4 is a preparation for bigger DMA windows.
> >>>>>>>> 2/4 is a small cleanup.
> >>>>>>>>
> >>>>>>>> Here is the kernel driver:
> >>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/vfio/pci?h=v5.0-rc6&id=7f92891778dff62303c070ac81de7b7d80de331a
> >>>>>>>>
> >>>>>>>> SLOF changes already went in.
> >>>>>>>>
> >>>>>>>> This depends on "pci: Move NVIDIA vendor id to the rest of ids" (posted separately).
> >>>>>>>
> >>>>>>> TBH, I'm not sure it was the best idea to let it live or die on it's
> >>>>>>> own when it now creates a build dependency for this series.
> >>>>>> I am sure that patch is disgustingly primitive and can make it to
> >>>>>> upstream in just one click and the rest of the series will take more
> >>>>>> time anyway (always does :) ).
> >>>>>>
> >>>>>>>> This is based on sha1
> >>>>>>>> 1ea6057 Mark Cave-Ayland "mac_newworld: change default NIC to sungem for mac99 machine".
> >>>>>>>
> >>>>>>> Perhaps this is why it doesn't apply cleanly against qemu.git.  Are
> >>>>>>> there dependencies we need to wait for in the ppc tree as well?
> >>>>>>
> >>>>>> There are few changes in spapr_pci so conflicts are possible, every time
> >>>>>> when David updates his tree and I rebase I get some minor ones.
> >>>>>>
> >>>>>>>> Please comment. Thanks.
> >>>>>>>
> >>>>>>> Besides the build dependency on PCI_VENDOR_ID_NVIDIA, I also get this:
> >>>>>>>
> >>>>>>> .../qemu.git/hw/vfio/spapr.c: In function ‘vfio_spapr_create_window’:
> >>>>>>> .../qemu.git/hw/vfio/spapr.c:212:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >>>>>>>          error_report("Failed to create a window, ret = %d (%m)", ret);
> >>>>>>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>>> cc1: all warnings being treated as errors
> >>>>>>
> >>>>>> Agrh. How exactly do you make them errors, not warnings? I get no
> >>>>>> warning/error with --disable-werror but not having  --disable-werror
> >>>>>> prints warnings, not errors so it does not fail the build and easy to miss.
> >>>>>
> >>>>> Are you sure it's printing warnings with --enable-werror?  Otherwise
> >>>>> it sounds like you just have a compiler version that's not picking
> >>>>> this up.
> >>>>
> >>>> Ah, here is my mistake - I thought not having --disable-werror means
> >>>> that it is enabled as ./configure does not advertise  "--enable-werror":
> >>>>
> >>>> [fstn1-p1 qemu]$ ./configure --help | grep werror
> >>>>   --disable-werror         disable compilation abort on warning
> >>>>
> >>>>
> >>>> Apparently it is a tri-state flag :)
> >>>
> >>> Um.. no.. I believe it should be on by default as well, I was just
> >>> saying --enable-werror because I thought it was clearer than the
> >>> double negative in "without --disable-werror".
> >>
> >> The default value of werror is "":
> >>
> >> https://git.qemu.org/?p=qemu.git;a=blob;f=configure;h=a61682c3c727f467ce2710c7469a33b261da8ff6;hb=HEAD#l935
> > 
> > Which turns into a "yes" on Linux - see line 1835-1844.
> 
> 
> No it does not - the 'test -d "$source_path/.git"' fails as "git
> worktree" makes .git a file, not a folder.

Oh, right.  Turns into a "yes" for people who don't have weird git
setups :-p.  Care to submit a patch to make it do the right thing in
your setup?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]