Skip to content

vswitch: Add vswitch component#685

Open
JDuchniewicz wants to merge 18 commits intomainfrom
jakub/vswitch
Open

vswitch: Add vswitch component#685
JDuchniewicz wants to merge 18 commits intomainfrom
jakub/vswitch

Conversation

@JDuchniewicz
Copy link
Copy Markdown
Contributor

@JDuchniewicz JDuchniewicz commented Mar 30, 2026

Adds a vswitch component following #636

Allows for connecting up to 62 clients and one TX/RX Virt pair with the vswitch. Tested with PR: au-ts/libvmm#207 and depends on microkit_sdf_gen PR: au-ts/microkit_sdf_gen#31

The support for connecting vswitches back to back might be added later.

Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
@midnightveil
Copy link
Copy Markdown
Contributor

midnightveil commented Mar 30, 2026

You appear to have broken all the echo server examples per CI.

It would be good to add an example/CI test for this. Same applies to libVMM itself.

(My other question: is vswitch a driver or an (Lions)OS component...?)

@JDuchniewicz
Copy link
Copy Markdown
Contributor Author

JDuchniewicz commented Mar 30, 2026

You appear to have broken all the echo server examples per CI.

It would be good to add an example/CI test for this. Same applies to libVMM itself.

Yes, I will add the necessary test, example is in libVMM. Should we also have one here?

(My other question: is vswitch a driver or an (Lions)OS component...?)

I don't foresee running it alone, it should be a plug-and-play component chosen during writing of the meta.py file.

Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
@midnightveil
Copy link
Copy Markdown
Contributor

I don't foresee running it alone, it should be a plug-and-play component chosen during writing of the meta.py file.

What I mean is "does it make more sense as part of LionsOS than sDDF"?

@JDuchniewicz
Copy link
Copy Markdown
Contributor Author

I don't foresee running it alone, it should be a plug-and-play component chosen during writing of the meta.py file.

What I mean is "does it make more sense as part of LionsOS than sDDF"?

I don't know where we draw the line between, this is "driver/should be in sDDF" and "this is too high-level for sDDF". I presume current components are crucial enough to be here. This is somewhere in-between as it is an optional system component.

eth->racc = RACC_LINEDIS | RACC_IPDIS | RACC_PRODIS;
/* Add the checksum for known IP protocols */
eth->tacc = TACC_PROCHK | TACC_IPCHK;
//eth->tacc = TACC_PROCHK | TACC_IPCHK; // TODO: disabled because VM does that as well
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is enabling hardware checksumming something that can be put into the net driver config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can extend that. We should probably align that with how it is done for LWIP - currently as #ifdef:

#if defined(CONFIG_PLAT_IMX8MM_EVK) || defined(CONFIG_PLAT_MAAXBOARD) || defined(CONFIG_PLAT_IMX8MP_EVK) \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more to define if the hardware has support for it, but we should also be able to optionally turn on/off certain hardware features

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ivan-Velickovic what are your thoughts - I can add this as part of this PR, or alternatively create a new PR with the option to set/unset that option. Also, what other options would we want to be able to configure dynamically?


/* This is a name for the 96 bit ethernet addresses available on many
systems. */
struct ether_addr {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a duplication of what's already in constants.h. Would it be possible to just extend that rather than redefining here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, IMO it's not a bad idea to split headers according to what they provide/represent instead of having one big constants.h but I am open to a discussion.

Copy link
Copy Markdown
Contributor

@Kswin01 Kswin01 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh I'm not opposed to that either, I believe the firewall includes in LionsOS already separate things out such as ethernet, icmp, tcp, and udp constants: https://github.com/au-ts/lionsos/blob/main/include/lions/firewall/ethernet.h.

Maybe it's worth bringing some of those into sDDF and extending them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need just what I included here right now. I don't think adding most of them makes sense unless we will use them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh probably only ethernet is relevant for sDDF

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commonalized somewhat - please take a look

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


#define VSWITCH_VIRT_PORT (SDDF_NET_MAX_CLIENTS - 1)

#define NUM_BUFFERS (512)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be passed in through the vswitch config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually an omission on my side - I have been thinking how to properly define this. This comes from:

// 4*drv_queue_capacity. It must be mapped R-W and zero-initialised.

As we initialize the drv_queue_capacity with the number of rx_buffers in SDFgen: https://github.com/au-ts/microkit_sdf_gen/blob/main/src/sddf/net.zig#L251

This was supposed to be temporary - I think this needs to be passed into the elf section through SDFgen.

net_connection_resource_t conn;
device_region_resource_t data;
device_region_resource_t data[SDDF_NET_MAX_CLIENTS];
uint8_t num_data;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is uint8 enough here? Most systems have more than 255 buffers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the name is confusing - we can have up to 63 Clients, therefore num_data will be also up to 63.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected, please take a look.

@Kswin01
Copy link
Copy Markdown
Contributor

Kswin01 commented Mar 30, 2026

I'll have a proper read through the vswitch implementation later, but a few nitpicks of the surrounding updates

Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
@JDuchniewicz
Copy link
Copy Markdown
Contributor Author

@Courtney3141 I think it's prudent to first merge this, then create a new PR for the new example with 2 echo_server's as there might be a substantial amount of new files. Also, libvmm depends on it and not necessarily on the presence of the sddf example.

@Courtney3141
Copy link
Copy Markdown
Contributor

@Courtney3141 I think it's prudent to first merge this, then create a new PR for the new example with 2 echo_server's as there might be a substantial amount of new files. Also, libvmm depends on it and not necessarily on the presence of the sddf example.

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants