On Wed, Sep 4, 2024 at 2:47 AM Dorjoy Chowdhury <dorjoychy...@gmail.com> wrote: > > > > On Wed, Sep 4, 2024, 2:32 AM Michael S. Tsirkin <m...@redhat.com> wrote: >> >> On Wed, Sep 04, 2024 at 01:58:15AM +0600, Dorjoy Chowdhury wrote: >> > On Thu, Aug 29, 2024 at 1:11 AM Michael S. Tsirkin <m...@redhat.com> wrote: >> > > >> > > On Thu, Aug 29, 2024 at 01:04:05AM +0600, Dorjoy Chowdhury wrote: >> > > > On Thu, Aug 29, 2024 at 12:28 AM Michael S. Tsirkin <m...@redhat.com> >> > > > wrote: >> > > > > >> > > > > On Thu, Aug 22, 2024 at 09:08:46PM +0600, Dorjoy Chowdhury wrote: >> > > > > > Nitro Secure Module (NSM)[1] device is used in AWS Nitro >> > > > > > Enclaves[2] >> > > > > > for stripped down TPM functionality like cryptographic attestation. >> > > > > > The requests to and responses from NSM device are CBOR[3] encoded. >> > > > > > >> > > > > > This commit adds support for NSM device in QEMU. Although related >> > > > > > to >> > > > > > AWS Nitro Enclaves, the virito-nsm device is independent and can be >> > > > > > used in other machine types as well. The libcbor[4] library has >> > > > > > been >> > > > > > used for the CBOR encoding and decoding functionalities. >> > > > > > >> > > > > > [1] >> > > > > > https://lists.oasis-open.org/archives/virtio-comment/202310/msg00387.html >> > > > > > [2] >> > > > > > https://docs.aws.amazon.com/enclaves/latest/user/nitro-enclave.html >> > > > > > [3] http://cbor.io/ >> > > > > > [4] https://libcbor.readthedocs.io/en/latest/ >> > > > > > >> > > > > > Signed-off-by: Dorjoy Chowdhury <dorjoychy...@gmail.com> >> > > > > > --- >> > > > > > MAINTAINERS | 10 + >> > > > > > hw/virtio/Kconfig | 5 + >> > > > > > hw/virtio/cbor-helpers.c | 326 ++++++ >> > > > > > hw/virtio/meson.build | 6 + >> > > > > > hw/virtio/virtio-nsm-pci.c | 73 ++ >> > > > > > hw/virtio/virtio-nsm.c | 1638 >> > > > > > ++++++++++++++++++++++++++++++ >> > > > > > include/hw/virtio/cbor-helpers.h | 46 + >> > > > > > include/hw/virtio/virtio-nsm.h | 59 ++ >> > > > > > meson.build | 2 + >> > > > > > 9 files changed, 2165 insertions(+) >> > > > >> > > > [...] >> > > > >> > > > > > +static void handle_input(VirtIODevice *vdev, VirtQueue *vq) >> > > > > > +{ >> > > > > > + g_autofree VirtQueueElement *out_elem = NULL; >> > > > > > + g_autofree VirtQueueElement *in_elem = NULL; >> > > > > > + VirtIONSM *vnsm = VIRTIO_NSM(vdev); >> > > > > > + Error *err = NULL; >> > > > > > + >> > > > > > + out_elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); >> > > > > > + if (!out_elem) { >> > > > > > + /* nothing in virtqueue */ >> > > > > > + return; >> > > > > > + } >> > > > > > + >> > > > > > + if (out_elem->out_num != 1) { >> > > > > > + virtio_error(vdev, "Expected one request buffer first in >> > > > > > virtqueue"); >> > > > > > + goto cleanup; >> > > > > > + } >> > > > > >> > > > > Seems to assume request in a single s/g element? >> > > > > We generally avoid this kind of thing. >> > > > > >> > > > > Applies equally elsewheree. >> > > > > >> > > > >> > > > Thank you for reviewing. I think I did it this way (first virqueue_pop >> > > > gives out_elem with out_num == 1 and the next virtqueue_pop gives >> > > > in_elem with in_num == 1) after seeing what the virqueue contains >> > > > (using printfs) when running in a VM and sending some NSM requests and >> > > > I noticed the above. Can you give me a bit more details about what >> > > > this should be like? Is there any existing virtio device code I can >> > > > look at for example? >> > > > Thanks! >> > > >> > > >> > > Use iov_to_buf / iov_from_buf >> > > >> > > there are many examples in the tree, I'd look for some recent ones. >> > > >> > >> > I am a bit stuck at this and I would appreciate some help. I looked at >> > other "iov_to_buf" and "iov_from_buf" examples in QEMU and in those I >> > see there are known request and response "structs" associated with it. >> > But in the case of NSM, the request and responses can be arbitrary >> > CBOR objects i.e., no specific structs or lengths associated. >> >> >> take whatever you want to access, move it to a buffer with iov_to_buf >> then access the buffer. >> >> reverse is even easier. put in a buffer, copy with iov_from_buf. > > > I guess I will just need to copy the iov buffer (whatever the length was in > the out_elem's out buf) to another buffer using iov_to_buf and then pass it > to the processing function and then copy the response to the in_elem's buffer > using iov_from_buf, right? Wouldn't the copying be redundant in this case as > we could just instead pass the original buffers (like the iov-s are passed > right now) to the processing function? > >> >> > So I am >> > not sure using "iov_to_buf" / "iov_from_buf" makes sense here. >> > And about the request response being in a single s/g element, I think >> > it's because of how the NSM driver is in drivers/misc/nsm.c (see >> > nsm_sendrecv_msg_locked function)in the linux kernel tree. >> >> yes but driver is free to change this. >> Isn't there a spec for this device to consult? >> Sending that to virtio tc will be needed before we add this to qemu. > > > I think this is the spec for this device (also mentioned in the commit > message of this patch) > https://lists.oasis-open.org/archives/virtio-comment/202310/msg00387.html >
Hi Michael. Did you get a chance to look at the NSM device spec above? I am not sure but from the description there I think the request response being in a single s/g element makes sense, right? So the current implementation of first checking out_elem with out_num == 1 and then an in_elem with in_num == 1 should be correct. Please correct me if I am wrong here and if I should change the implementation to something else. Also I had another look into using iov_to_buf and iov_from_buf. If I wanted to use iov_to_buf here, I would just be copying the out_elem->out_sg->iov_base to another buffer (by malloc-ing the same length) and then passing it to the processing function (get_nsm_request_response). And if I wanted to use the iov_from_buf then I would probably just make another buffer the same size of in_elem->in_sg->iov_base and then pass it to the processing function (get_nsm_request_response). The function tries to put the response CBOR object in the response buffer but if it is too small, it then tries to put the error response BufferTooSmall if it fails then it returns error. I don't see how using iov_to_buf and iov_from_buf makes any difference here other than passing in the original iov structs to the processing function instead. Seems like doing so would just be doing some unnecessary copying. Please let me know what you think so that I can better understand this. Sorry for the back and forth a bit on this one. Thanks. Regards, Dorjoy
