On 11.10.23 19:46, Greg Kroah-Hartman wrote:
On Wed, Oct 11, 2023 at 02:24:48PM +0200, Arnd Bergmann wrote:
On Tue, Oct 10, 2023, at 10:27, Greg Kroah-Hartman wrote:
On Tue, Oct 10, 2023 at 10:08:43AM +0200, Alexander Graf wrote:
On 10.10.23 10:03, Greg Kroah-Hartman wrote:
Out of these, the NSM communication protocol uses all except Semantic tags
and Floats. The CBOR library that this patch imports does not have special
handling for Semantic tags, which leaves only floats which are already
#ifdef'ed out. That means there is not much to trim.
What you see here is what's needed to parse CBOR in kernel - if that's what
we want to do. I'm happy to rip it out again and make it a pure user space
problem to do CBOR :).
Yes, why are we parsing this in the kernel? What could go wrong with
adding yet-another-parser in privileged context? :)
Why does this have to be in the kernel, the data sent/recieved is over
virtio, so why does the kernel have to parse it? I couldn't figure that
out from the driver, yet the driver seems to have a lot of hard-coded
parsing logic in it to assume specific message formats?
The parsing doesn't have to be in kernel and it probably shouldn't be
either. V3 of the patch was punting all the parsing to user space, at which
point you and Arnd said I should give it a try to do the protocol parsing in
kernel space instead. That's why the parser is here.
Arnd said that, not me :)
If we conclude that all this in-kernel parsing is not worth it, I'm very
happy to just go back to the the v3 ioctl interface and post v5 with hwrng
merged into misc, but remove all CBOR logic again :)
I think the less parsers we have in the kernel, the safer we are for
obvious reasons. Unless you have a parser for this in rust? :)
I don't really know, having a generic interface is good, but at the
expense of this api is probably not good. individual ioctls might be
better if there are not going to be any other drivers for this type of
thing?
I was definitely expecting something simpler than what was possible
in the v4 patch. I had another look now, and it's clear that the
ioctl interface is still not great because the variable data structures
shine through for some of the calls, and even to get to this point,
a whole lot of complexity is required underneath.
To get anything better, one would probably have to redesign the entire
interface stack (hypervisor, kernel and userland) to use regular
fixed data structures, and this seems unlikely to happen.
Why not fix this and do it properly? What's preventing that from
happening? We don't want to create an interface here that is broken, or
insecure, or a pain to maintain, right?
The interface is neither broken, insecure nor a pain to maintain. It's
merely not as pretty as could be. But it's there to stay because it's
what the hypervisor has been exposing for the last 2 years.
I also frankly don't think that doing static structures is any more
"proper" than what this interface is doing today. It's serialized data
of variable length. That's just the nature of the beast - the backing
data for some of these operations *is* dynamic.
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
