Re: [RFC PATCH 1/2] hw/nvme: add mi device

Re: [RFC PATCH 1/2] hw/nvme: add mi device

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2919 bytes --]

On Fri, Jul 09, 2021 at 07:25:45PM +0530, Padmakar Kalghatgi wrote:
> The enclosed patch contains the implementation of certain
> commands of nvme-mi specification.The MI commands are useful
> to manage/configure/monitor the device.Eventhough the MI commands
> can be sent via the inband NVMe-MI send/recieve commands, the idea here is
> to emulate the sideband interface for MI.
> 
> Since the nvme-mi specification deals in communicating
> to the nvme subsystem via. a sideband interface, in this
> qemu implementation, virtual-vsock is used for making the
> sideband communication, the guest VM needs to make the
> connection to the specific cid of the vsock of the qemu host.
> 
> One needs to specify the following command in the launch to
> specify the nvme-mi device, cid and to setup the vsock:
> -device nvme-mi,bus=<nvme bus number>
> -device vhost-vsock-pci, guest-cid=<vsock cid>
> 
> The following commands are tested with nvme-cli by hooking
> to the cid of the vsock as shown above and use the socket
> send/recieve commands to issue the commands and get the response.
> 
> we are planning to push the changes for nvme-cli as well to test the
> MI functionality.

Is the purpose of this feature (-device nvme-mi) testing MI with QEMU's
NVMe implementation?

My understanding is that instead of inventing an out-of-band interface
in the form of a new paravirtualized device, you decided to use vsock to
send MI commands from the guest to QEMU?

> As the connection can be established by the guest VM at any point,
> we have created a thread which is looking for a connection request.
> Please suggest if there is a native/better way to handle this.

QEMU has an event-driven architecture and uses threads sparingly. When
it uses threads it uses qemu_create_thread() instead of
pthread_create(), but I suggest using qemu_set_fd_handler() or a
coroutine with QIOChannel to integrate into the QEMU event loop instead.

I didn't see any thread synchronization, so I'm not sure if accessing
NVMe state from the MI thread is safe. Changing the code to use QEMU's
event loop can solve that problem since there's no separate thread.

> This module makes use of the NvmeCtrl structure of the nvme module,
> to fetch relevant information of the nvme device which are used in
> some of the mi commands. Eventhough certain commands might require
> modification to the nvme module, currently we have currently refrained
> from making changes to the nvme module.

Why did you decide to implement -device nvme-mi as a device on
TYPE_NVME_BUS? If the NVMe spec somehow requires this then I'm surprised
that there's no NVMe bus interface (callbacks). It seems like this could
just as easily be a property of an NVMe controller -device
nvme,mi=on|off or -device nvme-subsys,mi=on|off? I'm probably just not
familiar enough with MI and NVMe architecture...

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 1/2] hw/nvme: add mi device

[Christoph Hellwig]

On Mon, Jul 12, 2021 at 12:03:27PM +0100, Stefan Hajnoczi wrote:
> Why did you decide to implement -device nvme-mi as a device on
> TYPE_NVME_BUS? If the NVMe spec somehow requires this then I'm surprised
> that there's no NVMe bus interface (callbacks). It seems like this could
> just as easily be a property of an NVMe controller -device
> nvme,mi=on|off or -device nvme-subsys,mi=on|off? I'm probably just not
> familiar enough with MI and NVMe architecture...

I'm too far away from qemu these days to understand what TYPE_NVME_BUS
is.  Bt NVMe-MI has tree possible transports:

 1) out of band through smbus.  This seems something that could be
    trivially modelled in qemu
 2) out of band over MCTP / PCIe VDM.
 3) in band using NVMe admin commands that pass through MI commands

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 1/2] hw/nvme: add mi device

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2317 bytes --]

