Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify and generalize host control of machine #257

Open
diegonehab opened this issue Jul 8, 2024 · 8 comments
Open

Simplify and generalize host control of machine #257

diegonehab opened this issue Jul 8, 2024 · 8 comments
Labels
refactor Restructuring code, while not changing its original functionality

Comments

@diegonehab
Copy link
Contributor

Context

Controlling a machine engaged with rollups involves reading and writing to a bunch of different machine CSRs. The introduction of the send_cmio_response simplified, but it is still complicated.

run now returns the break_reason, which simplifies it further. But now there are unnecessary redundancies that may cause confusion.

Possible solutions

At the moment, iflags has fields PRV and the X, Y, and H flags.

PRV really is something internal that the host should never mess with (the current machine privilege level).

Let's promote iflags.PRV to its own full CSR iprv. This will simplify the state-access implementations, since they won't have to do field manipulation there anymore.

X, Y, and H, on the other hand, are things the host needs to look at and, in the case of Y, change.

There is never a case in which more than one of these flags is set. They are always set via HTIF from the inside.

In the case of X and Y, the host will also need to look into htif.tohost. This is because htif.tohost contains the reason for the yield and the amount of data written to tx_buffer.

X is set when the machine returns from an automatic yield. Let's remove X altogether, since the machine run already returns this as a break reason and X is cleared automatically.

Y when it returns from a manual yield.

H when it is permanently halted.

Let's relocate Y and H to htif.tohost. With some reorganization there, we can make this happen.
(This will also simplify HTIF implementation, since it won't need to change the iflags register anymore.)
Let's rename Y to YM to make the distinction obvious. It's not a generic yield flag, but rather a Manual Yield flag.

Perhaps we can be smart and use the device+cmd fields together as "the flag", with a few changes to make them uniquely identify the halt and the manual yields.

There already are many WARL CSRs that prevent certain bits from being changed.
htif.tohost would be one of these. If H is set, it would remain set forever. I think we can even use a write to htif.fromhost to clear YM, saving the need to modify htif.tohost when returning from a manual yield.

@diegonehab diegonehab added the refactor Restructuring code, while not changing its original functionality label Jul 8, 2024
@diegonehab
Copy link
Contributor Author

After some thought, here is a possibility.

A machine is halted if tohost has dev=HTIF_DEV_HALT, cmd=HTIF_HALT_CMD_HALT, and (data & 1).
A machine is yielded manually if tohost has dev = HTIF_DEV_YIELD and cmd = HTIF_YIELD_CMD_MANUAL.

We change the part of the interpret loop that checks for fixed-point yield/halt to the following:

tohost = read_tohost();
if (halted(tohost)) { // dev=HTIF_DEV_HALT, cmd=HTIF_HALT_CMD_HALT, (data & 1) 
    return break_reason::halted;
}
if (yielded(tohost)) { // dev = HTIF_DEV_YIELD, cmd = HTIF_YIELD_CMD_MANUAL
    formhost = read_fromhost();
    if (!yielded(fromhost)) { // unless host wrote a response to this htif-yield command...
        return break_reason::yielded_manually;
    } 
    // here we know the host responded, so we clear tohost and the machine is not yielded anymore
    write_tohost(0);
}

We change the HTIF protocol to be as follows:

From the inside, to use HTIF, guest code writes dev+cmd+data to tohost. HTIF device itself then clears fromhost. If device is halt or yield, the run() returns. From the outside, host can check tohost to see what is up. To respond to a yield, host copies dev+cmd to fromhost, but changes the data as desired and resumes the machine. If device was yield and fromhost has the right combination of dev+cmd, the machine clears tohost. From the inside, guest code reads the response in fromhost.

We also change write_tohost() to guard against the removal of a halted combination of dev+cmd even from the outside.

@diegonehab diegonehab changed the title Simplify host control of machine Simplify and generalize host control of machine Sep 27, 2024
@diegonehab

This comment has been minimized.

@diegonehab
Copy link
Contributor Author

diegonehab commented Feb 21, 2025

Hey @stskeeps, we are thinking of following this idea (or some refinement of it) on the release after the next one. Do you have reservations? @GCdePaula? @vfusco @mpolitzer

@mpolitzer
Copy link
Contributor

What you described seem to be the current behavior from the guest POV on yield[1].
https://github.com/cartesi/opensbi/blob/opensbi-1.3.1-ctsi-y/platform/cartesi/htif.c#L106

Is that so? would there be any required changes in: opensbi, linux or libcmt?

Seems close to the current workflow on the host side. I don't se an issue there.

@diegonehab
Copy link
Contributor Author

diegonehab commented Feb 24, 2025

I think we can do further than what I suggested above. Right now, we have DEV/CMD/(REASON/DATA). This seems to be very overkill.

We should perhaps have CMD(16)/DATA(48) and be done with it? With new CMD getting the current DEV/CMD/REASON. Or some other simpler division.

Also remove the LSB requirement from HALT, which is also overkill. If you issue a HALT, you are halted. :)

@GCdePaula
Copy link

GCdePaula commented Feb 24, 2025

I'm down for simplification, I don't mind updating our code. I'm trying to understand the changes.


Would this remove the possibility of querying "what was the latest break reason"? Like, it seems it's currently possible to look only at the current state of the machine and know what was the reason the machine broke last.


We should perhaps have CMD(16)/DATA(48) and be done with it? With new CMD getting the current DEV/CMD/REASON. Or some other simpler division.

What would be the possible values for CMD in this proposal? In the case of tohost, would it be representable (abstractly) by something like this?

type ToHost =
    | Automatic { reason: AutomaticReason }
    | Manual { reason: ManualReason }

type AutomaticReason =
    | Progress { mille_progress: u32 }
    | TxOutput { data_size: u48 }
    | TxReport { data_size: u48 }

type ManualReason {
    | RxAccepted { data_size: u48 } // will always be 32?
    | RxRejected
    | TxException { data_size: u48 }
    | GIO { domain: u16, data_size: u48 }

And the fromhost would be something like:

type FromHost
    | Advance  { data_size: u48 }
    | Inspect { data_size: u48 }

Why do we need both tohost and fromhost? Can't we use the same register?

@GCdePaula
Copy link

Also, do we need both halt and yield?

@diegonehab
Copy link
Contributor Author

The break reason would still be visible in tohost/mcycle.
There is a chance we can simplify this even further.
The important part is the external view, not the internal.
The difference between HALT and YIELD and even PUTCHAR is still the same: it's a different command that you are sending to HTIF. You don't want to mix "I am ok, send me more input" with "There is nothing you can do, I am done." or "Output some character."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Restructuring code, while not changing its original functionality
Projects
Status: Todo
Development

No branches or pull requests

3 participants