Re: [PATCH v6 0/3] hw/{i2c,nvme}: mctp endpoint, nvme management interface model
On Wed, Sep 20, 2023 at 06:31:25AM -0700, Klaus Jensen wrote: > On Sep 20 07:54, Corey Minyard wrote: > > On Wed, Sep 20, 2023 at 12:48:03PM +0100, Jonathan Cameron via wrote: > > > On Thu, 14 Sep 2023 11:53:40 +0200 > > > Klaus Jensen wrote: > > > > > > > This adds a generic MCTP endpoint model that other devices may derive > > > > from. > > > > > > > > Also included is a very basic implementation of an NVMe-MI device, > > > > supporting only a small subset of the required commands. > > > > > > > > Since this all relies on i2c target mode, this can currently only be > > > > used with an SoC that includes the Aspeed I2C controller. > > > > > > > > The easiest way to get up and running with this, is to grab my buildroot > > > > overlay[1] (aspeed_ast2600evb_nmi_defconfig). It includes modified a > > > > modified dts as well as a couple of required packages. > > > > > > > > QEMU can then be launched along these lines: > > > > > > > > qemu-system-arm \ > > > > -nographic \ > > > > -M ast2600-evb \ > > > > -kernel output/images/zImage \ > > > > -initrd output/images/rootfs.cpio \ > > > > -dtb output/images/aspeed-ast2600-evb-nmi.dtb \ > > > > -nic user,hostfwd=tcp::-:22 \ > > > > -device nmi-i2c,address=0x3a \ > > > > -serial mon:stdio > > > > > > > > From within the booted system, > > > > > > > > mctp addr add 8 dev mctpi2c15 > > > > mctp link set mctpi2c15 up > > > > mctp route add 9 via mctpi2c15 > > > > mctp neigh add 9 dev mctpi2c15 lladdr 0x3a > > > > mi-mctp 1 9 info > > > > > > > > Comments are very welcome! > > > > > > > > [1]: https://github.com/birkelund/hwtests/tree/main/br2-external > > > > > > > > Signed-off-by: Klaus Jensen > > > > > > Hi Klaus, > > > > > > Silly question, but who is likely to pick this up? + likely to be soon? > > > > > > I'm going to post the CXL stuff that makes use of the core support shortly > > > and whilst I can point at this patch set on list, I'd keen to see it > > > upstream > > > to reduce the dependencies (it's got 2 sets ahead of it of CXL stuff > > > anyway but that will all hopefully go through Michael Tsirkin's tree > > > for PCI stuff in one go). > > > > I can pick it up, but he can just request a merge, too. > > > > I did have a question I asked earlier about tests. It would be unusual > > at this point to add something like this without having some tests, > > especially injecting invalid data. > > > > Hi all, > > Sorry for the late reply. I'm currently at SDC, but I will write up some > tests when I get back to in the office on Monday. > > Corey, what kinds of tests would be best here? Avocado "acceptance" > tests or would you like to see something lower level? My main concern is testing what happens when bad data gets injected, to avoid people coming up with clever names for exploits in qemu. It's not so much for this code, it's for the changes that comes in the future. And, of course, normal functional tests to make sure it works. What a friend of mine calls "dead chicken" tests. You wave a dead chicken at it, and if the chicken is still dead everything is ok :). I'm fine with either type of tests, but I'm not sure you can do this with avocado. It's probably about the same amount of work either path you choose. -corey
Re: [PATCH v6 0/3] hw/{i2c,nvme}: mctp endpoint, nvme management interface model
On Wed, Sep 20, 2023 at 12:48:03PM +0100, Jonathan Cameron via wrote: > On Thu, 14 Sep 2023 11:53:40 +0200 > Klaus Jensen wrote: > > > This adds a generic MCTP endpoint model that other devices may derive > > from. > > > > Also included is a very basic implementation of an NVMe-MI device, > > supporting only a small subset of the required commands. > > > > Since this all relies on i2c target mode, this can currently only be > > used with an SoC that includes the Aspeed I2C controller. > > > > The easiest way to get up and running with this, is to grab my buildroot > > overlay[1] (aspeed_ast2600evb_nmi_defconfig). It includes modified a > > modified dts as well as a couple of required packages. > > > > QEMU can then be launched along these lines: > > > > qemu-system-arm \ > > -nographic \ > > -M ast2600-evb \ > > -kernel output/images/zImage \ > > -initrd output/images/rootfs.cpio \ > > -dtb output/images/aspeed-ast2600-evb-nmi.dtb \ > > -nic user,hostfwd=tcp::-:22 \ > > -device nmi-i2c,address=0x3a \ > > -serial mon:stdio > > > > From within the booted system, > > > > mctp addr add 8 dev mctpi2c15 > > mctp link set mctpi2c15 up > > mctp route add 9 via mctpi2c15 > > mctp neigh add 9 dev mctpi2c15 lladdr 0x3a > > mi-mctp 1 9 info > > > > Comments are very welcome! > > > > [1]: https://github.com/birkelund/hwtests/tree/main/br2-external > > > > Signed-off-by: Klaus Jensen > > Hi Klaus, > > Silly question, but who is likely to pick this up? + likely to be soon? > > I'm going to post the CXL stuff that makes use of the core support shortly > and whilst I can point at this patch set on list, I'd keen to see it upstream > to reduce the dependencies (it's got 2 sets ahead of it of CXL stuff > anyway but that will all hopefully go through Michael Tsirkin's tree > for PCI stuff in one go). I can pick it up, but he can just request a merge, too. I did have a question I asked earlier about tests. It would be unusual at this point to add something like this without having some tests, especially injecting invalid data. -corey > > Jonathan > > > --- > > Changes in v6: > > - Use nmi_scratch_append() directly where it makes sense. Fixes bug > > observed by Andrew. > > - Link to v5: > > https://lore.kernel.org/r/20230905-nmi-i2c-v5-0-0001d372a...@samsung.com > > > > Changes in v5: > > - Added a nmi_scratch_append() that asserts available space in the > > scratch buffer. This is a similar defensive strategy as used in > > hw/i2c/mctp.c > > - Various small fixups in response to review (Jonathan) > > - Link to v4: > > https://lore.kernel.org/r/20230823-nmi-i2c-v4-0-2b0f86e5b...@samsung.com > > > > --- > > Klaus Jensen (3): > > hw/i2c: add smbus pec utility function > > hw/i2c: add mctp core > > hw/nvme: add nvme management interface model > > > > MAINTAINERS | 7 + > > hw/arm/Kconfig| 1 + > > hw/i2c/Kconfig| 4 + > > hw/i2c/mctp.c | 432 > > ++ > > hw/i2c/meson.build| 1 + > > hw/i2c/smbus_master.c | 26 +++ > > hw/i2c/trace-events | 13 ++ > > hw/nvme/Kconfig | 4 + > > hw/nvme/meson.build | 1 + > > hw/nvme/nmi-i2c.c | 407 +++ > > hw/nvme/trace-events | 6 + > > include/hw/i2c/mctp.h | 125 > > include/hw/i2c/smbus_master.h | 2 + > > include/net/mctp.h| 35 > > 14 files changed, 1064 insertions(+) > > --- > > base-commit: 005ad32358f12fe9313a4a01918a55e60d4f39e5 > > change-id: 20230822-nmi-i2c-d804ed5be7e6 > > > > Best regards, > >
Re: [PATCH v6 0/3] hw/{i2c,nvme}: mctp endpoint, nvme management interface model
On Sep 20 07:54, Corey Minyard wrote: > On Wed, Sep 20, 2023 at 12:48:03PM +0100, Jonathan Cameron via wrote: > > On Thu, 14 Sep 2023 11:53:40 +0200 > > Klaus Jensen wrote: > > > > > This adds a generic MCTP endpoint model that other devices may derive > > > from. > > > > > > Also included is a very basic implementation of an NVMe-MI device, > > > supporting only a small subset of the required commands. > > > > > > Since this all relies on i2c target mode, this can currently only be > > > used with an SoC that includes the Aspeed I2C controller. > > > > > > The easiest way to get up and running with this, is to grab my buildroot > > > overlay[1] (aspeed_ast2600evb_nmi_defconfig). It includes modified a > > > modified dts as well as a couple of required packages. > > > > > > QEMU can then be launched along these lines: > > > > > > qemu-system-arm \ > > > -nographic \ > > > -M ast2600-evb \ > > > -kernel output/images/zImage \ > > > -initrd output/images/rootfs.cpio \ > > > -dtb output/images/aspeed-ast2600-evb-nmi.dtb \ > > > -nic user,hostfwd=tcp::-:22 \ > > > -device nmi-i2c,address=0x3a \ > > > -serial mon:stdio > > > > > > From within the booted system, > > > > > > mctp addr add 8 dev mctpi2c15 > > > mctp link set mctpi2c15 up > > > mctp route add 9 via mctpi2c15 > > > mctp neigh add 9 dev mctpi2c15 lladdr 0x3a > > > mi-mctp 1 9 info > > > > > > Comments are very welcome! > > > > > > [1]: https://github.com/birkelund/hwtests/tree/main/br2-external > > > > > > Signed-off-by: Klaus Jensen > > > > Hi Klaus, > > > > Silly question, but who is likely to pick this up? + likely to be soon? > > > > I'm going to post the CXL stuff that makes use of the core support shortly > > and whilst I can point at this patch set on list, I'd keen to see it > > upstream > > to reduce the dependencies (it's got 2 sets ahead of it of CXL stuff > > anyway but that will all hopefully go through Michael Tsirkin's tree > > for PCI stuff in one go). > > I can pick it up, but he can just request a merge, too. > > I did have a question I asked earlier about tests. It would be unusual > at this point to add something like this without having some tests, > especially injecting invalid data. > Hi all, Sorry for the late reply. I'm currently at SDC, but I will write up some tests when I get back to in the office on Monday. Corey, what kinds of tests would be best here? Avocado "acceptance" tests or would you like to see something lower level? Cheers, Klaus signature.asc Description: PGP signature
Re: [PATCH v6 0/3] hw/{i2c,nvme}: mctp endpoint, nvme management interface model
On Thu, 14 Sep 2023 11:53:40 +0200 Klaus Jensen wrote: > This adds a generic MCTP endpoint model that other devices may derive > from. > > Also included is a very basic implementation of an NVMe-MI device, > supporting only a small subset of the required commands. > > Since this all relies on i2c target mode, this can currently only be > used with an SoC that includes the Aspeed I2C controller. > > The easiest way to get up and running with this, is to grab my buildroot > overlay[1] (aspeed_ast2600evb_nmi_defconfig). It includes modified a > modified dts as well as a couple of required packages. > > QEMU can then be launched along these lines: > > qemu-system-arm \ > -nographic \ > -M ast2600-evb \ > -kernel output/images/zImage \ > -initrd output/images/rootfs.cpio \ > -dtb output/images/aspeed-ast2600-evb-nmi.dtb \ > -nic user,hostfwd=tcp::-:22 \ > -device nmi-i2c,address=0x3a \ > -serial mon:stdio > > From within the booted system, > > mctp addr add 8 dev mctpi2c15 > mctp link set mctpi2c15 up > mctp route add 9 via mctpi2c15 > mctp neigh add 9 dev mctpi2c15 lladdr 0x3a > mi-mctp 1 9 info > > Comments are very welcome! > > [1]: https://github.com/birkelund/hwtests/tree/main/br2-external > > Signed-off-by: Klaus Jensen Hi Klaus, Silly question, but who is likely to pick this up? + likely to be soon? I'm going to post the CXL stuff that makes use of the core support shortly and whilst I can point at this patch set on list, I'd keen to see it upstream to reduce the dependencies (it's got 2 sets ahead of it of CXL stuff anyway but that will all hopefully go through Michael Tsirkin's tree for PCI stuff in one go). Jonathan > --- > Changes in v6: > - Use nmi_scratch_append() directly where it makes sense. Fixes bug > observed by Andrew. > - Link to v5: > https://lore.kernel.org/r/20230905-nmi-i2c-v5-0-0001d372a...@samsung.com > > Changes in v5: > - Added a nmi_scratch_append() that asserts available space in the > scratch buffer. This is a similar defensive strategy as used in > hw/i2c/mctp.c > - Various small fixups in response to review (Jonathan) > - Link to v4: > https://lore.kernel.org/r/20230823-nmi-i2c-v4-0-2b0f86e5b...@samsung.com > > --- > Klaus Jensen (3): > hw/i2c: add smbus pec utility function > hw/i2c: add mctp core > hw/nvme: add nvme management interface model > > MAINTAINERS | 7 + > hw/arm/Kconfig| 1 + > hw/i2c/Kconfig| 4 + > hw/i2c/mctp.c | 432 > ++ > hw/i2c/meson.build| 1 + > hw/i2c/smbus_master.c | 26 +++ > hw/i2c/trace-events | 13 ++ > hw/nvme/Kconfig | 4 + > hw/nvme/meson.build | 1 + > hw/nvme/nmi-i2c.c | 407 +++ > hw/nvme/trace-events | 6 + > include/hw/i2c/mctp.h | 125 > include/hw/i2c/smbus_master.h | 2 + > include/net/mctp.h| 35 > 14 files changed, 1064 insertions(+) > --- > base-commit: 005ad32358f12fe9313a4a01918a55e60d4f39e5 > change-id: 20230822-nmi-i2c-d804ed5be7e6 > > Best regards,