On Tue, Jul 13, 2021 at 06:30:28AM +0100, Christoph Hellwig wrote:
On Tue, Jul 13, 2021 at 06:30:28AM +0100, Christoph Hellwig wrote:
> On Mon, Jul 12, 2021 at 12:03:27PM +0100, Stefan Hajnoczi wrote:
> > Why did you decide to implement -device nvme-mi as a device on
> > TYPE_NVME_BUS? If the NVMe spec somehow requires this then I'm surprised
> > that there's no NVMe bus interface (callbacks). It seems like this could
> > just as easily be a property of an NVMe controller -device
> > nvme,mi=on|off or -device nvme-subsys,mi=on|off? I'm probably just not
> > familiar enough with MI and NVMe architecture...
> 
> I'm too far away from qemu these days to understand what TYPE_NVME_BUS
> is.  Bt NVMe-MI has tree possible transports:
> 
>  1) out of band through smbus.  This seems something that could be
>     trivially modelled in qemu
>  2) out of band over MCTP / PCIe VDM.
>  3) in band using NVMe admin commands that pass through MI commands

Thanks for explaining!

Common NVMe-MI code can be shared by -device nvme-mi-smbus, in-band NVMe
MI commands (part of -device nvme), a vsock transport, etc. This patch
has nvme_mi_admin_command() as the entry point to common MI code, so not
much needs to be done to achieve this.

My question about why -device nvme-mi was because this "device" doesn't
implement any bus interface (callbacks). The bus effectively just serves
as an owner of this device. The guest does not access the device via the
bus. So I'm not sure a -device is appropriate, it's an usual device.

If the device is kept, please name it -device nvme-mi-vsock so it's
clear this is the NVMe-MI vsock transport. I think the device could be
dropped and instead an -device nvme,mi-vsock=on|off property could be
added to enable the MI vsock transport on a specific NVMe controller.
This raises the question of whether the port number should be
configurable so multiple vsock Management Endpoints can coexist.

I don't have time to explore the architectural model, but here's the
link in case anyone wants to think through all the options for NVMe MI
Management Endpoints and how QEMU should model them:
"1.4 NVM Subsystem Architectural Model"
https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-1.2-2021.06.02-Ratified.pdf

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 1/2] hw/nvme: add mi device

[Padmakar Kalghatgi]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

On Tue, Jul 13, 2021 at 10:37:23AM +0100, Stefan Hajnoczi wrote:
>On Tue, Jul 13, 2021 at 06:30:28AM +0100, Christoph Hellwig wrote:
>On Tue, Jul 13, 2021 at 06:30:28AM +0100, Christoph Hellwig wrote:
>> On Mon, Jul 12, 2021 at 12:03:27PM +0100, Stefan Hajnoczi wrote:
>> > Why did you decide to implement -device nvme-mi as a device on
>> > TYPE_NVME_BUS? If the NVMe spec somehow requires this then I'm surprised
>> > that there's no NVMe bus interface (callbacks). It seems like this could
>> > just as easily be a property of an NVMe controller -device
>> > nvme,mi=on|off or -device nvme-subsys,mi=on|off? I'm probably just not
>> > familiar enough with MI and NVMe architecture...
>>
>> I'm too far away from qemu these days to understand what TYPE_NVME_BUS
>> is.  Bt NVMe-MI has tree possible transports:
>>
>>  1) out of band through smbus.  This seems something that could be
>>     trivially modelled in qemu
>>  2) out of band over MCTP / PCIe VDM.
>>  3) in band using NVMe admin commands that pass through MI commands
>
>Thanks for explaining!
>
>Common NVMe-MI code can be shared by -device nvme-mi-smbus, in-band NVMe
>MI commands (part of -device nvme), a vsock transport, etc. This patch
>has nvme_mi_admin_command() as the entry point to common MI code, so not
>much needs to be done to achieve this.
>
>My question about why -device nvme-mi was because this "device" doesn't
>implement any bus interface (callbacks). The bus effectively just serves
>as an owner of this device. The guest does not access the device via the
>bus. So I'm not sure a -device is appropriate, it's an usual device.
>
>If the device is kept, please name it -device nvme-mi-vsock so it's
>clear this is the NVMe-MI vsock transport. I think the device could be
>dropped and instead an -device nvme,mi-vsock=on|off property could be
>added to enable the MI vsock transport on a specific NVMe controller.
>This raises the question of whether the port number should be
>configurable so multiple vsock Management Endpoints can coexist.
>
>I don't have time to explore the architectural model, but here's the
>link in case anyone wants to think through all the options for NVMe MI
>Management Endpoints and how QEMU should model them:
>"1.4 NVM Subsystem Architectural Model"
>https://protect2.fireeye.com/v1/url?k=8ee99db1-ee0b00ec-8ee816fe-000babd9f1ba-c174da71c1d11e79&q=1&e=b7b9709a-33ac-4d98-a6c0-ff53377a3278&u=https%3A%2F%2Fnvmexpress.org%2Fwp-content%2Fuploads%2FNVM-Express-Management-Interface-1.2-2021.06.02-Ratified.pdf
>
>Stefan
Thanks Stefan for the suggestion.


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 1/2] hw/nvme: add mi device

