linux-kernel - RE: [PATCH] uio_hv_generic: Enable interrupt for low speed VMBus devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <PH7PR21MB32633D172133A769357BD97ECE289@PH7PR21MB3263.namprd21.prod.outlook.com>
Date:   Tue, 18 Oct 2022 06:31:16 +0000
From:   Long Li <longli@...rosoft.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Saurabh Sengar <ssengar@...ux.microsoft.com>
CC:     Saurabh Singh Sengar <ssengar@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
Subject: RE: [PATCH] uio_hv_generic: Enable interrupt for low speed VMBus
 devices

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

> 
> 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@...ux.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?

Thanks,
Long