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.

Sign up for GitHub

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

Parentassigngrp #42

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Parentassigngrp #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3c373e
devmodel: add parentassigngrp
christoph-zededa Nov 30, 2023
78c6e32
proto: autogenerated protocol files
christoph-zededa Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 107 additions & 82 deletions go/config/devmodel.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions proto/config/devmodel.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,22 @@ message PhysicalIO {
// list of Virtual Functions (VF) for given Physical Function (PF)
// only applies for PHY_IO_TYPE_PHY_IO_NET_ETH_PF
VfList vflist = 9;

// parentassigngrp
//
// Parent Assignment Group is there to reference the parent assignment
// group in order to make the device dependent on a different device.
// Currently the concrete reason to do this is to make a usb device dependent
// on the PCI address the USB controller is using to prevent passthrough of
// the USB controller in one application while trying to passthrough
// a USB device on this controller to another application.
//
// We only allow these two cases:
// - children and their parent devices are assigned to the same application
// instance
// - parent device stays in host and one or more children (in separate
// assignment groups) are assigned to one or more application instances
string parentassigngrp = 10;
Copy link
Contributor

@milan-zededa milan-zededa Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What about instead having parent, referencing logical label of the parent device?
Seems more generic. In some cases (like yours) it can prevent separate assignment, in other (future) cases it could be useful for other purposes.
Plus assigngrp can be empty (meaning not assignable), which would make such device non-referenceable from child devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean with ?

parentassigngrp allows having several parent devices that are all in the same assigngrp; I think that's not possible if we reference the logicallabel.

Copy link
Contributor

@milan-zededa milan-zededa Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is supposed to be a question so it ends with question mark :)

If you have a pointer to the parent device, you can obtain its assigngrp (aka parentassigngrp), which can contain multiple devices.
I just think that having a tree-structure of physical devices could be useful for other cases as well, not just assignment restrictions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean with ?

github removed part of the text; I wanted to ask: "What do you mean with it can prevent separate assignment?"

Copy link
Contributor

Choose a reason for hiding this comment

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

By "prevent separate assignment" I meant your use-case, i.e. preventing to assign USB device and its controller separately. What I tried to say in that sentence is that with parent as opposed to parentassigngrp we could perhaps cover some more future use-cases, not only related to device passthrough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can just as well let EVE to determine if the given assignment is valid

I fear it is not possible in case we use ACS override to do passthrough of PCI devices to different apps even if they're in the same iommu group, but I might be wrong as my knowledge about ACS override is just too small.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to have the controller dictate the rules for assignment than have EVE do something more dynamic, since a dynamic behavior (based on the iommu groups) might change over time with different releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, let's use parentassigngrp as proposed by the PR then.
However, I still think we should describe the semantics of the field in the comment rather than just give one concrete example.
See one of my previous comments:

But I think we should further clarify and define the semantics of the parentassigngrp in the comment.
My understanding is that it limits device assignment and allows only these 2 cases:

  • children and all parent devices are assigned into the same app
  • children is assigned while all parent devices stay in the host

Correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

@milan-zededa In addition to what you said above, if the parent stays in the host, then different children can be assigned to different app instances. (And in case we ever have grandparents/grandchildren then the top of the tree needs to be in the host, and below that cut different subtrees can be assigned to different app instances.)

@christoph-zededa can you please add Milan's and the above text to the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done.

}

message VfList {
Expand Down
Loading
Loading