[Padmakar Kalghatgi]

[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]

On Mon, Jul 12, 2021 at 12:03:27PM +0100, Stefan Hajnoczi wrote:
>On Fri, Jul 09, 2021 at 07:25:45PM +0530, Padmakar Kalghatgi wrote:
>> The enclosed patch contains the implementation of certain
>> commands of nvme-mi specification.The MI commands are useful
>> to manage/configure/monitor the device.Eventhough the MI commands
>> can be sent via the inband NVMe-MI send/recieve commands, the idea here is
>> to emulate the sideband interface for MI.
>>
>> Since the nvme-mi specification deals in communicating
>> to the nvme subsystem via. a sideband interface, in this
>> qemu implementation, virtual-vsock is used for making the
>> sideband communication, the guest VM needs to make the
>> connection to the specific cid of the vsock of the qemu host.
>>
>> One needs to specify the following command in the launch to
>> specify the nvme-mi device, cid and to setup the vsock:
>> -device nvme-mi,bus=<nvme bus number>
>> -device vhost-vsock-pci, guest-cid=<vsock cid>
>>
>> The following commands are tested with nvme-cli by hooking
>> to the cid of the vsock as shown above and use the socket
>> send/recieve commands to issue the commands and get the response.
>>
>> we are planning to push the changes for nvme-cli as well to test the
>> MI functionality.
>
>Is the purpose of this feature (-device nvme-mi) testing MI with QEMU's
>NVMe implementation?
>
>My understanding is that instead of inventing an out-of-band interface
>in the form of a new paravirtualized device, you decided to use vsock to
>send MI commands from the guest to QEMU?
>
>> As the connection can be established by the guest VM at any point,
>> we have created a thread which is looking for a connection request.
>> Please suggest if there is a native/better way to handle this.
>
>QEMU has an event-driven architecture and uses threads sparingly. When
>it uses threads it uses qemu_create_thread() instead of
>pthread_create(), but I suggest using qemu_set_fd_handler() or a
>coroutine with QIOChannel to integrate into the QEMU event loop instead.
>
>I didn't see any thread synchronization, so I'm not sure if accessing
>NVMe state from the MI thread is safe. Changing the code to use QEMU's
>event loop can solve that problem since there's no separate thread.
>
vsock mimcs the sideband communication hence we used it. 
However we are working the smbus/i2c implementation for nvme-mi in 
qemu/nvme-cli, we will send the patch in few days. to communicate with 
nvme-mi over smbus/i2c, nvme-mi device needs to inherit from the i2c class 
which has callbacks for sending and recieving messages, this approach 
would get rid of the threads.

>> This module makes use of the NvmeCtrl structure of the nvme module,
>> to fetch relevant information of the nvme device which are used in
>> some of the mi commands. Eventhough certain commands might require
>> modification to the nvme module, currently we have currently refrained
>> from making changes to the nvme module.
>
>Why did you decide to implement -device nvme-mi as a device on
>TYPE_NVME_BUS? If the NVMe spec somehow requires this then I'm surprised
>that there's no NVMe bus interface (callbacks). It seems like this could
>just as easily be a property of an NVMe controller -device
>nvme,mi=on|off or -device nvme-subsys,mi=on|off? I'm probably just not
>familiar enough with MI and NVMe architecture...
>
>Stefan
since nvme communication happens over pcie and nvme-mi happens over
smbus/i2c nvme-mi cannot be a property of nvme rather it should be a separate
device which will be on the smbus/i2c.



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme