Re: [PATCH] virtio_console: Annotate struct port_buffer with __counted_by
On Fri, 2023-09-22 at 10:51 -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct port_buffer.
>
> [1]
> https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Amit Shah
> Cc: Arnd Bergmann
> Cc: Greg Kroah-Hartman
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Kees Cook
> ---
> drivers/char/virtio_console.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 680d1ef2a217..431e9e5bf9c1 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -106,7 +106,7 @@ struct port_buffer {
> unsigned int sgpages;
>
> /* sg is used if spages > 0. sg must be the last in is struct */
> - struct scatterlist sg[];
> + struct scatterlist sg[] __counted_by(sgpages);
> };
Reviewed-by: Amit Shah
Greg, please pick this up.
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/2] [RFC] virtio-rng entropy leak reporting feature
On Mon, 2023-03-13 at 19:05 +0100, Amit Shah wrote: > Hey Herbert / Jason / crypto maintainers, [...] > Let's wait a couple more days for responses, otherwise I suggest you > resubmit to kickstart a new discussion, with the note that Jason had > something else in mind - so that it doesn't appear as though we're > trying to override that. I reached out to Jason on IRC, and he mentioned he will follow up with a patch that incorporates ideas from your patch plus hooking into random.c. Sounds promising! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/2] [RFC] virtio-rng entropy leak reporting feature
Hey Herbert / Jason / crypto maintainers, On Mon, 2023-03-13 at 11:42 +0100, bchal...@amazon.es wrote: > Hi Amit, > > Thanks for taking the time to look into this. > > On 3/2/23 5:55 PM, Amit Shah wrote: > > CAUTION: This email originated from outside of the organization. Do not > > click links or open attachments unless you can confirm the sender and know > > the content is safe. > > > > > > > > Hey all, > > > > On Tue, 2023-01-31 at 17:27 +0100, Jason A. Donenfeld wrote: > > > You sent a v2, but I'm not back until the 11th to provide comments on > > > v1. I still think this isn't the right direction, as this needs tie-ins > > > to the rng to actually be useful. Please stop posting new versions of > > > this for now, so that somebody doesn't accidentally merge it; that'd be > > > a big mistake. I'll paste what I wrote you prior: > > > > I sat down to review the patchset but looks like there's some > > background I'm not aware of. > > > > It looks like Babis has implemented the guest side here according to > > the spec change that Michael has posted. > > > > Jason, do you have an alternative in mind? In that case, we should > > pick this up in the spec update thread instead. > > I am not sure what Jason meant here by "This needs to be integrated more > closely with random.c itself, similar to how vmgenid works", > but here's my take on this. > > The point of the patchset is to provide an implementation of Michael's spec > on which we can discuss. It implements the HW API and it has > some hooks showing how this API could be used. It is mainly directed towards > the user-space where we did not have a proper API to consume > VMGENID-like functionality. With regards to the random.c components it does > exactly what VMGENID does currently, i.e. whenever an entropy-leak > is detected it uses the new random bytes provided by the virtio-rng device as > entropy. This is as racy as VMGENID, as I mention in the cover > letter of the patchset. Yea, this does solve the race condition from the userspace pov, so does look better. Thanks for the details! Not sure if Jason's back yet - but Herbert, or other crypto maintainers, can you chime in from the crypto/rng perspective if this looks sane? Jason has previously NACKed the patch without follow-up, and I don't want the patch to linger without a path to merging, especially when it's not clear what Jason meant. > However, the new spec does allow us to do things _correctly_, i.e. not rely > on asynchronous handling of events to re-seed the kernel. For example, we > could achieve something like that by making use of the "copy-on-leak" > operation, so that a flag changes value before vCPUs get resumed, so we know > when a leak has happened when needed, e.g. before returning random bytes to > user-space. At least, that's what I remember us discussing during LPC. > Jason, Michael, Alex, please keep me honest here :) > > Unfortunately, I am not very familiar with the random.c code and did not want > to do something there that would most certainly be wrong, hence I posted > this as an RFC, asking for input on how we could achieve this better > integration. Hopefully, when Jason is back from his vacation he can share his > thoughts > on this, but if yourself (or anyone else interested) have any ideas on how we > could design this properly, I 'm happy to discuss! Let's wait a couple more days for responses, otherwise I suggest you resubmit to kickstart a new discussion, with the note that Jason had something else in mind - so that it doesn't appear as though we're trying to override that. Thanks for the patience, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/2] [RFC] virtio-rng entropy leak reporting feature
Hey all, On Tue, 2023-01-31 at 17:27 +0100, Jason A. Donenfeld wrote: > You sent a v2, but I'm not back until the 11th to provide comments on > v1. I still think this isn't the right direction, as this needs tie-ins > to the rng to actually be useful. Please stop posting new versions of > this for now, so that somebody doesn't accidentally merge it; that'd be > a big mistake. I'll paste what I wrote you prior: I sat down to review the patchset but looks like there's some background I'm not aware of. It looks like Babis has implemented the guest side here according to the spec change that Michael has posted. Jason, do you have an alternative in mind? In that case, we should pick this up in the spec update thread instead. Somehow it feels like I don't have the complete story for this feature set, having missed the discussion during LPC. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: console: remove check for cpkt value when nominating console port
On Tue, 2022-11-15 at 14:13 +1100, Andy Bui wrote: > On Mon, Nov 14, 2022 at 09:43:48AM -0800, Amit Shah wrote: > > > > On Sat, 2022-11-12 at 23:45 +1100, Andy Bui wrote: > > > The virtIO spec does not specify a need for a value when nominating a > > > port as a console port, yet the virtio_console driver requires the value > > > to be 1. > > > > > > Besides being a check that's not specified by the virtIO spec, I don't > > > see anywhere else in the kernel the value is used when the corresponding > > > event is VIRTIO_CONSOLE_CONSOLE_PORT. As an example QEMU also currently > > > only passes in value=1 when nominating a console port. > > > > The original virtio-console driver just had the one port, and it was, > > as the name suggests, was a console port. When we converted this > > console driver to a generic serial driver, the first port was kept as > > the console port to not break old drivers or hypervisors. I'm afraid > > we'll have to keep this bit of backward compatibility forever. > > > > I think I might be misunderstanding the exact usage of the cpkt value. > Is error'ing and not continuing with the console port nomination the intended > behaviour when value != 1? I'm guessing that there are some hypervisors out > there that set value to something not 1 and we don't want those ports to be > the console ports? I've not looked at the code in quite a bit - please bear with me as I refresh my memory. I think the backward compat concern I have here is that a really old hypervisor - one that doesn't do virtio-serial-ports yet, will not have any other messages sent from the host, but the original console setup messages. The is_serial_port() check looks for whether hvc is initialized. It's possible hvc may not have been initialized when some control message comes in. > If this is correct I think a warning should be emitted here? Not setting the > value to 1 can exhibit undefined behaviour (at least when interpreting the > virtIO spec) and it would help hypervisor developers. Did you actually run into some issue here? Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_console: Introduce an ID allocator for virtual console numbers
On Wed, 2022-11-23 at 11:17 +0100, Cédric Le Goater wrote:
> Hello Amit,
>
> On 11/22/22 18:03, Amit Shah wrote:
> > On Mon, 2022-11-14 at 18:38 +0100, Cédric Le Goater wrote:
> > > When a virtio console port is initialized, it is registered as an hvc
> > > console using a virtual console number. If a KVM guest is started with
> > > multiple virtio console devices, the same vtermno (or virtual console
> > > number) can be used to allocate different hvc consoles, which leads to
> > > various communication problems later on.
> > >
> > > This is also reported in debugfs :
> > >
> > ># grep vtermno /sys/kernel/debug/virtio-ports/*
> > >/sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
> > >/sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
> > >/sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
> > >/sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3
> > >
> > > Replace the next_vtermno global with an ID allocator and start the
> > > allocation at 1 as it is today. Also recycle IDs when a console port
> > > is removed.
> >
> > When the original virtio_console module was written, it didn't have
> > support for multiple ports to be used this way. So the oddity you're
> > seeing is left there deliberately: VMMs should not be instantiating
> > console ports this way.
> >
> > I don't know if we should take in this change, but can you walk through
> > all combinations of new/old guest and new/old hypervisor and ensure
> > nothing's going to break -- and confirm with the spec this is still OK
> > to do? It may not be a goal to still ensure launches of a new guest on
> > a very old (say) Centos5 guest still works -- but that was the point of
> > maintaining backward compat...
>
> 'next_vtermno' was introduced by d8a02bd58ab6 ("virtio: console:
> remove global var") to differentiate the underlying kernel hvc console
> associated with each virtio console port. Some drivers, like XEN,
> simply use a magic/cookie number for instance.
>
> This number is not related to the virtio specs. It is not exposed to
> QEMU nor the guest (a part from debugfs). It's an internal identifier
> related to the implementation in the kernel. I don't understand how
> this could break compatibility. The change even keeps the allocated
> range the same in case some assumption is made on vtermno 0. Am I
> missing something ?
No, you're right about this being kernel-internal -- just that it's
used with hvc instead of qemu like I mentioned.
I think this is the right change; just want to confirm hvc didn't get
confused.
>
> In the virtio console driver case, we could also generate a unique
> number from the tuple { virtio device index, virtio console port }.
> The ID allocator approach is simpler.
I think the bug is that we don't increment the vtermno today in all
places that we should; but this patch solves it too - I don't mind
adding the extra ida bits.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_console: Introduce an ID allocator for virtual console numbers
On Mon, 2022-11-14 at 18:38 +0100, Cédric Le Goater wrote: > When a virtio console port is initialized, it is registered as an hvc > console using a virtual console number. If a KVM guest is started with > multiple virtio console devices, the same vtermno (or virtual console > number) can be used to allocate different hvc consoles, which leads to > various communication problems later on. > > This is also reported in debugfs : > > # grep vtermno /sys/kernel/debug/virtio-ports/* > /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1 > /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1 > /sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2 > /sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3 > > Replace the next_vtermno global with an ID allocator and start the > allocation at 1 as it is today. Also recycle IDs when a console port > is removed. When the original virtio_console module was written, it didn't have support for multiple ports to be used this way. So the oddity you're seeing is left there deliberately: VMMs should not be instantiating console ports this way. I don't know if we should take in this change, but can you walk through all combinations of new/old guest and new/old hypervisor and ensure nothing's going to break -- and confirm with the spec this is still OK to do? It may not be a goal to still ensure launches of a new guest on a very old (say) Centos5 guest still works -- but that was the point of maintaining backward compat... Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: console: remove check for cpkt value when nominating console port
On Sat, 2022-11-12 at 23:45 +1100, Andy Bui wrote: > The virtIO spec does not specify a need for a value when nominating a > port as a console port, yet the virtio_console driver requires the value > to be 1. > > Besides being a check that's not specified by the virtIO spec, I don't > see anywhere else in the kernel the value is used when the corresponding > event is VIRTIO_CONSOLE_CONSOLE_PORT. As an example QEMU also currently > only passes in value=1 when nominating a console port. The original virtio-console driver just had the one port, and it was, as the name suggests, was a console port. When we converted this console driver to a generic serial driver, the first port was kept as the console port to not break old drivers or hypervisors. I'm afraid we'll have to keep this bit of backward compatibility forever. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/9] virtio_console: eliminate anonymous module_init & module_exit
On Wed, 2022-03-16 at 12:20 -0700, Randy Dunlap wrote:
> Eliminate anonymous module_init() and module_exit(), which can lead to
> confusion or ambiguity when reading System.map, crashes/oops/bugs,
> or an initcall_debug log.
>
> Give each of these init and exit functions unique driver-specific
> names to eliminate the anonymous names.
>
> Example 1: (System.map)
> 832fc78c t init
> 832fc79e t init
> 832fc8f8 t init
>
> Example 2: (initcall_debug log)
> calling init+0x0/0x12 @ 1
> initcall init+0x0/0x12 returned 0 after 15 usecs
> calling init+0x0/0x60 @ 1
> initcall init+0x0/0x60 returned 0 after 2 usecs
> calling init+0x0/0x9a @ 1
> initcall init+0x0/0x9a returned 0 after 74 usecs
>
> Fixes: 31610434bc35 ("Virtio console driver")
> Fixes: 7177876fea83 ("virtio: console: Add ability to remove module")
> Signed-off-by: Randy Dunlap <
> rdun...@infradead.org
> >
> Cc: Amit Shah <
> a...@kernel.org
> >
> Cc:
> virtualization@lists.linux-foundation.org
>
> Cc: Arnd Bergmann <
> a...@arndb.de
> >
> Cc: Greg Kroah-Hartman <
> gre...@linuxfoundation.org
> >
> ---
> drivers/char/virtio_console.c |8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Amit Shah
I don't think the Fixes-by really applies here, though - we don't
really want to push this into stable, nor do we want any automated
tools to pick this up because of that tag..
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: remove pointless check for debugfs_create_dir()
On Tue, 2021-02-16 at 16:04 +0100, Greg Kroah-Hartman wrote:
> It is impossible for debugfs_create_dir() to return NULL, so checking
> for it gives people a false sense that they actually are doing something
> if an error occurs. As there is no need to ever change kernel logic if
> debugfs is working "properly" or not, there is no need to check the
> return value of debugfs calls, so remove the checks here as they will
> never be triggered and are wrong.
>
> Cc: Amit Shah
> Cc: Arnd Bergmann
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/char/virtio_console.c | 23 +--
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1836cc56e357..59dfd9c421a1 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1456,18 +1456,15 @@ static int add_port(struct ports_device *portdev, u32
> id)
>*/
> send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
>
> - if (pdrvdata.debugfs_dir) {
> - /*
> - * Finally, create the debugfs file that we can use to
> - * inspect a port's state at any time
> - */
> - snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
> - port->portdev->vdev->index, id);
> - port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
> - pdrvdata.debugfs_dir,
> - port,
> - &port_debugfs_fops);
> - }
> + /*
> + * Finally, create the debugfs file that we can use to
> + * inspect a port's state at any time
> + */
> + snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
> + port->portdev->vdev->index, id);
> + port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
> + pdrvdata.debugfs_dir,
> + port, &port_debugfs_fops);
> return 0;
>
> free_inbufs:
> @@ -2244,8 +2241,6 @@ static int __init init(void)
> }
>
> pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> - if (!pdrvdata.debugfs_dir)
> - pr_warn("Error creating debugfs dir for virtio-ports\n");
> INIT_LIST_HEAD(&pdrvdata.consoles);
> INIT_LIST_HEAD(&pdrvdata.portdevs);
>
Reviewed-by: Amit Shah
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] char: virtio: Select VIRTIO from VIRTIO_CONSOLE.
On Mon, Aug 31, 2020 at 06:58:50PM +0200, Michal Suchanek wrote: > Make it possible to have virtio console built-in when > other virtio drivers are modular. > > Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah > --- > drivers/char/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig > index 3a144c000a38..9bd9917ca9af 100644 > --- a/drivers/char/Kconfig > +++ b/drivers/char/Kconfig > @@ -93,8 +93,9 @@ config PPDEV > > config VIRTIO_CONSOLE > tristate "Virtio console" > - depends on VIRTIO && TTY > + depends on TTY > select HVC_DRIVER > + select VIRTIO > help > Virtio console for use with hypervisors. > > -- > 2.28.0 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5/5] virtio_console: Constify some static variables
On (Wed) 01 Jul 2020 [22:09:50], Rikard Falkeborn wrote: > The id_table and feature_table pointers in struct virtio_driver are > pointers to const. Mark the corresponding static variables const to > allow the compiler to put them in read-only memory. > > Before: >textdata bss dec hex filename > 25447 713 76 26236667c drivers/char/virtio_console.o > > After: >textdata bss dec hex filename > 25488 673 76 26237667d drivers/char/virtio_console.o > > Signed-off-by: Rikard Falkeborn Reviewed-by: Amit Shah Please CC me on the entire series instead of individual patches in the future. Thanks, Amit -- http://amitshah.net/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: virtio_console: add missing MODULE_DEVICE_TABLE() for rproc serial
On Tue, 2020-03-10 at 14:05 +0300, Alexander Lobakin wrote:
> rproc_serial_id_table lacks an exposure to module devicetable, so
> when remoteproc firmware requests VIRTIO_ID_RPROC_SERIAL, no uevent
> is generated and no module autoloading occurs.
> Add missing MODULE_DEVICE_TABLE() annotation and move the existing
> one for VIRTIO_ID_CONSOLE right to the table itself.
>
> Fixes: 1b6370463e88 ("virtio_console: Add support for remoteproc
> serial")
> Cc: # v3.8+
> Signed-off-by: Alexander Lobakin
Reviewed-by: Amit Shah
Thanks,
> ---
> drivers/char/virtio_console.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/virtio_console.c
> b/drivers/char/virtio_console.c
> index 4df9b40d6342..7e1bc0f580a2 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -2116,6 +2116,7 @@ static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_CONSOLE, VIRTIO_DEV_ANY_ID },
> { 0 },
> };
> +MODULE_DEVICE_TABLE(virtio, id_table);
>
> static unsigned int features[] = {
> VIRTIO_CONSOLE_F_SIZE,
> @@ -2128,6 +2129,7 @@ static struct virtio_device_id
> rproc_serial_id_table[] = {
> #endif
> { 0 },
> };
> +MODULE_DEVICE_TABLE(virtio, rproc_serial_id_table);
>
> static unsigned int rproc_serial_features[] = {
> };
> @@ -2280,6 +2282,5 @@ static void __exit fini(void)
> module_init(init);
> module_exit(fini);
>
> -MODULE_DEVICE_TABLE(virtio, id_table);
> MODULE_DESCRIPTION("Virtio console driver");
> MODULE_LICENSE("GPL");
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed
On Tue, 2019-12-03 at 10:42 -0500, Michael S. Tsirkin wrote:
> On Tue, Dec 03, 2019 at 03:46:50PM +0100, Amit Shah wrote:
> > On Thu, 2019-11-14 at 13:25 +0100, Laurent Vivier wrote:
> > > When we hot unplug a virtserialport and then try to hot plug
> > > again,
> > > it fails:
> > >
> > > (qemu) chardev-add
> > > socket,id=serial0,path=/tmp/serial0,server,nowait
> > > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\
> > > chardev=serial0,id=serial0,name=serial0
> > > (qemu) device_del serial0
> > > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\
> > > chardev=serial0,id=serial0,name=serial0
> > > kernel error:
> > > virtio-ports vport2p2: Error allocating inbufs
> > > qemu error:
> > > virtio-serial-bus: Guest failure in adding port 2 for device \
> > > virtio-serial0.0
> > >
> > > This happens because buffers for the in_vq are allocated when the
> > > port is
> > > added but are not released when the port is unplugged.
> > >
> > > They are only released when virtconsole is removed (see
> > > a7a69ec0d8e4)
> > >
> > > To avoid the problem and to be symmetric, we could allocate all
> > > the
> > > buffers
> > > in init_vqs() as they are released in remove_vqs(), but it sounds
> > > like
> > > a waste of memory.
> > >
> > > Rather than that, this patch changes add_port() logic to ignore
> > > ENOSPC
> > > error in fill_queue(), which means queue has already been filled.
> > >
> > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > Cc: m...@redhat.com
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Laurent Vivier
> >
> > Reviewed-by: Amit Shah
> >
> > Thanks!
>
>
> Thanks, however this has already been merged by Linus.
> I can't add the tag retroactively, sorry about that.
Right, no problem - but I wanted to ensure it's on-list :)
>
> For bugfix patches like that, I think we can reasonably
> target a turn around of a couple of days, these
> shouldn't really need to wait weeks for review.
Sure, thanks for picking it up fast enough. Life happens, etc., and
it's not always possible to reply in a couple of days. Since we had
already covered the main comments in v1 and v2, v3 wasn't going to need
much attention anyway.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed
On Thu, 2019-11-14 at 13:25 +0100, Laurent Vivier wrote:
> When we hot unplug a virtserialport and then try to hot plug again,
> it fails:
>
> (qemu) chardev-add socket,id=serial0,path=/tmp/serial0,server,nowait
> (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\
> chardev=serial0,id=serial0,name=serial0
> (qemu) device_del serial0
> (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\
> chardev=serial0,id=serial0,name=serial0
> kernel error:
> virtio-ports vport2p2: Error allocating inbufs
> qemu error:
> virtio-serial-bus: Guest failure in adding port 2 for device \
> virtio-serial0.0
>
> This happens because buffers for the in_vq are allocated when the
> port is
> added but are not released when the port is unplugged.
>
> They are only released when virtconsole is removed (see a7a69ec0d8e4)
>
> To avoid the problem and to be symmetric, we could allocate all the
> buffers
> in init_vqs() as they are released in remove_vqs(), but it sounds
> like
> a waste of memory.
>
> Rather than that, this patch changes add_port() logic to ignore
> ENOSPC
> error in fill_queue(), which means queue has already been filled.
>
> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> Cc: m...@redhat.com
> Cc: sta...@vger.kernel.org
> Signed-off-by: Laurent Vivier
Reviewed-by: Amit Shah
Thanks!
> ---
>
> Notes:
> v3: add a comment about ENOSPC error
> v2: making fill_queue return int and testing return code for
> -ENOSPC
>
> drivers/char/virtio_console.c | 28 +---
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c
> b/drivers/char/virtio_console.c
> index 7270e7b69262..3259426f01dc 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1325,24 +1325,24 @@ static void set_console_size(struct port
> *port, u16 rows, u16 cols)
> port->cons.ws.ws_col = cols;
> }
>
> -static unsigned int fill_queue(struct virtqueue *vq, spinlock_t
> *lock)
> +static int fill_queue(struct virtqueue *vq, spinlock_t *lock)
> {
> struct port_buffer *buf;
> - unsigned int nr_added_bufs;
> + int nr_added_bufs;
> int ret;
>
> nr_added_bufs = 0;
> do {
> buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
> if (!buf)
> - break;
> + return -ENOMEM;
>
> spin_lock_irq(lock);
> ret = add_inbuf(vq, buf);
> if (ret < 0) {
> spin_unlock_irq(lock);
> free_buf(buf, true);
> - break;
> + return ret;
> }
> nr_added_bufs++;
> spin_unlock_irq(lock);
> @@ -1362,7 +1362,6 @@ static int add_port(struct ports_device
> *portdev, u32 id)
> char debugfs_name[16];
> struct port *port;
> dev_t devt;
> - unsigned int nr_added_bufs;
> int err;
>
> port = kmalloc(sizeof(*port), GFP_KERNEL);
> @@ -1421,11 +1420,13 @@ static int add_port(struct ports_device
> *portdev, u32 id)
> spin_lock_init(&port->outvq_lock);
> init_waitqueue_head(&port->waitqueue);
>
> - /* Fill the in_vq with buffers so the host can send us data. */
> - nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
> - if (!nr_added_bufs) {
> + /* We can safely ignore ENOSPC because it means
> + * the queue already has buffers. Buffers are removed
> + * only by virtcons_remove(), not by unplug_port()
> + */
> + err = fill_queue(port->in_vq, &port->inbuf_lock);
> + if (err < 0 && err != -ENOSPC) {
> dev_err(port->dev, "Error allocating inbufs\n");
> - err = -ENOMEM;
> goto free_device;
> }
>
> @@ -2059,14 +2060,11 @@ static int virtcons_probe(struct
> virtio_device *vdev)
> INIT_WORK(&portdev->control_work, &control_work_handler);
>
> if (multiport) {
> - unsigned int nr_added_bufs;
> -
> spin_lock_init(&portdev->c_ivq_lock);
> spin_lock_init(&portdev->c_ovq_lock);
>
> - nr_added_bufs = fill_queue(portdev->c_ivq,
> -&portdev->c_ivq_lock);
> - if (!nr_added_bufs) {
> + err = fill_queue(portdev->c_ivq, &portdev->c_ivq_lock);
> + if (err < 0) {
>
Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
On Wed, 2019-11-06 at 10:03 -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 03:02:25PM +0100, Laurent Vivier wrote:
> > On 06/11/2019 14:56, Michael S. Tsirkin wrote:
> > > On Fri, Oct 18, 2019 at 06:47:18PM +0200, Laurent Vivier wrote:
> > > > When we hot unplug a virtserialport and then try to hot plug
> > > > again,
> > > > it fails:
> > > >
> > > > (qemu) chardev-add
> > > > socket,id=serial0,path=/tmp/serial0,server,nowait
> > > > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\
> > > > chardev=serial0,id=serial0,name=serial0
> > > > (qemu) device_del serial0
> > > > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\
> > > > chardev=serial0,id=serial0,name=serial0
> > > > kernel error:
> > > > virtio-ports vport2p2: Error allocating inbufs
> > > > qemu error:
> > > > virtio-serial-bus: Guest failure in adding port 2 for device
> > > > \
> > > > virtio-serial0.0
> > > >
> > > > This happens because buffers for the in_vq are allocated when
> > > > the port is
> > > > added but are not released when the port is unplugged.
> > > >
> > > > They are only released when virtconsole is removed (see
> > > > a7a69ec0d8e4)
> > > >
> > > > To avoid the problem and to be symmetric, we could allocate all
> > > > the buffers
> > > > in init_vqs() as they are released in remove_vqs(), but it
> > > > sounds like
> > > > a waste of memory.
> > > >
> > > > Rather than that, this patch changes add_port() logic to only
> > > > allocate the
> > > > buffers if the in_vq has available free slots.
> > > >
> > > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after
> > > > reset")
> > > > Cc: m...@redhat.com
> > > > Signed-off-by: Laurent Vivier
> > > > ---
> > > > drivers/char/virtio_console.c | 17 +++--
> > > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/char/virtio_console.c
> > > > b/drivers/char/virtio_console.c
> > > > index 7270e7b69262..77105166fe01 100644
> > > > --- a/drivers/char/virtio_console.c
> > > > +++ b/drivers/char/virtio_console.c
> > > > @@ -1421,12 +1421,17 @@ static int add_port(struct ports_device
> > > > *portdev, u32 id)
> > > > spin_lock_init(&port->outvq_lock);
> > > > init_waitqueue_head(&port->waitqueue);
> > > >
> > > > - /* Fill the in_vq with buffers so the host can send us
> > > > data. */
> > > > - nr_added_bufs = fill_queue(port->in_vq, &port-
> > > > >inbuf_lock);
> > > > - if (!nr_added_bufs) {
> > > > - dev_err(port->dev, "Error allocating
> > > > inbufs\n");
> > > > - err = -ENOMEM;
> > > > - goto free_device;
> > > > + /* if the in_vq has not already been filled (the port
> > > > has already been
> > > > +* used and unplugged), fill the in_vq with buffers so
> > > > the host can
> > > > +* send us data.
> > > > +*/
> > > > + if (port->in_vq->num_free != 0) {
> > > > + nr_added_bufs = fill_queue(port->in_vq, &port-
> > > > >inbuf_lock);
> > > > + if (!nr_added_bufs) {
> > > > + dev_err(port->dev, "Error allocating
> > > > inbufs\n");
> > > > + err = -ENOMEM;
> > > > + goto free_device;
> > > > + }
> > > > }
> > > >
> > > > if (is_rproc_serial(port->portdev->vdev))
> > >
> > > Well fill_queue will just add slots as long as it can.
> > > So on a full queue it does nothing. How does this patch help?
> >
> > Yes, but in this case it returns 0 and so add_port() fails and
> > exits
> > with -ENOMEM and the device is freed. It's what this patch tries to
> > avoid.
> >
> > Thanks,
> > Laurent
>
> Oh I see. However it's a bit asymmetrical to special case ring full.
> How about making fill_queue return int and testing return code for
> -ENOSPC instead? Will also help propagate errors correctly.
Yes, I like this better too.
Can you call out which commit introduced this behaviour / regression?
>
> And I guess CC stable?
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: remove vq buf while unpluging port
On Fri, 2019-05-24 at 20:51 +0200, Greg KH wrote: > On Sun, Apr 28, 2019 at 09:50:04AM +0800, zhenwei pi wrote: > > A bug can be easily reproduced: > > Host# cat guest-agent.xml > > > > > >> state="connected"/> > > > > Host# virsh attach-device instance guest-agent.xml > > Host# virsh detach-device instance guest-agent.xml > > Host# virsh attach-device instance guest-agent.xml > > > > and guest report: virtio-ports vport0p1: Error allocating inbufs > > > > The reason is that the port is unplugged and the vq buf still > > remained. > > So, fix two cases in this patch: > > 1, fix memory leak with attach-device/detach-device. > > 2, fix logic bug with attach-device/detach-device/attach-device. The "leak" happens because the host-side of the connection is still connected. This is by design -- if a guest has written data before being unplugged, the port isn't released till the host connection goes down to ensure a host process reads all the data out of the port. Can you try similar, but also disconnecting the host side and see if that fixes things? > Amit, any ideas if this is valid or not and if this should be > applied? This had indeed been missed, thanks! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Qemu-devel] virtio-console downgrade the virtio-pci-blk performance
On (Thu) 11 Oct 2018 [18:15:41], Feng Li wrote:
> Add Amit Shah.
>
> After some tests, we found:
> - the virtio serial port number is inversely proportional to the iSCSI
> virtio-blk-pci performance.
> If we set the virio-serial ports to 2(" type='virtio-serial' index='0' ports='2'/>), the performance downgrade
> is minimal.
If you use multiple virtio-net (or blk) devices -- just register, not
necessarily use -- does that also bring the performance down? I
suspect it's the number of interrupts that get allocated for the
ports. Also, could you check if MSI is enabled? Can you try with and
without? Can you also reproduce if you have multiple virtio-serial
controllers with 2 ports each (totalling up to whatever number that
reproduces the issue).
Amit
>
> - use local disk/ram disk as virtio-blk-pci disk, the performance
> downgrade is still obvious.
>
>
> Could anyone give some help about this issue?
>
> Feng Li 于2018年10月1日周一 下午10:58写道：
> >
> > Hi Dave,
> > My comments are in-line.
> >
> > Dr. David Alan Gilbert 于2018年10月1日周一 下午7:41写道：
> > >
> > > * Feng Li (lifeng1...@gmail.com) wrote:
> > > > Hi,
> > > > I found an obvious performance downgrade when virtio-console combined
> > > > with virtio-pci-blk.
> > > >
> > > > This phenomenon exists in nearly all Qemu versions and all Linux
> > > > (CentOS7, Fedora 28, Ubuntu 18.04) distros.
> > > >
> > > > This is a disk cmd:
> > > > -drive
> > > > file=iscsi://127.0.0.1:3260/iqn.2016-02.com.test:system:fl-iscsi/1,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=native
> > > > -device
> > > > virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on
> > > >
> > > > If I add "-device
> > > > virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 ", the virtio
> > > > disk 4k iops (randread/randwrite) would downgrade from 60k to 40k.
> > > >
> > > > In VM, if I rmmod virtio-console, the performance will back to normal.
> > > >
> > > > Any idea about this issue?
> > > >
> > > > I don't know this is a qemu issue or kernel issue.
> > >
> > > It sounds odd; can you provide more details on:
> > > a) The benchmark you're using.
> > I'm using fio, the config is:
> > [global]
> > ioengine=libaio
> > iodepth=128
> > runtime=120
> > time_based
> > direct=1
> >
> > [randread]
> > stonewall
> > bs=4k
> > filename=/dev/vdb
> > rw=randread
> >
> > > b) the host and the guest config (number of cpus etc)
> > The qemu cmd is : /usr/libexec/qemu-kvm --device virtio-balloon -m 16G
> > --enable-kvm -cpu host -smp 8
> > or qemu-system-x86_64 --device virtio-balloon -m 16G --enable-kvm -cpu
> > host -smp 8
> >
> > The result is the same.
> >
> > > c) Why are you running it with iscsi back to the same host - why not
> > > just simplify the test back to a simple file?
> > >
> >
> > Because my ISCSI target could supply a high IOPS performance.
> > If using a slow disk, the performance downgrade would be not so obvious.
> > It's easy to be seen, you could try it.
> >
> >
> > > Dave
> > >
> > > >
> > > > Thanks in advance.
> > > > --
> > > > Thanks and Best Regards,
> > > > Alex
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> >
> >
> >
> > --
> > Thanks and Best Regards,
> > Feng Li(Alex)
>
>
>
> --
> Thanks and Best Regards,
> Feng Li(Alex)
Amit
--
http://amitshah.net/
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/6] virtio-console: spec compliance fixes
On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote: > On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote: > > (apologies if you received a dup) > > > > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote: > > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote: > > > > Turns out virtio console tries to take a buffer out of an active vq. > > > > Works by sheer luck, and is explicitly forbidden by spec. And while > > > > going over it I saw that error handling is also broken - > > > > failure is easy to trigger if I force allocations to fail. > > > > > > > > Lightly tested. > > > > > > Amit - any feedback before I push these patches? > > > > The changes look good spec-wise. > > > > There are a bunch of tests in avocado-vt that test virtio-console > > functionality. Can you give those a try before pushing? > > > > Amit > > I pushed before I did that test, will try to find the time later. BTW > do you still want to be on the maintainers list? Yes, I want to be on the maintainers list; but won't mind co-maintainers. The recent changes seem to be spec-related, and I'd expect you to have a good handle on that anyway. Amit -- http://amitshah.net/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/6] virtio-console: spec compliance fixes
On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote: > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote: > > Turns out virtio console tries to take a buffer out of an active vq. > > Works by sheer luck, and is explicitly forbidden by spec. And while > > going over it I saw that error handling is also broken - > > failure is easy to trigger if I force allocations to fail. > > > > Lightly tested. > > Amit - any feedback before I push these patches? The changes look good spec-wise. There are a bunch of tests in avocado-vt that test virtio-console functionality. Can you give those a try before pushing? Amit -- http://amitshah.net/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 13/35] drivers/char: Convert remaining use of pr_warning to pr_warn
On (Thu) 16 Feb 2017 [23:11:26], Joe Perches wrote: > To enable eventual removal of pr_warning > > This makes pr_warn use consistent for drivers/char > > Prior to this patch, there were 1 use of pr_warning and > 40 uses of pr_warn in drivers/char > > Signed-off-by: Joe Perches Reviewed-by: Amit Shah Thanks, Amit -- http://log.amitshah.net/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] MAINTAINERS: update my email address
On (Fri) 03 Feb 2017 [17:11:49], Michael S. Tsirkin wrote: > On Fri, Feb 03, 2017 at 04:48:14PM +0530, Amit Shah wrote: > > I'm leaving my job at Red Hat, this email address will stop working next > > week. > > Update it to one that I will have access to later. > > > > Signed-off-by: Amit Shah > > It's great that we'll still have you around! Do you want to send a pull > request with this patch? Or want me to merge it? Yes, I expect to be around :) Please merge it via your tree, thanks. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] MAINTAINERS: update my email address
I'm leaving my job at Red Hat, this email address will stop working next week. Update it to one that I will have access to later. Signed-off-by: Amit Shah --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3960e7f..187b961 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13065,7 +13065,7 @@ F: drivers/input/serio/userio.c F: include/uapi/linux/userio.h VIRTIO CONSOLE DRIVER -M: Amit Shah +M: Amit Shah L: virtualization@lists.linux-foundation.org S: Maintained F: drivers/char/virtio_console.c -- 1.8.3.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-console: avoid DMA from stack
On (Wed) 01 Feb 2017 [00:02:27], Omar Sandoval wrote: > From: Omar Sandoval > > put_chars() stuffs the buffer it gets into an sg, but that buffer may be > on the stack. This breaks with CONFIG_VMAP_STACK=y (for me, it > manifested as printks getting turned into NUL bytes). Seems reasonable. I wonder since all implementations of hvc do a memcpy, if we can abstract it - but that'll need some work. Reviewed-by: Amit Shah Michael, please add to the virtio queue. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio_console: fix a crash in config_work_handler
On (Mon) 16 Jan 2017 [10:45:02], G. Campana wrote: > Using control_work instead of config_work as the 3rd argument to > container_of results in an invalid portdev pointer. Indeed, the work > structure is initialized as below: > > INIT_WORK(&portdev->config_work, &config_work_handler); > > It leads to a crash when portdev->vdev is dereferenced later. This bug > is triggered when the guest uses a virtio-console without multiport > feature and receives a config_changed virtio interrupt. > > Signed-off-by: G. Campana Reviewed-by: Amit Shah Michael, can you please pick this up? Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: console: Unlock vqs while freeing buffers
On (Tue) 11 Oct 2016 [12:05:15], Matt Redfearn wrote:
> Commit c6017e793b93 ("virtio: console: add locks around buffer removal
> in port unplug path") added locking around the freeing of buffers in the
> vq. However, when free_buf() is called with can_sleep = true and rproc
> is enabled, it calls dma_free_coherent() directly, requiring interrupts
> to be enabled. Currently a WARNING is triggered due to the spin locking
> around free_buf, with a call stack like this:
>
> WARNING: CPU: 3 PID: 121 at ./include/linux/dma-mapping.h:433
> free_buf+0x1a8/0x288
> Call Trace:
> [<8040c538>] show_stack+0x74/0xc0
> [<80757240>] dump_stack+0xd0/0x110
> [<80430d98>] __warn+0xfc/0x130
> [<80430ee0>] warn_slowpath_null+0x2c/0x3c
> [<807e7c6c>] free_buf+0x1a8/0x288
> [<807ea590>] remove_port_data+0x50/0xac
> [<807ea6a0>] unplug_port+0xb4/0x1bc
> [<807ea858>] virtcons_remove+0xb0/0xfc
> [<807b6734>] virtio_dev_remove+0x58/0xc0
> [<807f918c>] __device_release_driver+0xac/0x134
> [<807f924c>] device_release_driver+0x38/0x50
> [<807f7edc>] bus_remove_device+0xfc/0x130
> [<807f4b74>] device_del+0x17c/0x21c
> [<807f4c38>] device_unregister+0x24/0x38
> [<807b6b50>] unregister_virtio_device+0x28/0x44
>
> Fix this by restructuring the loops to allow the locks to only be taken
> where it is necessary to protect the vqs, and release it while the
> buffer is being freed.
>
> Fixes: c6017e793b93 ("virtio: console: add locks around buffer removal in
> port unplug path")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Matt Redfearn
Reviewed-by: Amit Shah
Michael, can you pick this up?
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 02/11] virtio_console: Less function calls in init_vqs() after error detection
Hi, On (Wed) 14 Sep 2016 [16:01:28], SF Markus Elfring wrote: > From: Markus Elfring > Date: Wed, 14 Sep 2016 14:00:35 +0200 > > The kfree() function was called in up to five cases > by the init_vqs() function during error handling even if > the passed variable contained a null pointer. > > * Return directly after a call of the function "kmalloc_array" failed > at the beginning. > > * Split a condition check for memory allocation failures so that > each pointer from these function calls will be checked immediately. > > See also background information: > Topic "CWE-754: Improper check for unusual or exceptional conditions" > Link: https://cwe.mitre.org/data/definitions/754.html > > * Adjust jump targets according to the Linux coding style convention. So I've seen this series and I'm not yet sure how I feel about the patches - f.e. in this one, it adds more lines than it removes to achieve the same effect. I think the code is currently more readable than after these changes. And even if kfree is called multiple times, it isn't a huge bother -- it's error case anyway, very unlikely to trigger, but keeps everything very readble. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: Stop doing DMA on the stack
On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote: > virtio_console uses a small DMA buffer for control requests. Move > that buffer into heap memory. > > Doing virtio DMA on the stack is normally okay on non-DMA-API virtio > systems (which is currently most of them), but it breaks completely > if the stack is virtually mapped. > > Tested by typing both directions using picocom aimed at /dev/hvc0. > > Signed-off-by: Andy Lutomirski Looks fine, Reviewed-by: Amit Shah > --- > > Hi all- > > This is currently broken in tip:x86/asm. If you (Amit) like this patch, > would it make sense for Ingo to add it to -tip? Yes, I'm fine with that. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Ballooning on TPS!=HPS hosts
CC'ing virtualization list.
On (Thu) 31 Mar 2016 [19:00:24], Dr. David Alan Gilbert wrote:
> Hi,
> I was reading the balloon code and am confused as to how/if ballooning
> works on hosts where the host page size is larger than the
> target page size.
>
> static void balloon_page(void *addr, int deflate)
> {
> #if defined(__linux__)
> if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
> kvm_has_sync_mmu())) {
> qemu_madvise(addr, TARGET_PAGE_SIZE,
> deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
> }
> #endif
> }
>
> The virtio-balloon code only does stuff through ballon_page,
> and an madvise DONTNEED should fail if you try and do it on
> a size smaller than the host page size. So does ballooning work on
> Power/ARM?
>
> Am I misunderstanding this?
I think you're right. Guess no one's tested this in such scenarios
yet.
> Of course looking at the above we won't actually generate an error since
> we don't check the return of qemu_madvise.
... at least we can deflate the balloon in case the madvise fails, so
the guest can use the pages it's given us.
> We have three sizes:
> a) host page size
> b) target page size
> c) VIRTIO_BALLOON_PFN_SHIFT
>
> c == 12 (4k) for everyone
>
>
> 1) I think the virtio-balloon code needs to coallesce adjecent requests
> and call balloon_page on whole chunks at once passing a length.
> 2) why does balloon_page use TARGET_PAGE_SIZE, ignoring anything else
>shouldn't it be 1 << VIRTIO_BALLOON_PFN_SHIFT ?
> 3) I'm guessing the guest kernel doesn't know the host page size, so
>how can it know what size chunks of balloon to work in?
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC qemu 0/4] A PV solution for live migration optimization
On (Thu) 10 Mar 2016 [07:44:19], Li, Liang Z wrote: > > Hi Amit, > > Could provide more information on how to use virtio-serial to exchange data? > Thread , Wiki or code are all OK. > I have not find some useful information yet. See this commit in the Linux sources: 108fc82596e3b66b819df9d28c1ebbc9ab5de14c that adds a way to send guest trace data over to the host. I think that's the most relevant to your use-case. However, you'll have to add an in-kernel user of virtio-serial (like the virtio-console code -- the code that deals with tty and hvc currently). There's no other non-tty user right now, and this is the right kind of use-case to add one for! For many other (userspace) use-cases, see the qemu-guest-agent in the qemu sources. The API is documented in the wiki: http://www.linux-kvm.org/page/Virtio-serial_API and the feature pages have some information that may help as well: https://fedoraproject.org/wiki/Features/VirtioSerial There are some links in here too: http://log.amitshah.net/2010/09/communication-between-guests-and-hosts/ Hope this helps. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Qemu-devel] [RFC kernel 0/2]A PV solution for KVM live migration optimization
On (Thu) 10 Mar 2016 [12:31:32], Jitendra Kolhe wrote: > On 3/8/2016 4:44 PM, Amit Shah wrote: > >>>> Hi, > >>>> An interesting solution; I know a few different people have been > >>>> looking at > >>>> how to speed up ballooned VM migration. > >>>> > >>> > >>> Ooh, different solutions for the same purpose, and both based on the > >>> balloon. > >> > >> We were also tying to address similar problem, without actually needing to > >> modify > >> the guest driver. Please find patch details under mail with subject. > >> migration: skip sending ram pages released by virtio-balloon driver > > > > The scope of this patch series seems to be wider: don't send free > > pages to a dest at all, vs. don't send pages that are ballooned out. > > Hi, > > Thanks for your response. The scope of this patch series doesn’t seem to take > care > of ballooned out pages. To balloon out a guest ram page the guest balloon > driver does > a alloc_page() and then return the guest pfn to Qemu, so ballooned out pages > will not > be seen as free ram pages by the guest. > Thus we will still end up scanning (for zero page) for ballooned out pages > during > migration. It would be ideal if we could have both solutions. Yes, of course it would be nice to have both solutions. My response was to the line: > >>> Ooh, different solutions for the same purpose, and both based on the > >>> balloon. which sounded misleading to me for a couple of reasons: 1, as you describe, pages being considered by this patchset and yours are different; and 2, as I mentioned in the other mail, this patchset doesn't really depend on the balloon, and I believe it should not. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC qemu 0/4] A PV solution for live migration optimization
On (Fri) 04 Mar 2016 [15:02:47], Jitendra Kolhe wrote: > > > > > > * Liang Li (liang.z...@intel.com) wrote: > > > > The current QEMU live migration implementation mark the all the > > > > guest's RAM pages as dirtied in the ram bulk stage, all these pages > > > > will be processed and that takes quit a lot of CPU cycles. > > > > > > > > From guest's point of view, it doesn't care about the content in free > > > > pages. We can make use of this fact and skip processing the free pages > > > > in the ram bulk stage, it can save a lot CPU cycles and reduce the > > > > network traffic significantly while speed up the live migration > > > > process obviously. > > > > > > > > This patch set is the QEMU side implementation. > > > > > > > > The virtio-balloon is extended so that QEMU can get the free pages > > > > information from the guest through virtio. > > > > > > > > After getting the free pages information (a bitmap), QEMU can use it > > > > to filter out the guest's free pages in the ram bulk stage. This make > > > > the live migration process much more efficient. > > > > > > Hi, > > > An interesting solution; I know a few different people have been > > > looking at > > > how to speed up ballooned VM migration. > > > > > > > Ooh, different solutions for the same purpose, and both based on the > > balloon. > > We were also tying to address similar problem, without actually needing to > modify > the guest driver. Please find patch details under mail with subject. > migration: skip sending ram pages released by virtio-balloon driver The scope of this patch series seems to be wider: don't send free pages to a dest at all, vs. don't send pages that are ballooned out. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC qemu 0/4] A PV solution for live migration optimization
On (Thu) 03 Mar 2016 [18:44:24], Liang Li wrote: > The current QEMU live migration implementation mark the all the > guest's RAM pages as dirtied in the ram bulk stage, all these pages > will be processed and that takes quit a lot of CPU cycles. > > From guest's point of view, it doesn't care about the content in free > pages. We can make use of this fact and skip processing the free > pages in the ram bulk stage, it can save a lot CPU cycles and reduce > the network traffic significantly while speed up the live migration > process obviously. > > This patch set is the QEMU side implementation. > > The virtio-balloon is extended so that QEMU can get the free pages > information from the guest through virtio. > > After getting the free pages information (a bitmap), QEMU can use it > to filter out the guest's free pages in the ram bulk stage. This make > the live migration process much more efficient. > > This RFC version doesn't take the post-copy and RDMA into > consideration, maybe both of them can benefit from this PV solution > by with some extra modifications. I like the idea, just have to prove (review) and test it a lot to ensure we don't end up skipping pages that matter. However, there are a couple of points: In my opinion, the information that's exchanged between the guest and the host should be exchanged over a virtio-serial channel rather than virtio-balloon. First, there's nothing related to the balloon here. It just happens to be memory info. Second, I would never enable balloon in a guest that I want to be performance-sensitive. So even if you add this as part of balloon, you'll find no one is using this solution. Secondly, I suggest virtio-serial, because it's meant exactly to exchange free-flowing information between a host and a guest, and you don't need to extend any part of the protocol for it (hence no changes necessary to the spec). You can see how spice, vnc, etc., use virtio-serial to exchange data. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch v2] virtio_console: silence a static checker warning
On (Fri) 08 May 2015 [11:30:09], walter harms wrote: > > > Am 08.05.2015 11:16, schrieb Dan Carpenter: > > My static checker complains that this sprintf() can overflow but really > > it can't. Just silence the warning by using snprintf(). > > > > Signed-off-by: Dan Carpenter > > --- > > v2: the overflow is not possible so just leave the buffer size alone and > > silence the warning with snprintf(). > > > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > index 50754d20..8283989 100644 > > --- a/drivers/char/virtio_console.c > > +++ b/drivers/char/virtio_console.c > > @@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 > > id) > > * Finally, create the debugfs file that we can use to > > * inspect a port's state at any time > > */ > > - sprintf(debugfs_name, "vport%up%u", > > - port->portdev->vdev->index, id); > > + snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u", > > +port->portdev->vdev->index, id); > > > would it help to use %03u (or so) to make it more obvious ? > > Note: i prefer a leading 0 in my programms to make it more easy > to work with grep and friends. you may thing otherwise. Well we've been exposing names like /dev/vport0p0, /dev/vport2p15, etc., and there might be scripts relying on such names, so that's one argument against it. However we do have pretty names that map to these ports via udev rules, but not sure if we should change the name just to prepend 0s. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch v2] virtio_console: silence a static checker warning
On (Fri) 08 May 2015 [12:16:25], Dan Carpenter wrote: > My static checker complains that this sprintf() can overflow but really > it can't. Just silence the warning by using snprintf(). Reviewed-by: Amit Shah Thanks! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch] virtio_console: use snprintf() for safety
On (Fri) 08 May 2015 [09:19:02], Dan Carpenter wrote: > My static checker complains that this sprintf() can overflow. > > vdev->index is selected by ida_simple_get() in register_virtio_device() > so my reading of the code is that this overflow is theoretically > possible. The max value of "id" is configurable and I'm not sure what > typical values are. vdev->index is per-device, and starts with 0 for the first attached virtio-serial-pci device. So to overflow, a lot of devices have to be attached, which isn't possible with current qemu. 16 bytes was already overkill.. > Anyway, it's simple enough to make the buffer larger and I changed it to > snprintf() as well. Any reason to choose 28? I think 16 is enough. The snprintf change is fine, though. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 12/25] virtio_console: Use bool function return values of true/false not 1/0
On (Mon) 30 Mar 2015 [16:46:10], Joe Perches wrote: > Use the normal return values for bool functions > > Signed-off-by: Joe Perches > --- > drivers/char/virtio_console.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 72d7028..50754d20 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device > *portdev) >* early_init >*/ > if (!portdev->vdev) > - return 0; > + return false; Reviewed-by: Amit Shah Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: avoid config access from irq
On (Sat) 28 Feb 2015 [18:42:25], Michael S. Tsirkin wrote: > when multiport is off, virtio console invokes config access from irq > context, config access is blocking on s390. > Fix this up by scheduling work from config irq - similar to what we do > for multiport configs. > > Signed-off-by: Michael S. Tsirkin Reviewed-by: Amit Shah Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: init work unconditionally
On (Sat) 28 Feb 2015 [18:41:34], Michael S. Tsirkin wrote: > when multiport is off, we don't initialize config work, > but we then cancel uninitialized control_work on freeze. > > Signed-off-by: Michael S. Tsirkin Reviewed-by: Amit Shah Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 04/16] virtio/console: verify device has config space
On (Tue) 20 Jan 2015 [13:09:55], Michael S. Tsirkin wrote: > On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah wrote: > > On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote: > > > Some devices might not implement config space access > > > (e.g. remoteproc used not to - before 3.9). > > > virtio/console needs config space access so make it > > > fail gracefully if not there. > > > > Do we know any such devices? Wondering what prompted this patch. If > > it's just theoretical, I'd rather let it be like this, and pull this > > in when there's a device that doesn't have config space. > > Yes, with virtio 1.0 config space can be in a separate BAR now. If > that's not enabled by BIOS (e.g. out of space), we won't have config > space. I'm still not sure whether we should pull in this patch before actually seeing a failure. You do have a dev_err which tells why the probe failed, so it's an acceptable compromise I suppose. > > Also, just the console functionality (i.e. F_MULTIPORT is unset) is > > available w/o config space access. > > Supporting this by gracefully disabling F_MULTIPORT > would require getting this info from driver before > features are finalized. > Alternatively, check F_MULTIPORT and only fail if set? > Let me know, I'll cook up a patch. Yes, failing only if F_MULTIPORT is set is a better option (if we have to fail). > > In fact, getting this patch in > > would mean remoteproc wouldn't even run in its pre-config days... > > It seems to have get callback unconditionally now - or did I miss > something? What I meant was remoteproc doesn't depend on the config space, only uses the console functionality. If remoteproc devices didn't expose a config space, this patch would cause it to lose its console functionality for no apparent reason. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 04/16] virtio/console: verify device has config space
On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
> Some devices might not implement config space access
> (e.g. remoteproc used not to - before 3.9).
> virtio/console needs config space access so make it
> fail gracefully if not there.
Do we know any such devices? Wondering what prompted this patch. If
it's just theoretical, I'd rather let it be like this, and pull this
in when there's a device that doesn't have config space.
Also, just the console functionality (i.e. F_MULTIPORT is unset) is
available w/o config space access. In fact, getting this patch in
would mean remoteproc wouldn't even run in its pre-config days...
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index de03df9..26afb56 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device *vdev)
> bool multiport;
> bool early = early_put_chars != NULL;
>
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config access disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 01/15] virtio_pci: fix virtio spec compliance on restore
On (Tue) 07 Oct 2014 [15:53:55], Michael S. Tsirkin wrote: > On Mon, Oct 06, 2014 at 06:10:40PM +0300, Michael S. Tsirkin wrote: > > On restore, virtio pci does the following: > > + set features > > + init vqs etc - device can be used at this point! > > + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits > > > > This is in violation of the virtio spec, which > > requires the following order: > > - ACKNOWLEDGE > > - DRIVER > > - init vqs > > - DRIVER_OK > > > > This behaviour will break with hypervisors that assume spec compliant > > behaviour. It seems like a good idea to have this patch applied to > > stable branches to reduce the support butden for the hypervisors. > > Tested suspend to ram with virtio net and blk. I'd recommend running a continuous ping for virtio-net and a dd process running across s3/s4 for testing. That's what I had run while doing the original set of patches. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 01/15] virtio_pci: fix virtio spec compliance on restore
On (Mon) 06 Oct 2014 [18:10:40], Michael S. Tsirkin wrote: > On restore, virtio pci does the following: > + set features > + init vqs etc - device can be used at this point! > + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits > > This is in violation of the virtio spec, which > requires the following order: > - ACKNOWLEDGE > - DRIVER > - init vqs > - DRIVER_OK > > This behaviour will break with hypervisors that assume spec compliant > behaviour. It seems like a good idea to have this patch applied to > stable branches to reduce the support butden for the hypervisors. > > Cc: sta...@vger.kernel.org > Cc: Amit Shah > Signed-off-by: Michael S. Tsirkin I didn't see my previous questions answered from the initial posting -- can you please respond to them? Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] virtio_pci: fix virtio spec compliance on restore
On (Tue) 23 Sep 2014 [13:32:22], Michael S. Tsirkin wrote:
> On restore, virtio pci does the following:
> + set features
> + init vqs etc - device can be used at this point!
> + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
>
> This is in violation of the virtio spec, which
> requires the following order:
> - ACKNOWLEDGE
> - DRIVER
> - init vqs
> - DRIVER_OK
>
> Cc: sta...@vger.kernel.org
> Cc: Amit Shah
> Signed-off-by: Michael S. Tsirkin
> ---
>
> Lightly tested.
> Will repost as non-RFC once testing is done, sending
> out now for early flames/comments.
What tests are you running?
> Thanks!
>
> drivers/virtio/virtio_pci.c | 36
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 58f7e45..58cbf6e 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -785,6 +785,7 @@ static int virtio_pci_restore(struct device *dev)
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> struct virtio_driver *drv;
> + unsigned status = 0;
> int ret;
>
> drv = container_of(vp_dev->vdev.dev.driver,
> @@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
> return ret;
>
> pci_set_master(pci_dev);
> + /* We always start by resetting the device, in case a previous
> + * driver messed it up. */
> + vp_reset(&vp_dev->vdev);
> +
> + /* Acknowledge that we've seen the device. */
> + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
> + vp_set_status(&vp_dev->vdev, status);
> +
> + /* Maybe driver failed before freeze.
> + * Restore the failed status, for debugging. */
> + status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(&vp_dev->vdev, status);
> +
> + if (!drv)
> + return 0;
> +
> + /* We have a driver! */
> + status |= VIRTIO_CONFIG_S_DRIVER;
> + vp_set_status(&vp_dev->vdev, status);
> +
> vp_finalize_features(&vp_dev->vdev);
>
> - if (drv && drv->restore)
> - ret = drv->restore(&vp_dev->vdev);
> + if (!drv->restore)
> + return 0;
So in this case DRIVER_OK will never be set?
> +
> + ret = drv->restore(&vp_dev->vdev);
> + if (ret) {
> + status |= VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(&vp_dev->vdev, status);
> + return ret;
> + }
>
> /* Finally, tell the device we're all set */
> - if (!ret)
> - vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
> + status |= VIRTIO_CONFIG_S_DRIVER_OK;
> + vp_set_status(&vp_dev->vdev, status);
>
> return ret;
> }
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-rng: fix stuck in catting hwrng attributes
On (Wed) 10 Sep 2014 [17:07:07], Amos Kong wrote:
> When I check hwrng attributes in sysfs, cat process always gets
> stuck if guest has only 1 vcpu and uses a slow rng backend.
>
> Currently we check if there is any tasks waiting to be run on
> current cpu in rng_dev_read() by need_resched(). But need_resched()
> doesn't work because rng_dev_read() is executing in user context.
>
> This patch removed need_resched() and increase delay to 10 jiffies,
> then other tasks can have chance to execute protected code.
> Delaying 1 jiffy also works, but 10 jiffies is safer.
I'd prefer two patches for this one: one to remove the need_resched()
check, and the other to increase the timeout.
Anyway,
Reviewed-by: Amit Shah
>
> Signed-off-by: Amos Kong
> ---
> drivers/char/hw_random/core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index c591d7e..b5d1b6f 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char
> __user *buf,
>
> mutex_unlock(&rng_mutex);
>
> - if (need_resched())
> - schedule_timeout_interruptible(1);
> + schedule_timeout_interruptible(10);
>
> if (signal_pending(current)) {
> err = -ERESTARTSYS;
> --
> 1.9.3
>
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-rng cleanup: move some code out of mutex protection
On (Wed) 10 Sep 2014 [17:07:06], Amos Kong wrote:
> It doesn't save too much cpu time as expected, just a cleanup.
Frankly I won't bother with this. It doesn't completely remove all
copying from the mutex, so it's not worthwhile.
> Signed-off-by: Amos Kong
> ---
> drivers/char/hw_random/core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index aa30a25..c591d7e 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -270,8 +270,8 @@ static ssize_t hwrng_attr_current_show(struct device *dev,
> return -ERESTARTSYS;
> if (current_rng)
> name = current_rng->name;
> - ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
> mutex_unlock(&rng_mutex);
> + ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
>
> return ret;
> }
> @@ -284,19 +284,19 @@ static ssize_t hwrng_attr_available_show(struct device
> *dev,
> ssize_t ret = 0;
> struct hwrng *rng;
>
> + buf[0] = '\0';
> err = mutex_lock_interruptible(&rng_mutex);
> if (err)
> return -ERESTARTSYS;
> - buf[0] = '\0';
> list_for_each_entry(rng, &rng_list, list) {
> strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> ret += strlen(rng->name);
> strncat(buf, " ", PAGE_SIZE - ret - 1);
> ret++;
> }
> + mutex_unlock(&rng_mutex);
> strncat(buf, "\n", PAGE_SIZE - ret - 1);
> ret++;
> - mutex_unlock(&rng_mutex);
>
> return ret;
> }
> --
> 1.9.3
>
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/2] virtio-rng: skip reading when we start to remove the device
On (Wed) 10 Sep 2014 [14:11:37], Amos Kong wrote: > Before we really unregister the hwrng device, reading will get stuck if > the virtio device is reset. We should return error for reading when we > start to remove the device. > > Signed-off-by: Amos Kong > Cc: sta...@vger.kernel.org Reviewed-by: Amit Shah Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 1/2] virtio-rng: fix stuck of hot-unplugging busy device
On (Wed) 10 Sep 2014 [14:11:36], Amos Kong wrote: > When we try to hot-remove a busy virtio-rng device from QEMU monitor, > the device can't be hot-removed. Because virtio-rng driver hangs at > wait_for_completion_killable(). > > This patch exits the waiting by completing have_data completion before > unregistering, resets data_avail to avoid the hwrng core use wrong > buffer bytes. > > Signed-off-by: Amos Kong > Cc: sta...@vger.kernel.org Reviewed-by: Amit Shah Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: RFC virtio-rng: fail to read sysfs of a busy device
On (Tue) 09 Sep 2014 [23:23:07], Amos Kong wrote:
> (Resend to fix the subject)
>
> Hi Amit, Rusty
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1127062
> steps:
> - Read random data by 'dd if=/dev/hwrng of=/dev/null' in guest
> - check sysfs files in the same time, 'cat /sys/class/misc/hw_random/rng_*'
>
> Result: cat process will get stuck, it will return if we kill dd process.
How common is it going to be to have a long-running 'dd' process on
/dev/hwrng?
Also, with the new khwrng thread, reading from /dev/hwrng isn't
required -- just use /dev/random?
(This doesn't mean we shouldn't fix the issue here...)
> We have some static variables (eg, current_rng, data_avail, etc) in
> hw_random/core.c,
> they are protected by rng_mutex. I try to workaround this issue by
> undelay(100)
> after mutex_unlock() in rng_dev_read(). This gives chance for
> hwrng_attr_*_show()
> to get mutex.
>
> This patch also contains some cleanup, moving some code out of mutex
> protection.
>
> Do you have some suggestion? Thanks.
>
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index aa30a25..fa69020 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -194,6 +194,7 @@ static ssize_t rng_dev_read(struct file *filp, char
> __user *buf,
> }
>
> mutex_unlock(&rng_mutex);
> + udelay(100);
We have a need_resched() right below. Why doesn't that work?
> if (need_resched())
> schedule_timeout_interruptible(1);
> @@ -233,10 +234,10 @@ static ssize_t hwrng_attr_current_store(struct device
> *dev,
> int err;
> struct hwrng *rng;
The following hunk doesn't work:
> + err = -ENODEV;
> err = mutex_lock_interruptible(&rng_mutex);
err is being set to another value in the next line!
> if (err)
> return -ERESTARTSYS;
> - err = -ENODEV;
And all usage of err below now won't have -ENODEV but some other value.
> list_for_each_entry(rng, &rng_list, list) {
> if (strcmp(rng->name, buf) == 0) {
> if (rng == current_rng) {
> @@ -270,8 +271,8 @@ static ssize_t hwrng_attr_current_show(struct device *dev,
> return -ERESTARTSYS;
> if (current_rng)
> name = current_rng->name;
> - ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
> mutex_unlock(&rng_mutex);
> + ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
This looks OK...
>
> return ret;
> }
> @@ -284,19 +285,19 @@ static ssize_t hwrng_attr_available_show(struct device
> *dev,
> ssize_t ret = 0;
> struct hwrng *rng;
>
> + buf[0] = '\0';
> err = mutex_lock_interruptible(&rng_mutex);
> if (err)
> return -ERESTARTSYS;
>
> - buf[0] = '\0';
> list_for_each_entry(rng, &rng_list, list) {
> strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> ret += strlen(rng->name);
> strncat(buf, " ", PAGE_SIZE - ret - 1);
> ret++;
> }
> + mutex_unlock(&rng_mutex);
> strncat(buf, "\n", PAGE_SIZE - ret - 1);
> ret++;
> - mutex_unlock(&rng_mutex);
But this isn't resulting in savings; the majority of the time is being
spent in the for loop, and that writes to the buffer.
BTW I don't expect strcat'ing to the buf in each of these scenarios is
a long operation, so this reworking doesn't strike to me as something
we should pursue.
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio-rng: fix stuck of hot-unplugging busy device
Hi Amos, On (Tue) 09 Sep 2014 [19:14:02], Amos Kong wrote: > When we try to hot-remove a busy virtio-rng device from QEMU monitor, > the device can't be hot-removed. Because virtio-rng driver hangs at > wait_for_completion_killable(). > > This patch exits the waiting by completing have_data completion before > unregistering, resets data_avail to avoid the hwrng core use wrong > buffer bytes. Before real unregistering we should return -ENODEV for > reading. > > When we hot-unplug the device, dd process in guest will exit. > $ dd if=/dev/hwrng of=/dev/null > dd: error reading ‘/dev/hwrng’: No such device > > Signed-off-by: Amos Kong > Cc: sta...@vger.kernel.org > --- > V2: reset data_avail (Amit) > adjust unregister order Thanks, this looks good. Can you please split into two parts, the complete() in one, and the hwrng_register_done stuff into another? Thanks, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [3.16 stable PATCH v2 1/2] virtio: rng: delay hwrng_register() till driver is ready
Hey Greg,
Can you add these two patches to the 3.16 queue?
Thanks,
On (Tue) 12 Aug 2014 [13:23:45], Amit Shah wrote:
> Instead of calling hwrng_register() in the probe routing, call it in the
> scan routine. This ensures that when hwrng_register() is successful,
> and it requests a few random bytes to seed the kernel's pool at init,
> we're ready to service that request.
>
> This will also enable us to remove the workaround added previously to
> check whether probe was completed, and only then ask for data from the
> host. The revert follows in the next commit.
>
> There's a slight behaviour change here on unsuccessful hwrng_register().
> Previously, when hwrng_register() failed, the probe() routine would
> fail, and the vqs would be torn down, and driver would be marked not
> initialized. Now, the vqs will remain initialized, driver would be
> marked initialized as well, but won't be available in the list of RNGs
> available to hwrng core. To fix the failures, the procedure remains the
> same, i.e. unload and re-load the module, and hope things succeed the
> next time around.
>
> Signed-off-by: Amit Shah
> Signed-off-by: Rusty Russell
> (cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
> Signed-off-by: Amit Shah
> ---
> drivers/char/hw_random/virtio-rng.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/hw_random/virtio-rng.c
> b/drivers/char/hw_random/virtio-rng.c
> index e9b15bc..f4e04f3 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -36,6 +36,7 @@ struct virtrng_info {
> bool busy;
> char name[25];
> int index;
> + bool hwrng_register_done;
> };
>
> static bool probe_done;
> @@ -137,15 +138,6 @@ static int probe_common(struct virtio_device *vdev)
> return err;
> }
>
> - err = hwrng_register(&vi->hwrng);
> - if (err) {
> - vdev->config->del_vqs(vdev);
> - vi->vq = NULL;
> - kfree(vi);
> - ida_simple_remove(&rng_index_ida, index);
> - return err;
> - }
> -
> probe_done = true;
> return 0;
> }
> @@ -153,9 +145,11 @@ static int probe_common(struct virtio_device *vdev)
> static void remove_common(struct virtio_device *vdev)
> {
> struct virtrng_info *vi = vdev->priv;
> +
> vdev->config->reset(vdev);
> vi->busy = false;
> - hwrng_unregister(&vi->hwrng);
> + if (vi->hwrng_register_done)
> + hwrng_unregister(&vi->hwrng);
> vdev->config->del_vqs(vdev);
> ida_simple_remove(&rng_index_ida, vi->index);
> kfree(vi);
> @@ -171,6 +165,16 @@ static void virtrng_remove(struct virtio_device *vdev)
> remove_common(vdev);
> }
>
> +static void virtrng_scan(struct virtio_device *vdev)
> +{
> + struct virtrng_info *vi = vdev->priv;
> + int err;
> +
> + err = hwrng_register(&vi->hwrng);
> + if (!err)
> + vi->hwrng_register_done = true;
> +}
> +
> #ifdef CONFIG_PM_SLEEP
> static int virtrng_freeze(struct virtio_device *vdev)
> {
> @@ -195,6 +199,7 @@ static struct virtio_driver virtio_rng_driver = {
> .id_table = id_table,
> .probe =virtrng_probe,
> .remove = virtrng_remove,
> + .scan = virtrng_scan,
> #ifdef CONFIG_PM_SLEEP
> .freeze = virtrng_freeze,
> .restore = virtrng_restore,
> --
> 1.9.3
>
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] Install udev rules in system-default directory
On (Tue) 19 Aug 2014 [19:20:20], Paolo Bonzini wrote: > Il 07/08/2014 15:09, Amit Shah ha scritto: > > +udevrules_DATA = 90-virtio-rng.rules > > If rng-tools is packaged with "make dist" you need this to be > > dist_udevrules_DATA= 90-virtio-rng.rules > > or alternatively > > EXTRA_DIST = 90-virtio-rng.rules > udevrules_DATA = 90-virtio-rng.rules > > > +AC_ARG_WITH([udevrulesdir], > > + AS_HELP_STRING([--with-udevrulesdir=DIR], [Directory for udev rules]), > > + [], > > + [with_udevrulesdir=$($PKG_CONFIG --variable=udevdir udev)"/rules.d"]) > > +AC_SUBST([udevrulesdir], [$with_udevrulesdir]) > > You can use AM_CONDITIONAL to skip the installation if > $with_udevrulesdir is "no" (aka --without-udevrulesdir). Then you'd have > > EXTRA_DIST = 90-virtio-rng.rules > if INSTALL_UDEV_RULES > udevrules_DATA = 90-virtio-rng.rules > endif > > I don't know offhand if this works: > > if INSTALL_UDEV_RULES > dist_udevrules_DATA= 90-virtio-rng.rules > endif > > but I think so; you can check with "./configure --without-udevrulesdir > && make && make dist". Thanks! Will give this a shot for curiosity's sake. The khwrngd-based patch has already been merged upstream, so this is now obsolete. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 1/1] virtio: rng: add derating factor for use by hwrng core
The khwrngd thread is started when a hwrng device of sufficient quality is registered. The virtio-rng device is backed by the hypervisor, and we trust the hypervisor to provide real entropy. A malicious or badly-implemented hypervisor is a scenario that's irrelevant -- such a setup is bound to cause all sorts of badness, and a compromised hwrng is the least of the user's worries. Given this, we might as well assume that the quality of randomness we receive is perfectly trustworthy. Hence, we use 100% for the factor, indicating maximum confidence in the source. Signed-off-by: Amit Shah Reviewed-by: H. Peter Anvin Reviewed-by: Amos Kong Signed-off-by: Rusty Russell --- Pretty small and contained patch; would be great if it is picked up for 3.17. v2: re-word commit msg (hpa) v3: re-word commit msg (tytso) --- drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index 0027137..2e3139e 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev) .cleanup = virtio_cleanup, .priv = (unsigned long)vi, .name = vi->name, + .quality = 1000, }; vdev->priv = vi; -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PULL] virtio-rng: add derating factor for use by hwrng core
Hi Linus, Sending directly to you with the commit log changes Ted Ts'o pointed out. Not sure if Rusty's back after his travel, but this already has his s-o-b. Please pull. The following changes since commit c9d26423e56ce1ab4d786f92aebecf859d419293: Merge tag 'pm+acpi-3.17-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm (2014-08-14 18:13:46 -0600) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/amit/virtio.git rng-queue for you to fetch changes up to 34679ec7a0c45da8161507e1f2e1f72749dfd85c: virtio: rng: add derating factor for use by hwrng core (2014-08-15 10:26:01 +0530) ---- Amit Shah (1): virtio: rng: add derating factor for use by hwrng core drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+) -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/1] virtio: rng: add derating factor for use by hwrng core
The khwrngd thread is started when a hwrng device of sufficient quality is registered. The virtio-rng device is backed by the hypervisor, and we trust the hypervisor to provide real entropy. A malicious hypervisor is a scenario that's irrelevant -- such a setup is bound to cause all sorts of badness, and a compromised hwrng is not the biggest threat. Given this, we are certain the quality of randomness we receive is perfectly trustworthy. Hence, we use 100% for the factor, indicating maximum confidence in the source. Signed-off-by: Amit Shah --- Pretty small and contained patch; would be great if it is picked up for 3.17. v2: re-word commit msg --- drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index 0027137..2e3139e 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev) .cleanup = virtio_cleanup, .priv = (unsigned long)vi, .name = vi->name, + .quality = 1000, }; vdev->priv = vi; -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[3.16 stable PATCH v2 2/2] Revert "hwrng: virtio - ensure reads happen after successful probe"
This reverts commit e052dbf554610e2104c5a7518c4d8374bed701bb.
Now that we use the virtio ->scan() function to register with the hwrng
core, we will not get read requests till probe is successfully finished.
So revert the workaround we had in place to refuse read requests while
we were not yet setup completely.
Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell
(cherry picked from commit eeec626366ffe558fc3d5685bd2b49a962acf57d)
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 6 --
drivers/char/hw_random/virtio-rng.c | 9 -
2 files changed, 15 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
- /*
-* Currently only virtio-rng cannot return data during device
-* probe, and that's handled in virtio-rng.c itself. If there
-* are more such devices, this call to rng_get_data can be
-* made conditional here instead of doing it per-device.
-*/
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f4e04f3..f1aa13b 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -39,7 +39,6 @@ struct virtrng_info {
bool hwrng_register_done;
};
-static bool probe_done;
static void random_recv_done(struct virtqueue *vq)
{
@@ -70,13 +69,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
- /*
-* Don't ask host for data till we're setup. This call can
-* happen during hwrng_register(), after commit d9e7972619.
-*/
- if (unlikely(!probe_done))
- return 0;
-
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -138,7 +130,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [3.16 stable PATCH 1/2] virtio: rng: delay hwrng_register() till driver is ready
On (Tue) 12 Aug 2014 [15:41:35], Greg KH wrote: > On Tue, Aug 12, 2014 at 12:36:54PM +0530, Amit Shah wrote: > > Instead of calling hwrng_register() in the probe routing, call it in the > > scan routine. This ensures that when hwrng_register() is successful, > > and it requests a few random bytes to seed the kernel's pool at init, > > we're ready to service that request. > > > > This will also enable us to remove the workaround added previously to > > check whether probe was completed, and only then ask for data from the > > host. The revert follows in the next commit. > > > > There's a slight behaviour change here on unsuccessful hwrng_register(). > > Previously, when hwrng_register() failed, the probe() routine would > > fail, and the vqs would be torn down, and driver would be marked not > > initialized. Now, the vqs will remain initialized, driver would be > > marked initialized as well, but won't be available in the list of RNGs > > available to hwrng core. To fix the failures, the procedure remains the > > same, i.e. unload and re-load the module, and hope things succeed the > > next time around. > > > > Signed-off-by: Amit Shah > > Signed-off-by: Rusty Russell > > (cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893) > > Signed-off-by: Amit Shah > > > > Conflicts: > > drivers/char/hw_random/virtio-rng.c > > > > Context conflict due to not backporting two commits that changed > > struct layout. > > What is this "Conflicts:" stuff for? I don't need that... To note the patch didn't cherry-pick as-is; anyway, sent a v2 without that text. Thanks, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[3.16 stable PATCH v2 1/2] virtio: rng: delay hwrng_register() till driver is ready
Instead of calling hwrng_register() in the probe routing, call it in the
scan routine. This ensures that when hwrng_register() is successful,
and it requests a few random bytes to seed the kernel's pool at init,
we're ready to service that request.
This will also enable us to remove the workaround added previously to
check whether probe was completed, and only then ask for data from the
host. The revert follows in the next commit.
There's a slight behaviour change here on unsuccessful hwrng_register().
Previously, when hwrng_register() failed, the probe() routine would
fail, and the vqs would be torn down, and driver would be marked not
initialized. Now, the vqs will remain initialized, driver would be
marked initialized as well, but won't be available in the list of RNGs
available to hwrng core. To fix the failures, the procedure remains the
same, i.e. unload and re-load the module, and hope things succeed the
next time around.
Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell
(cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..f4e04f3 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -36,6 +36,7 @@ struct virtrng_info {
bool busy;
char name[25];
int index;
+ bool hwrng_register_done;
};
static bool probe_done;
@@ -137,15 +138,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- err = hwrng_register(&vi->hwrng);
- if (err) {
- vdev->config->del_vqs(vdev);
- vi->vq = NULL;
- kfree(vi);
- ida_simple_remove(&rng_index_ida, index);
- return err;
- }
-
probe_done = true;
return 0;
}
@@ -153,9 +145,11 @@ static int probe_common(struct virtio_device *vdev)
static void remove_common(struct virtio_device *vdev)
{
struct virtrng_info *vi = vdev->priv;
+
vdev->config->reset(vdev);
vi->busy = false;
- hwrng_unregister(&vi->hwrng);
+ if (vi->hwrng_register_done)
+ hwrng_unregister(&vi->hwrng);
vdev->config->del_vqs(vdev);
ida_simple_remove(&rng_index_ida, vi->index);
kfree(vi);
@@ -171,6 +165,16 @@ static void virtrng_remove(struct virtio_device *vdev)
remove_common(vdev);
}
+static void virtrng_scan(struct virtio_device *vdev)
+{
+ struct virtrng_info *vi = vdev->priv;
+ int err;
+
+ err = hwrng_register(&vi->hwrng);
+ if (!err)
+ vi->hwrng_register_done = true;
+}
+
#ifdef CONFIG_PM_SLEEP
static int virtrng_freeze(struct virtio_device *vdev)
{
@@ -195,6 +199,7 @@ static struct virtio_driver virtio_rng_driver = {
.id_table = id_table,
.probe =virtrng_probe,
.remove = virtrng_remove,
+ .scan = virtrng_scan,
#ifdef CONFIG_PM_SLEEP
.freeze = virtrng_freeze,
.restore = virtrng_restore,
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[3.16 stable PATCH 1/2] virtio: rng: delay hwrng_register() till driver is ready
Instead of calling hwrng_register() in the probe routing, call it in the
scan routine. This ensures that when hwrng_register() is successful,
and it requests a few random bytes to seed the kernel's pool at init,
we're ready to service that request.
This will also enable us to remove the workaround added previously to
check whether probe was completed, and only then ask for data from the
host. The revert follows in the next commit.
There's a slight behaviour change here on unsuccessful hwrng_register().
Previously, when hwrng_register() failed, the probe() routine would
fail, and the vqs would be torn down, and driver would be marked not
initialized. Now, the vqs will remain initialized, driver would be
marked initialized as well, but won't be available in the list of RNGs
available to hwrng core. To fix the failures, the procedure remains the
same, i.e. unload and re-load the module, and hope things succeed the
next time around.
Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell
(cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
Signed-off-by: Amit Shah
Conflicts:
drivers/char/hw_random/virtio-rng.c
Context conflict due to not backporting two commits that changed
struct layout.
---
drivers/char/hw_random/virtio-rng.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..f4e04f3 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -36,6 +36,7 @@ struct virtrng_info {
bool busy;
char name[25];
int index;
+ bool hwrng_register_done;
};
static bool probe_done;
@@ -137,15 +138,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- err = hwrng_register(&vi->hwrng);
- if (err) {
- vdev->config->del_vqs(vdev);
- vi->vq = NULL;
- kfree(vi);
- ida_simple_remove(&rng_index_ida, index);
- return err;
- }
-
probe_done = true;
return 0;
}
@@ -153,9 +145,11 @@ static int probe_common(struct virtio_device *vdev)
static void remove_common(struct virtio_device *vdev)
{
struct virtrng_info *vi = vdev->priv;
+
vdev->config->reset(vdev);
vi->busy = false;
- hwrng_unregister(&vi->hwrng);
+ if (vi->hwrng_register_done)
+ hwrng_unregister(&vi->hwrng);
vdev->config->del_vqs(vdev);
ida_simple_remove(&rng_index_ida, vi->index);
kfree(vi);
@@ -171,6 +165,16 @@ static void virtrng_remove(struct virtio_device *vdev)
remove_common(vdev);
}
+static void virtrng_scan(struct virtio_device *vdev)
+{
+ struct virtrng_info *vi = vdev->priv;
+ int err;
+
+ err = hwrng_register(&vi->hwrng);
+ if (!err)
+ vi->hwrng_register_done = true;
+}
+
#ifdef CONFIG_PM_SLEEP
static int virtrng_freeze(struct virtio_device *vdev)
{
@@ -195,6 +199,7 @@ static struct virtio_driver virtio_rng_driver = {
.id_table = id_table,
.probe =virtrng_probe,
.remove = virtrng_remove,
+ .scan = virtrng_scan,
#ifdef CONFIG_PM_SLEEP
.freeze = virtrng_freeze,
.restore = virtrng_restore,
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[3.16 stable PATCH 2/2] Revert "hwrng: virtio - ensure reads happen after successful probe"
This reverts commit e052dbf554610e2104c5a7518c4d8374bed701bb.
Now that we use the virtio ->scan() function to register with the hwrng
core, we will not get read requests till probe is successfully finished.
So revert the workaround we had in place to refuse read requests while
we were not yet setup completely.
Signed-off-by: Amit Shah
Signed-off-by: Rusty Russell
(cherry picked from commit eeec626366ffe558fc3d5685bd2b49a962acf57d)
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 6 --
drivers/char/hw_random/virtio-rng.c | 9 -
2 files changed, 15 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
- /*
-* Currently only virtio-rng cannot return data during device
-* probe, and that's handled in virtio-rng.c itself. If there
-* are more such devices, this call to rng_get_data can be
-* made conditional here instead of doing it per-device.
-*/
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f4e04f3..f1aa13b 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -39,7 +39,6 @@ struct virtrng_info {
bool hwrng_register_done;
};
-static bool probe_done;
static void random_recv_done(struct virtqueue *vq)
{
@@ -70,13 +69,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
- /*
-* Don't ask host for data till we're setup. This call can
-* happen during hwrng_register(), after commit d9e7972619.
-*/
- if (unlikely(!probe_done))
- return 0;
-
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -138,7 +130,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [3.16 stable PATCH 1/1] virtio-rng: fix multi-device startup
On (Tue) 12 Aug 2014 [13:55:48], Greg KH wrote: > On Tue, Aug 12, 2014 at 11:01:58AM +0530, Amit Shah wrote: > > On (Tue) 12 Aug 2014 [06:55:27], Greg KH wrote: > > > On Mon, Aug 11, 2014 at 06:11:47PM +0530, Amit Shah wrote: > > > > This is a 3.16-only patch. The linux.git fix is > > > > 5c06273401f2eb7b290cadbae18ee00f8f65e893, which fixes this issue in a > > > > different way. > > > > > > Why "different"? Why can't I take that original patch instead? What is > > > different in this patch, and why? > > > > The commit referenced moves the hwrng_register() call to the ->scan() > > callback instead of it being in probe(). This was done to ensure the > > virtio-rng devices can contribute to the initial system entropy > > introduced in commit d9e7972619334. > > > > That patch is quite small too, but will need a slight conflict > > resolution due to the previous two code-shuffling patches, and also > > the following revert. > > > > However, I decided against the backport of the ->scan() method, since > > it wasn't designed to solve this regression, it happens to solve it, > > and it actually introduces new functionality. I would be happy to > > provide a backport of the relevant patches, if you think that would be > > alright. > > I almost always want "original" patches as it causes less bugs overall, > and less confusion for everyone involved. Taking 2-3 patches is just as > easy as 1 patch, and even easier if I don't have to review it as "hard" > due to it not differing from what is in Linus's tree. Thanks; I'll send a backport of the patches, then. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [3.16 stable PATCH 1/1] virtio-rng: fix multi-device startup
On (Tue) 12 Aug 2014 [06:55:27], Greg KH wrote: > On Mon, Aug 11, 2014 at 06:11:47PM +0530, Amit Shah wrote: > > This is a 3.16-only patch. The linux.git fix is > > 5c06273401f2eb7b290cadbae18ee00f8f65e893, which fixes this issue in a > > different way. > > Why "different"? Why can't I take that original patch instead? What is > different in this patch, and why? The commit referenced moves the hwrng_register() call to the ->scan() callback instead of it being in probe(). This was done to ensure the virtio-rng devices can contribute to the initial system entropy introduced in commit d9e7972619334. That patch is quite small too, but will need a slight conflict resolution due to the previous two code-shuffling patches, and also the following revert. However, I decided against the backport of the ->scan() method, since it wasn't designed to solve this regression, it happens to solve it, and it actually introduces new functionality. I would be happy to provide a backport of the relevant patches, if you think that would be alright. Thanks, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] virtio: rng: add derating factor for use by hwrng core
On (Mon) 11 Aug 2014 [15:11:03], H. Peter Anvin wrote: > On 08/11/2014 11:49 AM, Amit Shah wrote: > > The khwrngd thread is started when a hwrng device of sufficient > > quality is registered. The virtio-rng device is backed by the > > hypervisor, and we trust the hypervisor to provide real entropy. A > > malicious hypervisor is a scenario that's ruled out, so we are certain > > the quality of randomness we receive is perfectly trustworthy. Hence, > > we use 100% for the factor, indicating maximum confidence in the source. > > > > Signed-off-by: Amit Shah > > It isn't "ruled out", it is just irrelevant: if the hypervisor is > malicious, the quality of your random number source is the least of your > problems. Yea; I meant ruled out in that sense. Should the commit msg be more verbose? Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/1] virtio: rng: add derating factor for use by hwrng core
The khwrngd thread is started when a hwrng device of sufficient quality is registered. The virtio-rng device is backed by the hypervisor, and we trust the hypervisor to provide real entropy. A malicious hypervisor is a scenario that's ruled out, so we are certain the quality of randomness we receive is perfectly trustworthy. Hence, we use 100% for the factor, indicating maximum confidence in the source. Signed-off-by: Amit Shah --- Pretty small and contained patch; would be great if it is picked up for 3.17. --- drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index 0027137..2e3139e 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev) .cleanup = virtio_cleanup, .priv = (unsigned long)vi, .name = vi->name, + .quality = 1000, }; vdev->priv = vi; -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[3.16 stable PATCH 1/1] virtio-rng: fix multi-device startup
This is a 3.16-only patch. The linux.git fix is
5c06273401f2eb7b290cadbae18ee00f8f65e893, which fixes this issue in a
different way.
Amos reported using multiple virtio-rng devices cause boot to freeze
even after the recent fixes. We need the probe_done test per device
rather than just once for the driver.
Reported-by: Amos Kong
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..124cac5 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -34,12 +34,11 @@ struct virtrng_info {
unsigned int data_avail;
struct completion have_data;
bool busy;
+ bool probe_done;
char name[25];
int index;
};
-static bool probe_done;
-
static void random_recv_done(struct virtqueue *vq)
{
struct virtrng_info *vi = vq->vdev->priv;
@@ -73,7 +72,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
* Don't ask host for data till we're setup. This call can
* happen during hwrng_register(), after commit d9e7972619.
*/
- if (unlikely(!probe_done))
+ if (unlikely(!vi->probe_done))
return 0;
if (!vi->busy) {
@@ -146,7 +145,7 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- probe_done = true;
+ vi->probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] rngd: add udev rule to source from hwrng if virtio-rng present
On (Fri) 08 Aug 2014 [14:46:27], H. Peter Anvin wrote: > On 08/08/2014 02:07 AM, Amit Shah wrote: > > > > > >> To > >> some degree the above is obsolete when we get khwrngd widely deployed, > >> but that is a new-kernel-only kind of thing. > > > > Right - I'm wondering if any such changes as propsed here are now > > obsolted already by khwrngd? > > > > In this case, yes, khwrngd would be a better solution for current kernels. I think that's OK with me. What's the suggested value for rng->quality, though, for virtio-rng that I can use to ensure the kthread starts? Should I use the 700 (70%) as proposed in the original patchset? I'm not exactly sure how that value will be used as well.. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] rngd: add udev rule to source from hwrng if virtio-rng present
On (Thu) 07 Aug 2014 [12:31:11], H. Peter Anvin wrote:
> On 08/07/2014 06:08 AM, Amit Shah wrote:
> > On KVM guests where the virtio-rng device is available, and set as the
> > current rng, this udev rule will start rngd which will feed in the
> > host-provided entropy to /dev/random.
> >
> > Signed-off-by: Amit Shah
> > ---
> > 90-virtio-rng.rules | 1 +
> > 1 file changed, 1 insertion(+)
> > create mode 100644 90-virtio-rng.rules
> >
> > diff --git a/90-virtio-rng.rules b/90-virtio-rng.rules
> > new file mode 100644
> > index 000..07f2f9c
> > --- /dev/null
> > +++ b/90-virtio-rng.rules
> > @@ -0,0 +1 @@
> > +KERNEL=="hw_random", ATTR{rng_current}=="virtio*", RUN+="/sbin/rngd -r
> > /dev/hwrng"
> >
>
> I'm not sure this is really right. rngd can receive input from a number
> of sources, and I suspect it would be better to rngd to be started much
> earlier and perhaps get sent a SIGHUP to reconfigure when a hw_random
> device appears ... not just the virtio one.
Right - yea. Modern distros do start rngd by default (at least Fedora
and RHEL do), so this does make sense.
But as you note below, not all distros may start rngd...
> Currently rngd exits if there is no daemon. There are a few ways to
> deal with that: one is to run rngd on changes and leave a pid file
> around; the rngd can try to signal the old rngd or launch itself;
> another option would be to have a different rngd for each source.
Currently the kernel can only have one rng_current, which is a pity.
So in the current case, rngd only needs one instance -- and it'll
always accept stuff from /dev/hwrng and it doesn't matter if the
rng_current is changed as long as there's some rng there.
> To
> some degree the above is obsolete when we get khwrngd widely deployed,
> but that is a new-kernel-only kind of thing.
Right - I'm wondering if any such changes as propsed here are now
obsolted already by khwrngd?
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] rngd: add udev rule to source from hwrng if virtio-rng present
On KVM guests where the virtio-rng device is available, and set as the
current rng, this udev rule will start rngd which will feed in the
host-provided entropy to /dev/random.
Signed-off-by: Amit Shah
---
90-virtio-rng.rules | 1 +
1 file changed, 1 insertion(+)
create mode 100644 90-virtio-rng.rules
diff --git a/90-virtio-rng.rules b/90-virtio-rng.rules
new file mode 100644
index 000..07f2f9c
--- /dev/null
+++ b/90-virtio-rng.rules
@@ -0,0 +1 @@
+KERNEL=="hw_random", ATTR{rng_current}=="virtio*", RUN+="/sbin/rngd -r
/dev/hwrng"
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] Install udev rules in system-default directory
not-quite-Signed-off-by: Amit Shah --- Makefile.am | 1 + configure.ac | 8 2 files changed, 9 insertions(+) diff --git a/Makefile.am b/Makefile.am index f8490a3..a2b8173 100644 --- a/Makefile.am +++ b/Makefile.am @@ -8,6 +8,7 @@ sbin_PROGRAMS= rngd bin_PROGRAMS= rngtest man_MANS= rngd.8 rngtest.1 noinst_LIBRARIES = librngd.a +udevrules_DATA = 90-virtio-rng.rules rngd_SOURCES = rngd.h rngd.c rngd_entsource.h rngd_entsource.c \ rngd_linux.h rngd_linux.c util.c \ diff --git a/configure.ac b/configure.ac index 27a2dba..4187aca 100644 --- a/configure.ac +++ b/configure.ac @@ -32,6 +32,14 @@ AC_ARG_WITH([libgcrypt], [with_libgcrypt=check] ) +PKG_PROG_PKG_CONFIG + +AC_ARG_WITH([udevrulesdir], + AS_HELP_STRING([--with-udevrulesdir=DIR], [Directory for udev rules]), + [], + [with_udevrulesdir=$($PKG_CONFIG --variable=udevdir udev)"/rules.d"]) +AC_SUBST([udevrulesdir], [$with_udevrulesdir]) + dnl Make sure anyone changing configure.ac/Makefile.am has a clue AM_MAINTAINER_MODE -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/2] rng-tools: add udev rule for virtio-rng
Hello, I didn't find a mailing list for rng-tools, so CC'ing the kernel lists and the last few committers. The first patch in this series adds a udev rule to start rngd for guests that have a virtio-rng device available. The second patch attempts to add autoconf magic to install the udev rule in the system dirs. However, installing as non-root will obviously fail. Prefixes are also not honoured. I don't know of a better way, and we probably should leave this to distributions. However, if someone has a better idea on how to expand this, please suggest. Amit Shah (2): rngd: add udev rule to source from hwrng if virtio-rng present Install udev rules in system-default directory 90-virtio-rng.rules | 1 + Makefile.am | 1 + configure.ac| 8 3 files changed, 10 insertions(+) create mode 100644 90-virtio-rng.rules -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-rng: complete have_data completion in removing device
On (Wed) 06 Aug 2014 [16:05:41], Amos Kong wrote: > On Wed, Aug 06, 2014 at 01:35:15AM +0800, Amos Kong wrote: > > When we try to hot-remove a busy virtio-rng device from QEMU monitor, > > the device can't be hot-removed. Because virtio-rng driver hangs at > > wait_for_completion_killable(). > > > > This patch fixed the hang by completing have_data completion before > > unregistering a virtio-rng device. > > Hi Amit, > > Before applying this patch, it's blocking insider > wait_for_completion_killable() > Applied this patch, wait_for_completion_killable() returns 0, > and vi->data_avail becomes 0, then rng_get_date() will return 0. Thanks for checking this. > Is it expected result? I think what will happen is vi->data_avail will be set to whatever it was set last. In case of a previous successful read request, the data_avail will be set to whatever number of bytes the host gave. On doing a hot-unplug on the succeeding wait, the value in data_avail will be re-used, and the hwrng core will wrongly take some bytes in the buffer as input from the host. So, I think we need to set vi->data_avail = 0; before calling wait_event_completion_killable(). Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: When I boot two virtio-rng devices, guest will hang
On (Mon) 28 Jul 2014 [16:49:20], Amos Kong wrote: > On Mon, Jul 28, 2014 at 01:25:14PM +0530, Amit Shah wrote: > > On (Mon) 28 Jul 2014 [15:32:42], Amos Kong wrote: > > > QEMU commandline: > > > > > > ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -m 2000 -drive > > > file=/images/nolvm.qcow2 --kernel /home/devel/linux/arch/x86/boot/bzImage > > > -append "ro root=/dev/sda1 console=ttyS0,115200" -monitor > > > unix:/tmp/m,nowait,server -device > > > virtio-net-pci,netdev=h0,vectors=17,mq=on,id=n0 -netdev > > > tap,id=h0,queues=8 -device virtio-net-pci,netdev=h1,vectors=0,mq=on,id=n1 > > > -netdev tap,id=h1,queues=8 -vnc :0 -mon > > > chardev=qmp,mode=control,pretty=on -chardev > > > socket,id=qmp,host=localhost,port=1234,server,nowait -serial stdio > > > -object rng-random,filename=/dev/urandom,id=rng0 -device > > > virtio-rng-pci,rng=rng0,id=h0 -object > > > rng-random,filename=/dev/urandom,id=rng1 -device > > > virtio-rng-pci,rng=rng1,id=h1 > > > > > > It works when I only add one virtio-rng device. Did you touch this > > > problem? > > > > > > I'm using latest net-next/master > > > (ac3d2e5a9ef2f4d8f57c50070c4883ecb7cec29f) > > Hi Amit, > > > > > > [0.223503] Non-volatile memory driver v1.3 > > > [1.172293] tsc: Refined TSC clocksource calibration: 2893.436 MHz > > > qemu: terminating on signal 2 <-- (I have to cancel > > > QEMU process by Ctrl + C) > > > > This looks similar to what I saw when driver asks for randomness from the > > host > > before probe is completed. > > > > Does the following patch help? > > This patch was already inclued in latest net-next/master > patch commit: e052dbf554610e2104c5a7518c4d8374bed701bb No, it's a different one, goes on top of the commit you referenced. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: When I boot two virtio-rng devices, guest will hang
On (Mon) 28 Jul 2014 [15:32:42], Amos Kong wrote:
> QEMU commandline:
>
> ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -m 2000 -drive
> file=/images/nolvm.qcow2 --kernel /home/devel/linux/arch/x86/boot/bzImage
> -append "ro root=/dev/sda1 console=ttyS0,115200" -monitor
> unix:/tmp/m,nowait,server -device
> virtio-net-pci,netdev=h0,vectors=17,mq=on,id=n0 -netdev tap,id=h0,queues=8
> -device virtio-net-pci,netdev=h1,vectors=0,mq=on,id=n1 -netdev
> tap,id=h1,queues=8 -vnc :0 -mon chardev=qmp,mode=control,pretty=on -chardev
> socket,id=qmp,host=localhost,port=1234,server,nowait -serial stdio -object
> rng-random,filename=/dev/urandom,id=rng0 -device
> virtio-rng-pci,rng=rng0,id=h0 -object
> rng-random,filename=/dev/urandom,id=rng1 -device
> virtio-rng-pci,rng=rng1,id=h1
>
> It works when I only add one virtio-rng device. Did you touch this
> problem?
>
> I'm using latest net-next/master (ac3d2e5a9ef2f4d8f57c50070c4883ecb7cec29f)
> [0.223503] Non-volatile memory driver v1.3
> [1.172293] tsc: Refined TSC clocksource calibration: 2893.436 MHz
> qemu: terminating on signal 2 <-- (I have to cancel QEMU
> process by Ctrl + C)
This looks similar to what I saw when driver asks for randomness from the host
before probe is completed.
Does the following patch help?
While the driver is setup (DRIVER_OK is set), the vqs aren't marked
usable before the probe for the other device finishes as well. That's
not a scenario I considered. We can try to put this patch in 3.16,
but it won't be applicable for 3.17, where this is handled the proper
way. Also, 3.15 isn't affected, since that doesn't have multi-device
support.
Also attaching a patch that enables traces in qemu so it's easier to
see what's going on.
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..124cac5 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -34,12 +34,11 @@ struct virtrng_info {
unsigned int data_avail;
struct completion have_data;
bool busy;
+ bool probe_done;
char name[25];
int index;
};
-static bool probe_done;
-
static void random_recv_done(struct virtqueue *vq)
{
struct virtrng_info *vi = vq->vdev->priv;
@@ -73,7 +72,7 @@ static int virtio_read(struct hwrng *rng, void *buf,
-size_t size, bool wait)
* Don't ask host for data till we're setup. This call can
* happen during hwrng_register(), after commit d9e7972619.
*/
- if (unlikely(!probe_done))
+ if (unlikely(!vi->probe_done))
return 0;
if (!vi->busy) {
@@ -146,7 +145,7 @@ static int probe_common(struct virtio_device
*vdev)
return err;
}
- probe_done = true;
+ vi->probe_done = true;
return 0;
}
Amit
>From ec1aa555b67628beefa0ac6902baa2cc2e156f58 Mon Sep 17 00:00:00 2001
Message-Id:
From: Amit Shah
Date: Mon, 21 Jul 2014 14:46:28 +0530
Subject: [PATCH 1/1] virtio-rng: add some trace events
Add some trace events to virtio-rng for easier debugging
Signed-off-by: Amit Shah
---
hw/virtio/virtio-rng.c | 7 +++
trace-events | 5 +
2 files changed, 12 insertions(+)
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 7c5a675..4a6472b 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -16,6 +16,7 @@
#include "hw/virtio/virtio-rng.h"
#include "sysemu/rng.h"
#include "qom/object_interfaces.h"
+#include "trace.h"
static bool is_guest_ready(VirtIORNG *vrng)
{
@@ -24,6 +25,7 @@ static bool is_guest_ready(VirtIORNG *vrng)
&& (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
return true;
}
+trace_virtio_rng_guest_not_ready(vrng);
return false;
}
@@ -62,6 +64,7 @@ static void chr_read(void *opaque, const void *buf, size_t
size)
offset += len;
virtqueue_push(vrng->vq, &elem, len);
+trace_virtio_rng_pushed(vrng, len);
}
virtio_notify(vdev, vrng->vq);
}
@@ -81,7 +84,11 @@ static void virtio_rng_process(VirtIORNG *vrng)
quota = MIN((uint64_t)vrng->quota_remaining, (uint64_t)UINT32_MAX);
}
size = get_request_size(vrng->vq, quota);
+
+trace_virtio_rng_request(vrng, size, quota);
+
size = MIN(vrng->quota_remaining, size);
+
if (size) {
rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
}
diff --git a/trace-events b/trace-events
index 11a17a8..99f39ac 100644
--- a/trace-events
+++ b/trace-events
@@ -41,6 +41,11 @@ virtio_irq(void *vq) "vq %p"
virtio_notify(void *vdev, void *vq) "vdev %p vq %p"
virtio_set_status(void *vdev, uint8_t val) "vdev
Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready
On (Mon) 21 Jul 2014 [17:15:51], Amit Shah wrote: > Instead of calling hwrng_register() in the probe routing, call it in the > scan routine. This ensures that when hwrng_register() is successful, > and it requests a few random bytes to seed the kernel's pool at init, > we're ready to service that request. > > This will also enable us to remove the workaround added previously to > check whether probe was completed, and only then ask for data from the > host. The revert follows in the next commit. > > There's a slight behaviour change here on unsuccessful hwrng_register(). > Previously, when hwrng_unregister() failed, the probe() routine would typo: should be hwrng_register(). Please fix this up when picking up the patch. Thanks, > fail, and the vqs would be torn down, and driver would be marked not > initialized. Now, the vqs will remain initialized, driver would be > marked initialized as well, but won't be available in the list of RNGs > available to hwrng core. To fix the failures, the procedure remains the > same, i.e. unload and re-load the module, and hope things succeed the > next time around. > > Signed-off-by: Amit Shah Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/4] virtio-rng: contribute to early randomness requests
On (Tue) 22 Jul 2014 [07:46:23], Jason Cooper wrote: > On Mon, Jul 21, 2014 at 05:15:48PM +0530, Amit Shah wrote: > > v2: > > - update patch 3 to store the hwrng_register_done bool per-device > >rather than global > > - add patch 2 that re-arranges struct elems for better packing. > > > > Hi, > > > > This series enables virtio-rng to service the early randomness > > requests made by the hwrng core (patch 3), with Herbert's idea of > > using the scan routine. > > > > Patch 4 reverts the previous restriction, which no longer applies, to > > not send read requests to the host before successful probe. > > > > Patches 1 and 2 are minor cleanups. > > > > Please review and apply, > > > > Amit Shah (4): > > virtio: rng: remove unused struct element > > virtio: rng: re-arrange struct elements for better packing > > virtio: rng: delay hwrng_register() till driver is ready > > Revert "hwrng: virtio - ensure reads happen after successful probe" > > > > drivers/char/hw_random/core.c | 6 -- > > drivers/char/hw_random/virtio-rng.c | 39 > > - > > 2 files changed, 17 insertions(+), 28 deletions(-) > > fwiw, > > Reviewed-by: Jason Cooper > > Thanks for putting this together Amit! Thanks, Jason! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready
On (Mon) 21 Jul 2014 [08:11:16], Jason Cooper wrote:
> On Mon, Jul 21, 2014 at 05:15:51PM +0530, Amit Shah wrote:
> > Instead of calling hwrng_register() in the probe routing, call it in the
> > scan routine. This ensures that when hwrng_register() is successful,
> > and it requests a few random bytes to seed the kernel's pool at init,
> > we're ready to service that request.
> >
> > This will also enable us to remove the workaround added previously to
> > check whether probe was completed, and only then ask for data from the
> > host. The revert follows in the next commit.
> >
> > There's a slight behaviour change here on unsuccessful hwrng_register().
> > Previously, when hwrng_unregister() failed, the probe() routine would
> > fail, and the vqs would be torn down, and driver would be marked not
> > initialized. Now, the vqs will remain initialized, driver would be
> > marked initialized as well, but won't be available in the list of RNGs
> > available to hwrng core. To fix the failures, the procedure remains the
> > same, i.e. unload and re-load the module, and hope things succeed the
> > next time around.
>
> I'm not too comfortable with this. I'll try to take a closer look
> tonight, but in the meantime...
>
> > Signed-off-by: Amit Shah
> > ---
> > drivers/char/hw_random/virtio-rng.c | 25 +++--
> > 1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/virtio-rng.c
> > b/drivers/char/hw_random/virtio-rng.c
> > index a156284..d9927eb 100644
> > --- a/drivers/char/hw_random/virtio-rng.c
> > +++ b/drivers/char/hw_random/virtio-rng.c
> > @@ -35,6 +35,7 @@ struct virtrng_info {
> > unsigned int data_avail;
> > int index;
> > bool busy;
> > + bool hwrng_register_done;
> > };
> >
> > static bool probe_done;
> > @@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
> > return err;
> > }
> >
> > - err = hwrng_register(&vi->hwrng);
> > - if (err) {
> > - vdev->config->del_vqs(vdev);
> > - vi->vq = NULL;
> > - kfree(vi);
> > - ida_simple_remove(&rng_index_ida, index);
> > - return err;
> > - }
> > -
>
> This needs to stay. register, and failure to do so, should occur in the
> probe routine.
Can you elaborate why?
> > probe_done = true;
> > return 0;
> > }
> > @@ -152,9 +144,11 @@ static int probe_common(struct virtio_device *vdev)
> > static void remove_common(struct virtio_device *vdev)
> > {
> > struct virtrng_info *vi = vdev->priv;
> > +
> > vdev->config->reset(vdev);
> > vi->busy = false;
> > - hwrng_unregister(&vi->hwrng);
> > + if (vi->hwrng_register_done)
> > + hwrng_unregister(&vi->hwrng);
> > vdev->config->del_vqs(vdev);
> > ida_simple_remove(&rng_index_ida, vi->index);
> > kfree(vi);
> > @@ -170,6 +164,16 @@ static void virtrng_remove(struct virtio_device *vdev)
> > remove_common(vdev);
> > }
> >
> > +static void virtrng_scan(struct virtio_device *vdev)
> > +{
> > + struct virtrng_info *vi = vdev->priv;
> > + int err;
> > +
> > + err = hwrng_register(&vi->hwrng);
> > + if (!err)
> > + vi->hwrng_register_done = true;
>
> Instead, perhaps we should just feed the entropy pool from here? We
> would still need to prevent the core from doing so. Perhaps back to the
> flag idea?
No way hwrng knows the difference between probe and scan for
virtio-rng, so it's back to the delayed workqueue idea, if this isn't
usable..
But I need to understand why this isn't workable.
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/4] virtio: rng: delay hwrng_register() till driver is ready
Instead of calling hwrng_register() in the probe routing, call it in the
scan routine. This ensures that when hwrng_register() is successful,
and it requests a few random bytes to seed the kernel's pool at init,
we're ready to service that request.
This will also enable us to remove the workaround added previously to
check whether probe was completed, and only then ask for data from the
host. The revert follows in the next commit.
There's a slight behaviour change here on unsuccessful hwrng_register().
Previously, when hwrng_unregister() failed, the probe() routine would
fail, and the vqs would be torn down, and driver would be marked not
initialized. Now, the vqs will remain initialized, driver would be
marked initialized as well, but won't be available in the list of RNGs
available to hwrng core. To fix the failures, the procedure remains the
same, i.e. unload and re-load the module, and hope things succeed the
next time around.
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index a156284..d9927eb 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -35,6 +35,7 @@ struct virtrng_info {
unsigned int data_avail;
int index;
bool busy;
+ bool hwrng_register_done;
};
static bool probe_done;
@@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- err = hwrng_register(&vi->hwrng);
- if (err) {
- vdev->config->del_vqs(vdev);
- vi->vq = NULL;
- kfree(vi);
- ida_simple_remove(&rng_index_ida, index);
- return err;
- }
-
probe_done = true;
return 0;
}
@@ -152,9 +144,11 @@ static int probe_common(struct virtio_device *vdev)
static void remove_common(struct virtio_device *vdev)
{
struct virtrng_info *vi = vdev->priv;
+
vdev->config->reset(vdev);
vi->busy = false;
- hwrng_unregister(&vi->hwrng);
+ if (vi->hwrng_register_done)
+ hwrng_unregister(&vi->hwrng);
vdev->config->del_vqs(vdev);
ida_simple_remove(&rng_index_ida, vi->index);
kfree(vi);
@@ -170,6 +164,16 @@ static void virtrng_remove(struct virtio_device *vdev)
remove_common(vdev);
}
+static void virtrng_scan(struct virtio_device *vdev)
+{
+ struct virtrng_info *vi = vdev->priv;
+ int err;
+
+ err = hwrng_register(&vi->hwrng);
+ if (!err)
+ vi->hwrng_register_done = true;
+}
+
#ifdef CONFIG_PM_SLEEP
static int virtrng_freeze(struct virtio_device *vdev)
{
@@ -194,6 +198,7 @@ static struct virtio_driver virtio_rng_driver = {
.id_table = id_table,
.probe =virtrng_probe,
.remove = virtrng_remove,
+ .scan = virtrng_scan,
#ifdef CONFIG_PM_SLEEP
.freeze = virtrng_freeze,
.restore = virtrng_restore,
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 4/4] Revert "hwrng: virtio - ensure reads happen after successful probe"
This reverts commit e052dbf554610e2104c5a7518c4d8374bed701bb.
Now that we use the virtio ->scan() function to register with the hwrng
core, we will not get read requests till probe is successfully finished.
So revert the workaround we had in place to refuse read requests while
we were not yet setup completely.
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 6 --
drivers/char/hw_random/virtio-rng.c | 9 -
2 files changed, 15 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
- /*
-* Currently only virtio-rng cannot return data during device
-* probe, and that's handled in virtio-rng.c itself. If there
-* are more such devices, this call to rng_get_data can be
-* made conditional here instead of doing it per-device.
-*/
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index d9927eb..0027137 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -38,7 +38,6 @@ struct virtrng_info {
bool hwrng_register_done;
};
-static bool probe_done;
static void random_recv_done(struct virtqueue *vq)
{
@@ -69,13 +68,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
- /*
-* Don't ask host for data till we're setup. This call can
-* happen during hwrng_register(), after commit d9e7972619.
-*/
- if (unlikely(!probe_done))
- return 0;
-
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -137,7 +129,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/4] virtio: rng: re-arrange struct elements for better packing
Re-arrange the elements of the virtrng_info struct to pack it better.
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index d8ffebd..a156284 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -30,11 +30,11 @@ static DEFINE_IDA(rng_index_ida);
struct virtrng_info {
struct hwrng hwrng;
struct virtqueue *vq;
- unsigned int data_avail;
struct completion have_data;
- bool busy;
char name[25];
+ unsigned int data_avail;
int index;
+ bool busy;
};
static bool probe_done;
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 0/4] virtio-rng: contribute to early randomness requests
v2: - update patch 3 to store the hwrng_register_done bool per-device rather than global - add patch 2 that re-arranges struct elems for better packing. Hi, This series enables virtio-rng to service the early randomness requests made by the hwrng core (patch 3), with Herbert's idea of using the scan routine. Patch 4 reverts the previous restriction, which no longer applies, to not send read requests to the host before successful probe. Patches 1 and 2 are minor cleanups. Please review and apply, Amit Shah (4): virtio: rng: remove unused struct element virtio: rng: re-arrange struct elements for better packing virtio: rng: delay hwrng_register() till driver is ready Revert "hwrng: virtio - ensure reads happen after successful probe" drivers/char/hw_random/core.c | 6 -- drivers/char/hw_random/virtio-rng.c | 39 - 2 files changed, 17 insertions(+), 28 deletions(-) -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/4] virtio: rng: remove unused struct element
vdev is unused in struct virtrng_info, remove it.
CC: Amos Kong
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..d8ffebd 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -28,7 +28,6 @@
static DEFINE_IDA(rng_index_ida);
struct virtrng_info {
- struct virtio_device *vdev;
struct hwrng hwrng;
struct virtqueue *vq;
unsigned int data_avail;
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] virtio: rng: delay hwrng_register() till driver is ready
On (Mon) 21 Jul 2014 [15:01:00], Amit Shah wrote:
> Instead of calling hwrng_register() in the probe routing, call it in the
> scan routine. This ensures that when hwrng_register() is successful,
> and it requests a few random bytes to seed the kernel's pool at init,
> we're ready to service that request.
>
> This will also enable us to remove the workaround added previously to
> check whether probe was completed, and only then ask for data from the
> host. The revert follows in the next commit.
>
> Signed-off-by: Amit Shah
> ---
> drivers/char/hw_random/virtio-rng.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/hw_random/virtio-rng.c
> b/drivers/char/hw_random/virtio-rng.c
> index d8ffebd..32e6373 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -38,6 +38,7 @@ struct virtrng_info {
> };
>
> static bool probe_done;
> +static bool hwrng_register_done;
Ah; this needs to be per-device after the recent multi-device work,
rather than a global.
New patches coming up.
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/3] virtio: rng: remove unused struct element
vdev is unused in struct virtrng_info, remove it.
CC: Amos Kong
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index e9b15bc..d8ffebd 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -28,7 +28,6 @@
static DEFINE_IDA(rng_index_ida);
struct virtrng_info {
- struct virtio_device *vdev;
struct hwrng hwrng;
struct virtqueue *vq;
unsigned int data_avail;
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/3] virtio: rng: delay hwrng_register() till driver is ready
Instead of calling hwrng_register() in the probe routing, call it in the
scan routine. This ensures that when hwrng_register() is successful,
and it requests a few random bytes to seed the kernel's pool at init,
we're ready to service that request.
This will also enable us to remove the workaround added previously to
check whether probe was completed, and only then ask for data from the
host. The revert follows in the next commit.
Signed-off-by: Amit Shah
---
drivers/char/hw_random/virtio-rng.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index d8ffebd..32e6373 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -38,6 +38,7 @@ struct virtrng_info {
};
static bool probe_done;
+static bool hwrng_register_done;
static void random_recv_done(struct virtqueue *vq)
{
@@ -136,15 +137,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- err = hwrng_register(&vi->hwrng);
- if (err) {
- vdev->config->del_vqs(vdev);
- vi->vq = NULL;
- kfree(vi);
- ida_simple_remove(&rng_index_ida, index);
- return err;
- }
-
probe_done = true;
return 0;
}
@@ -152,9 +144,11 @@ static int probe_common(struct virtio_device *vdev)
static void remove_common(struct virtio_device *vdev)
{
struct virtrng_info *vi = vdev->priv;
+
vdev->config->reset(vdev);
vi->busy = false;
- hwrng_unregister(&vi->hwrng);
+ if (hwrng_register_done)
+ hwrng_unregister(&vi->hwrng);
vdev->config->del_vqs(vdev);
ida_simple_remove(&rng_index_ida, vi->index);
kfree(vi);
@@ -170,6 +164,16 @@ static void virtrng_remove(struct virtio_device *vdev)
remove_common(vdev);
}
+static void virtrng_scan(struct virtio_device *vdev)
+{
+ struct virtrng_info *vi = vdev->priv;
+ int err;
+
+ err = hwrng_register(&vi->hwrng);
+ if (!err)
+ hwrng_register_done = true;
+}
+
#ifdef CONFIG_PM_SLEEP
static int virtrng_freeze(struct virtio_device *vdev)
{
@@ -194,6 +198,7 @@ static struct virtio_driver virtio_rng_driver = {
.id_table = id_table,
.probe =virtrng_probe,
.remove = virtrng_remove,
+ .scan = virtrng_scan,
#ifdef CONFIG_PM_SLEEP
.freeze = virtrng_freeze,
.restore = virtrng_restore,
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/3] virtio-rng: contribute to early randomness requests
Hi, This series enables virtio-rng to service the early randomness requests made by the hwrng core (patch 2), with Herbert's idea of using the scan routine. Patch 3 reverts the previous restriction, which no longer applies, to not send read requests to the host before successful probe. Patch 1 is a minor cleanup. Please review and apply, Amit Shah (3): virtio: rng: remove unused struct element virtio: rng: delay hwrng_register() till driver is ready Revert "hwrng: virtio - ensure reads happen after successful probe" drivers/char/hw_random/core.c | 6 -- drivers/char/hw_random/virtio-rng.c | 35 +++ 2 files changed, 15 insertions(+), 26 deletions(-) -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 3/3] Revert "hwrng: virtio - ensure reads happen after successful probe"
This reverts commit e052dbf554610e2104c5a7518c4d8374bed701bb.
Now that we use the virtio ->scan() function to register with the hwrng
core, we will not get read requests till probe is successfully finished.
So revert the workaround we had in place to refuse read requests while
we were not yet setup completely.
Signed-off-by: Amit Shah
Conflicts:
drivers/char/hw_random/virtio-rng.c
(Conflict due to change in context around the revert)
---
drivers/char/hw_random/core.c | 6 --
drivers/char/hw_random/virtio-rng.c | 9 -
2 files changed, 15 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
- /*
-* Currently only virtio-rng cannot return data during device
-* probe, and that's handled in virtio-rng.c itself. If there
-* are more such devices, this call to rng_get_data can be
-* made conditional here instead of doing it per-device.
-*/
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index 32e6373..2080630 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -37,7 +37,6 @@ struct virtrng_info {
int index;
};
-static bool probe_done;
static bool hwrng_register_done;
static void random_recv_done(struct virtqueue *vq)
@@ -69,13 +68,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
- /*
-* Don't ask host for data till we're setup. This call can
-* happen during hwrng_register(), after commit d9e7972619.
-*/
- if (unlikely(!probe_done))
- return 0;
-
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -137,7 +129,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
On (Fri) 18 Jul 2014 [17:14:08], Herbert Xu wrote: > On Fri, Jul 18, 2014 at 02:26:26PM +0530, Amit Shah wrote: > > > > > Sounds like a good idea to me. Though, changes in core.c that > > > increase the time in hwrng_register() or hwrng_init() may not get > > > noticed by rng drivers and they may suddenly start failing for no > > > apparent reason. Seems like a far stretch, though. Does anyone else > > > have an opinion on this? > > > > Herbert, do you have any preference? > > So it's only virtio-rng that's a problem, right? How about if we > abuse the scan hook in virtio and move the hwrng_register there? Oops, I had completely missed the scan hook, and looks like it was added for exactly the purpose we want here (so we won't even be abusing it!) Thanks! I'll post the patches when the one to revert gets an upstream commit id. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
On (Mon) 14 Jul 2014 [18:12:46], Amit Shah wrote:
> On (Mon) 14 Jul 2014 [08:37:00], Jason Cooper wrote:
> > On Mon, Jul 14, 2014 at 10:05:19AM +0530, Amit Shah wrote:
> > > Some RNG devices may not be ready to give early randomness at probe()
> > > time, and hence lose out on the opportunity to contribute to system
> > > randomness at boot- or device hotplug- time.
> > >
> > > This commit schedules a delayed work item for such devices, and fetches
> > > early randomness after a delay. Currently the delay is 500ms, which is
> > > enough for the lone device that needs such treatment: virtio-rng.
> > >
> > > CC: Kees Cook
> > > CC: Jason Cooper
> > > CC: Herbert Xu
> > > Signed-off-by: Amit Shah
> > > ---
> > > drivers/char/hw_random/core.c | 20 +++-
> > > include/linux/hw_random.h | 8
> > > 2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > index c4419ea..2a765fd 100644
> > > --- a/drivers/char/hw_random/core.c
> > > +++ b/drivers/char/hw_random/core.c
> > > @@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
> > > return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
> > > }
> > >
> > > -static void add_early_randomness(struct hwrng *rng)
> > > +static void get_early_randomness(struct hwrng *rng)
> > > {
> > > unsigned char bytes[16];
> > > int bytes_read;
> > > @@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
> > > add_device_randomness(bytes, bytes_read);
> > > }
> > >
> > > +static void sched_init_random(struct work_struct *work)
> > > +{
> > > + struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
> > > +
> > > + get_early_randomness(rng);
> > > +}
> > > +
> > > +static void add_early_randomness(struct hwrng *rng)
> >
> > The add/get naming seems awkward in the above hunks.
>
> Yea; I felt that too. I thought of a do_add_early_randomness()
> instead, but that seemed awkward too. I forgot to mention I was
> planning on revisiting this naming for v1.
>
> > > +{
> > > + if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
> > > + return get_early_randomness(rng);
> > > +
> > > + schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
> > > +}
> > > +
> >
> > Perhaps instead of rng->flags and a hardcoded delay, we could have
> > rng->seed_delay = msecs_to_jiffies(500) in virtio-rng? Then you can
> > just call unconditionally:
> >
> > schedule_delayed_work(&rng->dwork, rng->seed_delay);
BTW I didn't want to make this call unconditional -- i.e. the existing
behaviour of in-line fetching of randomness for all devices but one
should not be affected.
If indeed people are OK with this being done by a delayed work item
for all the drivers, the code can get a bit simpler here.
> > I think that would be a more extensible solution should other drivers
> > show up with the same issue.
>
> Sounds like a good idea to me. Though, changes in core.c that
> increase the time in hwrng_register() or hwrng_init() may not get
> noticed by rng drivers and they may suddenly start failing for no
> apparent reason. Seems like a far stretch, though. Does anyone else
> have an opinion on this?
Herbert, do you have any preference?
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/2] hwrng, virtio-rng: init-time fixes
On (Tue) 15 Jul 2014 [12:45:56], Herbert Xu wrote: > On Tue, Jul 15, 2014 at 10:10:28AM +0530, Amit Shah wrote: > > On (Mon) 14 Jul 2014 [20:50:06], Herbert Xu wrote: > > > On Thu, Jul 10, 2014 at 03:42:33PM +0530, Amit Shah wrote: > > > > v3: > > > > - Kees Cook pointed out a weird side-effect: devices which have > > > >->init() registered get their randomness added to the system each > > > >time they're switched in, but devices that don't have the init > > > >callback don't contribute to system randomness more than once. The > > > >weirdness is resolved here by using the randomness each time > > > >hwrng_init() is attempted, irrespective of the existence of the > > > >device's ->init() callback. > > > > > > All applied to crypto. Thanks! > > > > Thanks, Herbert. I didn't mention it, but pls queue it up for 3.16. > > Yes that's why it's in crypto as opposed to cryptodev. Ah; thanks! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/2] hwrng, virtio-rng: init-time fixes
On (Mon) 14 Jul 2014 [20:50:06], Herbert Xu wrote: > On Thu, Jul 10, 2014 at 03:42:33PM +0530, Amit Shah wrote: > > v3: > > - Kees Cook pointed out a weird side-effect: devices which have > >->init() registered get their randomness added to the system each > >time they're switched in, but devices that don't have the init > >callback don't contribute to system randomness more than once. The > >weirdness is resolved here by using the randomness each time > >hwrng_init() is attempted, irrespective of the existence of the > >device's ->init() callback. > > All applied to crypto. Thanks! Thanks, Herbert. I didn't mention it, but pls queue it up for 3.16. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
On (Mon) 14 Jul 2014 [08:37:00], Jason Cooper wrote:
> On Mon, Jul 14, 2014 at 10:05:19AM +0530, Amit Shah wrote:
> > Some RNG devices may not be ready to give early randomness at probe()
> > time, and hence lose out on the opportunity to contribute to system
> > randomness at boot- or device hotplug- time.
> >
> > This commit schedules a delayed work item for such devices, and fetches
> > early randomness after a delay. Currently the delay is 500ms, which is
> > enough for the lone device that needs such treatment: virtio-rng.
> >
> > CC: Kees Cook
> > CC: Jason Cooper
> > CC: Herbert Xu
> > Signed-off-by: Amit Shah
> > ---
> > drivers/char/hw_random/core.c | 20 +++-
> > include/linux/hw_random.h | 8
> > 2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index c4419ea..2a765fd 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
> > return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
> > }
> >
> > -static void add_early_randomness(struct hwrng *rng)
> > +static void get_early_randomness(struct hwrng *rng)
> > {
> > unsigned char bytes[16];
> > int bytes_read;
> > @@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
> > add_device_randomness(bytes, bytes_read);
> > }
> >
> > +static void sched_init_random(struct work_struct *work)
> > +{
> > + struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
> > +
> > + get_early_randomness(rng);
> > +}
> > +
> > +static void add_early_randomness(struct hwrng *rng)
>
> The add/get naming seems awkward in the above hunks.
Yea; I felt that too. I thought of a do_add_early_randomness()
instead, but that seemed awkward too. I forgot to mention I was
planning on revisiting this naming for v1.
> > +{
> > + if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
> > + return get_early_randomness(rng);
> > +
> > + schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
> > +}
> > +
>
> Perhaps instead of rng->flags and a hardcoded delay, we could have
> rng->seed_delay = msecs_to_jiffies(500) in virtio-rng? Then you can
> just call unconditionally:
>
> schedule_delayed_work(&rng->dwork, rng->seed_delay);
>
> I think that would be a more extensible solution should other drivers
> show up with the same issue.
Sounds like a good idea to me. Though, changes in core.c that
increase the time in hwrng_register() or hwrng_init() may not get
noticed by rng drivers and they may suddenly start failing for no
apparent reason. Seems like a far stretch, though. Does anyone else
have an opinion on this?
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 2/3] virtio: rng: only accept delayed early randomness requests
hw_random core can ask for initial randomness after a slight delay after probe() finishes, and we can contribute to system randomness at that point. Tell the hw_random core by setting the HWRNG_DELAY_READ_AT_INIT flag. CC: Kees Cook CC: Jason Cooper CC: Herbert Xu Signed-off-by: Amit Shah --- drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index d8ffebd..f53f1e8 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -123,6 +123,7 @@ static int probe_common(struct virtio_device *vdev) .cleanup = virtio_cleanup, .priv = (unsigned long)vi, .name = vi->name, + .flags = HWRNG_DELAY_READ_AT_INIT, }; vdev->priv = vi; -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
Some RNG devices may not be ready to give early randomness at probe()
time, and hence lose out on the opportunity to contribute to system
randomness at boot- or device hotplug- time.
This commit schedules a delayed work item for such devices, and fetches
early randomness after a delay. Currently the delay is 500ms, which is
enough for the lone device that needs such treatment: virtio-rng.
CC: Kees Cook
CC: Jason Cooper
CC: Herbert Xu
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 20 +++-
include/linux/hw_random.h | 8
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a765fd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
}
-static void add_early_randomness(struct hwrng *rng)
+static void get_early_randomness(struct hwrng *rng)
{
unsigned char bytes[16];
int bytes_read;
@@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
add_device_randomness(bytes, bytes_read);
}
+static void sched_init_random(struct work_struct *work)
+{
+ struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
+
+ get_early_randomness(rng);
+}
+
+static void add_early_randomness(struct hwrng *rng)
+{
+ if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
+ return get_early_randomness(rng);
+
+ schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
+}
+
static inline int hwrng_init(struct hwrng *rng)
{
if (rng->init) {
@@ -351,6 +366,7 @@ int hwrng_register(struct hwrng *rng)
goto out_unlock;
}
+ INIT_DELAYED_WORK(&rng->dwork, sched_init_random);
old_rng = current_rng;
if (!old_rng) {
err = hwrng_init(rng);
@@ -362,6 +378,7 @@ int hwrng_register(struct hwrng *rng)
if (!old_rng) {
err = register_miscdev();
if (err) {
+ cancel_delayed_work_sync(&rng->dwork);
hwrng_cleanup(rng);
current_rng = NULL;
goto out_unlock;
@@ -395,6 +412,7 @@ void hwrng_unregister(struct hwrng *rng)
mutex_lock(&rng_mutex);
list_del(&rng->list);
+ cancel_delayed_work_sync(&rng->dwork);
if (current_rng == rng) {
hwrng_cleanup(rng);
if (list_empty(&rng_list)) {
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index b4b0eef..8f7370d 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -14,6 +14,11 @@
#include
#include
+#include
+
+#define HWRNG_DELAY_READ_AT_INIT BIT(0)/* Schedule delayed work to
fetch
+* initial randomness instead of doing
+* it at ->init()-time */
/**
* struct hwrng - Hardware Random Number Generator driver
@@ -28,6 +33,7 @@
* Must not be NULL.*OBSOLETE*
* @read: New API. drivers can fill up to max bytes of data
* into the buffer. The buffer is aligned for any type.
+ * @flags: Per-device flags.
* @priv: Private data, for use by the RNG driver.
*/
struct hwrng {
@@ -37,9 +43,11 @@ struct hwrng {
int (*data_present)(struct hwrng *rng, int wait);
int (*data_read)(struct hwrng *rng, u32 *data);
int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
+ unsigned int flags;
unsigned long priv;
/* internal. */
+ struct delayed_work dwork;
struct list_head list;
};
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 3/3] Revert "virtio: rng: ensure reads happen after successful probe"
This reverts commit ceb5d72a2e27e95bc9570ce259c45b35f0e23462.
This commit was added for -stable so systems with virtio-rng don't
freeze at boot-time.
With the addition of the previous commits that delay the request for
initial randomness after probe() is successful, this is no longer
needed.
CC: Kees Cook
CC: Jason Cooper
CC: Herbert Xu
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 6 --
drivers/char/hw_random/virtio-rng.c | 10 --
2 files changed, 16 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 2a765fd..148d9be 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void get_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
- /*
-* Currently only virtio-rng cannot return data during device
-* probe, and that's handled in virtio-rng.c itself. If there
-* are more such devices, this call to rng_get_data can be
-* made conditional here instead of doing it per-device.
-*/
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f53f1e8..f7d1573 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -37,8 +37,6 @@ struct virtrng_info {
int index;
};
-static bool probe_done;
-
static void random_recv_done(struct virtqueue *vq)
{
struct virtrng_info *vi = vq->vdev->priv;
@@ -68,13 +66,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
- /*
-* Don't ask host for data till we're setup. This call can
-* happen during hwrng_register(), after commit d9e7972619.
-*/
- if (unlikely(!probe_done))
- return 0;
-
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -146,7 +137,6 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
- probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 0/3] hw_random: support for delayed init randomness requests
Hello, This series introduces a way to allow devices to contribute to initial system randomness after a certain delay. Specifically, the virtio-rng device can contribute initial randomness only after a successful probe(). A delayed workqueue item is queued in the system queue to fetch this randomness if the device indicates it's capable of contributing only after a delay, via the new HWRNG_DELAY_READ_AT_INIT flag. This series is intended for the next merge window. I've marked it as RFC because the last revert doesn't yet have a linux.git sha1. I'll re-post when the previous series gets committed. Please review, Amit Shah (3): hw_random: allow RNG devices to give early randomness after a delay virtio: rng: only accept delayed early randomness requests Revert "virtio: rng: ensure reads happen after successful probe" drivers/char/hw_random/core.c | 26 +++--- drivers/char/hw_random/virtio-rng.c | 11 +-- include/linux/hw_random.h | 8 3 files changed, 28 insertions(+), 17 deletions(-) -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH] hwrng: sysfs entry rng_seed_kernel, was: "Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init"
On (Wed) 09 Jul 2014 [12:07:25], Jason Cooper wrote: > Amit, Kees, (snip) > I'm cooling to the idea of the init function for virtio-rng, and it > might be best just to admit that there's no way to seed the entropy pool > from the virtio-rng at probe time. After all, once userspace is up, the > system should take advantage of /dev/hwrng for the generation of > long-term keys. Either via rngd feeding /dev/random, or directly. > > As for the follow-on patch you asked about, I think that's fine. More > entropy can't hurt. > > The below patch might be worth considering so that the user of a system > with only virtio-rng can kick the entropy pool as they see fit. It's > probably not too kosher as is, but if the idea is liked, I could clean > it up and submit. > > The advantage is that users don't need to have rngd installed and > running on the system in order to jump-start the entropy pool. ... so a udev rule that looks for the new sysfs file, and asks the kernel to do its thing? And maybe even a patch to rngd that looks for this file and does a similar thing? There's also the option to use a delayed workqueue item, that will succeed if probe has finished. This method doesn't have userspace dependencies. Thanks, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 2/2] virtio: rng: ensure reads happen after successful probe
The hwrng core asks for random data in the hwrng_register() call itself
from commit d9e7972619. This doesn't play well with virtio -- the
DRIVER_OK bit is only set by virtio core on a successful probe, and
we're not yet out of our probe routine when this call is made. This
causes the host to not acknowledge any requests we put in the virtqueue,
and the insmod or kernel boot process just waits for data to arrive from
the host, which never happens.
CC: Kees Cook
CC: Jason Cooper
CC: Herbert Xu
CC: # For v3.15+
Reviewed-by: Jason Cooper
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 6 ++
drivers/char/hw_random/virtio-rng.c | 10 ++
2 files changed, 16 insertions(+)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 2a451b1..c4419ea 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,6 +68,12 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
+ /*
+* Currently only virtio-rng cannot return data during device
+* probe, and that's handled in virtio-rng.c itself. If there
+* are more such devices, this call to rng_get_data can be
+* made conditional here instead of doing it per-device.
+*/
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f3e7150..e9b15bc 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -38,6 +38,8 @@ struct virtrng_info {
int index;
};
+static bool probe_done;
+
static void random_recv_done(struct virtqueue *vq)
{
struct virtrng_info *vi = vq->vdev->priv;
@@ -67,6 +69,13 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
+ /*
+* Don't ask host for data till we're setup. This call can
+* happen during hwrng_register(), after commit d9e7972619.
+*/
+ if (unlikely(!probe_done))
+ return 0;
+
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -137,6 +146,7 @@ static int probe_common(struct virtio_device *vdev)
return err;
}
+ probe_done = true;
return 0;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 1/2] hwrng: fetch randomness only after device init
Commit d9e7972619334 "hwrng: add randomness to system from rng sources"
added a call to rng_get_data() from the hwrng_register() function.
However, some rng devices need initialization before data can be read
from them.
This commit makes the call to rng_get_data() depend on no init fn
pointer being registered by the device. If an init function is
registered, this call is made after device init.
CC: Kees Cook
CC: Jason Cooper
CC: Herbert Xu
CC: # For v3.15+
Signed-off-by: Amit Shah
---
drivers/char/hw_random/core.c | 41 +
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601c..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -55,16 +55,35 @@ static DEFINE_MUTEX(rng_mutex);
static int data_avail;
static u8 *rng_buffer;
+static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
+ int wait);
+
static size_t rng_buffer_size(void)
{
return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
}
+static void add_early_randomness(struct hwrng *rng)
+{
+ unsigned char bytes[16];
+ int bytes_read;
+
+ bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ if (bytes_read > 0)
+ add_device_randomness(bytes, bytes_read);
+}
+
static inline int hwrng_init(struct hwrng *rng)
{
- if (!rng->init)
- return 0;
- return rng->init(rng);
+ if (rng->init) {
+ int ret;
+
+ ret = rng->init(rng);
+ if (ret)
+ return ret;
+ }
+ add_early_randomness(rng);
+ return 0;
}
static inline void hwrng_cleanup(struct hwrng *rng)
@@ -304,8 +323,6 @@ int hwrng_register(struct hwrng *rng)
{
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
- unsigned char bytes[16];
- int bytes_read;
if (rng->name == NULL ||
(rng->data_read == NULL && rng->read == NULL))
@@ -347,9 +364,17 @@ int hwrng_register(struct hwrng *rng)
INIT_LIST_HEAD(&rng->list);
list_add_tail(&rng->list, &rng_list);
- bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
- if (bytes_read > 0)
- add_device_randomness(bytes, bytes_read);
+ if (old_rng && !rng->init) {
+ /*
+* Use a new device's input to add some randomness to
+* the system. If this rng device isn't going to be
+* used right away, its init function hasn't been
+* called yet; so only use the randomness from devices
+* that don't need an init callback.
+*/
+ add_early_randomness(rng);
+ }
+
out_unlock:
mutex_unlock(&rng_mutex);
out:
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 0/2] hwrng, virtio-rng: init-time fixes
v3: - Kees Cook pointed out a weird side-effect: devices which have ->init() registered get their randomness added to the system each time they're switched in, but devices that don't have the init callback don't contribute to system randomness more than once. The weirdness is resolved here by using the randomness each time hwrng_init() is attempted, irrespective of the existence of the device's ->init() callback. v2: - this now separates both the patches; the virtio-rng fix is self-contained - re-work hwrng core to fetch randomness at device init time if ->init() is registered by the device, instead of not calling it at all. - virtio-rng: introduce a probe_done bool to ensure we don't ask host for data before successful probe Hi, When booting a recent kernel under KVM with the virtio-rng device enabled, the boot process was stalling. Bisect pointed to a commit made during the 3.15 window to fetch randomness from newly-registered devices in the hwrng core. The details are in the patches. Turns out there were two bugs: the initial randomness was being fetched w/o the device being initialized in cases where the init callback was registered and the device wasn't the first device being added to the hwrng core (unrelated to the virtio-rng case). The second bug is virtio can't communicate with the host without the device probe is successfully completed. The impact of this change is that a sole virtio-rng device in a system won't be able to contribute to the initial randomness. If a second virtio-rng device is hot-plugged, its input will be used. Though that scenario is extremely unlikely (why have two virtio-rng devices at all?). This isn't any different from the current behaviour, though - virtio-rng currently doesn't contribute to initial system randomness, and continues not to do so. A better way to get virtio-rng to contribute initially to the system random pool can be worked out later. Please apply, Amit Shah (2): hwrng: fetch randomness only after device init virtio: rng: ensure reads happen after successful probe drivers/char/hw_random/core.c | 47 ++--- drivers/char/hw_random/virtio-rng.c | 10 2 files changed, 49 insertions(+), 8 deletions(-) -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
On (Thu) 10 Jul 2014 [16:45:14], Herbert Xu wrote: > On Wed, Jul 09, 2014 at 12:18:36PM -0400, Jason Cooper wrote: > > On Sat, Jul 05, 2014 at 11:04:53AM +0530, Amit Shah wrote: > > > The hwrng core asks for random data in the hwrng_register() call itself > > > from commit d9e7972619. This doesn't play well with virtio -- the > > > DRIVER_OK bit is only set by virtio core on a successful probe, and > > > we're not yet out of our probe routine when this call is made. This > > > causes the host to not acknowledge any requests we put in the virtqueue, > > > and the insmod or kernel boot process just waits for data to arrive from > > > the host, which never happens. > > > > > > CC: Kees Cook > > > CC: Jason Cooper > > > CC: Herbert Xu > > > CC: # For v3.15+ > > > Signed-off-by: Amit Shah > > > --- > > > drivers/char/hw_random/core.c | 6 ++ > > > drivers/char/hw_random/virtio-rng.c | 10 ++ > > > 2 files changed, 16 insertions(+) > > > > Yeah, I don't think there's any viable way to get random data out of > > virtio-rng at probe time... :-( > > > > Reviewed-by: Jason Cooper > > OK, if there are no more objections I will take these two patches. Please wait for the v3 -- I like that one better as it doesn't have the init weirdness that Kees pointed out. I'm sending that out in a few mins. Thanks! Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
