linux-kernel - RE: [PATCH] uio_hv_generic: Set event for all channels on the device

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] [day] [month] [year] [list]

Message-ID:
 <SA6PR21MB42317B235E4904DC253E27E4CECC2@SA6PR21MB4231.namprd21.prod.outlook.com>
Date: Fri, 28 Feb 2025 20:14:22 +0000
From: Long Li <longli@...rosoft.com>
To: Michael Kelley <mhklinux@...look.com>, "longli@...uxonhyperv.com"
	<longli@...uxonhyperv.com>, KY Srinivasan <kys@...rosoft.com>, Haiyang Zhang
	<haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, Dexuan Cui
	<decui@...rosoft.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] uio_hv_generic: Set event for all channels on the device

> From: longli@...uxonhyperv.com <longli@...uxonhyperv.com> Sent:
> Wednesday, February 26, 2025 4:46 PM
> >
> > Hyper-V may offer a non latency sensitive device with subchannels
> > without monitor bit enabled. The decision is entirely on the Hyper-V
> > host not configurable within guest.
> >
> > When a device has subchannels, also signal events for the subchannel
> > if its monitor bit is disabled.
> >
> > Signed-off-by: Long Li <longli@...rosoft.com>
> > ---
> >  drivers/uio/uio_hv_generic.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/uio/uio_hv_generic.c
> > b/drivers/uio/uio_hv_generic.c index 3976360d0096..8b6df598a728
> 100644
> > --- a/drivers/uio/uio_hv_generic.c
> > +++ b/drivers/uio/uio_hv_generic.c
> > @@ -65,6 +65,16 @@ struct hv_uio_private_data {
> >  	char	send_name[32];
> >  };
> >
> > +static void set_event(struct vmbus_channel *channel, s32 irq_state) {
> > +	channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> > +	if (!channel->offermsg.monitor_allocated && irq_state) {
> > +		/* MB is needed for host to see the interrupt mask first */
> > +		virt_mb();
> > +		vmbus_setevent(channel);
> 
> A minor point, but vmbus_setevent() checks the "monitor_allocated"
> flag, and if not set, then calls vmbus_set_event().  Since
> monitor_allocated() is already checked here, couldn't vmbus_set_event() be
> called directly?  The existing code calls vmbus_setevent() so keeping
> vmbus_setevent() is less of a change, but it is still doing a redundant check.

Will change it to vmbus_set_event().

> 
> > +	}
> > +}
> > +
> >  /*
> >   * This is the irqcontrol callback to be registered to uio_info.
> >   * It can be used to disable/enable interrupt from user space processes.
> > @@ -79,12 +89,13 @@ hv_uio_irqcontrol(struct uio_info *info, s32
> > irq_state)  {
> >  	struct hv_uio_private_data *pdata = info->priv;
> >  	struct hv_device *dev = pdata->device;
> > +	struct vmbus_channel *primary, *sc;
> >
> > -	dev->channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> > -	virt_mb();
> > +	primary = dev->channel;
> > +	set_event(primary, irq_state);
> >
> > -	if (!dev->channel->offermsg.monitor_allocated && irq_state)
> > -		vmbus_setevent(dev->channel);
> > +	list_for_each_entry(sc, &primary->sc_list, sc_list)
> > +		set_event(sc, irq_state);
> 
> Walking the sc_list usually requires holding
> vmbus_connection.channel_mutex.
> Is there a reason it's safe to walk the list here without the mutex?

You are right, we need a lock here. Will be adding it.

> 
> >
> >  	return 0;
> >  }
> > @@ -95,12 +106,19 @@ hv_uio_irqcontrol(struct uio_info *info, s32
> > irq_state)  static void hv_uio_channel_cb(void *context)  {
> >  	struct vmbus_channel *chan = context;
> > -	struct hv_device *hv_dev = chan->device_obj;
> > -	struct hv_uio_private_data *pdata = hv_get_drvdata(hv_dev);
> > +	struct hv_device *hv_dev;
> > +	struct hv_uio_private_data *pdata;
> >
> >  	chan->inbound.ring_buffer->interrupt_mask = 1;
> >  	virt_mb();
> >
> > +	/*
> > +	 * The callback may come from a subchannel, in which case look
> > +	 * for the hv device in the primary channel
> > +	 */
> > +	hv_dev = chan->primary_channel ?
> > +		 chan->primary_channel->device_obj : chan->device_obj;
> 
> This certainly looks correct and necessary. But how did this work in the past?
> Wouldn't DPDK running on a synthetic NIC have gotten callbacks on a
> subchannel?  I'm just trying to understand whether this a bug fix, or if not,
> what new scenario requires this change. I'm not understanding how this
> change is related to the commit message.
> 
> Michael

In the past, host always offers sub channels with monitor bit enabled, and the interrupt mask on the channel is always set so we never got callbacks.

This has changed in that the host may offer channels without monitoring. This change also requires patches at DPDK netvsc driver to work on unmonitored channels. I think it's a new scenario that requires changes at both ends.

Long

> 
> > +	pdata = hv_get_drvdata(hv_dev);
> >  	uio_event_notify(&pdata->info);
> >  }
> >
> > --
> > 2.34.1
> >