Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
Hi Bryant, I love your patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on v4.17-rc2 next-20180424] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Bryant-G-Ly/misc-IBM-Virtual-Management-Channel-Driver/20180424-060306 config: powerpc64-allyesconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc64 Note: it may well be a FALSE warning. FWIW you are at least aware of it now. http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings All warnings (new ones prefixed by >>): drivers//misc/ibmvmc.c: In function 'ibmvmc_probe': >> drivers//misc/ibmvmc.c:2133:5: warning: 'rc' may be used uninitialized in >> this function [-Wmaybe-uninitialized] if (rc == H_RESOURCE) ^ drivers//misc/ibmvmc.c:2111:6: note: 'rc' was declared here int rc; ^~ vim +/rc +2133 drivers//misc/ibmvmc.c 2097 2098 /** 2099 * ibmvmc_init_crq_queue - Init CRQ Queue 2100 * 2101 * @adapter:crq_server_adapter struct 2102 * 2103 * Return: 2104 * 0 - Success 2105 * Non-zero - Failure 2106 */ 2107 static int ibmvmc_init_crq_queue(struct crq_server_adapter *adapter) 2108 { 2109 struct vio_dev *vdev = to_vio_dev(adapter->dev); 2110 struct crq_queue *queue = >queue; 2111 int rc; 2112 int retrc; 2113 2114 queue->msgs = (struct ibmvmc_crq_msg *)get_zeroed_page(GFP_KERNEL); 2115 2116 if (!queue->msgs) 2117 goto malloc_failed; 2118 2119 queue->size = PAGE_SIZE / sizeof(*queue->msgs); 2120 2121 queue->msg_token = dma_map_single(adapter->dev, queue->msgs, 2122queue->size * sizeof(*queue->msgs), 2123DMA_BIDIRECTIONAL); 2124 2125 if (dma_mapping_error(adapter->dev, queue->msg_token)) 2126 goto map_failed; 2127 2128 retrc = plpar_hcall_norets(H_REG_CRQ, 2129 vdev->unit_address, 2130 queue->msg_token, PAGE_SIZE); 2131 retrc = rc; 2132 > 2133 if (rc == H_RESOURCE) 2134 rc = ibmvmc_reset_crq_queue(adapter); 2135 2136 if (rc == 2) { 2137 dev_warn(adapter->dev, "Partner adapter not ready\n"); 2138 retrc = 0; 2139 } else if (rc != 0) { 2140 dev_err(adapter->dev, "Error %d opening adapter\n", rc); 2141 goto reg_crq_failed; 2142 } 2143 2144 queue->cur = 0; 2145 spin_lock_init(>lock); 2146 2147 tasklet_init(>work_task, ibmvmc_task, (unsigned long)adapter); 2148 2149 if (request_irq(vdev->irq, 2150 ibmvmc_handle_event, 2151 0, "ibmvmc", (void *)adapter) != 0) { 2152 dev_err(adapter->dev, "couldn't register irq 0x%x\n", 2153 vdev->irq); 2154 goto req_irq_failed; 2155 } 2156 2157 rc = vio_enable_interrupts(vdev); 2158 if (rc != 0) { 2159 dev_err(adapter->dev, "Error %d enabling interrupts!!!\n", rc); 2160 goto req_irq_failed; 2161 } 2162 2163 return retrc; 2164 2165 req_irq_failed: 2166 /* Cannot have any work since we either never got our IRQ registered, 2167 * or never got interrupts enabled 2168 */ 2169 tasklet_kill(>work_task); 2170 h_free_crq(vdev->unit_address); 2171 reg_crq_failed: 2172 dma_unmap_single(adapter->dev, 2173 queue->msg_token, 2174 queue->size * sizeof(*queue->msgs), DMA_BIDIRECTIONAL); 2175 map_failed: 2176 free_page((unsigned long)queue->msgs); 2177 malloc_failed: 2178 return -ENOMEM; 2179 } 2180 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
I don't know why I'm included in review of this driver, but why not :) it is good to include people. Thanks for your patch! It looks exciting and complicated, I like that kind of stuff. On Mon, Apr 23, 2018 at 4:46 PM, Bryant G. Lywrote: > This driver is a logical device which provides an > interface between the hypervisor and a management > partition. Please indicate what this management partition is to be used for, i.e. the intended use case. > This driver is to be used for the POWER Virtual > Management Channel Virtual Adapter on the PowerVM > platform. Is that virtualization on the PowerPC platform? > It provides both request/response and > async message support through the /dev/ibmvmc node. Is that a character or block device? I guess block, but it's nice to have some specifics in the commit message. > diff --git a/Documentation/misc-devices/ibmvmc.txt > b/Documentation/misc-devices/ibmvmc.txt The document suffers a bit from internal lingo, so it's be great if you improve it a bit with a style that will be welcoming for newcomers who have no idea what you are talking about. I will point out some items below. > +Description > +=== > + > +The Virtual Management Channel (VMC) is a logical device which provides an > +interface between the hypervisor and a management partition. What kind of interface? A bitstream? Message passing interface? Marshalling of structs? Some specifics would be great. > This > +management partition is intended to provide an alternative to HMC-based > +system management. Please expand the acronym HMC for the newcomer so we do not need to look it up, or it looks like the document is only for people who already know everything about virtualization specifics. > In the management partition, a Novalink application > +exists Is Novalink some kind of product name? In that case, can it be instead described with some neutral technology name and state Novalink as an examplf of this type of technology? > which enables a system administrator to configure the system’s > +partitioning characteristics Exemplify "partitioning characteristics", with a sentence in this vein: "partitioning characteristics such as FOO, BAR and BAZ". > via a command line interface (CLI) or > +Representational State Transfer Application (REST). I understand what CLI is but what is a representational state transfer application? Is that a fancy name for a GUI application doing the same thing as a CLI program would do? > You can also manage the > +server by using PowerVC There was PowerVM now there is PowerVC. What are those things really? PowerVM I guess is virtual machine on PowerPC? What is PowerVC? Virtual client (guest) in the PowerVM? This kind of lingo makes the document a hard read for me. > or other OpenStack solution. I heard about OpenStack. I have a vague idea about what it is, do you mean "other virtualization concepts"? > Support for > +conventional HMC management of the system may still be provided on a > +system; however, when an HMC is attached to the system, the VMC > +interface is disabled by the hypervisor. The VMC acronym is expanded below, instead expand it at first instance. The same for HMC (hypervisor management channel I guess? it is never really explained.) > +NovaLink runs on a Linux logical partition on a POWER8 or newer processor- > +based server that is virtualized by PowerVM. So this information needs to be in the top of the document. > System configuration, > +maintenance, and control functions which traditionally require an HMC can > +be implemented in the Novalink using a combination of HMC to hypervisor > +interfaces and existing operating system methods. This tool provides What tool? It is unclear what tool we are referring to here. A command line tool? A technology? etc. > a > +subset of the functions implemented by the HMC and enables basic partition > +configuration. The set of HMC to hypervisor messages This makes me think that probably the "h" in HMC stands for "hypervisor" so now I start to puzzle things together. But it's nicer if you just expand the acronym the first time you use it. > supported by the > +Novalink component I still haven't figured out what that is, in practice. > are passed to the hypervisor over a VMC interface, which > +is defined below. Maybe some illustration of how HMC, Novalink and VMC work together would be appropriate? > +Virtual Management Channel (VMC) > +A logical device, called the virtual management channel (VMC), is defined > +for communicating between the Novalink application and the hypervisor. So is "Novalink" some specific application, as in a proprietary piece of software or something? I'm still confused. > +This device, similar to a VSCSI server device, What is that acronym now, I get completely confused. > is presented to a designated > +management partition as a virtual device and is only presented when the > +system is not HMC managed. > +This
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
On 04/24/2018 07:29 AM, Greg KH wrote: > On Mon, Apr 23, 2018 at 02:17:28PM -0700, Randy Dunlap wrote: >> On 04/23/18 12:53, Greg KH wrote: >>> On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote: On 04/23/18 07:46, Bryant G. Ly wrote: > This driver is a logical device which provides an > interface between the hypervisor and a management > partition. > > This driver is to be used for the POWER Virtual > Management Channel Virtual Adapter on the PowerVM > platform. It provides both request/response and > async message support through the /dev/ibmvmc node. > > Signed-off-by: Bryant G. Ly> Reviewed-by: Steven Royer > Reviewed-by: Adam Reznechek > Tested-by: Taylor Jakobson > Tested-by: Brad Warrum > Cc: Greg Kroah-Hartman > Cc: Arnd Bergmann > Cc: Benjamin Herrenschmidt > Cc: Michael Ellerman > --- > Documentation/ioctl/ioctl-number.txt |1 + > Documentation/misc-devices/ibmvmc.txt | 161 +++ > MAINTAINERS |6 + > arch/powerpc/include/asm/hvcall.h |1 + > drivers/misc/Kconfig | 14 + > drivers/misc/Makefile |1 + > drivers/misc/ibmvmc.c | 2415 > + > drivers/misc/ibmvmc.h | 209 +++ > 8 files changed, 2808 insertions(+) > create mode 100644 Documentation/misc-devices/ibmvmc.txt > create mode 100644 drivers/misc/ibmvmc.c > create mode 100644 drivers/misc/ibmvmc.h > diff --git a/Documentation/misc-devices/ibmvmc.txt > b/Documentation/misc-devices/ibmvmc.txt > new file mode 100644 > index 000..bae1064 > --- /dev/null > +++ b/Documentation/misc-devices/ibmvmc.txt >>> >>> Aren't we doing new documentation in .rst format instead of .txt? >> >> I am not aware that .rst format is a *requirement* for new documentation. >> >> ?? > > Why wouldn't you create new documentation in that format? Just saves > the step of having to change it again in the future. Not everything needs to be in that format. If a document contains graphics or lots of tables, then sure, use RST. Do you do kernel Documentation builds yourself? Do you know how slow they are? -- ~Randy
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
On Mon, Apr 23, 2018 at 02:17:28PM -0700, Randy Dunlap wrote: > On 04/23/18 12:53, Greg KH wrote: > > On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote: > >> On 04/23/18 07:46, Bryant G. Ly wrote: > >>> This driver is a logical device which provides an > >>> interface between the hypervisor and a management > >>> partition. > >>> > >>> This driver is to be used for the POWER Virtual > >>> Management Channel Virtual Adapter on the PowerVM > >>> platform. It provides both request/response and > >>> async message support through the /dev/ibmvmc node. > >>> > >>> Signed-off-by: Bryant G. Ly> >>> Reviewed-by: Steven Royer > >>> Reviewed-by: Adam Reznechek > >>> Tested-by: Taylor Jakobson > >>> Tested-by: Brad Warrum > >>> Cc: Greg Kroah-Hartman > >>> Cc: Arnd Bergmann > >>> Cc: Benjamin Herrenschmidt > >>> Cc: Michael Ellerman > >>> --- > >>> Documentation/ioctl/ioctl-number.txt |1 + > >>> Documentation/misc-devices/ibmvmc.txt | 161 +++ > >>> MAINTAINERS |6 + > >>> arch/powerpc/include/asm/hvcall.h |1 + > >>> drivers/misc/Kconfig | 14 + > >>> drivers/misc/Makefile |1 + > >>> drivers/misc/ibmvmc.c | 2415 > >>> + > >>> drivers/misc/ibmvmc.h | 209 +++ > >>> 8 files changed, 2808 insertions(+) > >>> create mode 100644 Documentation/misc-devices/ibmvmc.txt > >>> create mode 100644 drivers/misc/ibmvmc.c > >>> create mode 100644 drivers/misc/ibmvmc.h > >> > >>> diff --git a/Documentation/misc-devices/ibmvmc.txt > >>> b/Documentation/misc-devices/ibmvmc.txt > >>> new file mode 100644 > >>> index 000..bae1064 > >>> --- /dev/null > >>> +++ b/Documentation/misc-devices/ibmvmc.txt > > > > Aren't we doing new documentation in .rst format instead of .txt? > > I am not aware that .rst format is a *requirement* for new documentation. > > ?? Why wouldn't you create new documentation in that format? Just saves the step of having to change it again in the future. thanks, greg k-h
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
On 04/23/18 12:53, Greg KH wrote: > On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote: >> On 04/23/18 07:46, Bryant G. Ly wrote: >>> This driver is a logical device which provides an >>> interface between the hypervisor and a management >>> partition. >>> >>> This driver is to be used for the POWER Virtual >>> Management Channel Virtual Adapter on the PowerVM >>> platform. It provides both request/response and >>> async message support through the /dev/ibmvmc node. >>> >>> Signed-off-by: Bryant G. Ly>>> Reviewed-by: Steven Royer >>> Reviewed-by: Adam Reznechek >>> Tested-by: Taylor Jakobson >>> Tested-by: Brad Warrum >>> Cc: Greg Kroah-Hartman >>> Cc: Arnd Bergmann >>> Cc: Benjamin Herrenschmidt >>> Cc: Michael Ellerman >>> --- >>> Documentation/ioctl/ioctl-number.txt |1 + >>> Documentation/misc-devices/ibmvmc.txt | 161 +++ >>> MAINTAINERS |6 + >>> arch/powerpc/include/asm/hvcall.h |1 + >>> drivers/misc/Kconfig | 14 + >>> drivers/misc/Makefile |1 + >>> drivers/misc/ibmvmc.c | 2415 >>> + >>> drivers/misc/ibmvmc.h | 209 +++ >>> 8 files changed, 2808 insertions(+) >>> create mode 100644 Documentation/misc-devices/ibmvmc.txt >>> create mode 100644 drivers/misc/ibmvmc.c >>> create mode 100644 drivers/misc/ibmvmc.h >> >>> diff --git a/Documentation/misc-devices/ibmvmc.txt >>> b/Documentation/misc-devices/ibmvmc.txt >>> new file mode 100644 >>> index 000..bae1064 >>> --- /dev/null >>> +++ b/Documentation/misc-devices/ibmvmc.txt > > Aren't we doing new documentation in .rst format instead of .txt? I am not aware that .rst format is a *requirement* for new documentation. ?? -- ~Randy
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
On 4/23/18 2:53 PM, Greg KH wrote: > On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote: >> On 04/23/18 07:46, Bryant G. Ly wrote: >>> This driver is a logical device which provides an >>> interface between the hypervisor and a management >>> partition. >>> >>> This driver is to be used for the POWER Virtual >>> Management Channel Virtual Adapter on the PowerVM >>> platform. It provides both request/response and >>> async message support through the /dev/ibmvmc node. >>> >>> Signed-off-by: Bryant G. Ly>>> Reviewed-by: Steven Royer >>> Reviewed-by: Adam Reznechek >>> Tested-by: Taylor Jakobson >>> Tested-by: Brad Warrum >>> Cc: Greg Kroah-Hartman >>> Cc: Arnd Bergmann >>> Cc: Benjamin Herrenschmidt >>> Cc: Michael Ellerman >>> --- >>> Documentation/ioctl/ioctl-number.txt |1 + >>> Documentation/misc-devices/ibmvmc.txt | 161 +++ >>> MAINTAINERS |6 + >>> arch/powerpc/include/asm/hvcall.h |1 + >>> drivers/misc/Kconfig | 14 + >>> drivers/misc/Makefile |1 + >>> drivers/misc/ibmvmc.c | 2415 >>> + >>> drivers/misc/ibmvmc.h | 209 +++ >>> 8 files changed, 2808 insertions(+) >>> create mode 100644 Documentation/misc-devices/ibmvmc.txt >>> create mode 100644 drivers/misc/ibmvmc.c >>> create mode 100644 drivers/misc/ibmvmc.h >>> diff --git a/Documentation/misc-devices/ibmvmc.txt >>> b/Documentation/misc-devices/ibmvmc.txt >>> new file mode 100644 >>> index 000..bae1064 >>> --- /dev/null >>> +++ b/Documentation/misc-devices/ibmvmc.txt > Aren't we doing new documentation in .rst format instead of .txt? > > thanks, > > greg k-h > I will convert to .rst and fix the comments from Randy in the next version. Thanks, Bryant
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote: > On 04/23/18 07:46, Bryant G. Ly wrote: > > This driver is a logical device which provides an > > interface between the hypervisor and a management > > partition. > > > > This driver is to be used for the POWER Virtual > > Management Channel Virtual Adapter on the PowerVM > > platform. It provides both request/response and > > async message support through the /dev/ibmvmc node. > > > > Signed-off-by: Bryant G. Ly> > Reviewed-by: Steven Royer > > Reviewed-by: Adam Reznechek > > Tested-by: Taylor Jakobson > > Tested-by: Brad Warrum > > Cc: Greg Kroah-Hartman > > Cc: Arnd Bergmann > > Cc: Benjamin Herrenschmidt > > Cc: Michael Ellerman > > --- > > Documentation/ioctl/ioctl-number.txt |1 + > > Documentation/misc-devices/ibmvmc.txt | 161 +++ > > MAINTAINERS |6 + > > arch/powerpc/include/asm/hvcall.h |1 + > > drivers/misc/Kconfig | 14 + > > drivers/misc/Makefile |1 + > > drivers/misc/ibmvmc.c | 2415 > > + > > drivers/misc/ibmvmc.h | 209 +++ > > 8 files changed, 2808 insertions(+) > > create mode 100644 Documentation/misc-devices/ibmvmc.txt > > create mode 100644 drivers/misc/ibmvmc.c > > create mode 100644 drivers/misc/ibmvmc.h > > > diff --git a/Documentation/misc-devices/ibmvmc.txt > > b/Documentation/misc-devices/ibmvmc.txt > > new file mode 100644 > > index 000..bae1064 > > --- /dev/null > > +++ b/Documentation/misc-devices/ibmvmc.txt Aren't we doing new documentation in .rst format instead of .txt? thanks, greg k-h
Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
On 04/23/18 07:46, Bryant G. Ly wrote: > This driver is a logical device which provides an > interface between the hypervisor and a management > partition. > > This driver is to be used for the POWER Virtual > Management Channel Virtual Adapter on the PowerVM > platform. It provides both request/response and > async message support through the /dev/ibmvmc node. > > Signed-off-by: Bryant G. Ly> Reviewed-by: Steven Royer > Reviewed-by: Adam Reznechek > Tested-by: Taylor Jakobson > Tested-by: Brad Warrum > Cc: Greg Kroah-Hartman > Cc: Arnd Bergmann > Cc: Benjamin Herrenschmidt > Cc: Michael Ellerman > --- > Documentation/ioctl/ioctl-number.txt |1 + > Documentation/misc-devices/ibmvmc.txt | 161 +++ > MAINTAINERS |6 + > arch/powerpc/include/asm/hvcall.h |1 + > drivers/misc/Kconfig | 14 + > drivers/misc/Makefile |1 + > drivers/misc/ibmvmc.c | 2415 > + > drivers/misc/ibmvmc.h | 209 +++ > 8 files changed, 2808 insertions(+) > create mode 100644 Documentation/misc-devices/ibmvmc.txt > create mode 100644 drivers/misc/ibmvmc.c > create mode 100644 drivers/misc/ibmvmc.h > diff --git a/Documentation/misc-devices/ibmvmc.txt > b/Documentation/misc-devices/ibmvmc.txt > new file mode 100644 > index 000..bae1064 > --- /dev/null > +++ b/Documentation/misc-devices/ibmvmc.txt > @@ -0,0 +1,161 @@ > +Kernel Driver ibmvmc > + > + > +Authors: > + Dave Engebretsen > + Adam Reznechek > + Steven Royer > + Bryant G. Ly > + > +Description > +=== > + ... > + > +Virtual Management Channel (VMC) > +A logical device, called the virtual management channel (VMC), is defined > +for communicating between the Novalink application and the hypervisor. > +This device, similar to a VSCSI server device, is presented to a designated > +management partition as a virtual device and is only presented when the > +system is not HMC managed. > +This communication device borrows aspects from both VSCSI and ILLAN devices > +and is implemented using the CRQ and the RDMA interfaces. A three-way > +handshake is defined that must take place to establish that both the > +hypervisor and management partition sides of the channel are running prior > +to sending/receiving any of the protocol messages. > +This driver also utilizes Transport Event CRQs. CRQ messages that are sent Drop "that" ? otherwise the sentence construction is ... odd. > +when the hypervisor detects one of the peer partitions has abnormally > +terminated, or one side has called H_FREE_CRQ to close their CRQ. > +Two new classes of CRQ messages are introduced for the VMC device. VMC > +Administrative messages are used for each partition using the VMC to > +communicate capabilities to their partner. HMC Interface messages are used > +for the actual flow of HMC messages between the management partition and > +the hypervisor. As most HMC messages far exceed the size of a CRQ bugger, what is a bugger? [reads more] oh, buffer? > +a virtual DMA (RMDA) of the HMC message data is done prior to each HMC > +Interface CRQ message. Only the management partition drives RDMA > +operations; hypervisors never directly causes the movement of message data. > + > +Example Management Partition VMC Driver Interface > += ... Looks pretty good. In general (IMO), it could use more white space (like blank lines after section names.) If you fix those small items (buggers) above, you can add: Reviewed-by: Randy Dunlap thanks, -- ~Randy