[pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[moddingfox via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 7213 bytes --]

From: moddingfox <moddingfox@gmail.com>
To: pve-devel@lists.proxmox.com
Cc: Tyst Marin <moddingfox@foxtek.us>
Subject: [PATCH] Add UI option for boot optional mapped usb device
Date: Tue, 26 Nov 2024 01:51:56 +0000
Message-ID: <20241126015157.1107116-1-moddingfox@foxtek.us>

From: Tyst Marin <moddingfox@foxtek.us>

This is intended to work with a seprately submitted patch to qemu-server which enables the behavior

Signed-off-by: Tyst Marin <moddingfox@foxtek.us>
---
 www/manager6/qemu/USBEdit.js | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/USBEdit.js b/www/manager6/qemu/USBEdit.js
index b372d53d..f3606ef0 100644
--- a/www/manager6/qemu/USBEdit.js
+++ b/www/manager6/qemu/USBEdit.js
@@ -62,6 +62,10 @@ Ext.define('PVE.qemu.USBInputPanel', {
 	    delete values.usb3;
 	    val += ',usb3=1';
 	}
+	if (values.bootwhenmissing) {
+	    delete values.bootwhenmissing;
+	    val += ',bootwhenmissing=1';
+	}
 	values[me.confid] = val;
 	return values;
     },
@@ -142,6 +146,15 @@ Ext.define('PVE.qemu.USBInputPanel', {
 		    reference: 'usb3',
 		    fieldLabel: gettext('Use USB3'),
 		},
+		{
+		    xtype: 'checkbox',
+		    name: 'bootwhenmissing',
+		    bind: { disabled: '{!mapped.checked}' },
+		    inputValue: true,
+		    checked: true,
+		    reference: 'bootwhenmissing',
+		    fieldLabel: gettext('Boot Missing'),
+		},
 	    ],
 	},
     ],
