Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Oct 2022 08:40:00 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Long Li <longli@microsoft.com>
Cc:     Saurabh Sengar <ssengar@linux.microsoft.com>,
        Saurabh Singh Sengar <ssengar@microsoft.com>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        "wei.liu@kernel.org" <wei.liu@kernel.org>,
        Dexuan Cui <decui@microsoft.com>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Michael Kelley (LINUX)" <mikelley@microsoft.com>
Subject: Re: [PATCH] uio_hv_generic: Enable interrupt for low speed VMBus
 devices
Message-ID: <Y05KQFQUR5Is3RuC@kroah.com>
References: <1665685754-13971-1-git-send-email-ssengar@linux.microsoft.com>
 <Y0kRIcXG+wNbcGx0@kroah.com>
 <PH7PR21MB32633D172133A769357BD97ECE289@PH7PR21MB3263.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PH7PR21MB32633D172133A769357BD97ECE289@PH7PR21MB3263.namprd21.prod.outlook.com>
Precedence: bulk

On Tue, Oct 18, 2022 at 06:31:16AM +0000, Long Li wrote:
> > Subject: Re: [PATCH] uio_hv_generic: Enable interrupt for low speed VMBus
> > devices
> > 
> > On Thu, Oct 13, 2022 at 11:29:14AM -0700, Saurabh Sengar wrote:
> > > Hyper-V is adding some "specialty" synthetic devices.
> > 
> > What devices are those specifically?
> > 
> > > Instead of writing new kernel-level VMBus drivers for these devices,
> > > the devices will be presented to user space via this existing Hyper-V
> > > generic UIO driver, so that a user space driver can handle the device.
> > > Since these new synthetic devices are low speed devices, they don't
> > > support monitor bits and we must use vmbus_setevent() to enable
> > > interrupts from the host.
> > 
> > That is not what the UIO interface is for.  Please write real drivers so that
> > they tie into the specific user/kernel apis for those device types.
> > 
> > Without a specific list of what these devices are, I can not recommend that
> > anyone use the UIO api for them as that's probably not a good idea.
> 
> There are some VMBUS drivers currently not implemented in Linux. Out of all
> VMBUS drivers, two use "monitored bits": they are network and storage drivers.
> All the rest VMBUS drivers use hypercall for host notification and signal for next
> interrupt. One example of such driver is to collect process level crash information
> for diagnostic purposes.
> 
> Also, we want to move some existing kernel mode VMBUS drivers to user-space,
> such as hv_kvp and hv_filecopy. They don't really fit into an existing kernel API, and
> they create their own devices under /dev and communicates with a user-mode
> daemon to do most of the work. It's a better model that we can move those drivers
> entirely into user-mode.

How are you going to be able to remove drivers that export an existing
user/kernel api and not break current systems?

> > Also, if you do do this, you need to list where the source for that userspace
> > code is so that users can get it and have their distros package it up for them.  I
> > do not see that here at all.
> > 
> > 
> > >
> > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > ---
> > >  drivers/uio/uio_hv_generic.c | 9 +++------
> > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/uio/uio_hv_generic.c
> > > b/drivers/uio/uio_hv_generic.c index c08a6cfd119f..8e5aa4a1247f 100644
> > > --- a/drivers/uio/uio_hv_generic.c
> > > +++ b/drivers/uio/uio_hv_generic.c
> > > @@ -84,6 +84,9 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
> > >  	dev->channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> > >  	virt_mb();
> > >
> > > +	if (!dev->channel->offermsg.monitor_allocated && irq_state)
> > > +		vmbus_setevent(dev->channel);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -239,12 +242,6 @@ hv_uio_probe(struct hv_device *dev,
> > >  	void *ring_buffer;
> > >  	int ret;
> > >
> > > -	/* Communicating with host has to be via shared memory not
> > hypercall */
> > > -	if (!channel->offermsg.monitor_allocated) {
> > > -		dev_err(&dev->device, "vmbus channel requires
> > hypercall\n");
> > 
> > I do not understand, why is this check not made anymore here?  Why
> > constantly make the call above in the irq handler instead?  Isn't that going to
> > be massively slow?
> 
> Some VMBUS devices exposed by the Hyper-V are not modeled as high speed, 
> they use hypercall, not monitored bits. Because they don't fit into other kernel
> API (as explained above), can we use UIO for those devices?

UIO is for mmaped memory regions, like PCI devices, how is this a valid
Hyper-V api at all?

confused,

greg k-h