* Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver
From: Dan Carpenter @ 2012-03-16 5:38 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
Alan Stern, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481B7663B1@TK5EX14MBXC126.redmond.corp.microsoft.com>
[-- Attachment #1.1: Type: text/plain, Size: 870 bytes --]
On Thu, Mar 15, 2012 at 11:36:16PM +0000, KY Srinivasan wrote:
>
> Dan,
>
> Sorry I could not get back to you earlier. You are right, in that I am trusting
> the host! I am of the firm belief that in a virtualized environment, if you don't trust
> the host, there is not a whole lot you can do in a guest! I have had this arguments
> with other on this mailing list in the past. Having said that, I think we have spent more
> time debating this than we should; your proposal is reasonable and I will go ahead and
> re-spin those patches based on your comments. I should be posting them shortly.
>
> Regards,
It's not about trusting the host or not trusting the host. It's
about "if you're going to specify a limitter, it can't be off by one
even if you don't expect to hit the limit".
But thanks for redoing these.
regards,
dan carpenter
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
^ permalink raw reply
* RE: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver
From: KY Srinivasan @ 2012-03-16 5:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh@linuxfoundation.org, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
Alan Stern, devel@linuxdriverproject.org
In-Reply-To: <20120316053829.GG3163@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 1:38 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; ohering@suse.com; linux-
> kernel@vger.kernel.org; virtualization@lists.osdl.org; Alan Stern;
> devel@linuxdriverproject.org
> Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> messages in the driver
>
> On Thu, Mar 15, 2012 at 11:36:16PM +0000, KY Srinivasan wrote:
> >
> > Dan,
> >
> > Sorry I could not get back to you earlier. You are right, in that I am trusting
> > the host! I am of the firm belief that in a virtualized environment, if you don't
> trust
> > the host, there is not a whole lot you can do in a guest! I have had this
> arguments
> > with other on this mailing list in the past. Having said that, I think we have spent
> more
> > time debating this than we should; your proposal is reasonable and I will go
> ahead and
> > re-spin those patches based on your comments. I should be posting them
> shortly.
> >
> > Regards,
>
> It's not about trusting the host or not trusting the host. It's
> about "if you're going to specify a limitter, it can't be off by one
> even if you don't expect to hit the limit".
>
> But thanks for redoing these.
Thank you, for taking the time to review.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: Dan Carpenter @ 2012-03-16 5:45 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering, Alan Stern
In-Reply-To: <1331858925-827-1-git-send-email-kys@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> /*
> * The windows host expects the key/value pair to be encoded
> * in utf16.
> */
> keylen = utf8s_to_utf16s(key_name, strlen(key_name), UTF16_HOST_ENDIAN,
> - (wchar_t *) kvp_data->data.key,
> + (wchar_t *) kvp_data->key,
> HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> +
I feel like a jerk for asking this, but is the output length correct
here? It seems like we could go over again. Also utf8s_to_utf16s()
can return negative error codes, why do we ignore those?
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [Xen-devel] [PATCH 2/4 v2] xen kconfig: relax INPUT_XEN_KBDDEV_FRONTEND deps
From: Dmitry Torokhov @ 2012-03-16 6:24 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Andrew Jones, xen-devel, FlorianSchandinat, jeremy,
virtualization, Konrad Rzeszutek Wilk
In-Reply-To: <20120315172305.GA32759@phenom.dumpdata.com>
On Thu, Mar 15, 2012 at 01:23:05PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Jan 11, 2012 at 11:29:20AM -0500, Andrew Jones wrote:
> >
> >
> > ----- Original Message -----
> > > On Mon, Jan 09, 2012 at 06:51:41PM +0100, Andrew Jones wrote:
> > > > PV-on-HVM guests may want to use the xen keyboard/mouse frontend,
> > > > but
> > > > they don't use the xen frame buffer frontend. For this case it
> > > > doesn't
> > >
> > > Ok, but PV does?
> > > > make much sense for INPUT_XEN_KBDDEV_FRONTEND to depend on
> > > > XEN_FBDEV_FRONTEND. The opposite direction always makes more sense,
> > > > i.e.
> > > > if you're using xenfb, then you'll want xenkbd. Switch the
> > > > dependencies.
> > >
> > > That sounds like it would be universal irregardless if it is
> > > PV or PVonHVM?
> >
> > This patch makes it such that if you want to use both, then you must
> > select both. It also says that if you want FB, then you need the
> > KBD. However, if you only want the KBD then you're fine with just
> > that. So there isn't any risk of breaking configs designed to use
> > FB, because FB should be manually selected for those configs anyway.
>
> Dmitry,
>
> I am OK with this patch. Should I pick it up on my tree for 3.4 or
> are you OK doing it via your tree?
Konrad,
I don't have a good copy of the patch so it you could pick it up that
would be great.
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: KY Srinivasan @ 2012-03-16 6:33 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh@linuxfoundation.org, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
Alan Stern, devel@linuxdriverproject.org
In-Reply-To: <20120316054556.GH3163@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 1:46 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> messages in the driver
>
> On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > /*
> > * The windows host expects the key/value pair to be encoded
> > * in utf16.
> > */
> > keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> UTF16_HOST_ENDIAN,
> > - (wchar_t *) kvp_data->data.key,
> > + (wchar_t *) kvp_data->key,
> > HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > +
>
> I feel like a jerk for asking this, but is the output length correct
> here? It seems like we could go over again. Also utf8s_to_utf16s()
> can return negative error codes, why do we ignore those?
We are returning the strings back to the host here. There are checks elsewhere
in the code to ensure that all strings we return to the host can be accommodated
in the available space. For the most part these are strings that the host gave us in the
first place that have already been validated. Furthermore, there are checks on the
host side to ensure that the returned size parameters are consistent with the protocol
definitions for the key value pair. For instance let us say somehow we got into a
situation where the converted utf16 string occupied the entire MAX sized array
without any room for the terminating character and we set the length parameter
to 2 more than the MAX value as this code would do. The host would simply discard the
message as an illegal message. This would be more appropriate than sending a
truncated key or value.
With regards to the negative values, negative values indicate a failure of some sort
in the conversion. Since the host is the recipient here, host will correctly deal with the
transaction by discarding the tuple.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: Dan Carpenter @ 2012-03-16 7:18 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
Alan Stern, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481B7666A2@TK5EX14MBXC126.redmond.corp.microsoft.com>
[-- Attachment #1.1: Type: text/plain, Size: 3069 bytes --]
On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Friday, March 16, 2012 1:46 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > Alan Stern
> > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > messages in the driver
> >
> > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > /*
> > > * The windows host expects the key/value pair to be encoded
> > > * in utf16.
> > > */
> > > keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > UTF16_HOST_ENDIAN,
> > > - (wchar_t *) kvp_data->data.key,
> > > + (wchar_t *) kvp_data->key,
> > > HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +
> >
> > I feel like a jerk for asking this, but is the output length correct
> > here? It seems like we could go over again. Also utf8s_to_utf16s()
> > can return negative error codes, why do we ignore those?
>
> We are returning the strings back to the host here. There are checks elsewhere
> in the code to ensure that all strings we return to the host can be accommodated
> in the available space. For the most part these are strings that the host gave us in the
> first place that have already been validated. Furthermore, there are checks on the
> host side to ensure that the returned size parameters are consistent with the protocol
> definitions for the key value pair. For instance let us say somehow we got into a
> situation where the converted utf16 string occupied the entire MAX sized array
> without any room for the terminating character and we set the length parameter
> to 2 more than the MAX value as this code would do. The host would simply discard the
> message as an illegal message. This would be more appropriate than sending a
> truncated key or value.
>
Uh... Looking at it again, this code is clearly off by one. If
we're not going to hit the limit, then we're not going to truncate,
so that's not a concern. Let's just use the correct limit here.
The problem is that off-by-ones tend to reproduce by copy and paste.
It's best to never introduce any, even harmless ones.
Either that or add a comment. /* Don't care about wrong limitter
because we trust the input. */.
> With regards to the negative values, negative values indicate a failure of some sort
> in the conversion. Since the host is the recipient here, host will correctly deal with the
> transaction by discarding the tuple.
I'm not super familiar with this subsystem. Where can I find code
for rejecting bad transactions? It seems like an easy thing to
handle the error in both places. It makes auditing the code a lot
simpler.
regards,
dan carpenter
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
^ permalink raw reply
* Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: Dan Carpenter @ 2012-03-16 7:37 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
Alan Stern, devel@linuxdriverproject.org
In-Reply-To: <20120316071858.GH3220@mwanda>
[-- Attachment #1.1: Type: text/plain, Size: 2599 bytes --]
On Fri, Mar 16, 2012 at 10:18:58AM +0300, Dan Carpenter wrote:
> > > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > > /*
> > > > * The windows host expects the key/value pair to be encoded
> > > > * in utf16.
> > > > */
> > > > keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > > UTF16_HOST_ENDIAN,
> > > > - (wchar_t *) kvp_data->data.key,
> > > > + (wchar_t *) kvp_data->key,
> > > > HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > > - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +
> > >
> > > I feel like a jerk for asking this, but is the output length correct
> > > here? It seems like we could go over again. Also utf8s_to_utf16s()
> > > can return negative error codes, why do we ignore those?
> >
> > We are returning the strings back to the host here. There are checks elsewhere
> > in the code to ensure that all strings we return to the host can be accommodated
> > in the available space. For the most part these are strings that the host gave us in the
> > first place that have already been validated. Furthermore, there are checks on the
> > host side to ensure that the returned size parameters are consistent with the protocol
> > definitions for the key value pair. For instance let us say somehow we got into a
> > situation where the converted utf16 string occupied the entire MAX sized array
> > without any room for the terminating character and we set the length parameter
> > to 2 more than the MAX value as this code would do. The host would simply discard the
> > message as an illegal message. This would be more appropriate than sending a
> > truncated key or value.
> >
>
> Uh... Looking at it again, this code is clearly off by one. If
> we're not going to hit the limit, then we're not going to truncate,
> so that's not a concern. Let's just use the correct limit here.
>
Another option of course would be to add a test after the
conversion.
if (keylen == HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2)
return -EINVAL;
What I'm saying is that I audit a lot of code for buffer overflows,
and I don't want to see an off by one and then I have to audit where
the string come from and audit where it's going.
If it's corrupts memory then I fix the bug and I can list it under
my achievements in my weekly status report. If it's wrong but it
doesn't corrupt memory, it's just a complete waste of my time and it
makes me really cross.
regards,
dan carpenter
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
^ permalink raw reply
* [V5 PATCH] virtio-net: send gratuitous packets when needed
From: Jason Wang @ 2012-03-16 9:01 UTC (permalink / raw)
To: netdev, rusty, virtualization, linux-kernel, mst
As hypervior does not have the knowledge of guest network configuration, it's
better to ask guest to send gratuitous packets when needed.
Guest tests VIRTIO_NET_S_ANNOUNCE bit during config change interrupt and when it
is set, a workqueue is scheduled to send gratuitous packet through
NETDEV_NOTIFY_PEERS. This feature is negotiated through bit
VIRTIO_NET_F_GUEST_ANNOUNCE.
Changes from v4:
- typos
- handle workqueue unconditionally
- move VIRTIO_NET_S_ANNOUNCE to bit 8 to separate rw bits from ro bits
Changes from v3:
- cancel the workqueue during freeze
Changes from v2:
- fix the race between unregister_dev() and workqueue
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 27 ++++++++++++++++++++++++++-
include/linux/virtio_net.h | 2 ++
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4880aa8..450d358 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -72,6 +72,9 @@ struct virtnet_info {
/* Work struct for refilling if we run low on memory. */
struct delayed_work refill;
+ /* Work struct for sending gratuitous packets. */
+ struct work_struct announce;
+
/* Chain pages by the private ptr. */
struct page *pages;
@@ -512,6 +515,13 @@ static void refill_work(struct work_struct *work)
queue_delayed_work(system_nrt_wq, &vi->refill, HZ/2);
}
+static void announce_work(struct work_struct *work)
+{
+ struct virtnet_info *vi = container_of(work, struct virtnet_info,
+ announce);
+ netif_notify_peers(vi->dev);
+}
+
static int virtnet_poll(struct napi_struct *napi, int budget)
{
struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
@@ -787,6 +797,7 @@ static int virtnet_close(struct net_device *dev)
/* Make sure refill_work doesn't re-enable napi! */
cancel_delayed_work_sync(&vi->refill);
+ cancel_work_sync(&vi->announce);
napi_disable(&vi->napi);
return 0;
@@ -962,11 +973,22 @@ static void virtnet_update_status(struct virtnet_info *vi)
return;
/* Ignore unknown (future) status bits */
- v &= VIRTIO_NET_S_LINK_UP;
+ v &= VIRTIO_NET_S_LINK_UP | VIRTIO_NET_S_ANNOUNCE;
if (vi->status == v)
return;
+ if (v & VIRTIO_NET_S_ANNOUNCE) {
+ v &= ~VIRTIO_NET_S_ANNOUNCE;
+ vi->vdev->config->set(vi->vdev,
+ offsetof(struct virtio_net_config,
+ status),
+ &v, sizeof(v));
+
+ if (v & VIRTIO_NET_S_LINK_UP)
+ schedule_work(&vi->announce);
+ }
+
vi->status = v;
if (vi->status & VIRTIO_NET_S_LINK_UP) {
@@ -1076,6 +1098,7 @@ static int virtnet_probe(struct virtio_device *vdev)
goto free;
INIT_DELAYED_WORK(&vi->refill, refill_work);
+ INIT_WORK(&vi->announce, announce_work);
sg_init_table(vi->rx_sg, ARRAY_SIZE(vi->rx_sg));
sg_init_table(vi->tx_sg, ARRAY_SIZE(vi->tx_sg));
@@ -1187,6 +1210,7 @@ static int virtnet_freeze(struct virtio_device *vdev)
virtqueue_disable_cb(vi->svq);
if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
virtqueue_disable_cb(vi->cvq);
+ cancel_work_sync(&vi->announce);
netif_device_detach(vi->dev);
cancel_delayed_work_sync(&vi->refill);
@@ -1233,6 +1257,7 @@ static unsigned int features[] = {
VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
+ VIRTIO_NET_F_GUEST_ANNOUNCE,
};
static struct virtio_driver virtio_net_driver = {
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 970d5a2..5094c2d 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -49,8 +49,10 @@
#define VIRTIO_NET_F_CTRL_RX 18 /* Control channel RX mode support */
#define VIRTIO_NET_F_CTRL_VLAN 19 /* Control channel VLAN filtering */
#define VIRTIO_NET_F_CTRL_RX_EXTRA 20 /* Extra RX mode control support */
+#define VIRTIO_NET_F_GUEST_ANNOUNCE 21 /* Guest can send gratituous packet */
#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
+#define VIRTIO_NET_S_ANNOUNCE 0x100 /* Announcement is needed */
struct virtio_net_config {
/* The config defining mac address (if VIRTIO_NET_F_MAC) */
^ permalink raw reply related
* RE: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: KY Srinivasan @ 2012-03-16 13:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Alan Stern
In-Reply-To: <20120316071858.GH3220@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 3:19 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> messages in the driver
>
> On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Friday, March 16, 2012 1:46 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > Alan Stern
> > > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > > messages in the driver
> > >
> > > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > > /*
> > > > * The windows host expects the key/value pair to be encoded
> > > > * in utf16.
> > > > */
> > > > keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > > UTF16_HOST_ENDIAN,
> > > > - (wchar_t *) kvp_data->data.key,
> > > > + (wchar_t *) kvp_data->key,
> > > > HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > > - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > > +
> > >
> > > I feel like a jerk for asking this, but is the output length correct
> > > here? It seems like we could go over again. Also utf8s_to_utf16s()
> > > can return negative error codes, why do we ignore those?
> >
> > We are returning the strings back to the host here. There are checks elsewhere
> > in the code to ensure that all strings we return to the host can be
> accommodated
> > in the available space. For the most part these are strings that the host gave us
> in the
> > first place that have already been validated. Furthermore, there are checks on
> the
> > host side to ensure that the returned size parameters are consistent with the
> protocol
> > definitions for the key value pair. For instance let us say somehow we got into a
> > situation where the converted utf16 string occupied the entire MAX sized array
> > without any room for the terminating character and we set the length
> parameter
> > to 2 more than the MAX value as this code would do. The host would simply
> discard the
> > message as an illegal message. This would be more appropriate than sending a
> > truncated key or value.
> >
>
> Uh... Looking at it again, this code is clearly off by one. If
> we're not going to hit the limit, then we're not going to truncate,
> so that's not a concern. Let's just use the correct limit here.
Dan,
I am trying to understand your concern here:
You will agree with me that the current code does not overflow the
buffer since the output is limited to MAX bytes and that is how
big the output buffer is sized. Now, as I pointed out earlier, if the
string to be converted were to fully occupy the MAX bytes or even be
larger than MAX bytes (no buffer overflow here since we limit the
conversion to MAX bytes), we will get a malformed packet that will be
sent to the host since the size field of the message would exceed
the protocol specified size limit. I was merely pointing out that in
this case, I would rather have the host reject the message than send
a truncated Key/Value string (if we were to ever get invalid key or
value strings).
>
> The problem is that off-by-ones tend to reproduce by copy and paste.
> It's best to never introduce any, even harmless ones.
>
> Either that or add a comment. /* Don't care about wrong limitter
> because we trust the input. */.
>
All I am saying if we have an invalid string we will (a) ensure that we don't overflow
the output buffer and (b) we will generate a malformed packet as we should since
the string we are handling is not valid. Furthermore, I am suggesting that it is better
to have a malformed packet in this case than to have
> > With regards to the negative values, negative values indicate a failure of some
> sort
> > in the conversion. Since the host is the recipient here, host will correctly deal
> with the
> > transaction by discarding the tuple.
>
> I'm not super familiar with this subsystem. Where can I find code
> for rejecting bad transactions? It seems like an easy thing to
> handle the error in both places. It makes auditing the code a lot
> simpler.
The host here is a windows host and the protocol verification engine there
will reject a mal-formed message. Having said this, what you are
proposing is reasonable, I will re-send the patches.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: Dan Carpenter @ 2012-03-16 14:26 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
Alan Stern, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481B76671F@TK5EX14MBXC126.redmond.corp.microsoft.com>
[-- Attachment #1.1: Type: text/plain, Size: 1704 bytes --]
On Fri, Mar 16, 2012 at 01:14:07PM +0000, KY Srinivasan wrote:
> Dan,
> I am trying to understand your concern here:
> You will agree with me that the current code does not overflow the
> buffer since the output is limited to MAX bytes and that is how
> big the output buffer is sized. Now, as I pointed out earlier, if the
> string to be converted were to fully occupy the MAX bytes or even be
> larger than MAX bytes (no buffer overflow here since we limit the
> conversion to MAX bytes), we will get a malformed packet that will be
> sent to the host since the size field of the message would exceed
> the protocol specified size limit. I was merely pointing out that in
> this case, I would rather have the host reject the message than send
> a truncated Key/Value string (if we were to ever get invalid key or
> value strings).
>
Sending malformed packets is a bad idea.
Normally, if you handle the error as close to the cause of the error
as possible then it makes everything easier to read. In this case
especially, it's so easy to catch any errors. If you decide to go
ahead and send the malformed message, at least put a comment.
When I read code, I spend all my time looking for what it does
wrong. So when code doesn't have any error handling and sets
keylen = -EINVAL then sure, it's fewer lines of code to read, but
it doesn't make my life easier. Readable code has obvious error
handling.
Introducing off by one errors is an especially bad idea. It doesn't
matter how harmless they are, because they end up getting copy and
pasted. I don't know how to make it any clearer than that. :/
Never never never do that!
regards,
dan carpenter
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
^ permalink raw reply
* Re: [Xen-devel] [PATCH 2/4 v2] xen kconfig: relax INPUT_XEN_KBDDEV_FRONTEND deps
From: Konrad Rzeszutek Wilk @ 2012-03-16 14:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Jones, xen-devel, FlorianSchandinat, jeremy,
virtualization, Konrad Rzeszutek Wilk
In-Reply-To: <20120316062428.GA16291@core.coreip.homeip.net>
On Thu, Mar 15, 2012 at 11:24:28PM -0700, Dmitry Torokhov wrote:
> On Thu, Mar 15, 2012 at 01:23:05PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jan 11, 2012 at 11:29:20AM -0500, Andrew Jones wrote:
> > >
> > >
> > > ----- Original Message -----
> > > > On Mon, Jan 09, 2012 at 06:51:41PM +0100, Andrew Jones wrote:
> > > > > PV-on-HVM guests may want to use the xen keyboard/mouse frontend,
> > > > > but
> > > > > they don't use the xen frame buffer frontend. For this case it
> > > > > doesn't
> > > >
> > > > Ok, but PV does?
> > > > > make much sense for INPUT_XEN_KBDDEV_FRONTEND to depend on
> > > > > XEN_FBDEV_FRONTEND. The opposite direction always makes more sense,
> > > > > i.e.
> > > > > if you're using xenfb, then you'll want xenkbd. Switch the
> > > > > dependencies.
> > > >
> > > > That sounds like it would be universal irregardless if it is
> > > > PV or PVonHVM?
> > >
> > > This patch makes it such that if you want to use both, then you must
> > > select both. It also says that if you want FB, then you need the
> > > KBD. However, if you only want the KBD then you're fine with just
> > > that. So there isn't any risk of breaking configs designed to use
> > > FB, because FB should be manually selected for those configs anyway.
> >
> > Dmitry,
> >
> > I am OK with this patch. Should I pick it up on my tree for 3.4 or
> > are you OK doing it via your tree?
>
> Konrad,
>
> I don't have a good copy of the patch so it you could pick it up that
> would be great.
OK, will stick you Ack on it. Thx!
>
> Thanks.
>
> --
> Dmitry
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply
* [PATCH 0000/0003] drivers: hv
From: K. Y. Srinivasan @ 2012-03-16 15:01 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
This patch-set further enhances the KVP functionality for Linux
guests:
1. Supports most of the Win8 KVP protocol.
2. Supports operations on all the pools.
This is a re-send of the series sent on March15. I have made
changes to address comments Dan Carpenter had posted.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH RESEND 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: Dan Carpenter @ 2012-03-16 15:01 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, virtualization, ohering, linux-kernel, devel
In-Reply-To: <1331910147-5259-1-git-send-email-kys@microsoft.com>
[-- Attachment #1.1: Type: text/plain, Size: 615 bytes --]
On Fri, Mar 16, 2012 at 08:02:25AM -0700, K. Y. Srinivasan wrote:
> Support the newly defined KVP message types. It turns out that the host
> pushes a set of standard key value pairs as soon as the guest opens the KVP channel.
> Since we cannot handle these tuples until the user level daemon loads up, defer
> reading the KVP channel until the user level daemon is launched.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Great. Thanks for doing this.
regards,
dan carpenter
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
^ permalink raw reply
* [PATCH RESEND 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
From: K. Y. Srinivasan @ 2012-03-16 15:02 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
In-Reply-To: <1331910089-5207-1-git-send-email-kys@microsoft.com>
Support the newly defined KVP message types. It turns out that the host
pushes a set of standard key value pairs as soon as the guest opens the KVP channel.
Since we cannot handle these tuples until the user level daemon loads up, defer
reading the KVP channel until the user level daemon is launched.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/hv/hv_kvp.c | 218 +++++++++++++++++++++++++++++++++++-----------
include/linux/hyperv.h | 2 +
tools/hv/hv_kvp_daemon.c | 7 ++
3 files changed, 176 insertions(+), 51 deletions(-)
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 779109b..cfe60b0 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -42,9 +42,10 @@
static struct {
bool active; /* transaction status - active or not */
int recv_len; /* number of bytes received. */
- int index; /* current index */
+ struct hv_kvp_msg *kvp_msg; /* current message */
struct vmbus_channel *recv_channel; /* chn we got the request */
u64 recv_req_id; /* request ID. */
+ void *kvp_context; /* for the channel callback */
} kvp_transaction;
static void kvp_send_key(struct work_struct *dummy);
@@ -110,12 +111,15 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
struct hv_kvp_msg_enumerate *data;
message = (struct hv_kvp_msg *)msg->data;
- if (message->kvp_hdr.operation == KVP_OP_REGISTER) {
+ switch (message->kvp_hdr.operation) {
+ case KVP_OP_REGISTER:
pr_info("KVP: user-mode registering done.\n");
kvp_register();
- }
+ kvp_transaction.active = false;
+ hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+ break;
- if (message->kvp_hdr.operation == KVP_OP_ENUMERATE) {
+ default:
data = &message->body.kvp_enum_data;
/*
* Complete the transaction by forwarding the key value
@@ -133,21 +137,104 @@ kvp_send_key(struct work_struct *dummy)
{
struct cn_msg *msg;
struct hv_kvp_msg *message;
- int index = kvp_transaction.index;
+ struct hv_kvp_msg *in_msg;
+ __u8 operation = kvp_transaction.kvp_msg->kvp_hdr.operation;
+ __u8 pool = kvp_transaction.kvp_msg->kvp_hdr.pool;
+ __u32 val32;
+ __u64 val64;
msg = kzalloc(sizeof(*msg) + sizeof(struct hv_kvp_msg) , GFP_ATOMIC);
+ if (!msg)
+ return;
- if (msg) {
- msg->id.idx = CN_KVP_IDX;
- msg->id.val = CN_KVP_VAL;
+ msg->id.idx = CN_KVP_IDX;
+ msg->id.val = CN_KVP_VAL;
- message = (struct hv_kvp_msg *)msg->data;
- message->kvp_hdr.operation = KVP_OP_ENUMERATE;
- message->body.kvp_enum_data.index = index;
- msg->len = sizeof(struct hv_kvp_msg);
- cn_netlink_send(msg, 0, GFP_ATOMIC);
- kfree(msg);
+ message = (struct hv_kvp_msg *)msg->data;
+ message->kvp_hdr.operation = operation;
+ message->kvp_hdr.pool = pool;
+ in_msg = kvp_transaction.kvp_msg;
+
+ /*
+ * The key/value strings sent from the host are encoded in
+ * in utf16; convert it to utf8 strings.
+ * The host assures us that the utf16 strings will not exceed
+ * the max lengths specified. We will however, reserve room
+ * for the string terminating character - in the utf16s_utf8s()
+ * function we limit the size of the buffer where the converted
+ * string is placed to HV_KVP_EXCHANGE_MAX_*_SIZE -1 to gaurantee
+ * that the strings can be properly terminated!
+ */
+
+ switch (message->kvp_hdr.operation) {
+ case KVP_OP_SET:
+ switch (in_msg->body.kvp_set.data.value_type) {
+ case REG_SZ:
+ /*
+ * The value is a string - utf16 encoding.
+ */
+ message->body.kvp_set.data.value_size =
+ utf16s_to_utf8s(
+ (wchar_t *)in_msg->body.kvp_set.data.value,
+ in_msg->body.kvp_set.data.value_size,
+ UTF16_LITTLE_ENDIAN,
+ message->body.kvp_set.data.value,
+ HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1) + 1;
+ break;
+
+ case REG_U32:
+ /*
+ * The value is a 32 bit scalar.
+ * We save this as a utf8 string.
+ */
+ val32 = in_msg->body.kvp_set.data.value_u32;
+ message->body.kvp_set.data.value_size =
+ sprintf(message->body.kvp_set.data.value,
+ "%d", val32) + 1;
+ break;
+
+ case REG_U64:
+ /*
+ * The value is a 64 bit scalar.
+ * We save this as a utf8 string.
+ */
+ val64 = in_msg->body.kvp_set.data.value_u64;
+ message->body.kvp_set.data.value_size =
+ sprintf(message->body.kvp_set.data.value,
+ "%llu", val64) + 1;
+ break;
+
+ }
+ case KVP_OP_GET:
+ message->body.kvp_set.data.key_size =
+ utf16s_to_utf8s(
+ (wchar_t *)in_msg->body.kvp_set.data.key,
+ in_msg->body.kvp_set.data.key_size,
+ UTF16_LITTLE_ENDIAN,
+ message->body.kvp_set.data.key,
+ HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
+ break;
+
+ case KVP_OP_DELETE:
+ message->body.kvp_delete.key_size =
+ utf16s_to_utf8s(
+ (wchar_t *)in_msg->body.kvp_delete.key,
+ in_msg->body.kvp_delete.key_size,
+ UTF16_LITTLE_ENDIAN,
+ message->body.kvp_delete.key,
+ HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
+ break;
+
+ case KVP_OP_ENUMERATE:
+ message->body.kvp_enum_data.index =
+ in_msg->body.kvp_enum_data.index;
+ break;
}
+
+ msg->len = sizeof(struct hv_kvp_msg);
+ cn_netlink_send(msg, 0, GFP_ATOMIC);
+ kfree(msg);
+
return;
}
@@ -159,10 +246,11 @@ static void
kvp_respond_to_host(char *key, char *value, int error)
{
struct hv_kvp_msg *kvp_msg;
- struct hv_kvp_msg_enumerate *kvp_data;
+ struct hv_kvp_exchg_msg_value *kvp_data;
char *key_name;
struct icmsg_hdr *icmsghdrp;
- int keylen, valuelen;
+ int keylen = 0;
+ int valuelen = 0;
u32 buf_len;
struct vmbus_channel *channel;
u64 req_id;
@@ -189,6 +277,9 @@ kvp_respond_to_host(char *key, char *value, int error)
kvp_transaction.active = false;
+ icmsghdrp = (struct icmsg_hdr *)
+ &recv_buffer[sizeof(struct vmbuspipe_hdr)];
+
if (channel->onchannel_callback == NULL)
/*
* We have raced with util driver being unloaded;
@@ -196,41 +287,66 @@ kvp_respond_to_host(char *key, char *value, int error)
*/
return;
- icmsghdrp = (struct icmsg_hdr *)
- &recv_buffer[sizeof(struct vmbuspipe_hdr)];
- kvp_msg = (struct hv_kvp_msg *)
- &recv_buffer[sizeof(struct vmbuspipe_hdr) +
- sizeof(struct icmsg_hdr)];
- kvp_data = &kvp_msg->body.kvp_enum_data;
- key_name = key;
/*
* If the error parameter is set, terminate the host's enumeration.
*/
if (error) {
/*
- * We don't support this index or the we have timedout;
+ * Something failed or the we have timedout;
* terminate the host-side iteration by returning an error.
*/
icmsghdrp->status = HV_E_FAIL;
goto response_done;
}
+ icmsghdrp->status = HV_S_OK;
+
+ kvp_msg = (struct hv_kvp_msg *)
+ &recv_buffer[sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
+
+ switch (kvp_transaction.kvp_msg->kvp_hdr.operation) {
+ case KVP_OP_GET:
+ kvp_data = &kvp_msg->body.kvp_get.data;
+ goto copy_value;
+
+ case KVP_OP_SET:
+ case KVP_OP_DELETE:
+ goto response_done;
+
+ default:
+ break;
+ }
+
+ kvp_data = &kvp_msg->body.kvp_enum_data.data;
+ key_name = key;
+
/*
* The windows host expects the key/value pair to be encoded
- * in utf16.
+ * in utf16. Ensure that the key/value size reported to the host
+ * will be less than or equal to the MAX size (including the
+ * terminating character).
*/
keylen = utf8s_to_utf16s(key_name, strlen(key_name), UTF16_HOST_ENDIAN,
- (wchar_t *) kvp_data->data.key,
- HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
- kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
+ (wchar_t *) kvp_data->key,
+ (HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2) - 2);
+ kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
+
+copy_value:
valuelen = utf8s_to_utf16s(value, strlen(value), UTF16_HOST_ENDIAN,
- (wchar_t *) kvp_data->data.value,
- HV_KVP_EXCHANGE_MAX_VALUE_SIZE / 2);
- kvp_data->data.value_size = 2*(valuelen + 1); /* utf16 encoding */
+ (wchar_t *) kvp_data->value,
+ (HV_KVP_EXCHANGE_MAX_VALUE_SIZE / 2) - 2);
+ kvp_data->value_size = 2*(valuelen + 1); /* utf16 encoding */
- kvp_data->data.value_type = REG_SZ; /* all our values are strings */
- icmsghdrp->status = HV_S_OK;
+ /*
+ * If the utf8s to utf16s conversion failed; notify host
+ * of the error.
+ */
+ if ((keylen < 0) || (valuelen < 0))
+ icmsghdrp->status = HV_E_FAIL;
+
+ kvp_data->value_type = REG_SZ; /* all our values are strings */
response_done:
icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
@@ -257,11 +373,18 @@ void hv_kvp_onchannelcallback(void *context)
u64 requestid;
struct hv_kvp_msg *kvp_msg;
- struct hv_kvp_msg_enumerate *kvp_data;
struct icmsg_hdr *icmsghdrp;
struct icmsg_negotiate *negop = NULL;
+ if (kvp_transaction.active) {
+ /*
+ * We will defer processing this callback once
+ * the current transaction is complete.
+ */
+ kvp_transaction.kvp_context = context;
+ return;
+ }
vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE, &recvlen, &requestid);
@@ -276,29 +399,16 @@ void hv_kvp_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr) +
sizeof(struct icmsg_hdr)];
- kvp_data = &kvp_msg->body.kvp_enum_data;
-
- /*
- * We only support the "get" operation on
- * "KVP_POOL_AUTO" pool.
- */
-
- if ((kvp_msg->kvp_hdr.pool != KVP_POOL_AUTO) ||
- (kvp_msg->kvp_hdr.operation !=
- KVP_OP_ENUMERATE)) {
- icmsghdrp->status = HV_E_FAIL;
- goto callback_done;
- }
-
/*
* Stash away this global state for completing the
* transaction; note transactions are serialized.
*/
+
kvp_transaction.recv_len = recvlen;
kvp_transaction.recv_channel = channel;
kvp_transaction.recv_req_id = requestid;
kvp_transaction.active = true;
- kvp_transaction.index = kvp_data->index;
+ kvp_transaction.kvp_msg = kvp_msg;
/*
* Get the information from the
@@ -316,8 +426,6 @@ void hv_kvp_onchannelcallback(void *context)
}
-callback_done:
-
icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
@@ -338,6 +446,14 @@ hv_kvp_init(struct hv_util_service *srv)
return err;
recv_buffer = srv->recv_buffer;
+ /*
+ * When this driver loads, the user level daemon that
+ * processes the host requests may not yet be running.
+ * Defer processing channel callbacks until the daemon
+ * has registered.
+ */
+ kvp_transaction.active = true;
+
return 0;
}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index a2d8c54..e88a979 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -119,6 +119,8 @@
*/
#define REG_SZ 1
+#define REG_U32 4
+#define REG_U64 8
enum hv_kvp_exchg_op {
KVP_OP_GET = 0,
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 00d3f7c..a98878c 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -389,10 +389,16 @@ int main(void)
}
continue;
+ case KVP_OP_SET:
+ case KVP_OP_GET:
+ case KVP_OP_DELETE:
default:
break;
}
+ if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
+ goto kvp_done;
+
hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
key_name = (char *)hv_msg->body.kvp_enum_data.data.key;
key_value = (char *)hv_msg->body.kvp_enum_data.data.value;
@@ -454,6 +460,7 @@ int main(void)
* already in the receive buffer. Update the cn_msg header to
* reflect the key value that has been added to the message
*/
+kvp_done:
incoming_cn_msg->id.idx = CN_KVP_IDX;
incoming_cn_msg->id.val = CN_KVP_VAL;
--
1.7.4.1
^ permalink raw reply related
* [PATCH RESEND 2/3] Tools: hv: Fully support the new KVP verbs in the user level daemon
From: K. Y. Srinivasan @ 2012-03-16 15:02 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
In-Reply-To: <1331910147-5259-1-git-send-email-kys@microsoft.com>
Now fully support the new KVP messages in the user level daemon. Hyper-V defines
multiple persistent pools to which the host can write/read/modify KVP tuples.
In this patch we implement a file for each specified pool, where the KVP tuples
will be stored in the guest.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
tools/hv/hv_kvp_daemon.c | 281 +++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 280 insertions(+), 1 deletions(-)
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index a98878c..2fb9c3d 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -39,7 +39,8 @@
#include <ifaddrs.h>
#include <netdb.h>
#include <syslog.h>
-
+#include <sys/stat.h>
+#include <fcntl.h>
/*
* KVP protocol: The user mode component first registers with the
@@ -79,6 +80,250 @@ static char *os_build;
static char *lic_version;
static struct utsname uts_buf;
+
+#define MAX_FILE_NAME 100
+#define ENTRIES_PER_BLOCK 50
+
+struct kvp_record {
+ __u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+ __u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+};
+
+struct kvp_file_state {
+ int fd;
+ int num_blocks;
+ struct kvp_record *records;
+ int num_records;
+ __u8 fname[MAX_FILE_NAME];
+};
+
+static struct kvp_file_state kvp_file_info[KVP_POOL_COUNT];
+
+static void kvp_acquire_lock(int pool)
+{
+ struct flock fl = {F_WRLCK, SEEK_SET, 0, 0, 0};
+ fl.l_pid = getpid();
+
+ if (fcntl(kvp_file_info[pool].fd, F_SETLKW, &fl) == -1) {
+ syslog(LOG_ERR, "Failed to acquire the lock pool: %d", pool);
+ exit(-1);
+ }
+}
+
+static void kvp_release_lock(int pool)
+{
+ struct flock fl = {F_UNLCK, SEEK_SET, 0, 0, 0};
+ fl.l_pid = getpid();
+
+ if (fcntl(kvp_file_info[pool].fd, F_SETLK, &fl) == -1) {
+ perror("fcntl");
+ syslog(LOG_ERR, "Failed to release the lock pool: %d", pool);
+ exit(-1);
+ }
+}
+
+static void kvp_update_file(int pool)
+{
+ FILE *filep;
+ size_t bytes_written;
+
+ /*
+ * We are going to write our in-memory registry out to
+ * disk; acquire the lock first.
+ */
+ kvp_acquire_lock(pool);
+
+ filep = fopen(kvp_file_info[pool].fname, "w");
+ if (!filep) {
+ kvp_release_lock(pool);
+ syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+ exit(-1);
+ }
+
+ bytes_written = fwrite(kvp_file_info[pool].records,
+ sizeof(struct kvp_record),
+ kvp_file_info[pool].num_records, filep);
+
+ fflush(filep);
+ kvp_release_lock(pool);
+}
+
+static int kvp_file_init(void)
+{
+ int ret, fd;
+ FILE *filep;
+ size_t records_read;
+ __u8 *fname;
+ struct kvp_record *record;
+ struct kvp_record *readp;
+ int num_blocks;
+ int i;
+ int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+ if (access("/var/opt/hyperv", F_OK)) {
+ if (mkdir("/var/opt/hyperv", S_IRUSR | S_IWUSR | S_IROTH)) {
+ syslog(LOG_ERR, " Failed to create /var/opt/hyperv");
+ exit(-1);
+ }
+ }
+
+ for (i = 0; i < KVP_POOL_COUNT; i++) {
+ fname = kvp_file_info[i].fname;
+ records_read = 0;
+ num_blocks = 1;
+ sprintf(fname, "/var/opt/hyperv/.kvp_pool_%d", i);
+ fd = open(fname, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR | S_IROTH);
+
+ if (fd == -1)
+ return 1;
+
+
+ filep = fopen(fname, "r");
+ if (!filep)
+ return 1;
+
+ record = malloc(alloc_unit * num_blocks);
+ if (record == NULL) {
+ fclose(filep);
+ return 1;
+ }
+ while (!feof(filep)) {
+ readp = &record[records_read];
+ records_read += fread(readp, sizeof(struct kvp_record),
+ ENTRIES_PER_BLOCK,
+ filep);
+
+ if (!feof(filep)) {
+ /*
+ * We have more data to read.
+ */
+ num_blocks++;
+ record = realloc(record, alloc_unit *
+ num_blocks);
+ if (record == NULL) {
+ fclose(filep);
+ return 1;
+ }
+ continue;
+ }
+ break;
+ }
+ kvp_file_info[i].fd = fd;
+ kvp_file_info[i].num_blocks = num_blocks;
+ kvp_file_info[i].records = record;
+ kvp_file_info[i].num_records = records_read;
+ fclose(filep);
+
+ }
+
+ return 0;
+}
+
+static int kvp_key_delete(int pool, __u8 *key, int key_size)
+{
+ int i;
+ int j, k;
+ int num_records = kvp_file_info[pool].num_records;
+ struct kvp_record *record = kvp_file_info[pool].records;
+
+ for (i = 0; i < num_records; i++) {
+ if (memcmp(key, record[i].key, key_size))
+ continue;
+ /*
+ * Found a match; just move the remaining
+ * entries up.
+ */
+ if (i == num_records) {
+ kvp_file_info[pool].num_records--;
+ kvp_update_file(pool);
+ return 0;
+ }
+
+ j = i;
+ k = j + 1;
+ for (; k < num_records; k++) {
+ strcpy(record[j].key, record[k].key);
+ strcpy(record[j].value, record[k].value);
+ j++;
+ }
+
+ kvp_file_info[pool].num_records--;
+ kvp_update_file(pool);
+ return 0;
+ }
+ return 1;
+}
+
+static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
+ int value_size)
+{
+ int i;
+ int j, k;
+ int num_records = kvp_file_info[pool].num_records;
+ struct kvp_record *record = kvp_file_info[pool].records;
+ int num_blocks = kvp_file_info[pool].num_blocks;
+
+ if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+ (value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+ return 1;
+
+ for (i = 0; i < num_records; i++) {
+ if (memcmp(key, record[i].key, key_size))
+ continue;
+ /*
+ * Found a match; just update the value -
+ * this is the modify case.
+ */
+ memcpy(record[i].value, value, value_size);
+ kvp_update_file(pool);
+ return 0;
+ }
+
+ /*
+ * Need to add a new entry;
+ */
+ if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
+ /* Need to allocate a larger array for reg entries. */
+ record = realloc(record, sizeof(struct kvp_record) *
+ ENTRIES_PER_BLOCK * (num_blocks + 1));
+
+ if (record == NULL)
+ return 1;
+ kvp_file_info[pool].num_blocks++;
+
+ }
+ memcpy(record[i].value, value, value_size);
+ memcpy(record[i].key, key, key_size);
+ kvp_file_info[pool].records = record;
+ kvp_file_info[pool].num_records++;
+ kvp_update_file(pool);
+ return 0;
+}
+
+static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
+ int value_size)
+{
+ int i;
+ int num_records = kvp_file_info[pool].num_records;
+ struct kvp_record *record = kvp_file_info[pool].records;
+
+ if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+ (value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+ return 1;
+
+ for (i = 0; i < num_records; i++) {
+ if (memcmp(key, record[i].key, key_size))
+ continue;
+ /*
+ * Found a match; just copy the value out.
+ */
+ memcpy(value, record[i].value, value_size);
+ return 0;
+ }
+
+ return 1;
+}
+
void kvp_get_os_info(void)
{
FILE *file;
@@ -315,6 +560,11 @@ int main(void)
*/
kvp_get_os_info();
+ if (kvp_file_init()) {
+ syslog(LOG_ERR, "Failed to initialize the pools");
+ exit(-1);
+ }
+
fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
if (fd < 0) {
syslog(LOG_ERR, "netlink socket creation failed; error:%d", fd);
@@ -389,9 +639,38 @@ int main(void)
}
continue;
+ /*
+ * The current protocol with the kernel component uses a
+ * NULL key name to pass an error condition.
+ * For the SET, GET and DELETE operations,
+ * use the existing protocol to pass back error.
+ */
+
case KVP_OP_SET:
+ if (kvp_key_add_or_modify(hv_msg->kvp_hdr.pool,
+ hv_msg->body.kvp_set.data.key,
+ hv_msg->body.kvp_set.data.key_size,
+ hv_msg->body.kvp_set.data.value,
+ hv_msg->body.kvp_set.data.value_size))
+ strcpy(hv_msg->body.kvp_set.data.key, "");
+ break;
+
case KVP_OP_GET:
+ if (kvp_get_value(hv_msg->kvp_hdr.pool,
+ hv_msg->body.kvp_set.data.key,
+ hv_msg->body.kvp_set.data.key_size,
+ hv_msg->body.kvp_set.data.value,
+ hv_msg->body.kvp_set.data.value_size))
+ strcpy(hv_msg->body.kvp_set.data.key, "");
+ break;
+
case KVP_OP_DELETE:
+ if (kvp_key_delete(hv_msg->kvp_hdr.pool,
+ hv_msg->body.kvp_delete.key,
+ hv_msg->body.kvp_delete.key_size))
+ strcpy(hv_msg->body.kvp_delete.key, "");
+ break;
+
default:
break;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH RESEND 3/3] Tools: hv: Support enumeration from all the pools
From: K. Y. Srinivasan @ 2012-03-16 15:02 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering; +Cc: K. Y. Srinivasan
In-Reply-To: <1331910147-5259-1-git-send-email-kys@microsoft.com>
We have only supported enumeration only from the AUTO pool. Now support
enumeration from all the available pools.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/hv/hv_kvp.c | 7 ++-
include/linux/hyperv.h | 1 +
tools/hv/hv_kvp_daemon.c | 124 +++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 122 insertions(+), 10 deletions(-)
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index cfe60b0..6186025 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -289,14 +289,15 @@ kvp_respond_to_host(char *key, char *value, int error)
/*
- * If the error parameter is set, terminate the host's enumeration.
+ * If the error parameter is set, terminate the host's enumeration
+ * on this pool.
*/
if (error) {
/*
* Something failed or the we have timedout;
- * terminate the host-side iteration by returning an error.
+ * terminate the current host-side iteration.
*/
- icmsghdrp->status = HV_E_FAIL;
+ icmsghdrp->status = HV_S_CONT;
goto response_done;
}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index e88a979..5852545 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -952,6 +952,7 @@ void vmbus_driver_unregister(struct hv_driver *hv_driver);
#define HV_S_OK 0x00000000
#define HV_E_FAIL 0x80004005
+#define HV_S_CONT 0x80070103
#define HV_ERROR_NOT_SUPPORTED 0x80070032
#define HV_ERROR_MACHINE_LOCKED 0x800704F7
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 2fb9c3d..146fd61 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -148,6 +148,51 @@ static void kvp_update_file(int pool)
kvp_release_lock(pool);
}
+static void kvp_update_mem_state(int pool)
+{
+ FILE *filep;
+ size_t records_read = 0;
+ struct kvp_record *record = kvp_file_info[pool].records;
+ struct kvp_record *readp;
+ int num_blocks = kvp_file_info[pool].num_blocks;
+ int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+ kvp_acquire_lock(pool);
+
+ filep = fopen(kvp_file_info[pool].fname, "r");
+ if (!filep) {
+ kvp_release_lock(pool);
+ syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+ exit(-1);
+ }
+ while (!feof(filep)) {
+ readp = &record[records_read];
+ records_read += fread(readp, sizeof(struct kvp_record),
+ ENTRIES_PER_BLOCK * num_blocks,
+ filep);
+
+ if (!feof(filep)) {
+ /*
+ * We have more data to read.
+ */
+ num_blocks++;
+ record = realloc(record, alloc_unit * num_blocks);
+
+ if (record == NULL) {
+ syslog(LOG_ERR, "malloc failed");
+ exit(-1);
+ }
+ continue;
+ }
+ break;
+ }
+
+ kvp_file_info[pool].num_blocks = num_blocks;
+ kvp_file_info[pool].records = record;
+ kvp_file_info[pool].num_records = records_read;
+
+ kvp_release_lock(pool);
+}
static int kvp_file_init(void)
{
int ret, fd;
@@ -223,8 +268,16 @@ static int kvp_key_delete(int pool, __u8 *key, int key_size)
{
int i;
int j, k;
- int num_records = kvp_file_info[pool].num_records;
- struct kvp_record *record = kvp_file_info[pool].records;
+ int num_records;
+ struct kvp_record *record;
+
+ /*
+ * First update the in-memory state.
+ */
+ kvp_update_mem_state(pool);
+
+ num_records = kvp_file_info[pool].num_records;
+ record = kvp_file_info[pool].records;
for (i = 0; i < num_records; i++) {
if (memcmp(key, record[i].key, key_size))
@@ -259,14 +312,23 @@ static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
{
int i;
int j, k;
- int num_records = kvp_file_info[pool].num_records;
- struct kvp_record *record = kvp_file_info[pool].records;
- int num_blocks = kvp_file_info[pool].num_blocks;
+ int num_records;
+ struct kvp_record *record;
+ int num_blocks;
if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
return 1;
+ /*
+ * First update the in-memory state.
+ */
+ kvp_update_mem_state(pool);
+
+ num_records = kvp_file_info[pool].num_records;
+ record = kvp_file_info[pool].records;
+ num_blocks = kvp_file_info[pool].num_blocks;
+
for (i = 0; i < num_records; i++) {
if (memcmp(key, record[i].key, key_size))
continue;
@@ -304,13 +366,21 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
int value_size)
{
int i;
- int num_records = kvp_file_info[pool].num_records;
- struct kvp_record *record = kvp_file_info[pool].records;
+ int num_records;
+ struct kvp_record *record;
if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
return 1;
+ /*
+ * First update the in-memory state.
+ */
+ kvp_update_mem_state(pool);
+
+ num_records = kvp_file_info[pool].num_records;
+ record = kvp_file_info[pool].records;
+
for (i = 0; i < num_records; i++) {
if (memcmp(key, record[i].key, key_size))
continue;
@@ -324,6 +394,31 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
return 1;
}
+static void kvp_pool_enumerate(int pool, int index, __u8 *key, int key_size,
+ __u8 *value, int value_size)
+{
+ struct kvp_record *record;
+
+ /*
+ * First update our in-memory database.
+ */
+ kvp_update_mem_state(pool);
+ record = kvp_file_info[pool].records;
+
+ if (index >= kvp_file_info[pool].num_records) {
+ /*
+ * This is an invalid index; terminate enumeration;
+ * - a NULL value will do the trick.
+ */
+ strcpy(value, "");
+ return;
+ }
+
+ memcpy(key, record[index].key, key_size);
+ memcpy(value, record[index].value, value_size);
+}
+
+
void kvp_get_os_info(void)
{
FILE *file;
@@ -678,6 +773,21 @@ int main(void)
if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
goto kvp_done;
+ /*
+ * If the pool is KVP_POOL_AUTO, dynamically generate
+ * both the key and the value; if not read from the
+ * appropriate pool.
+ */
+ if (hv_msg->kvp_hdr.pool != KVP_POOL_AUTO) {
+ kvp_pool_enumerate(hv_msg->kvp_hdr.pool,
+ hv_msg->body.kvp_enum_data.index,
+ hv_msg->body.kvp_enum_data.data.key,
+ HV_KVP_EXCHANGE_MAX_KEY_SIZE,
+ hv_msg->body.kvp_enum_data.data.value,
+ HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
+ goto kvp_done;
+ }
+
hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
key_name = (char *)hv_msg->body.kvp_enum_data.data.key;
key_value = (char *)hv_msg->body.kvp_enum_data.data.value;
--
1.7.4.1
^ permalink raw reply related
* [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte
From: Jason Wang @ 2012-03-16 15:20 UTC (permalink / raw)
To: qemu-devel, rusty, virtualization, linux-kernel, mst
This patch splits the device status field of virtio-net into ro and rw
byte. This would simplify the implementation of both host and guest
and make the layout more clean. As VIRTIO_NET_S_ANNOUNCE is a rw bit,
it was moved to bit 8 (0x100).
btw. looks like there's no implementation that depends on
VIRTIO_NET_S_ANNOUNCE, so the move is safe.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
virtio-0.9.4.lyx | 20 +++++++++++++++++++-
1 files changed, 19 insertions(+), 1 deletions(-)
diff --git a/virtio-0.9.4.lyx b/virtio-0.9.4.lyx
index 6c7bab1..ef3951c 100644
--- a/virtio-0.9.4.lyx
+++ b/virtio-0.9.4.lyx
@@ -58,6 +58,7 @@
\html_be_strict false
\author -608949062 "Rusty Russell,,,"
\author 1531152142 "pbonzini"
+\author 2090695081 "Jason"
\end_header
\begin_body
@@ -4012,8 +4013,19 @@ configuration
layout Two configuration fields are currently defined.
The mac address field always exists (though is only valid if VIRTIO_NET_F_MAC
is set), and the status field only exists if VIRTIO_NET_F_STATUS is set.
+
+\change_inserted 2090695081 1331907586
+ The low byte of status field is read-only, guest write to this byte would
+ be ignored.
+ Currently only one bit is defined for this byte: VIRTIO_NET_S_LINK_UP.
+ The high byte of status field is read-writable.
+ Currently only one bit is defined for this byte: VIRTIO_NET_S_ANNOUNCE.
+
+\change_deleted 2090695081 1331907489
Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
and VIRTIO_NET_S_ANNOUNCE.
+
+\change_unchanged
\begin_inset listings
inline false
@@ -4026,7 +4038,13 @@ status open
\begin_layout Plain Layout
-#define VIRTIO_NET_S_ANNOUNCE 2
+#define VIRTIO_NET_S_ANNOUNCE
+\change_inserted 2090695081 1331907493
+0x100
+\change_deleted 2090695081 1331907491
+2
+\change_unchanged
+
\end_layout
\begin_layout Plain Layout
^ permalink raw reply related
* [PATCH 0000/0002] drivers: scsi: storvsc
From: K. Y. Srinivasan @ 2012-03-16 20:24 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, jbottomley,
hch, linux-scsi
Cc: K. Y. Srinivasan
The current Windows hosts only handle a subset of scsi commands
sent from the guest and for commands that are not supported, they are
filtered on the host side a generic failure is returned to the guest
as SRB status. The returned error code does not permit the guest to
figure out if there was an error or if the command was not supported.
Based on the input I got from the community, I have convinced the windows
developers to return an error code that allows the guest to distinguish between
unsupported command and true failures. However, it is not clear when this fix will
be shipped.
This patch set addresses this issue by filtering the ATA_16 command on the guest
(note that currently the host is filtering this command as it is not supported). I also have
a patch here the correctly handles SRB_STATUS_INVALID_REQUEST error returns - note that
the current windows hosts don't return this today.
Regards,
K. Y
^ permalink raw reply
* [PATCH 1/2] Drivers: scsi: storvsc: Set the scsi result correctly when SRB status is INVALID
From: K. Y. Srinivasan @ 2012-03-16 20:24 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, jbottomley,
hch, linux-scsi
Cc: K. Y. Srinivasan
In-Reply-To: <1331929448-6838-1-git-send-email-kys@microsoft.com>
Currently Windows hosts only support a subset of scsi commands and for commands
that are not supported, the host returns a generic SRB failure status.
However, they have agreed to change the return value to indicate that
the command is not supported. In preparation for that, handle the
SRB_STATUS_INVALID_REQUEST return value correctly.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/scsi/storvsc_drv.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 44c7a48..8b967c9 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -202,6 +202,7 @@ enum storvsc_request_type {
#define SRB_STATUS_INVALID_LUN 0x20
#define SRB_STATUS_SUCCESS 0x01
#define SRB_STATUS_ERROR 0x04
+#define SRB_STATUS_INVALID_REQUEST 0x06
/*
* This is the end of Protocol specific defines.
@@ -779,6 +780,13 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
}
/*
+ * If the host returns with an invalid request, set
+ * the scsi command result correctly.
+ */
+ if (vm_srb->srb_status == SRB_STATUS_INVALID_REQUEST)
+ scmnd->result = ILLEGAL_REQUEST << 16;
+
+ /*
* If there is an error; offline the device since all
* error recovery strategies would have already been
* deployed on the host side.
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host
From: K. Y. Srinivasan @ 2012-03-16 20:24 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, jbottomley,
hch, linux-scsi
Cc: K. Y. Srinivasan
In-Reply-To: <1331929472-6877-1-git-send-email-kys@microsoft.com>
The current Windows hosts don't handle the ATA_16 command and furthermore
return a generic error code after filtering the command on the host side.
For now filter the command on the guest side until the host is modified to
properly handle unsupported commands.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/scsi/storvsc_drv.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 8b967c9..783bab8 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
/*
* smartd sends this command and the host does not handle
* this. So, don't send it.
+ * The current Windows hosts implement a subset of scsi commands
+ * and for the commands that are not implemented, the host filters
+ * them and returns a generic failure SRB status. I have been in
+ * discussions with the Windows team to return the appropriate SRB
+ * status code for unsupported scsi commands and while they have
+ * agreed to implement this, it is not clear when this change will be
+ * available. Consequently, filtering the command before submitting it
+ * to the host is a resonable interim solution.
*/
case SET_WINDOW:
+ case ATA_16:
scmnd->result = ILLEGAL_REQUEST << 16;
allowed = false;
break;
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/2] Drivers: scsi: storvsc: Set the scsi result correctly when SRB status is INVALID
From: Greg KH @ 2012-03-16 20:36 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: linux-kernel, devel, virtualization, ohering, jbottomley, hch,
linux-scsi
In-Reply-To: <1331929472-6877-1-git-send-email-kys@microsoft.com>
On Fri, Mar 16, 2012 at 01:24:31PM -0700, K. Y. Srinivasan wrote:
> Currently Windows hosts only support a subset of scsi commands and for commands
> that are not supported, the host returns a generic SRB failure status.
> However, they have agreed to change the return value to indicate that
> the command is not supported. In preparation for that, handle the
> SRB_STATUS_INVALID_REQUEST return value correctly.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/scsi/storvsc_drv.c | 8 ++++++++
I need an ack from the scsi maintainer before I can accept this patch.
James?
^ permalink raw reply
* Re: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host
From: Jeff Garzik @ 2012-03-17 15:04 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering, jbottomley,
hch, linux-scsi
In-Reply-To: <1331929472-6877-2-git-send-email-kys@microsoft.com>
On 03/16/2012 04:24 PM, K. Y. Srinivasan wrote:
> The current Windows hosts don't handle the ATA_16 command and furthermore
> return a generic error code after filtering the command on the host side.
> For now filter the command on the guest side until the host is modified to
> properly handle unsupported commands.
>
> Signed-off-by: K. Y. Srinivasan<kys@microsoft.com>
> Reviewed-by: Haiyang Zhang<haiyangz@microsoft.com>
> ---
> drivers/scsi/storvsc_drv.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 8b967c9..783bab8 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
> /*
> * smartd sends this command and the host does not handle
> * this. So, don't send it.
> + * The current Windows hosts implement a subset of scsi commands
> + * and for the commands that are not implemented, the host filters
> + * them and returns a generic failure SRB status. I have been in
> + * discussions with the Windows team to return the appropriate SRB
> + * status code for unsupported scsi commands and while they have
> + * agreed to implement this, it is not clear when this change will be
> + * available. Consequently, filtering the command before submitting it
> + * to the host is a resonable interim solution.
> */
> case SET_WINDOW:
> + case ATA_16:
> scmnd->result = ILLEGAL_REQUEST<< 16;
> allowed = false;
It would be even nicer to fill out a nice SCSI status for this. For an
illegal request such as this, the libata SCSI simulator
(drivers/ata/libata-scsi.c) uses a more complete sense buffer setup,
static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk,
u8 asc, u8 ascq)
{
cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
}
and calls this function for invalid/unknown/unsupported command ops thusly,
ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
/* "Invalid command operation code" */
Supplying asc/ascq provides additional information that may be useful in
determining whether or not to retry, provide better error diagnostics, etc.
The asc/ascq values are found in the SCSI standards documents.
Jeff
^ permalink raw reply
* Re: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host
From: Douglas Gilbert @ 2012-03-17 18:55 UTC (permalink / raw)
To: Jeff Garzik
Cc: K. Y. Srinivasan, gregkh, linux-kernel, devel, virtualization,
ohering, jbottomley, hch, linux-scsi
In-Reply-To: <4F64A80C.6040206@garzik.org>
On 12-03-17 04:04 PM, Jeff Garzik wrote:
> On 03/16/2012 04:24 PM, K. Y. Srinivasan wrote:
>> The current Windows hosts don't handle the ATA_16 command and furthermore
>> return a generic error code after filtering the command on the host side.
>> For now filter the command on the guest side until the host is modified to
>> properly handle unsupported commands.
>>
>> Signed-off-by: K. Y. Srinivasan<kys@microsoft.com>
>> Reviewed-by: Haiyang Zhang<haiyangz@microsoft.com>
>> ---
>> drivers/scsi/storvsc_drv.c | 9 +++++++++
>> 1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
>> index 8b967c9..783bab8 100644
>> --- a/drivers/scsi/storvsc_drv.c
>> +++ b/drivers/scsi/storvsc_drv.c
>> @@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
>> /*
>> * smartd sends this command and the host does not handle
>> * this. So, don't send it.
>> + * The current Windows hosts implement a subset of scsi commands
>> + * and for the commands that are not implemented, the host filters
>> + * them and returns a generic failure SRB status. I have been in
>> + * discussions with the Windows team to return the appropriate SRB
>> + * status code for unsupported scsi commands and while they have
>> + * agreed to implement this, it is not clear when this change will be
>> + * available. Consequently, filtering the command before submitting it
>> + * to the host is a resonable interim solution.
>> */
>> case SET_WINDOW:
>> + case ATA_16:
>> scmnd->result = ILLEGAL_REQUEST<< 16;
>> allowed = false;
>
> It would be even nicer to fill out a nice SCSI status for this. For an illegal
> request such as this, the libata SCSI simulator (drivers/ata/libata-scsi.c) uses
> a more complete sense buffer setup,
>
> static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk,
> u8 asc, u8 ascq)
> {
> cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
>
> scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
> }
>
> and calls this function for invalid/unknown/unsupported command ops thusly,
>
> ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
> /* "Invalid command operation code" */
>
> Supplying asc/ascq provides additional information that may be useful in
> determining whether or not to retry, provide better error diagnostics, etc.
>
> The asc/ascq values are found in the SCSI standards documents.
Jeff,
I agree with your suggestion and would point out that the
originally proposed:
scmnd->result = ILLEGAL_REQUEST<< 16;
is just plain wrong. result[23:16] is a Linux invented host byte
code (they typically start with "DID_") while ILLEGAL_REQUEST is
a SCSI sense key.
Doug Gilbert
^ permalink raw reply
* RE: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host
From: KY Srinivasan @ 2012-03-18 1:40 UTC (permalink / raw)
To: dgilbert@interlog.com, Jeff Garzik
Cc: linux-scsi@vger.kernel.org, gregkh@linuxfoundation.org,
ohering@suse.com, jbottomley@parallels.com,
linux-kernel@vger.kernel.org, hch@infradead.org,
virtualization@lists.osdl.org, devel@linuxdriverproject.org
In-Reply-To: <4F64DE2C.1030809@interlog.com>
> -----Original Message-----
> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> Sent: Saturday, March 17, 2012 2:56 PM
> To: Jeff Garzik
> Cc: KY Srinivasan; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
>
> On 12-03-17 04:04 PM, Jeff Garzik wrote:
> > On 03/16/2012 04:24 PM, K. Y. Srinivasan wrote:
> >> The current Windows hosts don't handle the ATA_16 command and
> furthermore
> >> return a generic error code after filtering the command on the host side.
> >> For now filter the command on the guest side until the host is modified to
> >> properly handle unsupported commands.
> >>
> >> Signed-off-by: K. Y. Srinivasan<kys@microsoft.com>
> >> Reviewed-by: Haiyang Zhang<haiyangz@microsoft.com>
> >> ---
> >> drivers/scsi/storvsc_drv.c | 9 +++++++++
> >> 1 files changed, 9 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >> index 8b967c9..783bab8 100644
> >> --- a/drivers/scsi/storvsc_drv.c
> >> +++ b/drivers/scsi/storvsc_drv.c
> >> @@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd
> *scmnd)
> >> /*
> >> * smartd sends this command and the host does not handle
> >> * this. So, don't send it.
> >> + * The current Windows hosts implement a subset of scsi commands
> >> + * and for the commands that are not implemented, the host filters
> >> + * them and returns a generic failure SRB status. I have been in
> >> + * discussions with the Windows team to return the appropriate SRB
> >> + * status code for unsupported scsi commands and while they have
> >> + * agreed to implement this, it is not clear when this change will be
> >> + * available. Consequently, filtering the command before submitting it
> >> + * to the host is a resonable interim solution.
> >> */
> >> case SET_WINDOW:
> >> + case ATA_16:
> >> scmnd->result = ILLEGAL_REQUEST<< 16;
> >> allowed = false;
> >
> > It would be even nicer to fill out a nice SCSI status for this. For an illegal
> > request such as this, the libata SCSI simulator (drivers/ata/libata-scsi.c) uses
> > a more complete sense buffer setup,
> >
> > static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk,
> > u8 asc, u8 ascq)
> > {
> > cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
> >
> > scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
> > }
> >
> > and calls this function for invalid/unknown/unsupported command ops thusly,
> >
> > ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
> > /* "Invalid command operation code" */
> >
> > Supplying asc/ascq provides additional information that may be useful in
> > determining whether or not to retry, provide better error diagnostics, etc.
> >
> > The asc/ascq values are found in the SCSI standards documents.
>
> Jeff,
> I agree with your suggestion and would point out that the
> originally proposed:
> scmnd->result = ILLEGAL_REQUEST<< 16;
>
> is just plain wrong. result[23:16] is a Linux invented host byte
> code (they typically start with "DID_") while ILLEGAL_REQUEST is
> a SCSI sense key.
>
> Doug Gilbert
>
Jeff, Doug,
Thank you. I will make the suggested changes.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte
From: Michael S. Tsirkin @ 2012-03-18 12:22 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, qemu-devel, virtualization
In-Reply-To: <20120316152026.6428.6983.stgit@jason-ThinkPad-T400>
On Fri, Mar 16, 2012 at 11:20:26PM +0800, Jason Wang wrote:
> This patch splits the device status field of virtio-net into ro and rw
> byte. This would simplify the implementation of both host and guest
> and make the layout more clean. As VIRTIO_NET_S_ANNOUNCE is a rw bit,
> it was moved to bit 8 (0x100).
>
> btw. looks like there's no implementation that depends on
> VIRTIO_NET_S_ANNOUNCE, so the move is safe.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Hmm, I know I proposed this myself, and I thought it will
prevent problems if we will add more rw bits,
but I missed the following race:
host writes VIRTIO_NET_S_ANNOUNCE = 1, interrupt
guest reads VIRTIO_NET_S_ANNOUNCE = 1
host writes VIRTIO_NET_S_SOME_NEW_FIELD = 1, interrupt
guest writes VIRTIO_NET_S_ANNOUNCE = 0
VIRTIO_NET_S_SOME_NEW_FIELD is overwritten
guest reads VIRTIO_NET_S_SOME_NEW_FIELD = 0
How about making the new bit write 1 to clear?
If we do, we can keep it where it is currently ...
> ---
> virtio-0.9.4.lyx | 20 +++++++++++++++++++-
> 1 files changed, 19 insertions(+), 1 deletions(-)
>
> diff --git a/virtio-0.9.4.lyx b/virtio-0.9.4.lyx
> index 6c7bab1..ef3951c 100644
> --- a/virtio-0.9.4.lyx
> +++ b/virtio-0.9.4.lyx
> @@ -58,6 +58,7 @@
> \html_be_strict false
> \author -608949062 "Rusty Russell,,,"
> \author 1531152142 "pbonzini"
> +\author 2090695081 "Jason"
> \end_header
>
> \begin_body
> @@ -4012,8 +4013,19 @@ configuration
> layout Two configuration fields are currently defined.
> The mac address field always exists (though is only valid if VIRTIO_NET_F_MAC
> is set), and the status field only exists if VIRTIO_NET_F_STATUS is set.
> +
> +\change_inserted 2090695081 1331907586
> + The low byte of status field is read-only, guest write to this byte would
> + be ignored.
> + Currently only one bit is defined for this byte: VIRTIO_NET_S_LINK_UP.
> + The high byte of status field is read-writable.
> + Currently only one bit is defined for this byte: VIRTIO_NET_S_ANNOUNCE.
> +
> +\change_deleted 2090695081 1331907489
> Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
> and VIRTIO_NET_S_ANNOUNCE.
> +
> +\change_unchanged
>
> \begin_inset listings
> inline false
> @@ -4026,7 +4038,13 @@ status open
>
> \begin_layout Plain Layout
>
> -#define VIRTIO_NET_S_ANNOUNCE 2
> +#define VIRTIO_NET_S_ANNOUNCE
> +\change_inserted 2090695081 1331907493
> +0x100
> +\change_deleted 2090695081 1331907491
> +2
> +\change_unchanged
> +
> \end_layout
>
> \begin_layout Plain Layout
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox