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

Parentassigngrp #42

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Conversation

christoph-zededa
Copy link
Contributor

add parentassigngrp field;

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 provent passthrough of the USB controller in one application while trying to passthrough
a USB device on this controller to another application.

// Currently the concrete reason to do this is to make a usb device dependent on the PCI address the USB
// controller is using to provent passthrough of the USB controller in one application while trying to passthrough
// a USB device on this controller to another application.
string parentassigngrp = 10;
Copy link
Contributor

@milan-zededa milan-zededa Dec 21, 2023

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

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.

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
All changes after the 'make'.

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark eriknordmark merged commit 10e8386 into lf-edge:main Jan 15, 2024
4 checks passed
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.

3 participants