@@ -180,7 +193,7 @@ Ext.define('PVE.qemu.USBEdit', {
 		}
 
 		let data = PVE.Parser.parsePropertyString(response.result.data[me.confid], 'host');
-		let port, hostdevice, mapped, usb3 = false;
+		let port, hostdevice, mapped, usb3, bootwhenmissing = false;
 		let usb;
 
 		if (data.host) {
@@ -199,6 +212,7 @@ Ext.define('PVE.qemu.USBEdit', {
 		}
 
 		usb3 = data.usb3 ?? false;
+		bootwhenmissing = data.bootwhenmissing ?? false;
 
 		var values = {
 		    usb,
@@ -206,6 +220,7 @@ Ext.define('PVE.qemu.USBEdit', {
 		    port,
 		    usb3,
 		    mapped,
+		    bootwhenmissing,
 		};
 
 		ipanel.setValues(values);
-- 
2.39.5



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[Dominik Csapak]

Hi,

thanks for wanting to contribute!

First, did you already see https://pve.proxmox.com/wiki/Developer_Documentation ?
(especially the CLA part at the end?)

Just a few high level comments/questions to the approach (did not look too much at the code yet).

Please correct me if I'm wrong, but my guess why you want this is to emulate
the behavior for 'raw' USB passed through devices? (since those don't have to
be there for the vm to start?)

I think maybe such a setting would be better suited on the mapping itself?

I say this because the mapping defines which devices can/should be used, so
there is IMHO the right part to decide if it should be used in a guest
when it's missing.

Also I'm not very sure if we'd need a setting for this at all, since
the 'raw' passthrough also simply pass it through.

Just for your understanding, the reason it's currently implemented this way
is to prevent booting a VM with a wrong device (at least when using the path),
or a without one since that can have bad consequences (depending on what the
guest does with the device and what devices are connected)

Additionally we currently don't properly track the use of usb devices on our
side (which can have weird side effects, e.g. if you try to pass the same
device to multiple running vms at the same time) but this is not really
possible when using vendor/device ids since there could be mulitple such devices.


with kind regards
Dominik


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[Tyst Marin]

Hey Dominik,

Appreciate the info and context you provided. I just sent the cla to
office@proxmox.com so hopefully that's good now(my bad for missing that).
Hopefully the below answers at least some part of your questions.

You are correct in that I'd like to optionally have the same behavior that
nonmapped usb passthrough devices have at vm start for mapped devices.

I thought about the same a small bit and initially went with it in vm
config, the thought being that the decision of a usb device being required
or not for boot should be on the vm that intends to use it much like if it
should operate in usb3 mode or not is up to the vm(mainly in how it may be
expected to use it). Though I see merit in having it on mapping config so
that any vm that uses that mapping would be affected by the param. Honestly
though either way seems like it would work.

 I currently think that having something similar to this is worth having in
the case when a mapping is used to resolve a device that is externally
powered/controlled and maybe different from host to host but perform the
same function.

I'm not sure exactly what you mean by booting with the wrong device when by
path(i'm assuming that's by port?). As far as I currently understand
mapping can be configured with either a specific port or vendor/device id
as targets on each machine. So the vm will either have control over a
specific port and attach any device in that port not caring what it is or
use a specific device by vendor/device id on the machine it is running on
based on the map config. As far as I can tell this doesn't seem all that
diff from a vm already booted with a device present in either version of
the map mode config then unplugging it and either changing the device
plugged into the assigned port or moving the device with the target
vendor/device id to a different port. In both cases current behavior is the
machine stays up and accepts the new device at the configured port(as it
should) or reattaches the vendor/device id target to the machine.

What you mentioned about device tracking sounds like a larger existing
issue/behavior with the USB passthrough system overall. I'm not exactly
seeing the where/how it matters in the context of this change request(Tho
that could be my newness to this code base. Please correct me here.).
Approaching from the perspective that after the map lookup the same
attachment mechanism is used with the info retrieved from the lookup(seems
to be how it works as far as I have seen). The behavior of how/if USB
devices are tracked and managed for VM's should already be defined along
with how vm's react to situations with these devices at runtime.


with PLUR
Tyst

On Wed, Dec 4, 2024 at 9:40 AM Dominik Csapak <d.csapak@proxmox.com> wrote:

> Hi,
>
> thanks for wanting to contribute!
>
> First, did you already see
> https://pve.proxmox.com/wiki/Developer_Documentation ?
> (especially the CLA part at the end?)
>
> Just a few high level comments/questions to the approach (did not look too
> much at the code yet).
>
> Please correct me if I'm wrong, but my guess why you want this is to
> emulate
> the behavior for 'raw' USB passed through devices? (since those don't have
> to
> be there for the vm to start?)
>
> I think maybe such a setting would be better suited on the mapping itself?
>
> I say this because the mapping defines which devices can/should be used, so
> there is IMHO the right part to decide if it should be used in a guest
> when it's missing.
>
> Also I'm not very sure if we'd need a setting for this at all, since
> the 'raw' passthrough also simply pass it through.
>
> Just for your understanding, the reason it's currently implemented this way
> is to prevent booting a VM with a wrong device (at least when using the
> path),
> or a without one since that can have bad consequences (depending on what
> the
> guest does with the device and what devices are connected)
>
> Additionally we currently don't properly track the use of usb devices on
> our
> side (which can have weird side effects, e.g. if you try to pass the same
> device to multiple running vms at the same time) but this is not really
> possible when using vendor/device ids since there could be mulitple such
> devices.
>
>
> with kind regards
> Dominik
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[Dominik Csapak]

On 12/4/24 22:50, Tyst Marin wrote:
> Hey Dominik,
> 

Hi,

> Appreciate the info and context you provided. I just sent the cla to office@proxmox.com 
> <mailto:office@proxmox.com> so hopefully that's good now(my bad for missing that). Hopefully the 
> below answers at least some part of your questions.
> 
> You are correct in that I'd like to optionally have the same behavior that nonmapped usb passthrough 
> devices have at vm start for mapped devices.
> 
> I thought about the same a small bit and initially went with it in vm config, the thought being that 
> the decision of a usb device being required or not for boot should be on the vm that intends to use 
> it much like if it should operate in usb3 mode or not is up to the vm(mainly in how it may be 
> expected to use it). Though I see merit in having it on mapping config so that any vm that uses that 
> mapping would be affected by the param. Honestly though either way seems like it would work.

The reason for wanting it on the mapping config is more because of ACLs. The mapping config requires
Map.Modify (or similar) to modify, while on the vm it would only need VM.Config.HWType and Mapping.Use.

The idea of the mapping was to give some peopel (those with Mapping.*) privileges the possibility
to give lower privileged users access to those devices.

> 
>   I currently think that having something similar to this is worth having in the case when a mapping 
> is used to resolve a device that is externally powered/controlled and maybe different from host to 
> host but perform the same function.
> 
> I'm not sure exactly what you mean by booting with the wrong device when by path(i'm assuming that's 
> by port?). As far as I currently understand mapping can be configured with either a specific port or 
> vendor/device id as targets on each machine. So the vm will either have control over a specific port 
> and attach any device in that port not caring what it is or use a specific device by vendor/device 
> id on the machine it is running on based on the map config. As far as I can tell this doesn't seem 
> all that diff from a vm already booted with a device present in either version of the map mode 
> config then unplugging it and either changing the device plugged into the assigned port or moving 
> the device with the target vendor/device id to a different port. In both cases current behavior is 
> the machine stays up and accepts the new device at the configured port(as it should) or reattaches 
> the vendor/device id target to the machine.

Yes i meant the 'port', so with PCI passthrough there is always a PCI path (e.g. 0000:00:01.0)
and a vendor/device id. There we also check that on boot so that not a wrong pci device is
accidentally passed through (e.g. changing paths can happen when installing new devices)

The idea here was to have the same for usb devices, though i admit that when using just the
vendor/device ids it's not really useful and for path/port it may also not wanted to do that.

What we should probably do is to add another piece of info to identify devices (e.g. serial number,
though that is seldom useful on usb devices, as they often have things like 123456 there...)
and make that part (incl. the 'it exists' check) an option such as 'require-exact-device' that
is by default on.

Does that make sense to you?

> 
> What you mentioned about device tracking sounds like a larger existing issue/behavior with the USB 
> passthrough system overall. I'm not exactly seeing the where/how it matters in the context of this 
> change request(Tho that could be my newness to this code base. Please correct me here.). Approaching 
> from the perspective that after the map lookup the same attachment mechanism is used with the info 
> retrieved from the lookup(seems to be how it works as far as I have seen). The behavior of how/if 
> USB devices are tracked and managed for VM's should already be defined along with how vm's react to 
> situations with these devices at runtime.
> 

Yeah this is a pre-existing issue and only slightly related to this. If we wanted to have tracking
of usb devices, we could more easily check if something is in use or not, circumventing
weird issues when one device is passed through multiple times, or when multiple devices have
e.g. the same vendor/device id.

So all in all, I would favor an option on the mapping itself (but leave the default as is)
though if you or anybody else has another argument why we should put it on the vm config
i would not be totally opposed to that. (we must document it either way)

with kind regards
Dominik

> 
> with PLUR
> Tyst
> 
> On Wed, Dec 4, 2024 at 9:40 AM Dominik Csapak <d.csapak@proxmox.com <mailto:d.csapak@proxmox.com>> 
> wrote:
> 
>     Hi,
> 
>     thanks for wanting to contribute!
> 
>     First, did you already see https://pve.proxmox.com/wiki/Developer_Documentation <https://
>     pve.proxmox.com/wiki/Developer_Documentation> ?
>     (especially the CLA part at the end?)
> 
>     Just a few high level comments/questions to the approach (did not look too much at the code yet).
> 
>     Please correct me if I'm wrong, but my guess why you want this is to emulate
>     the behavior for 'raw' USB passed through devices? (since those don't have to
>     be there for the vm to start?)
> 
>     I think maybe such a setting would be better suited on the mapping itself?
> 
>     I say this because the mapping defines which devices can/should be used, so
>     there is IMHO the right part to decide if it should be used in a guest
>     when it's missing.
> 
>     Also I'm not very sure if we'd need a setting for this at all, since
>     the 'raw' passthrough also simply pass it through.
> 
>     Just for your understanding, the reason it's currently implemented this way
>     is to prevent booting a VM with a wrong device (at least when using the path),
>     or a without one since that can have bad consequences (depending on what the
>     guest does with the device and what devices are connected)
> 
>     Additionally we currently don't properly track the use of usb devices on our
>     side (which can have weird side effects, e.g. if you try to pass the same
>     device to multiple running vms at the same time) but this is not really
>     possible when using vendor/device ids since there could be mulitple such devices.
> 
> 
>     with kind regards
>     Dominik
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[Tyst Marin]

I'm still not really all that convinced that Map.Modify is better suited
over VM.Config.HWType/Mapping.Use. Mainly as it seems reasonable to expect
the requirement/nonrequirement to still be a hw level config to the vm and
that the map should only have the role of saying which device for the
current node is but not anything about how it should be used by the vm. The
type of user that should make this change should be one with authority over
if the vm should be able to boot without the device. The mapping themselves
seem more like someone with the authority to say what that device is on a
particular node. Tho that's just my opinion and i'm happy to agree to
disagree on that point as both places are fairly reasonable and i'm likely
being a abit pedantic about it.

Ah cool I was thinking you meant port but wanted to be sure before I made
more of a fool of myself. ^.^ Would be nice to have some mechanism to pin
an exact usb device to a specific map on a node though that also sounds
more like an additional feature which takes the current "require a usb
device that matches" further to "require this exact usb device". I'm
assuming you mean the usb's iserial values, if so yeah they don't seem
super reliable from what I have seen, probably easier to give every chip
the same image for mass vs incrementing some part(idk tho). Either way if
there were a way to identify a device uniquely with a decent guarantee most
of the time I'd probably agree that on by default makes sense. But since
that appears not to be the case(at least from my limited exp), off by
default is more practical for a 'require-exact-device' feature. I guess
that also depends on the failure mode as well though. I would think that if
said 'require-exact-device' feature was on and no device could be found or
if 2+ devices that can't be disambiguated from each other were found that
this would be a no boot/halt condition for the vm, maybe less on the halt
side(existing behavior for precedent). Seems like from this thread that 4
modes of operation are posed 'optional-presence-any-matching-device',
'required-presence-any-matching-device', 'optional-presence-exact-device',
and 'required-presence-exact-device'. Tho that could boil down to 2
'optional-presence' and 'required-presence' maybe if there were some kinda
additional optional filter rules that each mapping/usb_config could have
that allowed matching various attrs of usb devices if specified. So I guess
extending the current device selection and then having a required vs not
flag similar ish to this patch.

with PLUR
Tyst

On Thu, Dec 12, 2024 at 8:19 AM Dominik Csapak <d.csapak@proxmox.com> wrote:

> On 12/4/24 22:50, Tyst Marin wrote:
> > Hey Dominik,
> >
>
> Hi,
>
> > Appreciate the info and context you provided. I just sent the cla to
> office@proxmox.com
> > <mailto:office@proxmox.com> so hopefully that's good now(my bad for
> missing that). Hopefully the
> > below answers at least some part of your questions.
> >
> > You are correct in that I'd like to optionally have the same behavior
> that nonmapped usb passthrough
> > devices have at vm start for mapped devices.
> >
> > I thought about the same a small bit and initially went with it in vm
> config, the thought being that
> > the decision of a usb device being required or not for boot should be on
> the vm that intends to use
> > it much like if it should operate in usb3 mode or not is up to the
> vm(mainly in how it may be
> > expected to use it). Though I see merit in having it on mapping config
> so that any vm that uses that
> > mapping would be affected by the param. Honestly though either way seems
> like it would work.
>
> The reason for wanting it on the mapping config is more because of ACLs.
> The mapping config requires
> Map.Modify (or similar) to modify, while on the vm it would only need
> VM.Config.HWType and Mapping.Use.
>
> The idea of the mapping was to give some peopel (those with Mapping.*)
> privileges the possibility
> to give lower privileged users access to those devices.
>
> >
> >   I currently think that having something similar to this is worth
> having in the case when a mapping
> > is used to resolve a device that is externally powered/controlled and
> maybe different from host to
> > host but perform the same function.
> >
> > I'm not sure exactly what you mean by booting with the wrong device when
> by path(i'm assuming that's
> > by port?). As far as I currently understand mapping can be configured
> with either a specific port or
> > vendor/device id as targets on each machine. So the vm will either have
> control over a specific port
> > and attach any device in that port not caring what it is or use a
> specific device by vendor/device
> > id on the machine it is running on based on the map config. As far as I
> can tell this doesn't seem
> > all that diff from a vm already booted with a device present in either
> version of the map mode
> > config then unplugging it and either changing the device plugged into
> the assigned port or moving
> > the device with the target vendor/device id to a different port. In both
> cases current behavior is
> > the machine stays up and accepts the new device at the configured
> port(as it should) or reattaches
> > the vendor/device id target to the machine.
>
> Yes i meant the 'port', so with PCI passthrough there is always a PCI path
> (e.g. 0000:00:01.0)
> and a vendor/device id. There we also check that on boot so that not a
> wrong pci device is
> accidentally passed through (e.g. changing paths can happen when
> installing new devices)
>
> The idea here was to have the same for usb devices, though i admit that
> when using just the
> vendor/device ids it's not really useful and for path/port it may also not
> wanted to do that.
>
> What we should probably do is to add another piece of info to identify
> devices (e.g. serial number,
> though that is seldom useful on usb devices, as they often have things
> like 123456 there...)
> and make that part (incl. the 'it exists' check) an option such as
> 'require-exact-device' that
> is by default on.
>
> Does that make sense to you?
>
> >
> > What you mentioned about device tracking sounds like a larger existing
> issue/behavior with the USB
> > passthrough system overall. I'm not exactly seeing the where/how it
> matters in the context of this
> > change request(Tho that could be my newness to this code base. Please
> correct me here.). Approaching
> > from the perspective that after the map lookup the same attachment
> mechanism is used with the info
> > retrieved from the lookup(seems to be how it works as far as I have
> seen). The behavior of how/if
> > USB devices are tracked and managed for VM's should already be defined
> along with how vm's react to
> > situations with these devices at runtime.
> >
>
> Yeah this is a pre-existing issue and only slightly related to this. If we
> wanted to have tracking
> of usb devices, we could more easily check if something is in use or not,
> circumventing
> weird issues when one device is passed through multiple times, or when
> multiple devices have
> e.g. the same vendor/device id.
>
> So all in all, I would favor an option on the mapping itself (but leave
> the default as is)
> though if you or anybody else has another argument why we should put it on
> the vm config
> i would not be totally opposed to that. (we must document it either way)
>
> with kind regards
> Dominik
>
> >
> > with PLUR
> > Tyst
> >
> > On Wed, Dec 4, 2024 at 9:40 AM Dominik Csapak <d.csapak@proxmox.com
> <mailto:d.csapak@proxmox.com>>
> > wrote:
> >
> >     Hi,
> >
> >     thanks for wanting to contribute!
> >
> >     First, did you already see
> https://pve.proxmox.com/wiki/Developer_Documentation <https://
> >     pve.proxmox.com/wiki/Developer_Documentation> ?
> >     (especially the CLA part at the end?)
> >
> >     Just a few high level comments/questions to the approach (did not
> look too much at the code yet).
> >
> >     Please correct me if I'm wrong, but my guess why you want this is to
> emulate
> >     the behavior for 'raw' USB passed through devices? (since those
> don't have to
> >     be there for the vm to start?)
> >
> >     I think maybe such a setting would be better suited on the mapping
> itself?
> >
> >     I say this because the mapping defines which devices can/should be
> used, so
> >     there is IMHO the right part to decide if it should be used in a
> guest
> >     when it's missing.
> >
> >     Also I'm not very sure if we'd need a setting for this at all, since
> >     the 'raw' passthrough also simply pass it through.
> >
> >     Just for your understanding, the reason it's currently implemented
> this way
> >     is to prevent booting a VM with a wrong device (at least when using
> the path),
> >     or a without one since that can have bad consequences (depending on
> what the
> >     guest does with the device and what devices are connected)
> >
> >     Additionally we currently don't properly track the use of usb
> devices on our
> >     side (which can have weird side effects, e.g. if you try to pass the
> same
> >     device to multiple running vms at the same time) but this is not
> really
> >     possible when using vendor/device ids since there could be mulitple
> such devices.
> >
> >
> >     with kind regards
> >     Dominik
> >
>
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[Dominik Csapak]

On 12/13/24 01:23, Tyst Marin wrote:
> I'm still not really all that convinced that Map.Modify is better suited over VM.Config.HWType/ 
> Mapping.Use. Mainly as it seems reasonable to expect the requirement/nonrequirement to still be a hw 
> level config to the vm and that the map should only have the role of saying which device for the 
> current node is but not anything about how it should be used by the vm. The type of user that should 
> make this change should be one with authority over if the vm should be able to boot without the 
> device. The mapping themselves seem more like someone with the authority to say what that device is 
> on a particular node. Tho that's just my opinion and i'm happy to agree to disagree on that point as 
> both places are fairly reasonable and i'm likely being a abit pedantic about it.

Mhmm.. let me think about this over the weekend, but I currently still lean towards
the map options since that is what the "higher privileged" user configures, and that person
should be able to restrict/permit the use of the hardware.

> 
> Ah cool I was thinking you meant port but wanted to be sure before I made more of a fool of myself. 
> ^.^ Would be nice to have some mechanism to pin an exact usb device to a specific map on a node 
> though that also sounds more like an additional feature which takes the current "require a usb 
> device that matches" further to "require this exact usb device". I'm assuming you mean the usb's 
> iserial values, if so yeah they don't seem super reliable from what I have seen, probably easier to 
> give every chip the same image for mass vs incrementing some part(idk tho). Either way if there were 
> a way to identify a device uniquely with a decent guarantee most of the time I'd probably agree that 
> on by default makes sense. But since that appears not to be the case(at least from my limited exp), 
> off by default is more practical for a 'require-exact-device' feature. I guess that also depends on 
> the failure mode as well though. I would think that if said 'require-exact-device' feature was on 
> and no device could be found or if 2+ devices that can't be disambiguated from each other were found 
> that this would be a no boot/halt condition for the vm, maybe less on the halt side(existing 
> behavior for precedent). Seems like from this thread that 4 modes of operation are posed 'optional- 
> presence-any-matching-device', 'required-presence-any-matching-device', 'optional-presence-exact- 
> device', and 'required-presence-exact-device'. Tho that could boil down to 2 'optional-presence' and 
> 'required-presence' maybe if there were some kinda additional optional filter rules that each 
> mapping/usb_config could have that allowed matching various attrs of usb devices if specified. So I 
> guess extending the current device selection and then having a required vs not flag similar ish to 
> this patch.

Yeah there is sadly not a general good way to uniquely identify hardware (same for pci devices)
(IMHO that is a big oversight in e.g. the pci/usb spec, but what can you do ;) )

we could introduce a single enum option for the failure mode with the options as values,
though i would boil it down to:

* require exact device (only works if it's unique with the properties)
* require "main properties" (only looks e.g. at the vendor/device id or port/path and ignores if 
multiple are present)
* "just configure it" mode

(names tbd)

I think that should cover most use cases (i think the 'optional-presence-exact-device' could not
work since we cannot check that if it does not exist?), and if we need more variations we
can still expand on the list and behavior.

Still, let me think about this over the weekend and maybe I can come up with something better
even.

> 
> with PLUR
> Tyst
> 
> On Thu, Dec 12, 2024 at 8:19 AM Dominik Csapak <d.csapak@proxmox.com <mailto:d.csapak@proxmox.com>> 
> wrote:
> 
>     On 12/4/24 22:50, Tyst Marin wrote:
>      > Hey Dominik,
>      >
> 
>     Hi,
> 
>      > Appreciate the info and context you provided. I just sent the cla to office@proxmox.com
>     <mailto:office@proxmox.com>
>      > <mailto:office@proxmox.com <mailto:office@proxmox.com>> so hopefully that's good now(my bad
>     for missing that). Hopefully the
>      > below answers at least some part of your questions.
>      >
>      > You are correct in that I'd like to optionally have the same behavior that nonmapped usb
>     passthrough
>      > devices have at vm start for mapped devices.
>      >
>      > I thought about the same a small bit and initially went with it in vm config, the thought
>     being that
>      > the decision of a usb device being required or not for boot should be on the vm that intends
>     to use
>      > it much like if it should operate in usb3 mode or not is up to the vm(mainly in how it may be
>      > expected to use it). Though I see merit in having it on mapping config so that any vm that
>     uses that
>      > mapping would be affected by the param. Honestly though either way seems like it would work.
> 
>     The reason for wanting it on the mapping config is more because of ACLs. The mapping config requires
>     Map.Modify (or similar) to modify, while on the vm it would only need VM.Config.HWType and
>     Mapping.Use.
> 
>     The idea of the mapping was to give some peopel (those with Mapping.*) privileges the possibility
>     to give lower privileged users access to those devices.
> 
>      >
>      >   I currently think that having something similar to this is worth having in the case when a
>     mapping
>      > is used to resolve a device that is externally powered/controlled and maybe different from
>     host to
>      > host but perform the same function.
>      >
>      > I'm not sure exactly what you mean by booting with the wrong device when by path(i'm assuming
>     that's
>      > by port?). As far as I currently understand mapping can be configured with either a specific
>     port or
>      > vendor/device id as targets on each machine. So the vm will either have control over a
>     specific port
>      > and attach any device in that port not caring what it is or use a specific device by vendor/
>     device
>      > id on the machine it is running on based on the map config. As far as I can tell this doesn't
>     seem
>      > all that diff from a vm already booted with a device present in either version of the map mode
>      > config then unplugging it and either changing the device plugged into the assigned port or
>     moving
>      > the device with the target vendor/device id to a different port. In both cases current
>     behavior is
>      > the machine stays up and accepts the new device at the configured port(as it should) or
>     reattaches
>      > the vendor/device id target to the machine.
> 
>     Yes i meant the 'port', so with PCI passthrough there is always a PCI path (e.g. 0000:00:01.0)
>     and a vendor/device id. There we also check that on boot so that not a wrong pci device is
>     accidentally passed through (e.g. changing paths can happen when installing new devices)
> 
>     The idea here was to have the same for usb devices, though i admit that when using just the
>     vendor/device ids it's not really useful and for path/port it may also not wanted to do that.
> 
>     What we should probably do is to add another piece of info to identify devices (e.g. serial number,
>     though that is seldom useful on usb devices, as they often have things like 123456 there...)
>     and make that part (incl. the 'it exists' check) an option such as 'require-exact-device' that
>     is by default on.
> 
>     Does that make sense to you?
> 
>      >
>      > What you mentioned about device tracking sounds like a larger existing issue/behavior with
>     the USB
>      > passthrough system overall. I'm not exactly seeing the where/how it matters in the context of
>     this
>      > change request(Tho that could be my newness to this code base. Please correct me here.).
>     Approaching
>      > from the perspective that after the map lookup the same attachment mechanism is used with the
>     info
>      > retrieved from the lookup(seems to be how it works as far as I have seen). The behavior of
>     how/if
>      > USB devices are tracked and managed for VM's should already be defined along with how vm's
>     react to
>      > situations with these devices at runtime.
>      >
> 
>     Yeah this is a pre-existing issue and only slightly related to this. If we wanted to have tracking
>     of usb devices, we could more easily check if something is in use or not, circumventing
>     weird issues when one device is passed through multiple times, or when multiple devices have
>     e.g. the same vendor/device id.
> 
>     So all in all, I would favor an option on the mapping itself (but leave the default as is)
>     though if you or anybody else has another argument why we should put it on the vm config
>     i would not be totally opposed to that. (we must document it either way)
> 
>     with kind regards
>     Dominik
> 
>      >
>      > with PLUR
>      > Tyst
>      >
>      > On Wed, Dec 4, 2024 at 9:40 AM Dominik Csapak <d.csapak@proxmox.com
>     <mailto:d.csapak@proxmox.com> <mailto:d.csapak@proxmox.com <mailto:d.csapak@proxmox.com>>>
>      > wrote:
>      >
>      >     Hi,
>      >
>      >     thanks for wanting to contribute!
>      >
>      >     First, did you already see https://pve.proxmox.com/wiki/Developer_Documentation <https://
>     pve.proxmox.com/wiki/Developer_Documentation> <https://
>      > pve.proxmox.com/wiki/Developer_Documentation <http://pve.proxmox.com/wiki/
>     Developer_Documentation>> ?
>      >     (especially the CLA part at the end?)
>      >
>      >     Just a few high level comments/questions to the approach (did not look too much at the
>     code yet).
>      >
>      >     Please correct me if I'm wrong, but my guess why you want this is to emulate
>      >     the behavior for 'raw' USB passed through devices? (since those don't have to
>      >     be there for the vm to start?)
>      >
>      >     I think maybe such a setting would be better suited on the mapping itself?
>      >
>      >     I say this because the mapping defines which devices can/should be used, so
>      >     there is IMHO the right part to decide if it should be used in a guest
>      >     when it's missing.
>      >
>      >     Also I'm not very sure if we'd need a setting for this at all, since
>      >     the 'raw' passthrough also simply pass it through.
>      >
>      >     Just for your understanding, the reason it's currently implemented this way
>      >     is to prevent booting a VM with a wrong device (at least when using the path),
>      >     or a without one since that can have bad consequences (depending on what the
>      >     guest does with the device and what devices are connected)
>      >
>      >     Additionally we currently don't properly track the use of usb devices on our
>      >     side (which can have weird side effects, e.g. if you try to pass the same
>      >     device to multiple running vms at the same time) but this is not really
>      >     possible when using vendor/device ids since there could be mulitple such devices.
>      >
>      >
>      >     with kind regards
>      >     Dominik
>      >
> 
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH] Add UI option for boot optional mapped usb device

