* [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-16 8:43 [PATCH 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
@ 2022-11-16 8:43 ` Klaus Jensen
2022-11-16 13:16 ` Peter Maydell
` (2 more replies)
2022-11-16 8:43 ` [PATCH RFC 2/3] hw/i2c: add mctp core Klaus Jensen
` (2 subsequent siblings)
3 siblings, 3 replies; 26+ messages in thread
From: Klaus Jensen @ 2022-11-16 8:43 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Jeremy Kerr, Joel Stanley,
Cédric Le Goater, Klaus Jensen, Klaus Jensen
From: Klaus Jensen <k.jensen@samsung.com>
It is not given that the current master will release the bus after a
transfer ends. Only schedule a pending master if the bus is idle.
Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/i2c/aspeed_i2c.c | 2 ++
hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
include/hw/i2c/i2c.h | 2 ++
3 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c166fd20fa11..1f071a3811f7 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
}
SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
aspeed_i2c_set_state(bus, I2CD_IDLE);
+
+ i2c_schedule_pending_master(bus->bus);
}
if (aspeed_i2c_bus_pkt_mode_en(bus)) {
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d4ba8146bffb..bed594fe599b 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
{
+ I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+ node->bh = bh;
+
+ QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+}
+
+void i2c_schedule_pending_master(I2CBus *bus)
+{
+ I2CPendingMaster *node;
+
if (i2c_bus_busy(bus)) {
- I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
- node->bh = bh;
-
- QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+ /* someone is already controlling the bus; wait for it to release it */
+ return;
+ }
+ if (QSIMPLEQ_EMPTY(&bus->pending_masters)) {
return;
}
- bus->bh = bh;
+ node = QSIMPLEQ_FIRST(&bus->pending_masters);
+ bus->bh = node->bh;
+
+ QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
+ g_free(node);
+
qemu_bh_schedule(bus->bh);
}
void i2c_bus_release(I2CBus *bus)
{
bus->bh = NULL;
+
+ i2c_schedule_pending_master(bus);
}
int i2c_start_recv(I2CBus *bus, uint8_t address)
@@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
g_free(node);
}
bus->broadcast = false;
-
- if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
- I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
- bus->bh = node->bh;
-
- QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
- g_free(node);
-
- qemu_bh_schedule(bus->bh);
- }
}
int i2c_send(I2CBus *bus, uint8_t data)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b9581d23097..2a3abacd1ba6 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
*/
int i2c_start_send_async(I2CBus *bus, uint8_t address);
+void i2c_schedule_pending_master(I2CBus *bus);
+
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
void i2c_ack(I2CBus *bus);
--
2.38.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-16 8:43 ` [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle Klaus Jensen
@ 2022-11-16 13:16 ` Peter Maydell
2022-11-16 13:43 ` Corey Minyard
2022-11-16 15:58 ` Cédric Le Goater
2 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2022-11-16 13:16 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, qemu-block, Jeremy Kerr,
Joel Stanley, Cédric Le Goater, Klaus Jensen
On Wed, 16 Nov 2022 at 08:43, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> It is not given that the current master will release the bus after a
> transfer ends. Only schedule a pending master if the bus is idle.
>
> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
If this is a bug fix we should consider for 7.2, you should
send it as a separate patch with a commit message that
describes the consequences of the bug (e.g. whether it
affects common workloads or if it's just an odd corner case).
As one patch in an otherwise RFC series it's going to get lost
otherwise.
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-16 8:43 ` [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle Klaus Jensen
2022-11-16 13:16 ` Peter Maydell
@ 2022-11-16 13:43 ` Corey Minyard
2022-11-16 15:58 ` Cédric Le Goater
2 siblings, 0 replies; 26+ messages in thread
From: Corey Minyard @ 2022-11-16 13:43 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Cédric Le Goater, Klaus Jensen
On Wed, Nov 16, 2022 at 09:43:10AM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> It is not given that the current master will release the bus after a
> transfer ends. Only schedule a pending master if the bus is idle.
>
Yes, I think this is correct.
Acked-by: Corey Minyard <cminyard@mvista.com>
Is there a reason you are thinking this is needed for 7.2? There's no
code in qemu proper that uses this yet. I had assumed that was coming
soon after the patch.
-corey
> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/i2c/aspeed_i2c.c | 2 ++
> hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> include/hw/i2c/i2c.h | 2 ++
> 3 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c166fd20fa11..1f071a3811f7 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> }
> SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> aspeed_i2c_set_state(bus, I2CD_IDLE);
> +
> + i2c_schedule_pending_master(bus->bus);
> }
>
> if (aspeed_i2c_bus_pkt_mode_en(bus)) {
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index d4ba8146bffb..bed594fe599b 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
>
> void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
> {
> + I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
> + node->bh = bh;
> +
> + QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
> +}
> +
> +void i2c_schedule_pending_master(I2CBus *bus)
> +{
> + I2CPendingMaster *node;
> +
> if (i2c_bus_busy(bus)) {
> - I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
> - node->bh = bh;
> -
> - QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
> + /* someone is already controlling the bus; wait for it to release it */
> + return;
> + }
>
> + if (QSIMPLEQ_EMPTY(&bus->pending_masters)) {
> return;
> }
>
> - bus->bh = bh;
> + node = QSIMPLEQ_FIRST(&bus->pending_masters);
> + bus->bh = node->bh;
> +
> + QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
> + g_free(node);
> +
> qemu_bh_schedule(bus->bh);
> }
>
> void i2c_bus_release(I2CBus *bus)
> {
> bus->bh = NULL;
> +
> + i2c_schedule_pending_master(bus);
> }
>
> int i2c_start_recv(I2CBus *bus, uint8_t address)
> @@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
> g_free(node);
> }
> bus->broadcast = false;
> -
> - if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
> - I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
> - bus->bh = node->bh;
> -
> - QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
> - g_free(node);
> -
> - qemu_bh_schedule(bus->bh);
> - }
> }
>
> int i2c_send(I2CBus *bus, uint8_t data)
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 9b9581d23097..2a3abacd1ba6 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
> */
> int i2c_start_send_async(I2CBus *bus, uint8_t address);
>
> +void i2c_schedule_pending_master(I2CBus *bus);
> +
> void i2c_end_transfer(I2CBus *bus);
> void i2c_nack(I2CBus *bus);
> void i2c_ack(I2CBus *bus);
> --
> 2.38.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-16 8:43 ` [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle Klaus Jensen
2022-11-16 13:16 ` Peter Maydell
2022-11-16 13:43 ` Corey Minyard
@ 2022-11-16 15:58 ` Cédric Le Goater
2022-11-17 6:40 ` Klaus Jensen
2 siblings, 1 reply; 26+ messages in thread
From: Cédric Le Goater @ 2022-11-16 15:58 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Jeremy Kerr, Joel Stanley,
Klaus Jensen
On 11/16/22 09:43, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> It is not given that the current master will release the bus after a
> transfer ends. Only schedule a pending master if the bus is idle.
>
> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/i2c/aspeed_i2c.c | 2 ++
> hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> include/hw/i2c/i2c.h | 2 ++
> 3 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c166fd20fa11..1f071a3811f7 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> }
> SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> aspeed_i2c_set_state(bus, I2CD_IDLE);
> +
> + i2c_schedule_pending_master(bus->bus);
Shouldn't it be i2c_bus_release() ?
Thanks,
C.
> }
>
> if (aspeed_i2c_bus_pkt_mode_en(bus)) {
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index d4ba8146bffb..bed594fe599b 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
>
> void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
> {
> + I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
> + node->bh = bh;
> +
> + QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
> +}
> +
> +void i2c_schedule_pending_master(I2CBus *bus)
> +{
> + I2CPendingMaster *node;
> +
> if (i2c_bus_busy(bus)) {
> - I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
> - node->bh = bh;
> -
> - QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
> + /* someone is already controlling the bus; wait for it to release it */
> + return;
> + }
>
> + if (QSIMPLEQ_EMPTY(&bus->pending_masters)) {
> return;
> }
>
> - bus->bh = bh;
> + node = QSIMPLEQ_FIRST(&bus->pending_masters);
> + bus->bh = node->bh;
> +
> + QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
> + g_free(node);
> +
> qemu_bh_schedule(bus->bh);
> }
>
> void i2c_bus_release(I2CBus *bus)
> {
> bus->bh = NULL;
> +
> + i2c_schedule_pending_master(bus);
> }
>
> int i2c_start_recv(I2CBus *bus, uint8_t address)
> @@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
> g_free(node);
> }
> bus->broadcast = false;
> -
> - if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
> - I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
> - bus->bh = node->bh;
> -
> - QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
> - g_free(node);
> -
> - qemu_bh_schedule(bus->bh);
> - }
> }
>
> int i2c_send(I2CBus *bus, uint8_t data)
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 9b9581d23097..2a3abacd1ba6 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
> */
> int i2c_start_send_async(I2CBus *bus, uint8_t address);
>
> +void i2c_schedule_pending_master(I2CBus *bus);
> +
> void i2c_end_transfer(I2CBus *bus);
> void i2c_nack(I2CBus *bus);
> void i2c_ack(I2CBus *bus);
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-16 15:58 ` Cédric Le Goater
@ 2022-11-17 6:40 ` Klaus Jensen
2022-11-17 6:56 ` Cédric Le Goater
0 siblings, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2022-11-17 6:40 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]
On Nov 16 16:58, Cédric Le Goater wrote:
> On 11/16/22 09:43, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > It is not given that the current master will release the bus after a
> > transfer ends. Only schedule a pending master if the bus is idle.
> >
> > Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> > hw/i2c/aspeed_i2c.c | 2 ++
> > hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> > include/hw/i2c/i2c.h | 2 ++
> > 3 files changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > index c166fd20fa11..1f071a3811f7 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > }
> > SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> > aspeed_i2c_set_state(bus, I2CD_IDLE);
> > +
> > + i2c_schedule_pending_master(bus->bus);
>
> Shouldn't it be i2c_bus_release() ?
>
The reason for having both i2c_bus_release() and
i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
with i2c_bus_master(). They either set or clear the bus->bh member.
In the current design, the controller (in this case the Aspeed I2C) is
an "implicit" master (it does not have a bottom half driving it), so
there is no bus->bh to clear.
I should (and will) write some documentation on the asynchronous API.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 6:40 ` Klaus Jensen
@ 2022-11-17 6:56 ` Cédric Le Goater
2022-11-17 7:37 ` Klaus Jensen
0 siblings, 1 reply; 26+ messages in thread
From: Cédric Le Goater @ 2022-11-17 6:56 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
On 11/17/22 07:40, Klaus Jensen wrote:
> On Nov 16 16:58, Cédric Le Goater wrote:
>> On 11/16/22 09:43, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> It is not given that the current master will release the bus after a
>>> transfer ends. Only schedule a pending master if the bus is idle.
>>>
>>> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>> hw/i2c/aspeed_i2c.c | 2 ++
>>> hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
>>> include/hw/i2c/i2c.h | 2 ++
>>> 3 files changed, 26 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index c166fd20fa11..1f071a3811f7 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>> }
>>> SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
>>> aspeed_i2c_set_state(bus, I2CD_IDLE);
>>> +
>>> + i2c_schedule_pending_master(bus->bus);
>>
>> Shouldn't it be i2c_bus_release() ?
>>
>
> The reason for having both i2c_bus_release() and
> i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
> with i2c_bus_master(). They either set or clear the bus->bh member.
>
> In the current design, the controller (in this case the Aspeed I2C) is
> an "implicit" master (it does not have a bottom half driving it), so
> there is no bus->bh to clear.
>
> I should (and will) write some documentation on the asynchronous API.
I found the routine names confusing. Thanks for the clarification.
Maybe we could do this rename :
i2c_bus_release() -> i2c_bus_release_and_clear()
i2c_schedule_pending_master() -> i2c_bus_release()
and keep i2c_schedule_pending_master() internal the I2C core subsystem.
C.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 6:56 ` Cédric Le Goater
@ 2022-11-17 7:37 ` Klaus Jensen
2022-11-17 8:01 ` Cédric Le Goater
0 siblings, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2022-11-17 7:37 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]
On Nov 17 07:56, Cédric Le Goater wrote:
> On 11/17/22 07:40, Klaus Jensen wrote:
> > On Nov 16 16:58, Cédric Le Goater wrote:
> > > On 11/16/22 09:43, Klaus Jensen wrote:
> > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > >
> > > > It is not given that the current master will release the bus after a
> > > > transfer ends. Only schedule a pending master if the bus is idle.
> > > >
> > > > Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > ---
> > > > hw/i2c/aspeed_i2c.c | 2 ++
> > > > hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> > > > include/hw/i2c/i2c.h | 2 ++
> > > > 3 files changed, 26 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > > > index c166fd20fa11..1f071a3811f7 100644
> > > > --- a/hw/i2c/aspeed_i2c.c
> > > > +++ b/hw/i2c/aspeed_i2c.c
> > > > @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > > > }
> > > > SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> > > > aspeed_i2c_set_state(bus, I2CD_IDLE);
> > > > +
> > > > + i2c_schedule_pending_master(bus->bus);
> > >
> > > Shouldn't it be i2c_bus_release() ?
> > >
> >
> > The reason for having both i2c_bus_release() and
> > i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
> > with i2c_bus_master(). They either set or clear the bus->bh member.
> >
> > In the current design, the controller (in this case the Aspeed I2C) is
> > an "implicit" master (it does not have a bottom half driving it), so
> > there is no bus->bh to clear.
> >
> > I should (and will) write some documentation on the asynchronous API.
>
> I found the routine names confusing. Thanks for the clarification.
>
> Maybe we could do this rename :
>
> i2c_bus_release() -> i2c_bus_release_and_clear()
> i2c_schedule_pending_master() -> i2c_bus_release()
>
> and keep i2c_schedule_pending_master() internal the I2C core subsystem.
>
How about renaming i2c_bus_master to i2c_bus_acquire() such that it
pairs with i2c_bus_release().
And then add an i2c_bus_yield() to be used by the controller? I think we
should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
I'll take a closer look at that.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 7:37 ` Klaus Jensen
@ 2022-11-17 8:01 ` Cédric Le Goater
2022-11-17 11:58 ` Klaus Jensen
0 siblings, 1 reply; 26+ messages in thread
From: Cédric Le Goater @ 2022-11-17 8:01 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
On 11/17/22 08:37, Klaus Jensen wrote:
> On Nov 17 07:56, Cédric Le Goater wrote:
>> On 11/17/22 07:40, Klaus Jensen wrote:
>>> On Nov 16 16:58, Cédric Le Goater wrote:
>>>> On 11/16/22 09:43, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> It is not given that the current master will release the bus after a
>>>>> transfer ends. Only schedule a pending master if the bus is idle.
>>>>>
>>>>> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>> hw/i2c/aspeed_i2c.c | 2 ++
>>>>> hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
>>>>> include/hw/i2c/i2c.h | 2 ++
>>>>> 3 files changed, 26 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>>>> index c166fd20fa11..1f071a3811f7 100644
>>>>> --- a/hw/i2c/aspeed_i2c.c
>>>>> +++ b/hw/i2c/aspeed_i2c.c
>>>>> @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>>> }
>>>>> SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
>>>>> aspeed_i2c_set_state(bus, I2CD_IDLE);
>>>>> +
>>>>> + i2c_schedule_pending_master(bus->bus);
>>>>
>>>> Shouldn't it be i2c_bus_release() ?
>>>>
>>>
>>> The reason for having both i2c_bus_release() and
>>> i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
>>> with i2c_bus_master(). They either set or clear the bus->bh member.
>>>
>>> In the current design, the controller (in this case the Aspeed I2C) is
>>> an "implicit" master (it does not have a bottom half driving it), so
>>> there is no bus->bh to clear.
>>>
>>> I should (and will) write some documentation on the asynchronous API.
>>
>> I found the routine names confusing. Thanks for the clarification.
>>
>> Maybe we could do this rename :
>>
>> i2c_bus_release() -> i2c_bus_release_and_clear()
>> i2c_schedule_pending_master() -> i2c_bus_release()
>>
>> and keep i2c_schedule_pending_master() internal the I2C core subsystem.
>>
>
> How about renaming i2c_bus_master to i2c_bus_acquire() such that it
> pairs with i2c_bus_release().
Looks good to me.
> And then add an i2c_bus_yield() to be used by the controller? I think we
> should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
> I'll take a closer look at that.
I am using your i2c-echo slave device to track regressions in the Aspeed
machines. May be we could merge it for tests ?
Thanks,
C.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 8:01 ` Cédric Le Goater
@ 2022-11-17 11:58 ` Klaus Jensen
2022-11-17 13:40 ` Cédric Le Goater
0 siblings, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2022-11-17 11:58 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]
On Nov 17 09:01, Cédric Le Goater wrote:
> On 11/17/22 08:37, Klaus Jensen wrote:
> > On Nov 17 07:56, Cédric Le Goater wrote:
> > > On 11/17/22 07:40, Klaus Jensen wrote:
> > > > On Nov 16 16:58, Cédric Le Goater wrote:
> > > > > On 11/16/22 09:43, Klaus Jensen wrote:
> > > > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > > > >
> > > > > > It is not given that the current master will release the bus after a
> > > > > > transfer ends. Only schedule a pending master if the bus is idle.
> > > > > >
> > > > > > Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> > > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > > > ---
> > > > > > hw/i2c/aspeed_i2c.c | 2 ++
> > > > > > hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> > > > > > include/hw/i2c/i2c.h | 2 ++
> > > > > > 3 files changed, 26 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > > > > > index c166fd20fa11..1f071a3811f7 100644
> > > > > > --- a/hw/i2c/aspeed_i2c.c
> > > > > > +++ b/hw/i2c/aspeed_i2c.c
> > > > > > @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > > > > > }
> > > > > > SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> > > > > > aspeed_i2c_set_state(bus, I2CD_IDLE);
> > > > > > +
> > > > > > + i2c_schedule_pending_master(bus->bus);
> > > > >
> > > > > Shouldn't it be i2c_bus_release() ?
> > > > >
> > > >
> > > > The reason for having both i2c_bus_release() and
> > > > i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
> > > > with i2c_bus_master(). They either set or clear the bus->bh member.
> > > >
> > > > In the current design, the controller (in this case the Aspeed I2C) is
> > > > an "implicit" master (it does not have a bottom half driving it), so
> > > > there is no bus->bh to clear.
> > > >
> > > > I should (and will) write some documentation on the asynchronous API.
> > >
> > > I found the routine names confusing. Thanks for the clarification.
> > >
> > > Maybe we could do this rename :
> > >
> > > i2c_bus_release() -> i2c_bus_release_and_clear()
> > > i2c_schedule_pending_master() -> i2c_bus_release()
> > >
> > > and keep i2c_schedule_pending_master() internal the I2C core subsystem.
> > >
> >
> > How about renaming i2c_bus_master to i2c_bus_acquire() such that it
> > pairs with i2c_bus_release().
>
> Looks good to me.
>
> > And then add an i2c_bus_yield() to be used by the controller? I think we
> > should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
> > I'll take a closer look at that.
>
> I am using your i2c-echo slave device to track regressions in the Aspeed
> machines. May be we could merge it for tests ?
>
Oh, cool.
Sure, I'd be happy to help "maintain" it ;)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 11:58 ` Klaus Jensen
@ 2022-11-17 13:40 ` Cédric Le Goater
2022-11-18 6:59 ` Klaus Jensen
2022-11-22 8:45 ` Klaus Jensen
0 siblings, 2 replies; 26+ messages in thread
From: Cédric Le Goater @ 2022-11-17 13:40 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
On 11/17/22 12:58, Klaus Jensen wrote:
> On Nov 17 09:01, Cédric Le Goater wrote:
>> On 11/17/22 08:37, Klaus Jensen wrote:
>>> On Nov 17 07:56, Cédric Le Goater wrote:
>>>> On 11/17/22 07:40, Klaus Jensen wrote:
>>>>> On Nov 16 16:58, Cédric Le Goater wrote:
>>>>>> On 11/16/22 09:43, Klaus Jensen wrote:
>>>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>>>
>>>>>>> It is not given that the current master will release the bus after a
>>>>>>> transfer ends. Only schedule a pending master if the bus is idle.
>>>>>>>
>>>>>>> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
>>>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>>>> ---
>>>>>>> hw/i2c/aspeed_i2c.c | 2 ++
>>>>>>> hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
>>>>>>> include/hw/i2c/i2c.h | 2 ++
>>>>>>> 3 files changed, 26 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>>>>>> index c166fd20fa11..1f071a3811f7 100644
>>>>>>> --- a/hw/i2c/aspeed_i2c.c
>>>>>>> +++ b/hw/i2c/aspeed_i2c.c
>>>>>>> @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>>>>> }
>>>>>>> SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
>>>>>>> aspeed_i2c_set_state(bus, I2CD_IDLE);
>>>>>>> +
>>>>>>> + i2c_schedule_pending_master(bus->bus);
>>>>>>
>>>>>> Shouldn't it be i2c_bus_release() ?
>>>>>>
>>>>>
>>>>> The reason for having both i2c_bus_release() and
>>>>> i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
>>>>> with i2c_bus_master(). They either set or clear the bus->bh member.
>>>>>
>>>>> In the current design, the controller (in this case the Aspeed I2C) is
>>>>> an "implicit" master (it does not have a bottom half driving it), so
>>>>> there is no bus->bh to clear.
>>>>>
>>>>> I should (and will) write some documentation on the asynchronous API.
>>>>
>>>> I found the routine names confusing. Thanks for the clarification.
>>>>
>>>> Maybe we could do this rename :
>>>>
>>>> i2c_bus_release() -> i2c_bus_release_and_clear()
>>>> i2c_schedule_pending_master() -> i2c_bus_release()
>>>>
>>>> and keep i2c_schedule_pending_master() internal the I2C core subsystem.
>>>>
>>>
>>> How about renaming i2c_bus_master to i2c_bus_acquire() such that it
>>> pairs with i2c_bus_release().
>>
>> Looks good to me.
>>
>>> And then add an i2c_bus_yield() to be used by the controller? I think we
>>> should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
>>> I'll take a closer look at that.
>>
>> I am using your i2c-echo slave device to track regressions in the Aspeed
>> machines. May be we could merge it for tests ?
>>
>
> Oh, cool.
>
> Sure, I'd be happy to help "maintain" it ;)
And so, I am seeing errors with the little POC you sent.
without:
console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
console: [ 4.252431] i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64
console: i2cset -y 3 0x42 0x64 0x00 0xaa i
/console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
console: 0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
console: poweroff
console: 0000010 ffff ffff ffff ffff ffff ffff ffff ffff
console: *
console: 0000100
with:
console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
console: [ 4.413210] i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64
console: i2cset -y 3 0x42 0x64 0x00 0xaa i
console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
console: 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
console: *
console: 0000100
C.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 13:40 ` Cédric Le Goater
@ 2022-11-18 6:59 ` Klaus Jensen
2022-11-22 8:45 ` Klaus Jensen
1 sibling, 0 replies; 26+ messages in thread
From: Klaus Jensen @ 2022-11-18 6:59 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 4588 bytes --]
On Nov 17 14:40, Cédric Le Goater wrote:
> On 11/17/22 12:58, Klaus Jensen wrote:
> > On Nov 17 09:01, Cédric Le Goater wrote:
> > > On 11/17/22 08:37, Klaus Jensen wrote:
> > > > On Nov 17 07:56, Cédric Le Goater wrote:
> > > > > On 11/17/22 07:40, Klaus Jensen wrote:
> > > > > > On Nov 16 16:58, Cédric Le Goater wrote:
> > > > > > > On 11/16/22 09:43, Klaus Jensen wrote:
> > > > > > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > > > > > >
> > > > > > > > It is not given that the current master will release the bus after a
> > > > > > > > transfer ends. Only schedule a pending master if the bus is idle.
> > > > > > > >
> > > > > > > > Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> > > > > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > > > > > ---
> > > > > > > > hw/i2c/aspeed_i2c.c | 2 ++
> > > > > > > > hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> > > > > > > > include/hw/i2c/i2c.h | 2 ++
> > > > > > > > 3 files changed, 26 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > > > > > > > index c166fd20fa11..1f071a3811f7 100644
> > > > > > > > --- a/hw/i2c/aspeed_i2c.c
> > > > > > > > +++ b/hw/i2c/aspeed_i2c.c
> > > > > > > > @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > > > > > > > }
> > > > > > > > SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> > > > > > > > aspeed_i2c_set_state(bus, I2CD_IDLE);
> > > > > > > > +
> > > > > > > > + i2c_schedule_pending_master(bus->bus);
> > > > > > >
> > > > > > > Shouldn't it be i2c_bus_release() ?
> > > > > > >
> > > > > >
> > > > > > The reason for having both i2c_bus_release() and
> > > > > > i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
> > > > > > with i2c_bus_master(). They either set or clear the bus->bh member.
> > > > > >
> > > > > > In the current design, the controller (in this case the Aspeed I2C) is
> > > > > > an "implicit" master (it does not have a bottom half driving it), so
> > > > > > there is no bus->bh to clear.
> > > > > >
> > > > > > I should (and will) write some documentation on the asynchronous API.
> > > > >
> > > > > I found the routine names confusing. Thanks for the clarification.
> > > > >
> > > > > Maybe we could do this rename :
> > > > >
> > > > > i2c_bus_release() -> i2c_bus_release_and_clear()
> > > > > i2c_schedule_pending_master() -> i2c_bus_release()
> > > > >
> > > > > and keep i2c_schedule_pending_master() internal the I2C core subsystem.
> > > > >
> > > >
> > > > How about renaming i2c_bus_master to i2c_bus_acquire() such that it
> > > > pairs with i2c_bus_release().
> > >
> > > Looks good to me.
> > >
> > > > And then add an i2c_bus_yield() to be used by the controller? I think we
> > > > should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
> > > > I'll take a closer look at that.
> > >
> > > I am using your i2c-echo slave device to track regressions in the Aspeed
> > > machines. May be we could merge it for tests ?
> > >
> >
> > Oh, cool.
> >
> > Sure, I'd be happy to help "maintain" it ;)
>
> And so, I am seeing errors with the little POC you sent.
>
> without:
> console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: [ 4.252431] i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64
> console: i2cset -y 3 0x42 0x64 0x00 0xaa i
> /console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
> console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
> console: 0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
> console: poweroff
> console: 0000010 ffff ffff ffff ffff ffff ffff ffff ffff
> console: *
> console: 0000100
>
> with:
> console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: [ 4.413210] i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64
> console: i2cset -y 3 0x42 0x64 0x00 0xaa i
> console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
> console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
> console: 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
> console: *
> console: 0000100
> C.
Interesting,
I'll check it out.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle
2022-11-17 13:40 ` Cédric Le Goater
2022-11-18 6:59 ` Klaus Jensen
@ 2022-11-22 8:45 ` Klaus Jensen
1 sibling, 0 replies; 26+ messages in thread
From: Klaus Jensen @ 2022-11-22 8:45 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 6041 bytes --]
On Nov 17 14:40, Cédric Le Goater wrote:
> On 11/17/22 12:58, Klaus Jensen wrote:
> > On Nov 17 09:01, Cédric Le Goater wrote:
> > > On 11/17/22 08:37, Klaus Jensen wrote:
> > > > On Nov 17 07:56, Cédric Le Goater wrote:
> > > > > On 11/17/22 07:40, Klaus Jensen wrote:
> > > > > > On Nov 16 16:58, Cédric Le Goater wrote:
> > > > > > > On 11/16/22 09:43, Klaus Jensen wrote:
> > > > > > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > > > > > >
> > > > > > > > It is not given that the current master will release the bus after a
> > > > > > > > transfer ends. Only schedule a pending master if the bus is idle.
> > > > > > > >
> > > > > > > > Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> > > > > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > > > > > ---
> > > > > > > > hw/i2c/aspeed_i2c.c | 2 ++
> > > > > > > > hw/i2c/core.c | 37 ++++++++++++++++++++++---------------
> > > > > > > > include/hw/i2c/i2c.h | 2 ++
> > > > > > > > 3 files changed, 26 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > > > > > > > index c166fd20fa11..1f071a3811f7 100644
> > > > > > > > --- a/hw/i2c/aspeed_i2c.c
> > > > > > > > +++ b/hw/i2c/aspeed_i2c.c
> > > > > > > > @@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > > > > > > > }
> > > > > > > > SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
> > > > > > > > aspeed_i2c_set_state(bus, I2CD_IDLE);
> > > > > > > > +
> > > > > > > > + i2c_schedule_pending_master(bus->bus);
> > > > > > >
> > > > > > > Shouldn't it be i2c_bus_release() ?
> > > > > > >
> > > > > >
> > > > > > The reason for having both i2c_bus_release() and
> > > > > > i2c_schedule_pending_master() is that i2c_bus_release() sort of pairs
> > > > > > with i2c_bus_master(). They either set or clear the bus->bh member.
> > > > > >
> > > > > > In the current design, the controller (in this case the Aspeed I2C) is
> > > > > > an "implicit" master (it does not have a bottom half driving it), so
> > > > > > there is no bus->bh to clear.
> > > > > >
> > > > > > I should (and will) write some documentation on the asynchronous API.
> > > > >
> > > > > I found the routine names confusing. Thanks for the clarification.
> > > > >
> > > > > Maybe we could do this rename :
> > > > >
> > > > > i2c_bus_release() -> i2c_bus_release_and_clear()
> > > > > i2c_schedule_pending_master() -> i2c_bus_release()
> > > > >
> > > > > and keep i2c_schedule_pending_master() internal the I2C core subsystem.
> > > > >
> > > >
> > > > How about renaming i2c_bus_master to i2c_bus_acquire() such that it
> > > > pairs with i2c_bus_release().
> > >
> > > Looks good to me.
> > >
> > > > And then add an i2c_bus_yield() to be used by the controller? I think we
> > > > should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
> > > > I'll take a closer look at that.
> > >
> > > I am using your i2c-echo slave device to track regressions in the Aspeed
> > > machines. May be we could merge it for tests ?
> > >
> >
> > Oh, cool.
> >
> > Sure, I'd be happy to help "maintain" it ;)
>
> And so, I am seeing errors with the little POC you sent.
>
> without:
> console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: [ 4.252431] i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64
> console: i2cset -y 3 0x42 0x64 0x00 0xaa i
> /console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
> console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
> console: 0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
> console: poweroff
> console: 0000010 ffff ffff ffff ffff ffff ffff ffff ffff
> console: *
> console: 0000100
>
> with:
> console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
> console: [ 4.413210] i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64
> console: i2cset -y 3 0x42 0x64 0x00 0xaa i
> console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
> console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
> console: 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
> console: *
> console: 0000100
> C.
Right.
This is because the i2c-echo device is scheduling the bottom half
initially on its own. What happens is that the bottom half gets queued
up in the pending masters list instead of being scheduling directly. And
since the i2c controller is idle, the bottom half is never scheduled.
Fixing i2c_bus_acquire() to schedulue it directly if the bus is free
seems the proper way to do it. I'll include that in v2.
While it is not directly invalid, the echo device should be fixed to
better align with the api, like so:
--- hw/misc/i2c-echo.c.orig 2022-11-22 09:35:00.478173652 +0100
+++ hw/misc/i2c-echo.c 2022-11-22 09:34:31.428174379 +0100
@@ -9,7 +9,6 @@
enum i2c_echo_state {
I2C_ECHO_STATE_IDLE,
- I2C_ECHO_STATE_REQUEST_MASTER,
I2C_ECHO_STATE_START_SEND,
I2C_ECHO_STATE_ACK,
};
@@ -34,11 +33,6 @@
case I2C_ECHO_STATE_IDLE:
return;
- case I2C_ECHO_STATE_REQUEST_MASTER:
- i2c_bus_acquire(state->bus, state->bh);
- state->state = I2C_ECHO_STATE_START_SEND;
- return;
-
case I2C_ECHO_STATE_START_SEND:
if (i2c_start_send_async(state->bus, state->data[0])) {
goto release_bus;
@@ -85,8 +79,8 @@
case I2C_FINISH:
state->pos = 0;
- state->state = I2C_ECHO_STATE_REQUEST_MASTER;
- qemu_bh_schedule(state->bh);
+ state->state = I2C_ECHO_STATE_START_SEND;
+ i2c_bus_acquire(state->bus, state->bh);
break;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-16 8:43 [PATCH 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
2022-11-16 8:43 ` [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle Klaus Jensen
@ 2022-11-16 8:43 ` Klaus Jensen
2022-11-16 14:27 ` Corey Minyard
2022-11-18 5:56 ` Jeremy Kerr
2022-11-16 8:43 ` [PATCH RFC 3/3] hw/nvme: add nvme management interface model Klaus Jensen
2022-11-16 9:18 ` [PATCH 0/3] hw/{i2c,nvme}: mctp endpoint, " Jeremy Kerr
3 siblings, 2 replies; 26+ messages in thread
From: Klaus Jensen @ 2022-11-16 8:43 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Jeremy Kerr, Joel Stanley,
Cédric Le Goater, Klaus Jensen, Klaus Jensen
From: Klaus Jensen <k.jensen@samsung.com>
Add an abstract MCTP over I2C endpoint model. This implements MCTP
control message handling as well as handling the actual I2C transport
(packetization).
Devices are intended to derive from this and implement the class
methods.
Parts of this implementation is inspired by code[1] previously posted by
Jonathan Cameron.
[1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/arm/Kconfig | 1 +
hw/i2c/Kconfig | 4 +
hw/i2c/mctp.c | 365 +++++++++++++++++++++++++++++++++++++++++
hw/i2c/meson.build | 1 +
hw/i2c/trace-events | 12 ++
include/hw/i2c/mctp.h | 83 ++++++++++
include/hw/misc/mctp.h | 43 +++++
7 files changed, 509 insertions(+)
create mode 100644 hw/i2c/mctp.c
create mode 100644 include/hw/i2c/mctp.h
create mode 100644 include/hw/misc/mctp.h
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 17fcde8e1ccc..3233bdc193d7 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -444,6 +444,7 @@ config ASPEED_SOC
select DS1338
select FTGMAC100
select I2C
+ select MCTP_I2C
select DPS310
select PCA9552
select SERIAL
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 9bb8870517f8..5dd43d550c32 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -41,3 +41,7 @@ config PCA954X
config PMBUS
bool
select SMBUS
+
+config MCTP_I2C
+ bool
+ select I2C
diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
new file mode 100644
index 000000000000..46376de95a98
--- /dev/null
+++ b/hw/i2c/mctp.c
@@ -0,0 +1,365 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
+ * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+
+#include "hw/qdev-properties.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/mctp.h"
+
+#include "trace.h"
+
+static uint8_t crc8(uint16_t data)
+{
+#define POLY (0x1070U << 3)
+ int i;
+
+ for (i = 0; i < 8; i++) {
+ if (data & 0x8000) {
+ data = data ^ POLY;
+ }
+
+ data = data << 1;
+ }
+
+ return (uint8_t)(data >> 8);
+#undef POLY
+}
+
+static uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ crc = crc8((crc ^ buf[i]) << 8);
+ }
+
+ return crc;
+}
+
+void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp)
+{
+ I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(DEVICE(mctp)));
+
+ mctp->tx.state = I2C_MCTP_STATE_TX_START_SEND;
+
+ i2c_bus_master(i2c, mctp->tx.bh);
+}
+
+static void i2c_mctp_tx(void *opaque)
+{
+ DeviceState *dev = DEVICE(opaque);
+ I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
+ I2CSlave *slave = I2C_SLAVE(dev);
+ MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
+ MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
+ MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
+ uint8_t flags = 0;
+
+ switch (mctp->tx.state) {
+ case I2C_MCTP_STATE_TX_SEND_BYTE:
+ if (mctp->pos < mctp->len) {
+ uint8_t byte = mctp->buffer[mctp->pos];
+
+ trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
+
+ /* send next byte */
+ i2c_send_async(i2c, byte);
+
+ mctp->pos++;
+
+ break;
+ }
+
+ /* packet sent */
+ i2c_end_transfer(i2c);
+
+ /* fall through */
+
+ case I2C_MCTP_STATE_TX_START_SEND:
+ if (mctp->tx.is_control) {
+ /* packet payload is already in buffer */
+ flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
+ } else {
+ /* get message bytes from derived device */
+ mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
+ I2C_MCTP_MAXMTU, &flags);
+ }
+
+ if (!mctp->len) {
+ trace_i2c_mctp_tx_done();
+
+ /* no more packets needed; release the bus */
+ i2c_bus_release(i2c);
+
+ mctp->state = I2C_MCTP_STATE_IDLE;
+ mctp->tx.is_control = false;
+
+ break;
+ }
+
+ mctp->state = I2C_MCTP_STATE_TX;
+
+ pkt->i2c = (MCTPI2CPacketHeader) {
+ .dest = mctp->tx.addr & ~0x1,
+ .prot = 0xf,
+ .byte_count = 5 + mctp->len,
+ .source = slave->address << 1 | 0x1,
+ };
+
+ pkt->mctp.hdr = (MCTPPacketHeader) {
+ .version = 0x1,
+ .eid.dest = mctp->tx.eid,
+ .eid.source = mctp->my_eid,
+ .flags = flags | (mctp->tx.pktseq++ & 0x3) << 4 | mctp->tx.flags,
+ };
+
+ mctp->len += sizeof(MCTPI2CPacket);
+ mctp->buffer[mctp->len] = i2c_smbus_pec(0, mctp->buffer, mctp->len);
+ mctp->len++;
+
+ trace_i2c_mctp_tx_start_send(mctp->len);
+
+ i2c_start_send_async(i2c, pkt->i2c.dest >> 1);
+
+ /* already "sent" the destination slave address */
+ mctp->pos = 1;
+
+ mctp->tx.state = I2C_MCTP_STATE_TX_SEND_BYTE;
+
+ break;
+ }
+}
+
+#define i2c_mctp_control_data(buf) \
+ (i2c_mctp_payload(buf) + offsetof(MCTPControlMessage, data))
+
+static void i2c_mctp_handle_control_set_eid(MCTPI2CEndpoint *mctp, uint8_t eid)
+{
+ mctp->my_eid = eid;
+
+ uint8_t buf[] = {
+ 0x0, 0x0, eid, 0x0,
+ };
+
+ memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
+ mctp->len += sizeof(buf);
+}
+
+static void i2c_mctp_handle_control_get_eid(MCTPI2CEndpoint *mctp)
+{
+ uint8_t buf[] = {
+ 0x0, mctp->my_eid, 0x0, 0x0,
+ };
+
+ memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
+ mctp->len += sizeof(buf);
+}
+
+static void i2c_mctp_handle_control_get_version(MCTPI2CEndpoint *mctp)
+{
+ uint8_t buf[] = {
+ 0x0, 0x1, 0x0, 0x1, 0x3, 0x1,
+ };
+
+ memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
+ mctp->len += sizeof(buf);
+}
+
+enum {
+ MCTP_CONTROL_SET_EID = 0x01,
+ MCTP_CONTROL_GET_EID = 0x02,
+ MCTP_CONTROL_GET_VERSION = 0x04,
+ MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT = 0x05,
+};
+
+static void i2c_mctp_handle_control(MCTPI2CEndpoint *mctp)
+{
+ MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
+ MCTPControlMessage *msg = (MCTPControlMessage *)i2c_mctp_payload(mctp->buffer);
+
+ /* clear Rq/D */
+ msg->flags &= 0x1f;
+
+ mctp->len = offsetof(MCTPControlMessage, data);
+
+ trace_i2c_mctp_handle_control(msg->command);
+
+ switch (msg->command) {
+ case MCTP_CONTROL_SET_EID:
+ i2c_mctp_handle_control_set_eid(mctp, msg->data[1]);
+ break;
+
+ case MCTP_CONTROL_GET_EID:
+ i2c_mctp_handle_control_get_eid(mctp);
+ break;
+
+ case MCTP_CONTROL_GET_VERSION:
+ i2c_mctp_handle_control_get_version(mctp);
+ break;
+
+ case MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT:
+ mctp->len += mc->get_message_types(mctp, i2c_mctp_control_data(mctp->buffer));
+ break;
+
+ default:
+ trace_i2c_mctp_unhandled_control(msg->command);
+
+ msg->data[0] = MCTP_CONTROL_ERROR_UNSUPPORTED_CMD;
+ mctp->len++;
+
+ break;
+ }
+
+ i2c_mctp_schedule_send(mctp);
+}
+
+static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
+{
+ MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
+ MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
+ MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
+ size_t payload_len;
+ uint8_t pec;
+
+ switch (event) {
+ case I2C_START_SEND:
+ if (mctp->state != I2C_MCTP_STATE_IDLE) {
+ return -1;
+ }
+
+ /* the i2c core eats the slave address, so put it back in */
+ pkt->i2c.dest = i2c->address << 1;
+ mctp->len = 1;
+
+ mctp->state = I2C_MCTP_STATE_RX_STARTED;
+
+ return 0;
+
+ case I2C_FINISH:
+ payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, mctp.payload));
+
+ if (pkt->i2c.byte_count + 3 != mctp->len - 1) {
+ trace_i2c_mctp_drop_invalid_length(pkt->i2c.byte_count + 3,
+ mctp->len - 1);
+ goto drop;
+ }
+
+ pec = i2c_smbus_pec(0, mctp->buffer, mctp->len - 1);
+ if (mctp->buffer[mctp->len - 1] != pec) {
+ trace_i2c_mctp_drop_invalid_pec(mctp->buffer[mctp->len - 1], pec);
+ goto drop;
+ }
+
+ if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
+ trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
+ mctp->my_eid);
+ goto drop;
+ }
+
+ if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) {
+ mctp->tx.is_control = false;
+
+ if (mctp->state == I2C_MCTP_STATE_RX) {
+ mc->reset_message(mctp);
+ }
+
+ mctp->state = I2C_MCTP_STATE_RX;
+
+ mctp->tx.addr = pkt->i2c.source;
+ mctp->tx.eid = pkt->mctp.hdr.eid.source;
+ mctp->tx.flags = pkt->mctp.hdr.flags & 0x7;
+ mctp->tx.pktseq = (pkt->mctp.hdr.flags >> 4) & 0x3;
+
+ if ((pkt->mctp.payload[0] & 0x7f) == MCTP_MESSAGE_TYPE_CONTROL) {
+ mctp->tx.is_control = true;
+
+ i2c_mctp_handle_control(mctp);
+
+ return 0;
+ }
+ } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
+ trace_i2c_mctp_drop("expected SOM");
+ goto drop;
+ } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ & 0x3)) {
+ trace_i2c_mctp_drop_invalid_pktseq((pkt->mctp.hdr.flags >> 4) & 0x3,
+ mctp->tx.pktseq & 0x3);
+ goto drop;
+ }
+
+ mc->put_message_bytes(mctp, i2c_mctp_payload(mctp->buffer), payload_len);
+
+ if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_EOM) {
+ mc->handle_message(mctp);
+ mctp->state = I2C_MCTP_STATE_WAIT_TX;
+ }
+
+ return 0;
+
+ default:
+ return -1;
+ }
+
+drop:
+ mc->reset_message(mctp);
+
+ mctp->state = I2C_MCTP_STATE_IDLE;
+
+ return 0;
+}
+
+static int i2c_mctp_send_cb(I2CSlave *i2c, uint8_t data)
+{
+ MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
+
+ if (mctp->len < I2C_MCTP_MAX_LENGTH) {
+ mctp->buffer[mctp->len++] = data;
+ return 0;
+ }
+
+ return -1;
+}
+
+static void i2c_mctp_instance_init(Object *obj)
+{
+ MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(obj);
+
+ mctp->tx.bh = qemu_bh_new(i2c_mctp_tx, mctp);
+}
+
+static Property mctp_i2c_props[] = {
+ DEFINE_PROP_UINT8("eid", MCTPI2CEndpoint, my_eid, 0x9),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void i2c_mctp_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+ I2CSlaveClass *k = I2C_SLAVE_CLASS(oc);
+
+ k->event = i2c_mctp_event_cb;
+ k->send = i2c_mctp_send_cb;
+
+ device_class_set_props(dc, mctp_i2c_props);
+}
+
+static const TypeInfo i2c_mctp_info = {
+ .name = TYPE_MCTP_I2C_ENDPOINT,
+ .parent = TYPE_I2C_SLAVE,
+ .abstract = true,
+ .instance_init = i2c_mctp_instance_init,
+ .instance_size = sizeof(MCTPI2CEndpoint),
+ .class_init = i2c_mctp_class_init,
+ .class_size = sizeof(MCTPI2CEndpointClass),
+};
+
+static void register_types(void)
+{
+ type_register_static(&i2c_mctp_info);
+}
+
+type_init(register_types)
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index d3df273251f7..97d0bfd9ac67 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -1,5 +1,6 @@
i2c_ss = ss.source_set()
i2c_ss.add(when: 'CONFIG_I2C', if_true: files('core.c'))
+i2c_ss.add(when: 'CONFIG_MCTP_I2C', if_true: files('mctp.c'))
i2c_ss.add(when: 'CONFIG_SMBUS', if_true: files('smbus_slave.c', 'smbus_master.c'))
i2c_ss.add(when: 'CONFIG_ACPI_SMBUS', if_true: files('pm_smbus.c'))
i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('smbus_ich9.c'))
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index af181d43ee64..778139d08166 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -33,3 +33,15 @@ npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s
pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x"
pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
+
+# mctp.c
+i2c_mctp_tx_start_send(size_t len) "len %zu"
+i2c_mctp_tx_send_byte(size_t pos, uint8_t byte) "pos %zu byte 0x%"PRIx8""
+i2c_mctp_tx_done(void) "packet sent"
+i2c_mctp_handle_control(uint8_t command) "command 0x%"PRIx8""
+i2c_mctp_unhandled_control(uint8_t command) "command 0x%"PRIx8""
+i2c_mctp_drop(const char *reason) "%s"
+i2c_mctp_drop_invalid_length(unsigned byte_count, size_t expected) "byte_count %u expected %zu"
+i2c_mctp_drop_invalid_pec(uint8_t pec, uint8_t expected) "pec 0x%"PRIx8" expected 0x%"PRIx8""
+i2c_mctp_drop_invalid_eid(uint8_t eid, uint8_t expected) "eid 0x%"PRIx8" expected 0x%"PRIx8""
+i2c_mctp_drop_invalid_pktseq(uint8_t pktseq, uint8_t expected) "pktseq 0x%"PRIx8" expected 0x%"PRIx8""
diff --git a/include/hw/i2c/mctp.h b/include/hw/i2c/mctp.h
new file mode 100644
index 000000000000..17dae662138b
--- /dev/null
+++ b/include/hw/i2c/mctp.h
@@ -0,0 +1,83 @@
+#ifndef QEMU_I2C_MCTP_H
+#define QEMU_I2C_MCTP_H
+
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+#include "hw/misc/mctp.h"
+
+typedef struct MCTPI2CPacketHeader {
+ uint8_t dest;
+ uint8_t prot;
+ uint8_t byte_count;
+ uint8_t source;
+} MCTPI2CPacketHeader;
+
+typedef struct MCTPI2CPacket {
+ MCTPI2CPacketHeader i2c;
+ MCTPPacket mctp;
+} MCTPI2CPacket;
+
+#define i2c_mctp_payload(buf) (buf + offsetof(MCTPI2CPacket, mctp.payload))
+
+#define TYPE_MCTP_I2C_ENDPOINT "mctp-i2c-endpoint"
+OBJECT_DECLARE_TYPE(MCTPI2CEndpoint, MCTPI2CEndpointClass, MCTP_I2C_ENDPOINT)
+
+struct MCTPI2CEndpointClass {
+ I2CSlaveClass parent_class;
+
+ int (*put_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf, size_t len);
+ size_t (*get_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf,
+ size_t maxlen, uint8_t *mctp_flags);
+
+ void (*handle_message)(MCTPI2CEndpoint *mctp);
+ void (*reset_message)(MCTPI2CEndpoint *mctp);
+
+ size_t (*get_message_types)(MCTPI2CEndpoint *mctp, uint8_t *data);
+};
+
+#define I2C_MCTP_MAXBLOCK 255
+#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1))
+#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1)
+
+typedef enum {
+ I2C_MCTP_STATE_IDLE,
+ I2C_MCTP_STATE_RX_STARTED,
+ I2C_MCTP_STATE_RX,
+ I2C_MCTP_STATE_WAIT_TX,
+ I2C_MCTP_STATE_TX,
+} MCTPState;
+
+typedef enum {
+ I2C_MCTP_STATE_TX_START_SEND,
+ I2C_MCTP_STATE_TX_SEND_BYTE,
+} MCTPTxState;
+
+typedef struct MCTPI2CEndpoint {
+ I2CSlave parent_obj;
+ I2CBus *i2c;
+
+ MCTPState state;
+
+ /* mctp endpoint identifier */
+ uint8_t my_eid;
+
+ uint8_t buffer[I2C_MCTP_MAX_LENGTH];
+ uint64_t pos;
+ size_t len;
+
+ struct {
+ MCTPTxState state;
+ bool is_control;
+
+ uint8_t eid;
+ uint8_t addr;
+ uint8_t pktseq;
+ uint8_t flags;
+
+ QEMUBH *bh;
+ } tx;
+} MCTPI2CEndpoint;
+
+void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp);
+
+#endif /* QEMU_I2C_MCTP_H */
diff --git a/include/hw/misc/mctp.h b/include/hw/misc/mctp.h
new file mode 100644
index 000000000000..c936224ecf60
--- /dev/null
+++ b/include/hw/misc/mctp.h
@@ -0,0 +1,43 @@
+#ifndef QEMU_MCTP_H
+#define QEMU_MCTP_H
+
+#define MCTP_BASELINE_MTU 64
+
+enum {
+ MCTP_H_FLAGS_EOM = 1 << 6,
+ MCTP_H_FLAGS_SOM = 1 << 7,
+};
+
+enum {
+ MCTP_MESSAGE_TYPE_CONTROL = 0x0,
+ MCTP_MESSAGE_TYPE_NMI = 0x4,
+
+ MCTP_MESSAGE_IC = 1 << 7,
+};
+
+typedef struct MCTPPacketHeader {
+ uint8_t version;
+ struct {
+ uint8_t dest;
+ uint8_t source;
+ } eid;
+ uint8_t flags;
+} MCTPPacketHeader;
+
+typedef struct MCTPPacket {
+ MCTPPacketHeader hdr;
+ uint8_t payload[];
+} MCTPPacket;
+
+typedef struct MCTPControlMessage {
+ uint8_t type;
+ uint8_t flags;
+ uint8_t command;
+ uint8_t data[];
+} MCTPControlMessage;
+
+enum {
+ MCTP_CONTROL_ERROR_UNSUPPORTED_CMD = 0x5,
+};
+
+#endif /* QEMU_MCTP_H */
--
2.38.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-16 8:43 ` [PATCH RFC 2/3] hw/i2c: add mctp core Klaus Jensen
@ 2022-11-16 14:27 ` Corey Minyard
2022-11-17 6:51 ` Klaus Jensen
2022-11-18 5:56 ` Jeremy Kerr
1 sibling, 1 reply; 26+ messages in thread
From: Corey Minyard @ 2022-11-16 14:27 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Cédric Le Goater, Klaus Jensen
On Wed, Nov 16, 2022 at 09:43:11AM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add an abstract MCTP over I2C endpoint model. This implements MCTP
> control message handling as well as handling the actual I2C transport
> (packetization).
>
> Devices are intended to derive from this and implement the class
> methods.
>
> Parts of this implementation is inspired by code[1] previously posted by
> Jonathan Cameron.
I have some comments inline, mostly about buffer handling. Buffer
handling is scary to me, so you might see some paranoia here :-).
>
> [1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/arm/Kconfig | 1 +
> hw/i2c/Kconfig | 4 +
> hw/i2c/mctp.c | 365 +++++++++++++++++++++++++++++++++++++++++
> hw/i2c/meson.build | 1 +
> hw/i2c/trace-events | 12 ++
> include/hw/i2c/mctp.h | 83 ++++++++++
> include/hw/misc/mctp.h | 43 +++++
> 7 files changed, 509 insertions(+)
> create mode 100644 hw/i2c/mctp.c
> create mode 100644 include/hw/i2c/mctp.h
> create mode 100644 include/hw/misc/mctp.h
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 17fcde8e1ccc..3233bdc193d7 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -444,6 +444,7 @@ config ASPEED_SOC
> select DS1338
> select FTGMAC100
> select I2C
> + select MCTP_I2C
> select DPS310
> select PCA9552
> select SERIAL
> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> index 9bb8870517f8..5dd43d550c32 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -41,3 +41,7 @@ config PCA954X
> config PMBUS
> bool
> select SMBUS
> +
> +config MCTP_I2C
> + bool
> + select I2C
> diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
> new file mode 100644
> index 000000000000..46376de95a98
> --- /dev/null
> +++ b/hw/i2c/mctp.c
> @@ -0,0 +1,365 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
> + * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +
> +#include "hw/qdev-properties.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/mctp.h"
> +
> +#include "trace.h"
> +
> +static uint8_t crc8(uint16_t data)
> +{
> +#define POLY (0x1070U << 3)
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + if (data & 0x8000) {
> + data = data ^ POLY;
> + }
> +
> + data = data << 1;
> + }
> +
> + return (uint8_t)(data >> 8);
> +#undef POLY
> +}
> +
> +static uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len)
> +{
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + crc = crc8((crc ^ buf[i]) << 8);
> + }
> +
> + return crc;
> +}
The PEC calculation probably belongs in it's own smbus.c file, since
it's generic, so someone looking will find it.
> +
> +void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp)
> +{
> + I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(DEVICE(mctp)));
> +
> + mctp->tx.state = I2C_MCTP_STATE_TX_START_SEND;
> +
> + i2c_bus_master(i2c, mctp->tx.bh);
> +}
> +
> +static void i2c_mctp_tx(void *opaque)
> +{
> + DeviceState *dev = DEVICE(opaque);
> + I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> + I2CSlave *slave = I2C_SLAVE(dev);
> + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> + uint8_t flags = 0;
> +
> + switch (mctp->tx.state) {
> + case I2C_MCTP_STATE_TX_SEND_BYTE:
> + if (mctp->pos < mctp->len) {
> + uint8_t byte = mctp->buffer[mctp->pos];
> +
> + trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> +
> + /* send next byte */
> + i2c_send_async(i2c, byte);
> +
> + mctp->pos++;
> +
> + break;
> + }
> +
> + /* packet sent */
> + i2c_end_transfer(i2c);
> +
> + /* fall through */
> +
> + case I2C_MCTP_STATE_TX_START_SEND:
> + if (mctp->tx.is_control) {
> + /* packet payload is already in buffer */
> + flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> + } else {
> + /* get message bytes from derived device */
> + mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> + I2C_MCTP_MAXMTU, &flags);
> + }
> +
> + if (!mctp->len) {
> + trace_i2c_mctp_tx_done();
> +
> + /* no more packets needed; release the bus */
> + i2c_bus_release(i2c);
> +
> + mctp->state = I2C_MCTP_STATE_IDLE;
> + mctp->tx.is_control = false;
> +
> + break;
> + }
> +
> + mctp->state = I2C_MCTP_STATE_TX;
> +
> + pkt->i2c = (MCTPI2CPacketHeader) {
> + .dest = mctp->tx.addr & ~0x1,
> + .prot = 0xf,
> + .byte_count = 5 + mctp->len,
> + .source = slave->address << 1 | 0x1,
> + };
> +
> + pkt->mctp.hdr = (MCTPPacketHeader) {
> + .version = 0x1,
> + .eid.dest = mctp->tx.eid,
> + .eid.source = mctp->my_eid,
> + .flags = flags | (mctp->tx.pktseq++ & 0x3) << 4 | mctp->tx.flags,
> + };
> +
> + mctp->len += sizeof(MCTPI2CPacket);
Do you need overflow checking here? There are lots of increments of
mctp->len in the code that might or might not need overflow checks.
It does seem like you have pre-calculated everything so it fits; I worry
more about later changes that might violate those assumptions.
You could use something like i2c_mctp_send_cb() to send all data. Not
sure, but something to think about.
> + mctp->buffer[mctp->len] = i2c_smbus_pec(0, mctp->buffer, mctp->len);
> + mctp->len++;
> +
> + trace_i2c_mctp_tx_start_send(mctp->len);
> +
> + i2c_start_send_async(i2c, pkt->i2c.dest >> 1);
> +
> + /* already "sent" the destination slave address */
> + mctp->pos = 1;
> +
> + mctp->tx.state = I2C_MCTP_STATE_TX_SEND_BYTE;
> +
> + break;
> + }
> +}
> +
> +#define i2c_mctp_control_data(buf) \
> + (i2c_mctp_payload(buf) + offsetof(MCTPControlMessage, data))
> +
> +static void i2c_mctp_handle_control_set_eid(MCTPI2CEndpoint *mctp, uint8_t eid)
> +{
> + mctp->my_eid = eid;
> +
> + uint8_t buf[] = {
> + 0x0, 0x0, eid, 0x0,
> + };
> +
> + memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> + mctp->len += sizeof(buf);
> +}
> +
> +static void i2c_mctp_handle_control_get_eid(MCTPI2CEndpoint *mctp)
> +{
> + uint8_t buf[] = {
> + 0x0, mctp->my_eid, 0x0, 0x0,
> + };
> +
> + memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> + mctp->len += sizeof(buf);
> +}
> +
> +static void i2c_mctp_handle_control_get_version(MCTPI2CEndpoint *mctp)
> +{
> + uint8_t buf[] = {
> + 0x0, 0x1, 0x0, 0x1, 0x3, 0x1,
> + };
> +
> + memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> + mctp->len += sizeof(buf);
> +}
> +
> +enum {
> + MCTP_CONTROL_SET_EID = 0x01,
> + MCTP_CONTROL_GET_EID = 0x02,
> + MCTP_CONTROL_GET_VERSION = 0x04,
> + MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT = 0x05,
> +};
> +
> +static void i2c_mctp_handle_control(MCTPI2CEndpoint *mctp)
> +{
> + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> + MCTPControlMessage *msg = (MCTPControlMessage *)i2c_mctp_payload(mctp->buffer);
> +
> + /* clear Rq/D */
> + msg->flags &= 0x1f;
> +
> + mctp->len = offsetof(MCTPControlMessage, data);
> +
> + trace_i2c_mctp_handle_control(msg->command);
> +
> + switch (msg->command) {
> + case MCTP_CONTROL_SET_EID:
> + i2c_mctp_handle_control_set_eid(mctp, msg->data[1]);
> + break;
> +
> + case MCTP_CONTROL_GET_EID:
> + i2c_mctp_handle_control_get_eid(mctp);
> + break;
> +
> + case MCTP_CONTROL_GET_VERSION:
> + i2c_mctp_handle_control_get_version(mctp);
> + break;
> +
> + case MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT:
> + mctp->len += mc->get_message_types(mctp, i2c_mctp_control_data(mctp->buffer));
You don't pass in how much data is available for the subclass to use.
That's generally good API behavior.
> + break;
> +
> + default:
> + trace_i2c_mctp_unhandled_control(msg->command);
> +
> + msg->data[0] = MCTP_CONTROL_ERROR_UNSUPPORTED_CMD;
> + mctp->len++;
> +
> + break;
> + }
> +
> + i2c_mctp_schedule_send(mctp);
> +}
> +
> +static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
> +{
> + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> + size_t payload_len;
> + uint8_t pec;
> +
> + switch (event) {
> + case I2C_START_SEND:
> + if (mctp->state != I2C_MCTP_STATE_IDLE) {
> + return -1;
> + }
> +
> + /* the i2c core eats the slave address, so put it back in */
> + pkt->i2c.dest = i2c->address << 1;
This seems like a bit of a hack since pkt->i2c.dest never seems to be
used. I guess it's ok, since it's matching what the specifications say,
but it seems a bit odd since you don't need it.
> + mctp->len = 1;
> +
> + mctp->state = I2C_MCTP_STATE_RX_STARTED;
> +
> + return 0;
> +
> + case I2C_FINISH:
> + payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, mctp.payload));
Is there a way this can underflow?
> +
> + if (pkt->i2c.byte_count + 3 != mctp->len - 1) {
> + trace_i2c_mctp_drop_invalid_length(pkt->i2c.byte_count + 3,
> + mctp->len - 1);
> + goto drop;
> + }
> +
> + pec = i2c_smbus_pec(0, mctp->buffer, mctp->len - 1);
> + if (mctp->buffer[mctp->len - 1] != pec) {
> + trace_i2c_mctp_drop_invalid_pec(mctp->buffer[mctp->len - 1], pec);
> + goto drop;
> + }
> +
> + if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
> + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
> + mctp->my_eid);
> + goto drop;
> + }
> +
> + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) {
> + mctp->tx.is_control = false;
> +
> + if (mctp->state == I2C_MCTP_STATE_RX) {
> + mc->reset_message(mctp);
> + }
> +
> + mctp->state = I2C_MCTP_STATE_RX;
> +
> + mctp->tx.addr = pkt->i2c.source;
> + mctp->tx.eid = pkt->mctp.hdr.eid.source;
> + mctp->tx.flags = pkt->mctp.hdr.flags & 0x7;
> + mctp->tx.pktseq = (pkt->mctp.hdr.flags >> 4) & 0x3;
> +
> + if ((pkt->mctp.payload[0] & 0x7f) == MCTP_MESSAGE_TYPE_CONTROL) {
> + mctp->tx.is_control = true;
> +
> + i2c_mctp_handle_control(mctp);
> +
> + return 0;
> + }
> + } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
> + trace_i2c_mctp_drop("expected SOM");
> + goto drop;
> + } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ & 0x3)) {
> + trace_i2c_mctp_drop_invalid_pktseq((pkt->mctp.hdr.flags >> 4) & 0x3,
> + mctp->tx.pktseq & 0x3);
> + goto drop;
> + }
> +
> + mc->put_message_bytes(mctp, i2c_mctp_payload(mctp->buffer), payload_len);
> +
> + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_EOM) {
> + mc->handle_message(mctp);
> + mctp->state = I2C_MCTP_STATE_WAIT_TX;
> + }
> +
> + return 0;
> +
> + default:
> + return -1;
> + }
> +
> +drop:
> + mc->reset_message(mctp);
> +
> + mctp->state = I2C_MCTP_STATE_IDLE;
> +
> + return 0;
> +}
> +
> +static int i2c_mctp_send_cb(I2CSlave *i2c, uint8_t data)
> +{
> + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> +
> + if (mctp->len < I2C_MCTP_MAX_LENGTH) {
> + mctp->buffer[mctp->len++] = data;
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> +static void i2c_mctp_instance_init(Object *obj)
> +{
> + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(obj);
> +
> + mctp->tx.bh = qemu_bh_new(i2c_mctp_tx, mctp);
> +}
> +
> +static Property mctp_i2c_props[] = {
> + DEFINE_PROP_UINT8("eid", MCTPI2CEndpoint, my_eid, 0x9),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void i2c_mctp_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + I2CSlaveClass *k = I2C_SLAVE_CLASS(oc);
> +
> + k->event = i2c_mctp_event_cb;
> + k->send = i2c_mctp_send_cb;
> +
> + device_class_set_props(dc, mctp_i2c_props);
> +}
> +
> +static const TypeInfo i2c_mctp_info = {
> + .name = TYPE_MCTP_I2C_ENDPOINT,
> + .parent = TYPE_I2C_SLAVE,
> + .abstract = true,
> + .instance_init = i2c_mctp_instance_init,
> + .instance_size = sizeof(MCTPI2CEndpoint),
> + .class_init = i2c_mctp_class_init,
> + .class_size = sizeof(MCTPI2CEndpointClass),
> +};
> +
> +static void register_types(void)
> +{
> + type_register_static(&i2c_mctp_info);
> +}
> +
> +type_init(register_types)
> diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
> index d3df273251f7..97d0bfd9ac67 100644
> --- a/hw/i2c/meson.build
> +++ b/hw/i2c/meson.build
> @@ -1,5 +1,6 @@
> i2c_ss = ss.source_set()
> i2c_ss.add(when: 'CONFIG_I2C', if_true: files('core.c'))
> +i2c_ss.add(when: 'CONFIG_MCTP_I2C', if_true: files('mctp.c'))
> i2c_ss.add(when: 'CONFIG_SMBUS', if_true: files('smbus_slave.c', 'smbus_master.c'))
> i2c_ss.add(when: 'CONFIG_ACPI_SMBUS', if_true: files('pm_smbus.c'))
> i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('smbus_ich9.c'))
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index af181d43ee64..778139d08166 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -33,3 +33,15 @@ npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s
>
> pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x"
> pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
> +
> +# mctp.c
> +i2c_mctp_tx_start_send(size_t len) "len %zu"
> +i2c_mctp_tx_send_byte(size_t pos, uint8_t byte) "pos %zu byte 0x%"PRIx8""
> +i2c_mctp_tx_done(void) "packet sent"
> +i2c_mctp_handle_control(uint8_t command) "command 0x%"PRIx8""
> +i2c_mctp_unhandled_control(uint8_t command) "command 0x%"PRIx8""
> +i2c_mctp_drop(const char *reason) "%s"
> +i2c_mctp_drop_invalid_length(unsigned byte_count, size_t expected) "byte_count %u expected %zu"
> +i2c_mctp_drop_invalid_pec(uint8_t pec, uint8_t expected) "pec 0x%"PRIx8" expected 0x%"PRIx8""
> +i2c_mctp_drop_invalid_eid(uint8_t eid, uint8_t expected) "eid 0x%"PRIx8" expected 0x%"PRIx8""
> +i2c_mctp_drop_invalid_pktseq(uint8_t pktseq, uint8_t expected) "pktseq 0x%"PRIx8" expected 0x%"PRIx8""
> diff --git a/include/hw/i2c/mctp.h b/include/hw/i2c/mctp.h
> new file mode 100644
> index 000000000000..17dae662138b
> --- /dev/null
> +++ b/include/hw/i2c/mctp.h
> @@ -0,0 +1,83 @@
> +#ifndef QEMU_I2C_MCTP_H
> +#define QEMU_I2C_MCTP_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "hw/misc/mctp.h"
> +
> +typedef struct MCTPI2CPacketHeader {
> + uint8_t dest;
> + uint8_t prot;
> + uint8_t byte_count;
> + uint8_t source;
> +} MCTPI2CPacketHeader;
> +
> +typedef struct MCTPI2CPacket {
> + MCTPI2CPacketHeader i2c;
> + MCTPPacket mctp;
> +} MCTPI2CPacket;
> +
> +#define i2c_mctp_payload(buf) (buf + offsetof(MCTPI2CPacket, mctp.payload))
> +
> +#define TYPE_MCTP_I2C_ENDPOINT "mctp-i2c-endpoint"
> +OBJECT_DECLARE_TYPE(MCTPI2CEndpoint, MCTPI2CEndpointClass, MCTP_I2C_ENDPOINT)
> +
> +struct MCTPI2CEndpointClass {
> + I2CSlaveClass parent_class;
> +
> + int (*put_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf, size_t len);
> + size_t (*get_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf,
> + size_t maxlen, uint8_t *mctp_flags);
> +
> + void (*handle_message)(MCTPI2CEndpoint *mctp);
> + void (*reset_message)(MCTPI2CEndpoint *mctp);
> +
> + size_t (*get_message_types)(MCTPI2CEndpoint *mctp, uint8_t *data);
> +};
> +
> +#define I2C_MCTP_MAXBLOCK 255
> +#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1))
> +#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1)
> +
> +typedef enum {
> + I2C_MCTP_STATE_IDLE,
> + I2C_MCTP_STATE_RX_STARTED,
> + I2C_MCTP_STATE_RX,
> + I2C_MCTP_STATE_WAIT_TX,
> + I2C_MCTP_STATE_TX,
> +} MCTPState;
> +
> +typedef enum {
> + I2C_MCTP_STATE_TX_START_SEND,
> + I2C_MCTP_STATE_TX_SEND_BYTE,
> +} MCTPTxState;
> +
> +typedef struct MCTPI2CEndpoint {
> + I2CSlave parent_obj;
> + I2CBus *i2c;
> +
> + MCTPState state;
> +
> + /* mctp endpoint identifier */
> + uint8_t my_eid;
> +
> + uint8_t buffer[I2C_MCTP_MAX_LENGTH];
> + uint64_t pos;
> + size_t len;
> +
> + struct {
> + MCTPTxState state;
> + bool is_control;
> +
> + uint8_t eid;
> + uint8_t addr;
> + uint8_t pktseq;
> + uint8_t flags;
> +
> + QEMUBH *bh;
> + } tx;
> +} MCTPI2CEndpoint;
> +
> +void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp);
> +
> +#endif /* QEMU_I2C_MCTP_H */
> diff --git a/include/hw/misc/mctp.h b/include/hw/misc/mctp.h
> new file mode 100644
> index 000000000000..c936224ecf60
> --- /dev/null
> +++ b/include/hw/misc/mctp.h
> @@ -0,0 +1,43 @@
> +#ifndef QEMU_MCTP_H
> +#define QEMU_MCTP_H
> +
> +#define MCTP_BASELINE_MTU 64
> +
> +enum {
> + MCTP_H_FLAGS_EOM = 1 << 6,
> + MCTP_H_FLAGS_SOM = 1 << 7,
> +};
> +
> +enum {
> + MCTP_MESSAGE_TYPE_CONTROL = 0x0,
> + MCTP_MESSAGE_TYPE_NMI = 0x4,
> +
> + MCTP_MESSAGE_IC = 1 << 7,
> +};
> +
> +typedef struct MCTPPacketHeader {
> + uint8_t version;
> + struct {
> + uint8_t dest;
> + uint8_t source;
> + } eid;
> + uint8_t flags;
> +} MCTPPacketHeader;
> +
> +typedef struct MCTPPacket {
> + MCTPPacketHeader hdr;
> + uint8_t payload[];
> +} MCTPPacket;
> +
> +typedef struct MCTPControlMessage {
> + uint8_t type;
> + uint8_t flags;
> + uint8_t command;
> + uint8_t data[];
> +} MCTPControlMessage;
> +
> +enum {
> + MCTP_CONTROL_ERROR_UNSUPPORTED_CMD = 0x5,
> +};
> +
> +#endif /* QEMU_MCTP_H */
> --
> 2.38.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-16 14:27 ` Corey Minyard
@ 2022-11-17 6:51 ` Klaus Jensen
0 siblings, 0 replies; 26+ messages in thread
From: Klaus Jensen @ 2022-11-17 6:51 UTC (permalink / raw)
To: Corey Minyard
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Cédric Le Goater, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 11315 bytes --]
On Nov 16 08:27, Corey Minyard wrote:
> On Wed, Nov 16, 2022 at 09:43:11AM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > control message handling as well as handling the actual I2C transport
> > (packetization).
> >
> > Devices are intended to derive from this and implement the class
> > methods.
> >
> > Parts of this implementation is inspired by code[1] previously posted by
> > Jonathan Cameron.
>
> I have some comments inline, mostly about buffer handling. Buffer
> handling is scary to me, so you might see some paranoia here :-).
>
Totally understood :) Thanks for the review!
> >
> > [1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> > hw/arm/Kconfig | 1 +
> > hw/i2c/Kconfig | 4 +
> > hw/i2c/mctp.c | 365 +++++++++++++++++++++++++++++++++++++++++
> > hw/i2c/meson.build | 1 +
> > hw/i2c/trace-events | 12 ++
> > include/hw/i2c/mctp.h | 83 ++++++++++
> > include/hw/misc/mctp.h | 43 +++++
> > 7 files changed, 509 insertions(+)
> > create mode 100644 hw/i2c/mctp.c
> > create mode 100644 include/hw/i2c/mctp.h
> > create mode 100644 include/hw/misc/mctp.h
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 17fcde8e1ccc..3233bdc193d7 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -444,6 +444,7 @@ config ASPEED_SOC
> > select DS1338
> > select FTGMAC100
> > select I2C
> > + select MCTP_I2C
> > select DPS310
> > select PCA9552
> > select SERIAL
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 9bb8870517f8..5dd43d550c32 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -41,3 +41,7 @@ config PCA954X
> > config PMBUS
> > bool
> > select SMBUS
> > +
> > +config MCTP_I2C
> > + bool
> > + select I2C
> > diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
> > new file mode 100644
> > index 000000000000..46376de95a98
> > --- /dev/null
> > +++ b/hw/i2c/mctp.c
> > @@ -0,0 +1,365 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
> > + * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> > +
> > +#include "hw/qdev-properties.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "hw/i2c/mctp.h"
> > +
> > +#include "trace.h"
> > +
> > +static uint8_t crc8(uint16_t data)
> > +{
> > +#define POLY (0x1070U << 3)
> > + int i;
> > +
> > + for (i = 0; i < 8; i++) {
> > + if (data & 0x8000) {
> > + data = data ^ POLY;
> > + }
> > +
> > + data = data << 1;
> > + }
> > +
> > + return (uint8_t)(data >> 8);
> > +#undef POLY
> > +}
> > +
> > +static uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < len; i++) {
> > + crc = crc8((crc ^ buf[i]) << 8);
> > + }
> > +
> > + return crc;
> > +}
>
> The PEC calculation probably belongs in it's own smbus.c file, since
> it's generic, so someone looking will find it.
>
Makes sense. I'll move it.
> > +
> > +void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp)
> > +{
> > + I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(DEVICE(mctp)));
> > +
> > + mctp->tx.state = I2C_MCTP_STATE_TX_START_SEND;
> > +
> > + i2c_bus_master(i2c, mctp->tx.bh);
> > +}
> > +
> > +static void i2c_mctp_tx(void *opaque)
> > +{
> > + DeviceState *dev = DEVICE(opaque);
> > + I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> > + I2CSlave *slave = I2C_SLAVE(dev);
> > + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> > + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > + uint8_t flags = 0;
> > +
> > + switch (mctp->tx.state) {
> > + case I2C_MCTP_STATE_TX_SEND_BYTE:
> > + if (mctp->pos < mctp->len) {
> > + uint8_t byte = mctp->buffer[mctp->pos];
> > +
> > + trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> > +
> > + /* send next byte */
> > + i2c_send_async(i2c, byte);
> > +
> > + mctp->pos++;
> > +
> > + break;
> > + }
> > +
> > + /* packet sent */
> > + i2c_end_transfer(i2c);
> > +
> > + /* fall through */
> > +
> > + case I2C_MCTP_STATE_TX_START_SEND:
> > + if (mctp->tx.is_control) {
> > + /* packet payload is already in buffer */
> > + flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> > + } else {
> > + /* get message bytes from derived device */
> > + mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> > + I2C_MCTP_MAXMTU, &flags);
> > + }
> > +
> > + if (!mctp->len) {
> > + trace_i2c_mctp_tx_done();
> > +
> > + /* no more packets needed; release the bus */
> > + i2c_bus_release(i2c);
> > +
> > + mctp->state = I2C_MCTP_STATE_IDLE;
> > + mctp->tx.is_control = false;
> > +
> > + break;
> > + }
> > +
> > + mctp->state = I2C_MCTP_STATE_TX;
> > +
> > + pkt->i2c = (MCTPI2CPacketHeader) {
> > + .dest = mctp->tx.addr & ~0x1,
> > + .prot = 0xf,
> > + .byte_count = 5 + mctp->len,
> > + .source = slave->address << 1 | 0x1,
> > + };
> > +
> > + pkt->mctp.hdr = (MCTPPacketHeader) {
> > + .version = 0x1,
> > + .eid.dest = mctp->tx.eid,
> > + .eid.source = mctp->my_eid,
> > + .flags = flags | (mctp->tx.pktseq++ & 0x3) << 4 | mctp->tx.flags,
> > + };
> > +
> > + mctp->len += sizeof(MCTPI2CPacket);
>
> Do you need overflow checking here? There are lots of increments of
> mctp->len in the code that might or might not need overflow checks.
> It does seem like you have pre-calculated everything so it fits; I worry
> more about later changes that might violate those assumptions.
> You could use something like i2c_mctp_send_cb() to send all data. Not
> sure, but something to think about.
>
I agree. It would be better to be a bit defensive here. I'll rework it.
> > + mctp->buffer[mctp->len] = i2c_smbus_pec(0, mctp->buffer, mctp->len);
> > + mctp->len++;
> > +
> > + trace_i2c_mctp_tx_start_send(mctp->len);
> > +
> > + i2c_start_send_async(i2c, pkt->i2c.dest >> 1);
> > +
> > + /* already "sent" the destination slave address */
> > + mctp->pos = 1;
> > +
> > + mctp->tx.state = I2C_MCTP_STATE_TX_SEND_BYTE;
> > +
> > + break;
> > + }
> > +}
> > +
> > +#define i2c_mctp_control_data(buf) \
> > + (i2c_mctp_payload(buf) + offsetof(MCTPControlMessage, data))
> > +
> > +static void i2c_mctp_handle_control_set_eid(MCTPI2CEndpoint *mctp, uint8_t eid)
> > +{
> > + mctp->my_eid = eid;
> > +
> > + uint8_t buf[] = {
> > + 0x0, 0x0, eid, 0x0,
> > + };
> > +
> > + memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> > + mctp->len += sizeof(buf);
> > +}
> > +
> > +static void i2c_mctp_handle_control_get_eid(MCTPI2CEndpoint *mctp)
> > +{
> > + uint8_t buf[] = {
> > + 0x0, mctp->my_eid, 0x0, 0x0,
> > + };
> > +
> > + memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> > + mctp->len += sizeof(buf);
> > +}
> > +
> > +static void i2c_mctp_handle_control_get_version(MCTPI2CEndpoint *mctp)
> > +{
> > + uint8_t buf[] = {
> > + 0x0, 0x1, 0x0, 0x1, 0x3, 0x1,
> > + };
> > +
> > + memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> > + mctp->len += sizeof(buf);
> > +}
> > +
> > +enum {
> > + MCTP_CONTROL_SET_EID = 0x01,
> > + MCTP_CONTROL_GET_EID = 0x02,
> > + MCTP_CONTROL_GET_VERSION = 0x04,
> > + MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT = 0x05,
> > +};
> > +
> > +static void i2c_mctp_handle_control(MCTPI2CEndpoint *mctp)
> > +{
> > + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > + MCTPControlMessage *msg = (MCTPControlMessage *)i2c_mctp_payload(mctp->buffer);
> > +
> > + /* clear Rq/D */
> > + msg->flags &= 0x1f;
> > +
> > + mctp->len = offsetof(MCTPControlMessage, data);
> > +
> > + trace_i2c_mctp_handle_control(msg->command);
> > +
> > + switch (msg->command) {
> > + case MCTP_CONTROL_SET_EID:
> > + i2c_mctp_handle_control_set_eid(mctp, msg->data[1]);
> > + break;
> > +
> > + case MCTP_CONTROL_GET_EID:
> > + i2c_mctp_handle_control_get_eid(mctp);
> > + break;
> > +
> > + case MCTP_CONTROL_GET_VERSION:
> > + i2c_mctp_handle_control_get_version(mctp);
> > + break;
> > +
> > + case MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT:
> > + mctp->len += mc->get_message_types(mctp, i2c_mctp_control_data(mctp->buffer));
>
> You don't pass in how much data is available for the subclass to use.
> That's generally good API behavior.
>
True, I'll fix it up.
> > + break;
> > +
> > + default:
> > + trace_i2c_mctp_unhandled_control(msg->command);
> > +
> > + msg->data[0] = MCTP_CONTROL_ERROR_UNSUPPORTED_CMD;
> > + mctp->len++;
> > +
> > + break;
> > + }
> > +
> > + i2c_mctp_schedule_send(mctp);
> > +}
> > +
> > +static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
> > +{
> > + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> > + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > + size_t payload_len;
> > + uint8_t pec;
> > +
> > + switch (event) {
> > + case I2C_START_SEND:
> > + if (mctp->state != I2C_MCTP_STATE_IDLE) {
> > + return -1;
> > + }
> > +
> > + /* the i2c core eats the slave address, so put it back in */
> > + pkt->i2c.dest = i2c->address << 1;
>
> This seems like a bit of a hack since pkt->i2c.dest never seems to be
> used. I guess it's ok, since it's matching what the specifications say,
> but it seems a bit odd since you don't need it.
>
Yeah it is definitely a hack around the i2c core. I need it to calculate
the PEC, so I have to put it into the buffer at some point. I think the
smbus implementation would suffer from this as well. We could maybe fold
that into the i2c_smbus_pec() call instead. I'll see what I can come up
with.
> > + mctp->len = 1;
> > +
> > + mctp->state = I2C_MCTP_STATE_RX_STARTED;
> > +
> > + return 0;
> > +
> > + case I2C_FINISH:
> > + payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, mctp.payload));
>
> Is there a way this can underflow?
>
Hmm. Potentially. I'll audit it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-16 8:43 ` [PATCH RFC 2/3] hw/i2c: add mctp core Klaus Jensen
2022-11-16 14:27 ` Corey Minyard
@ 2022-11-18 5:56 ` Jeremy Kerr
2022-11-18 6:15 ` Jeremy Kerr
2022-11-18 7:01 ` Klaus Jensen
1 sibling, 2 replies; 26+ messages in thread
From: Jeremy Kerr @ 2022-11-18 5:56 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Joel Stanley,
Cédric Le Goater, Klaus Jensen, Matt Johnston
Hi Klaus,
> Add an abstract MCTP over I2C endpoint model. This implements MCTP
> control message handling as well as handling the actual I2C transport
> (packetization).
>
> Devices are intended to derive from this and implement the class
> methods.
Looks good, nice to see how it's used by the nmi device later too.
A couple of issues with the state machine though, comments inline, and
a bit of a patch below.
> +static void i2c_mctp_tx(void *opaque)
> +{
> + DeviceState *dev = DEVICE(opaque);
> + I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> + I2CSlave *slave = I2C_SLAVE(dev);
> + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> + uint8_t flags = 0;
> +
> + switch (mctp->tx.state) {
> + case I2C_MCTP_STATE_TX_SEND_BYTE:
> + if (mctp->pos < mctp->len) {
> + uint8_t byte = mctp->buffer[mctp->pos];
> +
> + trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> +
> + /* send next byte */
> + i2c_send_async(i2c, byte);
> +
> + mctp->pos++;
> +
> + break;
> + }
> +
> + /* packet sent */
> + i2c_end_transfer(i2c);
If we're sending a control message, mctp->len will be set to the control
msg len here, then:
> +
> + /* fall through */
> +
> + case I2C_MCTP_STATE_TX_START_SEND:
> + if (mctp->tx.is_control) {
> + /* packet payload is already in buffer */
> + flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> + } else {
> + /* get message bytes from derived device */
> + mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> + I2C_MCTP_MAXMTU, &flags);
> + }
... it doesn't get cleared above, so:
> +
> + if (!mctp->len) {
... we don't hit this conditional, and hence keep sending unlimited
bytes. This presents as continuous interrupts to the aspeed i2c driver
when replying to any control message.
I think we need a mctp->len = 0 with the i2c_end_transfer(). With that,
I can get control protocol communication working with mctpd.
> +static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
> +{
> + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> + size_t payload_len;
> + uint8_t pec;
> +
> + switch (event) {
> + case I2C_START_SEND:
> + if (mctp->state != I2C_MCTP_STATE_IDLE) {
> + return -1;
mctp->state may (validly) be I2C_MCTP_STATE_RX here, if we're receiving
the start event for the second packet of a multi-packet message.
> + }
> +
> + /* the i2c core eats the slave address, so put it back in */
> + pkt->i2c.dest = i2c->address << 1;
> + mctp->len = 1;
> +
> + mctp->state = I2C_MCTP_STATE_RX_STARTED;
> +
> + return 0;
> +
> + case I2C_FINISH:
> + payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, mctp.payload));
> +
> + if (pkt->i2c.byte_count + 3 != mctp->len - 1) {
> + trace_i2c_mctp_drop_invalid_length(pkt->i2c.byte_count +
> 3,
> + mctp->len - 1);
> + goto drop;
> + }
> +
> + pec = i2c_smbus_pec(0, mctp->buffer, mctp->len - 1);
> + if (mctp->buffer[mctp->len - 1] != pec) {
> + trace_i2c_mctp_drop_invalid_pec(mctp->buffer[mctp->len - 1], pec);
> + goto drop;
> + }
> +
> + if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
> + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
> + mctp->my_eid);
> + goto drop;
> + }
> +
> + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) {
> + mctp->tx.is_control = false;
> +
> + if (mctp->state == I2C_MCTP_STATE_RX) {
> + mc->reset_message(mctp);
> + }
> +
> + mctp->state = I2C_MCTP_STATE_RX;
> +
> + mctp->tx.addr = pkt->i2c.source;
> + mctp->tx.eid = pkt->mctp.hdr.eid.source;
> + mctp->tx.flags = pkt->mctp.hdr.flags & 0x7;
> + mctp->tx.pktseq = (pkt->mctp.hdr.flags >> 4) & 0x3;
> +
> + if ((pkt->mctp.payload[0] & 0x7f) == MCTP_MESSAGE_TYPE_CONTROL) {
> + mctp->tx.is_control = true;
> +
> + i2c_mctp_handle_control(mctp);
> +
> + return 0;
> + }
> + } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
> + trace_i2c_mctp_drop("expected SOM");
> + goto drop;
> + } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ & 0x3)) {
The pktseq is the sequence number of the last packet seen, so you want a
pre-increment there: ++mctp->tx.pktseq & 0x3
} else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 0x3)) {
With those changes, I can get control protocol going, and multi-packet
messages work. There's a couple of failures from unsupported commands,
but otherwise looks good:
# mctp addr add 8 dev mctpi2c6
# mctp link set mctpi2c6 up
# mctp link set mctpi2c6 mtu 254
# systemctl restart mctpd
# busctl --no-pager call xyz.openbmc_project.MCTP \
/xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \
SetupEndpoint say mctpi2c6 1 0x1d
yisb 9 1 "/xyz/openbmc_project/mctp/1/9" true
# mctp route del 9 via mctpi2c6
# mctp route add 9 via mctpi2c6 mtu 68
# mi-mctp 1 9 info
nmi message type 0x2 not handled
Identify Controller failed, no quirks applied
NVMe MI subsys info:
num ports: 1
major ver: 1
minor ver: 1
NVMe MI port info:
port 0
type SMBus[2]
MCTP MTU: 64
MEB size: 0
SMBus address: 0x00
VPD access freq: 0x00
MCTP address: 0x1d
MCTP access freq: 0x01
NVMe basic management: disabled
nmi command 0x1 not handled
mi-mctp: can't perform Health Status Poll operation: Success
# mi-mctp 1 9 get-config 0
nmi message type 0x2 not handled
Identify Controller failed, no quirks applied
SMBus access frequency (port 0): 100k [0x1]
MCTP MTU (port 0): 64
I've included a patch below, with some fixes for the above, in case that
helps.
Cheers,
Jeremy
diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
index 46376de95a..1775deb46f 100644
--- a/hw/i2c/mctp.c
+++ b/hw/i2c/mctp.c
@@ -78,6 +78,9 @@ static void i2c_mctp_tx(void *opaque)
/* packet sent */
i2c_end_transfer(i2c);
+ /* end of any control data */
+ mctp->len = 0;
+
/* fall through */
case I2C_MCTP_STATE_TX_START_SEND:
@@ -228,7 +231,9 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
switch (event) {
case I2C_START_SEND:
- if (mctp->state != I2C_MCTP_STATE_IDLE) {
+ if (mctp->state == I2C_MCTP_STATE_IDLE) {
+ mctp->state = I2C_MCTP_STATE_RX_STARTED;
+ } else if (mctp->state != I2C_MCTP_STATE_RX) {
return -1;
}
@@ -236,8 +241,6 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
pkt->i2c.dest = i2c->address << 1;
mctp->len = 1;
- mctp->state = I2C_MCTP_STATE_RX_STARTED;
-
return 0;
case I2C_FINISH:
@@ -285,7 +288,7 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
} else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
trace_i2c_mctp_drop("expected SOM");
goto drop;
- } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ & 0x3)) {
+ } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 0x3)) {
trace_i2c_mctp_drop_invalid_pktseq((pkt->mctp.hdr.flags >> 4) & 0x3,
mctp->tx.pktseq & 0x3);
goto drop;
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-18 5:56 ` Jeremy Kerr
@ 2022-11-18 6:15 ` Jeremy Kerr
2022-11-18 7:03 ` Klaus Jensen
2022-11-18 7:01 ` Klaus Jensen
1 sibling, 1 reply; 26+ messages in thread
From: Jeremy Kerr @ 2022-11-18 6:15 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Joel Stanley,
Cédric Le Goater, Klaus Jensen, Matt Johnston
Hi Klaus,
> With those changes, I can get control protocol going, and multi-
> packet messages work.
Ah, I also needed a change to the aspeed I2C driver, as I'm seeing
the interrupt handler being invoked with both a stop and a start event
pending.
Patch below; if this seems sensible I will propose upstream.
Cheers,
Jeremy
---
From 6331cf2499c182606979029d2aaed93ee3e544fa Mon Sep 17 00:00:00 2001
From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: Fri, 18 Nov 2022 14:04:50 +0800
Subject: [PATCH] i2c: aspeed: Allow combined STOP & START irqs
Currently, if we enter our interrupt handler with both a stop and start
condition pending, the stop event handler will override the current
state, so we'll then lose the start condition.
This change handles the stop condition before anything else, which means
we can then restart the state machine on any pending start state.
Because of this, we no longer need the ASPEED_I2C_SLAVE_STOP state,
because we're just reverting directly to INACTIVE.
I have only seen this condition on emulation; it may be impossible to
hit on real HW.
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
drivers/i2c/busses/i2c-aspeed.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 185dedfebbac..45f2766b2b66 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -135,7 +135,6 @@ enum aspeed_i2c_slave_state {
ASPEED_I2C_SLAVE_READ_PROCESSED,
ASPEED_I2C_SLAVE_WRITE_REQUESTED,
ASPEED_I2C_SLAVE_WRITE_RECEIVED,
- ASPEED_I2C_SLAVE_STOP,
};
struct aspeed_i2c_bus {
@@ -250,6 +249,14 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
command = readl(bus->base + ASPEED_I2C_CMD_REG);
+ /* handle a stop before starting any new events */
+ if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE &&
+ irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+ bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
+ }
+
/* Slave was requested, restart state machine. */
if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
@@ -278,15 +285,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
}
- /* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
- irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
- bus->slave_state = ASPEED_I2C_SLAVE_STOP;
- }
if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
- bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+ bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
}
switch (bus->slave_state) {
@@ -316,13 +319,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
break;
- case ASPEED_I2C_SLAVE_STOP:
- i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
- bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
- break;
case ASPEED_I2C_SLAVE_START:
/* Slave was just started. Waiting for the next event. */;
break;
+ case ASPEED_I2C_SLAVE_INACTIVE:
+ break;
default:
dev_err(bus->dev, "unknown slave_state: %d\n",
bus->slave_state);
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-18 6:15 ` Jeremy Kerr
@ 2022-11-18 7:03 ` Klaus Jensen
2022-11-18 7:09 ` Jeremy Kerr
0 siblings, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2022-11-18 7:03 UTC (permalink / raw)
To: Jeremy Kerr
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Joel Stanley, Cédric Le Goater, Klaus Jensen, Matt Johnston
[-- Attachment #1: Type: text/plain, Size: 654 bytes --]
On Nov 18 14:15, Jeremy Kerr wrote:
> Hi Klaus,
>
> > With those changes, I can get control protocol going, and multi-
> > packet messages work.
>
> Ah, I also needed a change to the aspeed I2C driver, as I'm seeing
> the interrupt handler being invoked with both a stop and a start event
> pending.
>
I had to reverse the target mode functionality in QEMU from the linux
driver, so I am really not too sure if having START and STOP set in the
interrupt register is allowed behavior or not (or if we are doing
something wrong in the target mode implementation). I guess thats a
question for Aspeed I2C maintainers (here and on lkml).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-18 7:03 ` Klaus Jensen
@ 2022-11-18 7:09 ` Jeremy Kerr
0 siblings, 0 replies; 26+ messages in thread
From: Jeremy Kerr @ 2022-11-18 7:09 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Joel Stanley, Cédric Le Goater, Klaus Jensen, Matt Johnston
Hi Klaus,
> I had to reverse the target mode functionality in QEMU from the linux
> driver, so I am really not too sure if having START and STOP set in
> the interrupt register is allowed behavior or not
From my interpretation of things, there's nothing explicitly preventing
both a pending start and stop - more that the interrupt is very likely
to have been serviced between those two events on the kind of speeds we
would see on the i2c bus.
I guess we could try (temporarily) masking the irq on real hardware and
see what happens? :D
Cheers,
Jeremy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-18 5:56 ` Jeremy Kerr
2022-11-18 6:15 ` Jeremy Kerr
@ 2022-11-18 7:01 ` Klaus Jensen
2022-11-21 8:04 ` Matt Johnston
1 sibling, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2022-11-18 7:01 UTC (permalink / raw)
To: Jeremy Kerr
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Joel Stanley, Cédric Le Goater, Klaus Jensen, Matt Johnston
[-- Attachment #1: Type: text/plain, Size: 7829 bytes --]
On Nov 18 13:56, Jeremy Kerr wrote:
> Hi Klaus,
>
> > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > control message handling as well as handling the actual I2C transport
> > (packetization).
> >
> > Devices are intended to derive from this and implement the class
> > methods.
>
> Looks good, nice to see how it's used by the nmi device later too.
>
> A couple of issues with the state machine though, comments inline, and
> a bit of a patch below.
>
> > +static void i2c_mctp_tx(void *opaque)
> > +{
> > + DeviceState *dev = DEVICE(opaque);
> > + I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> > + I2CSlave *slave = I2C_SLAVE(dev);
> > + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> > + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > + uint8_t flags = 0;
> > +
> > + switch (mctp->tx.state) {
> > + case I2C_MCTP_STATE_TX_SEND_BYTE:
> > + if (mctp->pos < mctp->len) {
> > + uint8_t byte = mctp->buffer[mctp->pos];
> > +
> > + trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> > +
> > + /* send next byte */
> > + i2c_send_async(i2c, byte);
> > +
> > + mctp->pos++;
> > +
> > + break;
> > + }
> > +
> > + /* packet sent */
> > + i2c_end_transfer(i2c);
>
> If we're sending a control message, mctp->len will be set to the control
> msg len here, then:
>
> > +
> > + /* fall through */
> > +
> > + case I2C_MCTP_STATE_TX_START_SEND:
> > + if (mctp->tx.is_control) {
> > + /* packet payload is already in buffer */
> > + flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> > + } else {
> > + /* get message bytes from derived device */
> > + mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> > + I2C_MCTP_MAXMTU, &flags);
> > + }
>
> ... it doesn't get cleared above, so:
>
> > +
> > + if (!mctp->len) {
>
> ... we don't hit this conditional, and hence keep sending unlimited
> bytes. This presents as continuous interrupts to the aspeed i2c driver
> when replying to any control message.
>
> I think we need a mctp->len = 0 with the i2c_end_transfer(). With that,
> I can get control protocol communication working with mctpd.
>
> > +static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
> > +{
> > + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> > + MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > + MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > + size_t payload_len;
> > + uint8_t pec;
> > +
> > + switch (event) {
> > + case I2C_START_SEND:
> > + if (mctp->state != I2C_MCTP_STATE_IDLE) {
> > + return -1;
>
> mctp->state may (validly) be I2C_MCTP_STATE_RX here, if we're receiving
> the start event for the second packet of a multi-packet message.
>
> > + }
> > +
> > + /* the i2c core eats the slave address, so put it back in */
> > + pkt->i2c.dest = i2c->address << 1;
> > + mctp->len = 1;
> > +
> > + mctp->state = I2C_MCTP_STATE_RX_STARTED;
> > +
> > + return 0;
> > +
> > + case I2C_FINISH:
> > + payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, mctp.payload));
> > +
> > + if (pkt->i2c.byte_count + 3 != mctp->len - 1) {
> > + trace_i2c_mctp_drop_invalid_length(pkt->i2c.byte_count +
> > 3,
> > + mctp->len - 1);
> > + goto drop;
> > + }
> > +
> > + pec = i2c_smbus_pec(0, mctp->buffer, mctp->len - 1);
> > + if (mctp->buffer[mctp->len - 1] != pec) {
> > + trace_i2c_mctp_drop_invalid_pec(mctp->buffer[mctp->len - 1], pec);
> > + goto drop;
> > + }
> > +
> > + if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
> > + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
> > + mctp->my_eid);
> > + goto drop;
> > + }
> > +
> > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) {
> > + mctp->tx.is_control = false;
> > +
> > + if (mctp->state == I2C_MCTP_STATE_RX) {
> > + mc->reset_message(mctp);
> > + }
> > +
> > + mctp->state = I2C_MCTP_STATE_RX;
> > +
> > + mctp->tx.addr = pkt->i2c.source;
> > + mctp->tx.eid = pkt->mctp.hdr.eid.source;
> > + mctp->tx.flags = pkt->mctp.hdr.flags & 0x7;
> > + mctp->tx.pktseq = (pkt->mctp.hdr.flags >> 4) & 0x3;
> > +
> > + if ((pkt->mctp.payload[0] & 0x7f) == MCTP_MESSAGE_TYPE_CONTROL) {
> > + mctp->tx.is_control = true;
> > +
> > + i2c_mctp_handle_control(mctp);
> > +
> > + return 0;
> > + }
> > + } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
> > + trace_i2c_mctp_drop("expected SOM");
> > + goto drop;
> > + } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ & 0x3)) {
>
> The pktseq is the sequence number of the last packet seen, so you want a
> pre-increment there: ++mctp->tx.pktseq & 0x3
>
> } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 0x3)) {
>
> With those changes, I can get control protocol going, and multi-packet
> messages work. There's a couple of failures from unsupported commands,
> but otherwise looks good:
>
> # mctp addr add 8 dev mctpi2c6
> # mctp link set mctpi2c6 up
> # mctp link set mctpi2c6 mtu 254
> # systemctl restart mctpd
> # busctl --no-pager call xyz.openbmc_project.MCTP \
> /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \
> SetupEndpoint say mctpi2c6 1 0x1d
> yisb 9 1 "/xyz/openbmc_project/mctp/1/9" true
> # mctp route del 9 via mctpi2c6
> # mctp route add 9 via mctpi2c6 mtu 68
> # mi-mctp 1 9 info
> nmi message type 0x2 not handled
> Identify Controller failed, no quirks applied
> NVMe MI subsys info:
> num ports: 1
> major ver: 1
> minor ver: 1
> NVMe MI port info:
> port 0
> type SMBus[2]
> MCTP MTU: 64
> MEB size: 0
> SMBus address: 0x00
> VPD access freq: 0x00
> MCTP address: 0x1d
> MCTP access freq: 0x01
> NVMe basic management: disabled
> nmi command 0x1 not handled
> mi-mctp: can't perform Health Status Poll operation: Success
> # mi-mctp 1 9 get-config 0
> nmi message type 0x2 not handled
> Identify Controller failed, no quirks applied
> SMBus access frequency (port 0): 100k [0x1]
> MCTP MTU (port 0): 64
>
> I've included a patch below, with some fixes for the above, in case that
> helps.
>
Thanks for the review and patch,
Definitely helps!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH RFC 2/3] hw/i2c: add mctp core
2022-11-18 7:01 ` Klaus Jensen
@ 2022-11-21 8:04 ` Matt Johnston
0 siblings, 0 replies; 26+ messages in thread
From: Matt Johnston @ 2022-11-21 8:04 UTC (permalink / raw)
To: Klaus Jensen
Cc: Jeremy Kerr, qemu-devel, Andrew Jeffery, Keith Busch,
Corey Minyard, Peter Delevoryas, qemu-arm, Peter Maydell,
qemu-block, Joel Stanley, Cédric Le Goater, Klaus Jensen
On Fri, Nov 18, 2022 at 08:01:40AM +0100, Klaus Jensen wrote:
> On Nov 18 13:56, Jeremy Kerr wrote:
> > Hi Klaus,
> >
> > > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > > control message handling as well as handling the actual I2C transport
> > > (packetization).
> > >
> > With those changes, I can get control protocol going, and multi-packet
> > messages work. There's a couple of failures from unsupported commands,
> > but otherwise looks good:
> >
> > # mctp addr add 8 dev mctpi2c6
> > # mctp link set mctpi2c6 up
> > # mctp link set mctpi2c6 mtu 254
> > # systemctl restart mctpd
> > # busctl --no-pager call xyz.openbmc_project.MCTP \
> > /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \
> > SetupEndpoint say mctpi2c6 1 0x1d
Hi Klaus,
Thanks for the MCTP model, it's useful here.
I needed the following patch to be able to call SetupEndpoint again when a
device has already been assigned an EID. That tries a Set Endpoint ID/
Get Endpoint ID, addressed to EID 0.
Cheers,
Matt
---
From cb7ad91474367f8e47bdaf03aba9a822f2648f41 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@codeconstruct.com.au>
Date: Mon, 21 Nov 2022 15:10:13 +0800
Subject: [PATCH] i2c/mctp: Allow receiving messages to dest eid 0
The Null Destination ID, 0, is used for MCTP control messages when
addressing by physical ID. That is used for Get Endpoint ID and
Set Endpoint ID when querying/assigning an EID to an endpoint.
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
hw/i2c/mctp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
index 1775deb46f..9d9e519ba9 100644
--- a/hw/i2c/mctp.c
+++ b/hw/i2c/mctp.c
@@ -258,7 +258,8 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
goto drop;
}
- if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
+ if (!(pkt->mctp.hdr.eid.dest == mctp->my_eid
+ || pkt->mctp.hdr.eid.dest == 0)) {
trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
mctp->my_eid);
goto drop;
--
2.37.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH RFC 3/3] hw/nvme: add nvme management interface model
2022-11-16 8:43 [PATCH 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
2022-11-16 8:43 ` [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle Klaus Jensen
2022-11-16 8:43 ` [PATCH RFC 2/3] hw/i2c: add mctp core Klaus Jensen
@ 2022-11-16 8:43 ` Klaus Jensen
2022-11-18 7:56 ` Philippe Mathieu-Daudé
2022-11-16 9:18 ` [PATCH 0/3] hw/{i2c,nvme}: mctp endpoint, " Jeremy Kerr
3 siblings, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2022-11-16 8:43 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Jeremy Kerr, Joel Stanley,
Cédric Le Goater, Klaus Jensen, Klaus Jensen
From: Klaus Jensen <k.jensen@samsung.com>
Add the 'nmi-i2c' device that emulates an NVMe Management Interface
controller.
Initial support is very basic (Read NMI DS, Configuration Get).
This is based on previously posted code by Padmakar Kalghatgi, Arun
Kumar Agasar and Saurav Kumar.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/nvme/meson.build | 1 +
hw/nvme/nmi-i2c.c | 381 +++++++++++++++++++++++++++++++++++++++++++
hw/nvme/trace-events | 6 +
3 files changed, 388 insertions(+)
create mode 100644 hw/nvme/nmi-i2c.c
diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
index 3cf40046eea9..b231e3fa12c2 100644
--- a/hw/nvme/meson.build
+++ b/hw/nvme/meson.build
@@ -1 +1,2 @@
softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 'ns.c', 'subsys.c'))
+softmmu_ss.add(when: 'CONFIG_MCTP_I2C', if_true: files('nmi-i2c.c'))
diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
new file mode 100644
index 000000000000..79fd18cdc5cf
--- /dev/null
+++ b/hw/nvme/nmi-i2c.c
@@ -0,0 +1,381 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-only
+ *
+ * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
+ *
+ * SPDX-FileContributor: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
+ * SPDX-FileContributor: Arun Kumar Agasar <arun.kka@samsung.com>
+ * SPDX-FileContributor: Saurav Kumar <saurav.29@partner.samsung.com>
+ * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/crc32c.h"
+#include "hw/i2c/i2c.h"
+#include "hw/registerfields.h"
+#include "hw/i2c/mctp.h"
+#include "trace.h"
+
+#define NMI_MAX_MESSAGE_LENGTH 4224
+
+#define TYPE_NMI_I2C_DEVICE "nmi-i2c"
+OBJECT_DECLARE_SIMPLE_TYPE(NMIDevice, NMI_I2C_DEVICE)
+
+typedef struct NMIDevice {
+ MCTPI2CEndpoint mctp;
+
+ uint8_t buffer[NMI_MAX_MESSAGE_LENGTH];
+ uint8_t scratch[NMI_MAX_MESSAGE_LENGTH];
+
+ size_t len;
+ int64_t pos;
+} NMIDevice;
+
+FIELD(NMI_NMP, ROR, 7, 1)
+FIELD(NMI_NMP, NMIMT, 3, 4)
+
+#define NMI_NMP_NMIMT_NMI_CMD 0x1
+#define NMI_NMP_NMIMT_NM_ADMIN 0x2
+
+typedef struct NMIMessage {
+ uint8_t mctpd;
+ uint8_t nmp;
+ uint8_t rsvd2[2];
+ uint8_t payload[]; /* includes the Message Integrity Check */
+} NMIMessage;
+
+typedef struct NMIRequest {
+ uint8_t opc;
+ uint8_t rsvd1[3];
+ uint32_t dw0;
+ uint32_t dw1;
+ uint32_t mic;
+} NMIRequest;
+
+typedef struct NMIResponse {
+ uint8_t status;
+ uint8_t response[3];
+ uint8_t payload[]; /* includes the Message Integrity Check */
+} NMIResponse;
+
+typedef enum NMIReadDSType {
+ NMI_CMD_READ_NMI_DS_SUBSYSTEM = 0x0,
+ NMI_CMD_READ_NMI_DS_PORTS = 0x1,
+ NMI_CMD_READ_NMI_DS_CTRL_LIST = 0x2,
+ NMI_CMD_READ_NMI_DS_CTRL_INFO = 0x3,
+ NMI_CMD_READ_NMI_DS_CMD_SUPPORT = 0x4,
+ NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT = 0x5,
+} NMIReadDSType;
+
+static void nmi_handle_mi_read_nmi_ds(NMIDevice *nmi, NMIRequest *request)
+{
+ I2CSlave *i2c = I2C_SLAVE(nmi);
+
+ uint32_t dw0 = le32_to_cpu(request->dw0);
+ uint8_t dtyp = (dw0 >> 24) & 0xf;
+ uint8_t *buf;
+ size_t len;
+
+ trace_nmi_handle_mi_read_nmi_ds(dtyp);
+
+ static uint8_t nmi_ds_subsystem[36] = {
+ 0x00, /* success */
+ 0x20, /* response data length */
+ 0x00, 0x00, /* reserved */
+ 0x00, /* number of ports */
+ 0x01, /* major version */
+ 0x01, /* minor version */
+ };
+
+ static uint8_t nmi_ds_ports[36] = {
+ 0x00, /* success */
+ 0x20, /* response data length */
+ 0x00, 0x00, /* reserved */
+ 0x02, /* port type (smbus) */
+ 0x00, /* reserved */
+ 0x40, 0x00, /* maximum mctp transission unit size (64 bytes) */
+ 0x00, 0x00, 0x00, 0x00, /* management endpoint buffer size */
+ 0x00, 0x00, /* vpd i2c address/freq */
+ 0x00, 0x01, /* management endpoint i2c address/freq */
+ };
+
+ static uint8_t nmi_ds_error[4] = {
+ 0x04, /* invalid parameter */
+ 0x00, /* first invalid bit position */
+ 0x00, 0x00, /* first invalid byte position */
+ };
+
+ static uint8_t nmi_ds_empty[8] = {
+ 0x00, /* success */
+ 0x02, /* response data length */
+ 0x00, 0x00, /* reserved */
+ 0x00, 0x00, /* number of controllers */
+ 0x00, 0x00, /* padding */
+ };
+
+ switch (dtyp) {
+ case NMI_CMD_READ_NMI_DS_SUBSYSTEM:
+ len = 36;
+ buf = nmi_ds_subsystem;
+
+ break;
+
+ case NMI_CMD_READ_NMI_DS_PORTS:
+ len = 36;
+ buf = nmi_ds_ports;
+
+ /* patch in the i2c address of the endpoint */
+ buf[14] = i2c->address;
+
+ break;
+
+ case NMI_CMD_READ_NMI_DS_CTRL_INFO:
+ len = 4;
+ buf = nmi_ds_error;
+
+ break;
+
+ case NMI_CMD_READ_NMI_DS_CTRL_LIST:
+ case NMI_CMD_READ_NMI_DS_CMD_SUPPORT:
+ case NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT:
+ len = 8;
+ buf = nmi_ds_empty;
+
+ break;
+
+ default:
+ len = 4;
+ buf = nmi_ds_error;
+
+ /* patch in the invalid parameter position */
+ buf[2] = 0x03; /* first invalid byte position (dtyp) */
+
+ break;
+ }
+
+ memcpy(nmi->scratch + nmi->pos, buf, len);
+ nmi->pos += len;
+}
+
+enum {
+ NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ = 0x1,
+ NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE = 0x2,
+ NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT = 0x3,
+};
+
+static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest *request)
+{
+ uint32_t dw0 = le32_to_cpu(request->dw0);
+ uint8_t identifier = dw0 & 0xff;
+ uint8_t *buf;
+
+ trace_nmi_handle_mi_config_get(identifier);
+
+ switch (identifier) {
+ case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
+ buf = (uint8_t[]) {
+ 0x0, 0x1, 0x0, 0x0,
+ };
+
+ break;
+
+ case NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE:
+ buf = (uint8_t[]) {
+ 0x0, 0x0, 0x0, 0x0,
+ };
+
+ break;
+
+ case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
+ buf = (uint8_t[]) {
+ 0x0, 0x40, 0x0, 0x0,
+ };
+
+ break;
+ }
+
+ memcpy(nmi->scratch + nmi->pos, buf, 4);
+ nmi->pos += 4;
+}
+
+enum {
+ NMI_CMD_READ_NMI_DS = 0x0,
+ NMI_CMD_CONFIGURATION_GET = 0x4,
+};
+
+static void nmi_set_parameter_error(NMIDevice *nmi, uint8_t bit, uint16_t byte)
+{
+ nmi->scratch[nmi->pos++] = 0x4;
+ nmi->scratch[nmi->pos++] = bit;
+ nmi->scratch[nmi->pos++] = (byte >> 4) & 0xf;
+ nmi->scratch[nmi->pos++] = byte & 0xf;
+}
+
+static void nmi_set_error(NMIDevice *nmi, uint8_t status)
+{
+ uint8_t buf[4] = {};
+
+ buf[0] = status;
+
+ memcpy(nmi->scratch + nmi->pos, buf, 4);
+ nmi->pos += 4;
+}
+
+static void nmi_handle_mi(NMIDevice *nmi, NMIMessage *msg)
+{
+ NMIRequest *request = (NMIRequest *)msg->payload;
+
+ trace_nmi_handle_mi(request->opc);
+
+ switch (request->opc) {
+ case NMI_CMD_READ_NMI_DS:
+ nmi_handle_mi_read_nmi_ds(nmi, request);
+ break;
+
+ case NMI_CMD_CONFIGURATION_GET:
+ nmi_handle_mi_config_get(nmi, request);
+ break;
+
+ default:
+ nmi_set_parameter_error(nmi, 0x0, 0x0);
+ fprintf(stderr, "nmi command 0x%x not handled\n", request->opc);
+
+ break;
+ }
+}
+
+enum {
+ NMI_MESSAGE_TYPE_NMI = 0x1,
+};
+
+static void nmi_handle_message(MCTPI2CEndpoint *mctp)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+ NMIMessage *msg = (NMIMessage *)nmi->buffer;
+ uint32_t crc;
+ uint8_t nmimt;
+
+ uint8_t buf[] = {
+ MCTP_MESSAGE_TYPE_NMI | MCTP_MESSAGE_IC,
+ FIELD_DP8(msg->nmp, NMI_NMP, ROR, 1),
+ 0x0, 0x0,
+ };
+
+ memcpy(nmi->scratch, buf, sizeof(buf));
+ nmi->pos = sizeof(buf);
+
+ nmimt = FIELD_EX8(msg->nmp, NMI_NMP, NMIMT);
+
+ trace_nmi_handle_msg(nmimt);
+
+ switch (nmimt) {
+ case NMI_MESSAGE_TYPE_NMI:
+ nmi_handle_mi(nmi, msg);
+ break;
+
+ default:
+ fprintf(stderr, "nmi message type 0x%x not handled\n", nmimt);
+
+ nmi_set_error(nmi, 0x3);
+
+ break;
+ }
+
+ /* add message integrity check */
+ memset(nmi->scratch + nmi->pos, 0x0, sizeof(crc));
+
+ crc = crc32c(0xffffffff, nmi->scratch, nmi->pos);
+ memcpy(nmi->scratch + nmi->pos, &crc, sizeof(crc));
+
+ nmi->len = nmi->pos + sizeof(crc);
+ nmi->pos = 0;
+
+ i2c_mctp_schedule_send(mctp);
+}
+
+static size_t nmi_get_message_bytes(MCTPI2CEndpoint *mctp, uint8_t *buf,
+ size_t maxlen, uint8_t *mctp_flags)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+ size_t len;
+
+ len = MIN(maxlen, nmi->len - nmi->pos);
+
+ if (len == 0) {
+ return 0;
+ }
+
+ if (nmi->pos == 0) {
+ *mctp_flags |= MCTP_H_FLAGS_SOM;
+ }
+
+ memcpy(buf, nmi->scratch + nmi->pos, len);
+ nmi->pos += len;
+
+ if (nmi->pos == nmi->len) {
+ *mctp_flags |= MCTP_H_FLAGS_EOM;
+
+ nmi->pos = nmi->len = 0;
+ }
+
+ return len;
+}
+
+static int nmi_put_message_bytes(MCTPI2CEndpoint *mctp, uint8_t *buf,
+ size_t len)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+
+ if (nmi->len + len > NMI_MAX_MESSAGE_LENGTH) {
+ return -1;
+ }
+
+ memcpy(nmi->buffer + nmi->len, buf, len);
+ nmi->len += len;
+
+ return 0;
+}
+
+static void nmi_reset_message(MCTPI2CEndpoint *mctp)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+ nmi->len = 0;
+}
+
+static size_t nmi_get_message_types(MCTPI2CEndpoint *mctp, uint8_t *data)
+{
+ uint8_t buf[] = {
+ 0x0, 0x1, 0x4,
+ };
+
+ memcpy(data, buf, sizeof(buf));
+
+ return sizeof(buf);
+}
+
+static void nvme_mi_class_init(ObjectClass *oc, void *data)
+{
+ MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_CLASS(oc);
+
+ mc->get_message_types = nmi_get_message_types;
+
+ mc->get_message_bytes = nmi_get_message_bytes;
+ mc->put_message_bytes = nmi_put_message_bytes;
+
+ mc->handle_message = nmi_handle_message;
+ mc->reset_message = nmi_reset_message;
+}
+
+static const TypeInfo nvme_mi = {
+ .name = TYPE_NMI_I2C_DEVICE,
+ .parent = TYPE_MCTP_I2C_ENDPOINT,
+ .instance_size = sizeof(NMIDevice),
+ .class_init = nvme_mi_class_init,
+};
+
+static void register_types(void)
+{
+ type_register_static(&nvme_mi);
+}
+
+type_init(register_types);
diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
index fccb79f48973..669785780b2a 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -214,3 +214,9 @@ pci_nvme_ub_db_wr_invalid_sq(uint32_t qid) "submission queue doorbell write for
pci_nvme_ub_db_wr_invalid_sqtail(uint32_t qid, uint16_t new_tail) "submission queue doorbell write value beyond queue size, sqid=%"PRIu32", new_head=%"PRIu16", ignoring"
pci_nvme_ub_unknown_css_value(void) "unknown value in cc.css field"
pci_nvme_ub_too_many_mappings(void) "too many prp/sgl mappings"
+
+# nmi-i2c
+nmi_handle_mi_read_nmi_ds(uint8_t dtyp) "dtyp 0x%"PRIx8""
+nmi_handle_mi_config_get(uint8_t identifier) "identifier 0x%"PRIx8""
+nmi_handle_mi(uint8_t opc) "opc 0x%"PRIx8""
+nmi_handle_msg(uint8_t nmint) "nmint 0x%"PRIx8""
--
2.38.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH RFC 3/3] hw/nvme: add nvme management interface model
2022-11-16 8:43 ` [PATCH RFC 3/3] hw/nvme: add nvme management interface model Klaus Jensen
@ 2022-11-18 7:56 ` Philippe Mathieu-Daudé
2022-11-18 7:58 ` Klaus Jensen
0 siblings, 1 reply; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-18 7:56 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Jeremy Kerr, Joel Stanley,
Cédric Le Goater, Klaus Jensen
On 16/11/22 09:43, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add the 'nmi-i2c' device that emulates an NVMe Management Interface
> controller.
>
> Initial support is very basic (Read NMI DS, Configuration Get).
>
> This is based on previously posted code by Padmakar Kalghatgi, Arun
> Kumar Agasar and Saurav Kumar.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/nvme/meson.build | 1 +
> hw/nvme/nmi-i2c.c | 381 +++++++++++++++++++++++++++++++++++++++++++
> hw/nvme/trace-events | 6 +
> 3 files changed, 388 insertions(+)
> create mode 100644 hw/nvme/nmi-i2c.c
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,381 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-only
Just curious, is this restricted license choice on purpose?
> + *
> + * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
> + *
> + * SPDX-FileContributor: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
> + * SPDX-FileContributor: Arun Kumar Agasar <arun.kka@samsung.com>
> + * SPDX-FileContributor: Saurav Kumar <saurav.29@partner.samsung.com>
> + * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
> + */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 3/3] hw/nvme: add nvme management interface model
2022-11-18 7:56 ` Philippe Mathieu-Daudé
@ 2022-11-18 7:58 ` Klaus Jensen
0 siblings, 0 replies; 26+ messages in thread
From: Klaus Jensen @ 2022-11-18 7:58 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Andrew Jeffery, Keith Busch, Corey Minyard,
Peter Delevoryas, qemu-arm, Peter Maydell, qemu-block,
Jeremy Kerr, Joel Stanley, Cédric Le Goater, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 940 bytes --]
On Nov 18 08:56, Philippe Mathieu-Daudé wrote:
> On 16/11/22 09:43, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Add the 'nmi-i2c' device that emulates an NVMe Management Interface
> > controller.
> >
> > Initial support is very basic (Read NMI DS, Configuration Get).
> >
> > This is based on previously posted code by Padmakar Kalghatgi, Arun
> > Kumar Agasar and Saurav Kumar.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> > hw/nvme/meson.build | 1 +
> > hw/nvme/nmi-i2c.c | 381 +++++++++++++++++++++++++++++++++++++++++++
> > hw/nvme/trace-events | 6 +
> > 3 files changed, 388 insertions(+)
> > create mode 100644 hw/nvme/nmi-i2c.c
>
> > +++ b/hw/nvme/nmi-i2c.c
> > @@ -0,0 +1,381 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-only
>
> Just curious, is this restricted license choice on purpose?
>
No! That is a mistake!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] hw/{i2c,nvme}: mctp endpoint, nvme management interface model
2022-11-16 8:43 [PATCH 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
` (2 preceding siblings ...)
2022-11-16 8:43 ` [PATCH RFC 3/3] hw/nvme: add nvme management interface model Klaus Jensen
@ 2022-11-16 9:18 ` Jeremy Kerr
3 siblings, 0 replies; 26+ messages in thread
From: Jeremy Kerr @ 2022-11-16 9:18 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Andrew Jeffery, Keith Busch, Corey Minyard, Peter Delevoryas,
qemu-arm, Peter Maydell, qemu-block, Joel Stanley,
Cédric Le Goater, Klaus Jensen, Matt Johnston
Hi Klaus,
[+CC Matt]
> This adds a generic MCTP endpoint model that other devices may derive
> from. I'm not 100% happy with the design of the class methods, but
> it's a start.
Thanks for posting these! I'll have a more thorough look through soon,
but wanted to tackle some of the larger design-points first (and we've
already spoken a bit about these, but rehashing a little of that for
others CCed too).
For me, the big decision here is where we want to run the NVMe-MI
device model. Doing it in the qemu process certainly makes things
easier to set up, and we can just configure the machine+nvme-mi device
as the one operation.
The alternative would be to have the NVMe-MI model run as an external
process, and not part of the qemu tree; it looks like Peter D is going
for that approach with [1]. The advantage there is that we would be
able to test against closer-to-reality "MI firmware" (say, a device
vendor running their NVMe-MI firmware directly in another emulator? are
folks interested in doing that?)
The complexity around the latter approach will be where we split the
processes, and arrange for IPC. [1] suggests at the i2c layer, but that
does seem to have complexities with i2c controller model compatibility;
we could certainly extend that to a "generic" i2c-over-something
protocol (which would also be handy for other things), or go higher up
and use MCTP directly as the transport (say, the serial binding over a
chardev). The former would be more useful for direct firmware
emulation.
My interest is mainly in testing the software stack, so either approach
is fine; I assume your interest is from the device implementation side?
Cheers,
Jeremy
[1]: https://github.com/facebook/openbmc/blob/helium/common/recipes-devtools/qemu/qemu/0007-hw-misc-Add-i2c-netdev-device.patch
^ permalink raw reply [flat|nested] 26+ messages in thread