[Dominik Csapak]

Hi,

On 12/13/24 09:34, Dominik Csapak wrote:
> On 12/13/24 01:23, Tyst Marin wrote:
>> I'm still not really all that convinced that Map.Modify is better suited over VM.Config.HWType/ 
>> Mapping.Use. Mainly as it seems reasonable to expect the requirement/nonrequirement to still be a 
>> hw level config to the vm and that the map should only have the role of saying which device for 
>> the current node is but not anything about how it should be used by the vm. The type of user that 
>> should make this change should be one with authority over if the vm should be able to boot without 
>> the device. The mapping themselves seem more like someone with the authority to say what that 
>> device is on a particular node. Tho that's just my opinion and i'm happy to agree to disagree on 
>> that point as both places are fairly reasonable and i'm likely being a abit pedantic about it.
> 
> Mhmm.. let me think about this over the weekend, but I currently still lean towards
> the map options since that is what the "higher privileged" user configures, and that person
> should be able to restrict/permit the use of the hardware.
> 

Sorry for it being much longer than a weekend... with the holidays and other things i overlooked
this thread then.

Anyway since no one else commented on this in the meantime and I did not figure out a better
way, I'd say we should go with a 'mode' enum on the mapping itself, with the same reasoning:

The one creating the mapping must have the privilege to manage the device, whereas the
user adding it to the vm might not, only the priv to use it. And IMO the user with
the higher privilege in this case should be the one to decide if it should be
"reserved" or exist when the guest starts.

Hope that still aligns so that it solves your issues.
Would you be willing to provide new version of patches for that?
(if there is any question on how to start on this, just ask)

Best regards
Dominik


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel