| Message ID | 1458053080-29170-1-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"Michael S. Tsirkin" <mst@redhat.com> writes: > Allowing arbitary file names on command line is setting us up for > failure: future guests will look for a specific QEMU-specified name and > will get confused finding a user file there. > > We do warn but people are conditioned to ignore warnings by now, > so at best that will help users debug problem, not avoid it. > > Disable this by default, so distros don't get to deal with it, > but leave an option for developers to configure this in, > with scary warnings so people only do it if they know > what they are doing. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> I'm having a hard time to see the point. There are *countless* ways users can mess up their guests from the command line or the monitor. Yet we get so hung up on this rather obscure one to attempt to catch some (but far from all) misuses that may trigger it, and then, because that interferes with debugging, add a *configure* option to suppress it again. Misuse of -fw_cfg isn't even a molehill, but adding a configure option *is* making a mountain out of it. If you know what "fw cfg" is (few users do), and know how to put the user-specified fw cfg files feature to use (even fewer), you're damn well expected to know to stick to /opt or else. We already print a reminder. That's as far as I'd want us to go.
On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > Allowing arbitary file names on command line is setting us up for > > failure: future guests will look for a specific QEMU-specified name and > > will get confused finding a user file there. > > > > We do warn but people are conditioned to ignore warnings by now, > > so at best that will help users debug problem, not avoid it. > > > > Disable this by default, so distros don't get to deal with it, > > but leave an option for developers to configure this in, > > with scary warnings so people only do it if they know > > what they are doing. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > I'm having a hard time to see the point. Frankly, I am having a hard time to see the point of exposing fw cfg to users at all. It was designed as an internal interface between QEMU PC hardware and firmware. As a PC maintainer, I do not like it that users get to poke at PC internals. So it is yet another way to pass binaries to Linux guests. Don't we have enough of these? But Gerd likes it for some reason, and merged it. OK. But please find a way to make sure it does not conflict with its current usage in PC. Asking that all files have an "opt/" prefix is one way but only if it is enforced. This is polluting global namespace. > There are *countless* ways > users can mess up their guests from the command line or the monitor. > Yet we get so hung up on this rather obscure one to attempt to catch > some (but far from all) misuses that may trigger it, and then, because > that interferes with debugging, add a *configure* option to suppress it > again. In fact, how does it interfere with debugging? debugging what? by whom? I think we should figure that out instead of carrying more opaque binaries around. E.g. if people want to replace built-in ACPI tables, say so, and we'll look at sane ways to do this that do not involve poking at acpi internals. > Misuse of -fw_cfg isn't even a molehill, but adding a configure option > *is* making a mountain out of it. > Yea. That configure hack is an attempt at a compromise. But maybe the real fix is to rip fw cfg tweaking from command line out completely, at least for 2.6. > If you know what "fw cfg" is (few users do), and know how to put the > user-specified fw cfg files feature to use (even fewer), you're damn > well expected to know to stick to /opt or else. We already print a > reminder. That's as far as I'd want us to go. What happens is that inevitable someone finds a bug, and then finds a work around, and blog posts spring up advertising the fix, and you can never go back again. Or a tool uses this, no one notices for years, and after an update someone gets to debug it until finally the warning is noticed. Or more likely it all ends up in the launchpad/bugzilla, with yet another useful tool broken for no good reason.
On Wed, 16 Mar 2016 at 18:50:57 +0200, Michael S. Tsirkin wrote: > On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote: > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > Allowing arbitary file names on command line is setting us up for > > > failure: future guests will look for a specific QEMU-specified name and > > > will get confused finding a user file there. > > > > > > We do warn but people are conditioned to ignore warnings by now, > > > so at best that will help users debug problem, not avoid it. > > > > > > Disable this by default, so distros don't get to deal with it, > > > but leave an option for developers to configure this in, > > > with scary warnings so people only do it if they know > > > what they are doing. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > I'm having a hard time to see the point. > > Frankly, I am having a hard time to see the point of exposing fw cfg to > users at all. It was designed as an internal interface between QEMU PC > hardware and firmware. As a PC maintainer, I do not like it that users > get to poke at PC internals. > > So it is yet another way to pass binaries to Linux guests. Don't we > have enough of these? But Gerd likes it for some reason, and merged it. > OK. As the author of the feature, I feel I should jump back in and clarify: It's a way to pass arbitrary blobs to any type of guest, with two important properties: 1. asynchronous, and 2. out-of-band. When I started looking, all existing methods involved either having the host start polling for the guest to bring up qga and be ready to accept an out-of-band connection (i.e., *not* asynchronous), or have the guest mount some special cdrom or floppy image prepared by the host (i.e., *not* out of band). fw_cfg is both asynchronous and out-of-band, so it appeared to be the perfect choice. > But please find a way to make sure it does not conflict with its current > usage in PC. Asking that all files have an "opt/" prefix is one way > but only if it is enforced. Enforcing the "opt/" prefix was clearly on the table when I submitted the feature (and totally acceptable for my own needs). At the time, however, most of the advice I received on the list was to leave it as a warning only (i.e., "mechanism, not policy"), especially since other respondents expressed interest in passing in non-"/opt" blobs for easier development and debugging of alternative firmware (such as OVMF, iirc). Having a mis-use of this feature become "institutionalized" over time was seen as a low/negligible risk at the time. Do we have any new reasons to worry about it ? Thanks much, --Gabriel
On 03/16/16 19:15, Gabriel L. Somlo wrote: > On Wed, 16 Mar 2016 at 18:50:57 +0200, Michael S. Tsirkin wrote: >> On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote: >>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>> >>>> Allowing arbitary file names on command line is setting us up for >>>> failure: future guests will look for a specific QEMU-specified name and >>>> will get confused finding a user file there. >>>> >>>> We do warn but people are conditioned to ignore warnings by now, >>>> so at best that will help users debug problem, not avoid it. >>>> >>>> Disable this by default, so distros don't get to deal with it, >>>> but leave an option for developers to configure this in, >>>> with scary warnings so people only do it if they know >>>> what they are doing. >>>> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> I'm having a hard time to see the point. >> >> Frankly, I am having a hard time to see the point of exposing fw cfg to >> users at all. It was designed as an internal interface between QEMU PC >> hardware and firmware. As a PC maintainer, I do not like it that users >> get to poke at PC internals. >> >> So it is yet another way to pass binaries to Linux guests. Don't we >> have enough of these? But Gerd likes it for some reason, and merged it. >> OK. > > As the author of the feature, I feel I should jump back in and clarify: > > It's a way to pass arbitrary blobs to any type of guest, with two > important properties: 1. asynchronous, and 2. out-of-band. When I > started looking, all existing methods involved either having the host > start polling for the guest to bring up qga and be ready to accept an > out-of-band connection (i.e., *not* asynchronous), or have the guest > mount some special cdrom or floppy image prepared by the host (i.e., > *not* out of band). > > fw_cfg is both asynchronous and out-of-band, so it appeared to be the > perfect choice. > >> But please find a way to make sure it does not conflict with its current >> usage in PC. Asking that all files have an "opt/" prefix is one way >> but only if it is enforced. > > Enforcing the "opt/" prefix was clearly on the table when I submitted > the feature (and totally acceptable for my own needs). At the time, however, > most of the advice I received on the list was to leave it as a warning > only (i.e., "mechanism, not policy"), especially since other respondents > expressed interest in passing in non-"/opt" blobs for easier development > and debugging of alternative firmware (such as OVMF, iirc). > > Having a mis-use of this feature become "institutionalized" over time was > seen as a low/negligible risk at the time. Do we have any new reasons > to worry about it ? OVMF uses this feature for a few flags. They are all called "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which shouldn't be surprising since I seem to have reviewed every patch for that file): > NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" > when using the "-fw_cfg" command line option, to avoid conflicting with > item names used internally by QEMU. For instance: > > -fw_cfg name=opt/my_item_name,file=./my_blob.bin > > Similarly, QEMU developers *SHOULD NOT* use item names prefixed with > "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file(). It says "should", not "must". I liked (and like) the "mechanism, not policy" thing. Letting developers pass in whatever they want, for development / debugging / testing purposes, is a plus to me. Thanks Laszlo
On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: > On 03/16/16 19:15, Gabriel L. Somlo wrote: > > On Wed, 16 Mar 2016 at 18:50:57 +0200, Michael S. Tsirkin wrote: > >> On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote: > >>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >>> > >>>> Allowing arbitary file names on command line is setting us up for > >>>> failure: future guests will look for a specific QEMU-specified name and > >>>> will get confused finding a user file there. > >>>> > >>>> We do warn but people are conditioned to ignore warnings by now, > >>>> so at best that will help users debug problem, not avoid it. > >>>> > >>>> Disable this by default, so distros don't get to deal with it, > >>>> but leave an option for developers to configure this in, > >>>> with scary warnings so people only do it if they know > >>>> what they are doing. > >>>> > >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> > >>> I'm having a hard time to see the point. > >> > >> Frankly, I am having a hard time to see the point of exposing fw cfg to > >> users at all. It was designed as an internal interface between QEMU PC > >> hardware and firmware. As a PC maintainer, I do not like it that users > >> get to poke at PC internals. > >> > >> So it is yet another way to pass binaries to Linux guests. Don't we > >> have enough of these? But Gerd likes it for some reason, and merged it. > >> OK. > > > > As the author of the feature, I feel I should jump back in and clarify: > > > > It's a way to pass arbitrary blobs to any type of guest, with two > > important properties: 1. asynchronous, and 2. out-of-band. When I > > started looking, all existing methods involved either having the host > > start polling for the guest to bring up qga and be ready to accept an > > out-of-band connection (i.e., *not* asynchronous), or have the guest > > mount some special cdrom or floppy image prepared by the host (i.e., > > *not* out of band). > > > > fw_cfg is both asynchronous and out-of-band, so it appeared to be the > > perfect choice. > > > >> But please find a way to make sure it does not conflict with its current > >> usage in PC. Asking that all files have an "opt/" prefix is one way > >> but only if it is enforced. > > > > Enforcing the "opt/" prefix was clearly on the table when I submitted > > the feature (and totally acceptable for my own needs). At the time, however, > > most of the advice I received on the list was to leave it as a warning > > only (i.e., "mechanism, not policy"), especially since other respondents > > expressed interest in passing in non-"/opt" blobs for easier development > > and debugging of alternative firmware (such as OVMF, iirc). > > > > Having a mis-use of this feature become "institutionalized" over time was > > seen as a low/negligible risk at the time. Do we have any new reasons > > to worry about it ? > > OVMF uses this feature for a few flags. They are all called > "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which > shouldn't be surprising since I seem to have reviewed every patch for > that file): > > > NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" > > when using the "-fw_cfg" command line option, to avoid conflicting with > > item names used internally by QEMU. For instance: > > > > -fw_cfg name=opt/my_item_name,file=./my_blob.bin > > > > Similarly, QEMU developers *SHOULD NOT* use item names prefixed with > > "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file(). > > It says "should", not "must". should means "might be ok". So change it to must then? > I liked (and like) the "mechanism, not > policy" thing. Letting developers pass in whatever they want, for > development / debugging / testing purposes, is a plus to me. > > Thanks > Laszlo Could you flesh out the development / debugging / testing and how is inserting files in PC namespace useful for that?
On 03/16/16 19:43, Michael S. Tsirkin wrote: > On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: >> On 03/16/16 19:15, Gabriel L. Somlo wrote: >>> On Wed, 16 Mar 2016 at 18:50:57 +0200, Michael S. Tsirkin wrote: >>>> On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote: >>>>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>>>> >>>>>> Allowing arbitary file names on command line is setting us up for >>>>>> failure: future guests will look for a specific QEMU-specified name and >>>>>> will get confused finding a user file there. >>>>>> >>>>>> We do warn but people are conditioned to ignore warnings by now, >>>>>> so at best that will help users debug problem, not avoid it. >>>>>> >>>>>> Disable this by default, so distros don't get to deal with it, >>>>>> but leave an option for developers to configure this in, >>>>>> with scary warnings so people only do it if they know >>>>>> what they are doing. >>>>>> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> >>>>> I'm having a hard time to see the point. >>>> >>>> Frankly, I am having a hard time to see the point of exposing fw cfg to >>>> users at all. It was designed as an internal interface between QEMU PC >>>> hardware and firmware. As a PC maintainer, I do not like it that users >>>> get to poke at PC internals. >>>> >>>> So it is yet another way to pass binaries to Linux guests. Don't we >>>> have enough of these? But Gerd likes it for some reason, and merged it. >>>> OK. >>> >>> As the author of the feature, I feel I should jump back in and clarify: >>> >>> It's a way to pass arbitrary blobs to any type of guest, with two >>> important properties: 1. asynchronous, and 2. out-of-band. When I >>> started looking, all existing methods involved either having the host >>> start polling for the guest to bring up qga and be ready to accept an >>> out-of-band connection (i.e., *not* asynchronous), or have the guest >>> mount some special cdrom or floppy image prepared by the host (i.e., >>> *not* out of band). >>> >>> fw_cfg is both asynchronous and out-of-band, so it appeared to be the >>> perfect choice. >>> >>>> But please find a way to make sure it does not conflict with its current >>>> usage in PC. Asking that all files have an "opt/" prefix is one way >>>> but only if it is enforced. >>> >>> Enforcing the "opt/" prefix was clearly on the table when I submitted >>> the feature (and totally acceptable for my own needs). At the time, however, >>> most of the advice I received on the list was to leave it as a warning >>> only (i.e., "mechanism, not policy"), especially since other respondents >>> expressed interest in passing in non-"/opt" blobs for easier development >>> and debugging of alternative firmware (such as OVMF, iirc). >>> >>> Having a mis-use of this feature become "institutionalized" over time was >>> seen as a low/negligible risk at the time. Do we have any new reasons >>> to worry about it ? >> >> OVMF uses this feature for a few flags. They are all called >> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which >> shouldn't be surprising since I seem to have reviewed every patch for >> that file): >> >>> NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" >>> when using the "-fw_cfg" command line option, to avoid conflicting with >>> item names used internally by QEMU. For instance: >>> >>> -fw_cfg name=opt/my_item_name,file=./my_blob.bin >>> >>> Similarly, QEMU developers *SHOULD NOT* use item names prefixed with >>> "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file(). >> >> It says "should", not "must". > > should means "might be ok". Yes, if there is a pressing reason. And even then, "you have been warned". > So change it to must then? I didn't suggest that. (For consistency, your patch should be attempting to modify the code and the docs together -- but this doesn't mean that I agree with the patch.) >> I liked (and like) the "mechanism, not >> policy" thing. Letting developers pass in whatever they want, for >> development / debugging / testing purposes, is a plus to me. >> >> Thanks >> Laszlo > > Could you flesh out the development / debugging / testing and > how is inserting files in PC namespace useful for that? I don't know -- which is part of the argument. Lack of a ready example doesn't imply (to me) that the possibility should be excluded. As Paolo said, I believe we've been through this discussion. I have no new arguments; the old ones are valid to me still. I won't try to influence this discussion any further; I only chimed in because OVMF was mentioned (and because I noticed that the docs would get out of sync with the code). Thanks Laszlo
On Wed, Mar 16, 2016 at 08:15:07PM +0100, Laszlo Ersek wrote: > On 03/16/16 19:43, Michael S. Tsirkin wrote: > > On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: > >> On 03/16/16 19:15, Gabriel L. Somlo wrote: > >>> On Wed, 16 Mar 2016 at 18:50:57 +0200, Michael S. Tsirkin wrote: > >>>> On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote: > >>>>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >>>>> > >>>>>> Allowing arbitary file names on command line is setting us up for > >>>>>> failure: future guests will look for a specific QEMU-specified name and > >>>>>> will get confused finding a user file there. > >>>>>> > >>>>>> We do warn but people are conditioned to ignore warnings by now, > >>>>>> so at best that will help users debug problem, not avoid it. > >>>>>> > >>>>>> Disable this by default, so distros don't get to deal with it, > >>>>>> but leave an option for developers to configure this in, > >>>>>> with scary warnings so people only do it if they know > >>>>>> what they are doing. > >>>>>> > >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>>> > >>>>> I'm having a hard time to see the point. > >>>> > >>>> Frankly, I am having a hard time to see the point of exposing fw cfg to > >>>> users at all. It was designed as an internal interface between QEMU PC > >>>> hardware and firmware. As a PC maintainer, I do not like it that users > >>>> get to poke at PC internals. > >>>> > >>>> So it is yet another way to pass binaries to Linux guests. Don't we > >>>> have enough of these? But Gerd likes it for some reason, and merged it. > >>>> OK. > >>> > >>> As the author of the feature, I feel I should jump back in and clarify: > >>> > >>> It's a way to pass arbitrary blobs to any type of guest, with two > >>> important properties: 1. asynchronous, and 2. out-of-band. When I > >>> started looking, all existing methods involved either having the host > >>> start polling for the guest to bring up qga and be ready to accept an > >>> out-of-band connection (i.e., *not* asynchronous), or have the guest > >>> mount some special cdrom or floppy image prepared by the host (i.e., > >>> *not* out of band). > >>> > >>> fw_cfg is both asynchronous and out-of-band, so it appeared to be the > >>> perfect choice. > >>> > >>>> But please find a way to make sure it does not conflict with its current > >>>> usage in PC. Asking that all files have an "opt/" prefix is one way > >>>> but only if it is enforced. > >>> > >>> Enforcing the "opt/" prefix was clearly on the table when I submitted > >>> the feature (and totally acceptable for my own needs). At the time, however, > >>> most of the advice I received on the list was to leave it as a warning > >>> only (i.e., "mechanism, not policy"), especially since other respondents > >>> expressed interest in passing in non-"/opt" blobs for easier development > >>> and debugging of alternative firmware (such as OVMF, iirc). > >>> > >>> Having a mis-use of this feature become "institutionalized" over time was > >>> seen as a low/negligible risk at the time. Do we have any new reasons > >>> to worry about it ? > >> > >> OVMF uses this feature for a few flags. They are all called > >> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which > >> shouldn't be surprising since I seem to have reviewed every patch for > >> that file): > >> > >>> NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" > >>> when using the "-fw_cfg" command line option, to avoid conflicting with > >>> item names used internally by QEMU. For instance: > >>> > >>> -fw_cfg name=opt/my_item_name,file=./my_blob.bin > >>> > >>> Similarly, QEMU developers *SHOULD NOT* use item names prefixed with > >>> "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file(). > >> > >> It says "should", not "must". > > > > should means "might be ok". > > Yes, if there is a pressing reason. And even then, "you have been warned". > > > So change it to must then? > > I didn't suggest that. > > (For consistency, your patch should be attempting to modify the code and > the docs together -- but this doesn't mean that I agree with the patch.) > > >> I liked (and like) the "mechanism, not > >> policy" thing. Letting developers pass in whatever they want, for > >> development / debugging / testing purposes, is a plus to me. > >> > >> Thanks > >> Laszlo > > > > Could you flesh out the development / debugging / testing and > > how is inserting files in PC namespace useful for that? > > I don't know -- which is part of the argument. Lack of a ready example > doesn't imply (to me) that the possibility should be excluded. > > As Paolo said, I believe we've been through this discussion. I have no > new arguments; the old ones are valid to me still. I won't try to > influence this discussion any further; I only chimed in because OVMF was > mentioned (and because I noticed that the docs would get out of sync > with the code). > > Thanks > Laszlo By the way, these are developer docs. User docs do not say anything: -fw_cfg [name=]name,file=file Add named fw_cfg entry from file. name determines the name of the entry in the fw_cfg file directory exposed to the guest. -fw_cfg [name=]name,string=str Add named fw_cfg entry from string.
On Wed, Mar 16, 2016 at 08:15:07PM +0100, Laszlo Ersek wrote: > >> I liked (and like) the "mechanism, not > >> policy" thing. Letting developers pass in whatever they want, for > >> development / debugging / testing purposes, is a plus to me. > >> > >> Thanks > >> Laszlo > > > > Could you flesh out the development / debugging / testing and > > how is inserting files in PC namespace useful for that? > > I don't know -- which is part of the argument. Lack of a ready example > doesn't imply (to me) that the possibility should be excluded. When it comes to an interface that has to be maintained indefinitely, only allowing what you absolutely need seems like exactly the correct design.
On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: > OVMF uses this feature for a few flags. They are all called > "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which > shouldn't be surprising since I seem to have reviewed every patch for > that file): Wait a second. You are saying upsteam OVMF puts files there. If users add their own flags that happen to be in opt/ovmf/ then what happens? And how do they *know* they should avoid that? There's no warning, guest just breaks in various ways. This is exactly the kind of mess I was worried about. We have a global namespace with no way to control what goes where. opt/ was for end users not firmware. The right thing to do would be for ovmf to reserve itself a directory under root. This really needs more thought. For now I'd suggest we drop the whole interface from 2.6 and come back to it after 2.6.
Hi, > > Having a mis-use of this feature become "institutionalized" over time was > > seen as a low/negligible risk at the time. Do we have any new reasons > > to worry about it ? > > OVMF uses this feature for a few flags. They are all called > "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which > shouldn't be surprising since I seem to have reviewed every patch for > that file): seabios has a few flags too, in etc/. Some of them are supported directly by qemu (such as setting the boot menu delay). Others are not, and I'd like to be able to use -fw_cfg for them for testing/debugging (any use cases beyond that should be supported by adding a less obscure way to set them to qemu, similar to the boot delay). Oh, and that'll most likely be more seabios testing than qemu testing, so why require me build a special qemu version for that? On the ovmf flags: What kind of flags are there? Anything a normal user might want to set? cheers, Gerd
On 03/16/16 21:31, Michael S. Tsirkin wrote: > On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: >> OVMF uses this feature for a few flags. They are all called >> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which >> shouldn't be surprising since I seem to have reviewed every patch for >> that file): > > Wait a second. You are saying upsteam OVMF puts files there. Sorry, I wasn't clear enough. OVMF consumes files that are put there by the user. > If users add their own flags that happen to be in opt/ovmf/ then > what happens? And how do they *know* they should avoid that? > There's no warning, guest just breaks in various ways. > > This is exactly the kind of mess I was worried about. We have a global > namespace with no way to control what goes where. > > opt/ was for end users not firmware. > > The right thing to do would be for ovmf to reserve itself > a directory under root. > > This really needs more thought. For now I'd suggest we drop the whole > interface from 2.6 and come back to it after 2.6. I very strongly disagree. -fw_cfg was invented exactly for the purpose that guest code (mostly, but not exclusively, guest firmware code) can take settings from the user without QEMU's knowledge. We were conscious of the namespace question, which is why the opt/ prefix was strongly recommended for such knobs. Trying to control it all from QEMU (beyond setting aside the opt/ subdirectory, regardless if it's worded "should" or "must"), introducing a central registry for prefixes, would defeat the entire purpose. Subdividing the namespace under opt/ was purposely left open. Then I went with opt/ovmf/ for files that OVMF should take from users because that's the obvious choice. If we want to be extremely paranoid about the namespace, we can modify the recommendation to say "use opt/UUID/...", where UUID is generated with "uuidgen". That's just an example I know from UEFI; it is how the entire namespace question is side-stepped without a central registry. Nonetheless, OVMF is the second of two guest firmwares in total, so "opt/ovmf/" is the obvious choice. Removing the interface would break existing code in OVMF that (a) consumes opt/ovmf/... files and (b) doesn't disturb anything at all otherwise. Plus, the -fw_cfg option was released in 2.4 (added around commit 81b2b81062). Laszlo
On 17/03/2016 09:49, Laszlo Ersek wrote: > On 03/16/16 21:31, Michael S. Tsirkin wrote: >> On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: >>> OVMF uses this feature for a few flags. They are all called >>> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which >>> shouldn't be surprising since I seem to have reviewed every patch for >>> that file): >> >> Wait a second. You are saying upsteam OVMF puts files there. > > Sorry, I wasn't clear enough. OVMF consumes files that are put there by > the user. I think what Michael is saying is that OVMF now has to worry about users calling their own files "opt/ovmf/foo" and causing a conflict. I actually agree with his worry, but probably not with how to resolve it. For me, the way to resolve it would be: 1) files should actually be named etc/ovmf/foo. OVMF could optionally accept both the old and the new names for a while, you would decide whether this is useful. 2) in turn, because of (1) even the warning on opt/ should be removed. Paolo
On 03/17/16 09:42, Gerd Hoffmann wrote: > Hi, > >>> Having a mis-use of this feature become "institutionalized" over time was >>> seen as a low/negligible risk at the time. Do we have any new reasons >>> to worry about it ? >> >> OVMF uses this feature for a few flags. They are all called >> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which >> shouldn't be surprising since I seem to have reviewed every patch for >> that file): > > seabios has a few flags too, in etc/. Some of them are supported > directly by qemu (such as setting the boot menu delay). Others are not, > and I'd like to be able to use -fw_cfg for them for testing/debugging > (any use cases beyond that should be supported by adding a less obscure > way to set them to qemu, similar to the boot delay). > > Oh, and that'll most likely be more seabios testing than qemu testing, > so why require me build a special qemu version for that? +1 > On the ovmf flags: What kind of flags are there? Anything a normal > user might want to set? Occasionally, yes. - "opt/ovmf/PcdPropertiesTableEnable" controls whether the "properties table" UEFI feature is enabled. Some guest OSes support it, some others are broken by it. It defaults to false. - "opt/ovmf/PcdSetNxForStack" controls whether the stack is made non-executable at the start of the DXE phase. This is meant as a security feature in edk2 generally, it is not specific to OVMF; only the way it is exposed to users is specific to OVMF. It used to default to TRUE, but then people reported that a non-executable stack was breaking grub2-efi binaries that had been shipped on Debian installer ISOs sometime during the middle ages. So I was forced to change the default to FALSE (and I hate that compromise to this day). - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which the 64-bit MMIO BARs are allocated. It defaults to 32GB (and as you suggested, a natural alignment is also ensured, both for the default 32GB, and for the user's choice, if any). The "X-" prefix signals that this is an experimental knob without any promise of stability. There are two imporant use cases for this knob at the moment: (a) if a user has a bunch of MMIO-hungry assignable devices, allow him/her to experiment with this flag, and report his/her data to us, so we can come up with smart heuristics, (b) allow a user to disable the high allocation of 64-bit MMIO BARs entirely, by setting the value to 0. The edk2 BAR allocation code prefers to allocate 64-bit BARs actually high -- unlike SeaBIOS --, and this has already exposed problems. For example, such devices don't work -- at the moment -- behind PXBs, if you use OVMF. Marcel is going to enhance PXB to support this (thank you!), but even until then (and for yet unknown similar issues), this flag allows a user to force the edk2 allocator to keep those 64-bit BARs under 4GB, at least if there is enough room for them there.) In downstream, we have two more (same purpose but separately for ARM and x86): - opt/aavmf/PcdResizeXterm - opt/ovmf/PcdResizeXterm (These settings are downstream only because the underlying feature that they control was rejected by upstream.) The feature they control is the following: assume you are at the UEFI shell prompt, and you have at least two console devices (e.g., one serial console you are looking at in xterm, and one video display you are looking at with spice), with the text output being mirrored to both. The MODE command allows you to select various text resolutions that are supported by all of your console devices simultaneously. Let's say you change from the default 80x25 to 100x31. This will correctly resize your video display, and also change the textual resolution used on the serial console, but it will *not* resize your xterm. Which means the serial console output will be garbled when you scroll, when you use commands that print wide tables, and so on. If the user sets the above knob to Y, then the xterm will also be resized (via control sequences). Sometimes this feature is invaluable, sometimes it's very annoying. It defaults to N. - Another flag I might expose later is "PcdHiiOsRuntimeSupport". This would control whether various bits of UEFI configuration that are normally accessible through TUI forms in the UEFI setup are also exposed as a large binary blob to the runtime OS. As I understand it, this feature is meant to allow OS-level vendor applications to mess with various firmware settings. For now OVMF inherits the central default Y. It is somewhat costly, so I think I'll flip the default for OVMF to N, but expose the knob under "opt/ovmf/PcdHiiOsRuntimeSupport" to support people who want to experiment with this. -*- The X-... knob might go away at any time, the others are likely to stay. And, obviously, exposing this kind of knob with dedicated QEMU options is out of question. Thanks Laszlo
On 03/17/16 09:42, Gerd Hoffmann wrote: > Hi, > >>> Having a mis-use of this feature become "institutionalized" over time was >>> seen as a low/negligible risk at the time. Do we have any new reasons >>> to worry about it ? >> >> OVMF uses this feature for a few flags. They are all called >> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which >> shouldn't be surprising since I seem to have reviewed every patch for >> that file): > > seabios has a few flags too, in etc/. Some of them are supported > directly by qemu (such as setting the boot menu delay). I forgot to confirm: clearly, when some flag already exists on which we can foist, kicking and screaming, a somewhat similar interpretation in OVMF, we do that without inventing a new flag. The boot menu delay is like this. Namely, if you set "-boot menu=on,splash-time=5000", you will get a progress bar for five seconds before auto-booting the otherwise relevant boot option. The QEMU manual documents the -boot option as guest firmware-specific, and OVMF operates with that in mind -- support what it can, as closely it can. Thanks Laszlo > Others are not, > and I'd like to be able to use -fw_cfg for them for testing/debugging > (any use cases beyond that should be supported by adding a less obscure > way to set them to qemu, similar to the boot delay). > > Oh, and that'll most likely be more seabios testing than qemu testing, > so why require me build a special qemu version for that? > > On the ovmf flags: What kind of flags are there? Anything a normal > user might want to set? > > cheers, > Gerd >
Top level reply, because this isn't in reply to any specific message in the thread, more like in reply to all of them. FW CFG's primary user is QEMU, which uses it to expose configuration information (in the widest sense) to Firmware. Thus the name FW CFG. FW CFG can also be used by others for their own purposes. QEMU is merely acting as transport then. I think there are actually three separate questions that should not be mixed up. 1. Is it a good idea to offer FW CFG as a transport to others? I have no opinion on this myself, but I trust Gerd's and Laszlo's judgement. Their answer seems to be a clear yes. 2. Is it a good idea to let users mess with QEMU's use of FW CFG? I think this is a special case of a more general question: should we try to protect the user from himself? We should definitely try not to trap the user. Obvious usage should be as safe as we can make it. Risky usage should be marked in the docs and/or warn on use. However, we should not try to stop our users from doing stupid things, as that would also stop them from doing clever things. 3. How should the FW CFG name space be shared among its users? Bad things can happen if others use the namespace in ways that conflict with QEMU's use, or conflict with another "other". This isn't an issue specific to FW CFG. For instance, upstream QEMU and the various downstream QEMUs all use the QMP command name space, and bad things can happen if they conflict. The difference is they'd conflict at compile time. Conflicts are easier to detect, but just as hard to resolve. QMP's solution is to reserve part of the name space for "others" (downstreams), and subdivide the reserved part further via RFQDN: owning a DNS domain name makes you own that RFQDN subdivision. For FW CFG, we did only the first half, namely reserving part of the name space for others: /opt/. We neglected to spell out rules for its safe sharing, i.e. the second part. I don't think it's too late to fix that: amend the specification to stipulate that owning a DNS domain name makes you own /opt/RFQDN/. Throw in known existing uses like /opt/ovmf/ as special cases.
Hi, > Occasionally, yes. > > - "opt/ovmf/PcdPropertiesTableEnable" controls whether the "properties > - "opt/ovmf/PcdSetNxForStack" controls whether the stack is made > - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which the > In downstream, we have two more (same purpose but separately for ARM and > x86): > - opt/aavmf/PcdResizeXterm > - opt/ovmf/PcdResizeXterm > > - Another flag I might expose later is "PcdHiiOsRuntimeSupport". This I think we should probably move those out of the opt/ namespace space, as this is reserved for user-defined files. I think either "etc/efi/" or "efi/" is useful (so we don't have different names on ovmf and aavmf). Possibly add "pcd/". Maybe even support setting *any* pcd via "efi/pcd/$name" (just an idea, not sure whenever that is useful and is possible without being too invasive). > - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which Hmm. Leave as-is for now I'd say. If we decide to keep it we should probably rename it and place it below etc/, next to reserved-memory-end. Export the size in bytes, like reserved-memory-end does. Add an option to qemu to set it, so you can specify the size as "32G" using the standard qemu size parser. cheers, Gerd
On Thu, Mar 17, 2016 at 10:40:24AM +0100, Paolo Bonzini wrote: > > > On 17/03/2016 09:49, Laszlo Ersek wrote: > > On 03/16/16 21:31, Michael S. Tsirkin wrote: > >> On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote: > >>> OVMF uses this feature for a few flags. They are all called > >>> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which > >>> shouldn't be surprising since I seem to have reviewed every patch for > >>> that file): > >> > >> Wait a second. You are saying upsteam OVMF puts files there. > > > > Sorry, I wasn't clear enough. OVMF consumes files that are put there by > > the user. > > I think what Michael is saying is that OVMF now has to worry about users > calling their own files "opt/ovmf/foo" and causing a conflict. > > I actually agree with his worry, but probably not with how to resolve > it. For me, the way to resolve it would be: > > 1) files should actually be named etc/ovmf/foo. OVMF could optionally > accept both the old and the new names for a while, you would decide > whether this is useful. IOW if etc/ovmf exists, then ignore opt/ovmf? OK. > 2) in turn, because of (1) even the warning on opt/ should be removed. > > Paolo If we do 2) then users might put files in etc/ovmf/foo.
On 17/03/2016 12:32, Michael S. Tsirkin wrote: > > 1) files should actually be named etc/ovmf/foo. OVMF could optionally > > accept both the old and the new names for a while, you would decide > > whether this is useful. > > IOW if etc/ovmf exists, then ignore opt/ovmf? OK. > > > 2) in turn, because of (1) even the warning on opt/ should be removed. > > If we do 2) then users might put files in etc/ovmf/foo. Right, that's exactly the point. These are user-configurable knobs for the firmware (user-configurable meaning they can't be expected to recompile QEMU). Paolo
On Thu, Mar 17, 2016 at 11:09:24AM +0100, Markus Armbruster wrote: > Top level reply, because this isn't in reply to any specific message in > the thread, more like in reply to all of them. > > FW CFG's primary user is QEMU, which uses it to expose configuration > information (in the widest sense) to Firmware. Thus the name FW CFG. > > FW CFG can also be used by others for their own purposes. QEMU is > merely acting as transport then. > > I think there are actually three separate questions that should not be > mixed up. > > 1. Is it a good idea to offer FW CFG as a transport to others? > > I have no opinion on this myself, but I trust Gerd's and Laszlo's > judgement. Their answer seems to be a clear yes. > > 2. Is it a good idea to let users mess with QEMU's use of FW CFG? > > I think this is a special case of a more general question: should we > try to protect the user from himself? > > We should definitely try not to trap the user. Obvious usage should > be as safe as we can make it. Risky usage should be marked in the > docs and/or warn on use. > > However, we should not try to stop our users from doing stupid > things, as that would also stop them from doing clever things. > > 3. How should the FW CFG name space be shared among its users? > > Bad things can happen if others use the namespace in ways that > conflict with QEMU's use, or conflict with another "other". > > This isn't an issue specific to FW CFG. For instance, upstream QEMU > and the various downstream QEMUs all use the QMP command name space, > and bad things can happen if they conflict. The difference is they'd > conflict at compile time. Conflicts are easier to detect, but just > as hard to resolve. > > QMP's solution is to reserve part of the name space for "others" > (downstreams), and subdivide the reserved part further via RFQDN: > owning a DNS domain name makes you own that RFQDN subdivision. > > For FW CFG, we did only the first half, namely reserving part of the > name space for others: /opt/. We neglected to spell out rules for > its safe sharing, i.e. the second part. > > I don't think it's too late to fix that: amend the specification to > stipulate that owning a DNS domain name makes you own /opt/RFQDN/. > Throw in known existing uses like /opt/ovmf/ as special cases. As usual, very well put. This makes complete sense to me. I would rate the current interface rather as attempting to trap users. The only way to figure it out is to read the source, and the source includes ovmf and maybe other firmware. Here is a proposal trying to address the above, including a way to do stupid things: QEMU command line: A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts with QEMU hardware B. -fw-cfg org.qemu/unsupported/XXX as a hack, removes org.qemu/unsupported/ and leaves just XXX, for people who want to break^?^?^?^?^?debug QEMU hardware C. -fw-cfg opt/FOO accepts any path, for backwards compatibility D. any other use fails QEMU Documentation Document the above in the man page OVMF: Can use the compatible opt/ovmf/ for now. Long term: take one of two paths: A. Move part of values passed to OVMF into QEMU codebase If it's commonly needed for guests to boot, we should provide sane defaults and a friendly interface, with range checking etc. or B. Gradually transition OVMF to look up paths in usr/org.uefi/: if nothing is found there, look up in opt/ovmf/ for backwards compatibility. This makes sense if the feature is esoteric and very rarely used.
On Thu, Mar 17, 2016 at 02:12:56PM +0100, Paolo Bonzini wrote: > > > On 17/03/2016 12:32, Michael S. Tsirkin wrote: > > > 1) files should actually be named etc/ovmf/foo. OVMF could optionally > > > accept both the old and the new names for a while, you would decide > > > whether this is useful. > > > > IOW if etc/ovmf exists, then ignore opt/ovmf? OK. > > > > > 2) in turn, because of (1) even the warning on opt/ should be removed. > > > > If we do 2) then users might put files in etc/ovmf/foo. > > Right, that's exactly the point. These are user-configurable knobs for > the firmware (user-configurable meaning they can't be expected to > recompile QEMU). > > Paolo Right. See Markus' answer and my proposal that attempts to be a superset of this.
On Thu, Mar 17, 2016 at 10:43:08AM +0100, Laszlo Ersek wrote: > And, obviously, exposing this kind of knob with dedicated QEMU options > is out of question. We are moving away from dedicated options anyway. But these could easily be machine properties, with the benefit that there is actual validation of values, some help and documentation for users, nice shortcuts like G/M/K for numbers, introspection, unit tests and more.
On 03/17/16 11:22, Gerd Hoffmann wrote: > Hi, > >> Occasionally, yes. >> >> - "opt/ovmf/PcdPropertiesTableEnable" controls whether the "properties > >> - "opt/ovmf/PcdSetNxForStack" controls whether the stack is made > >> - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which the > >> In downstream, we have two more (same purpose but separately for ARM and >> x86): >> - opt/aavmf/PcdResizeXterm >> - opt/ovmf/PcdResizeXterm >> >> - Another flag I might expose later is "PcdHiiOsRuntimeSupport". This > > I think we should probably move those out of the opt/ namespace space, > as this is reserved for user-defined files. > > I think either "etc/efi/" This is more or less what Paolo suggests. It confuses me. I have now re-read the last section of "docs/specs/fw_cfg.txt" to make sure my current mental image of the original idea has not diverged from the actual original idea. I don't think so. So, "user defined files" exist so that users can control "stuff" with them, without QEMU's knowledge. OVMF is "stuff". Just because it's firmware, it remains "stuff". Whatever Gabriel wants to control with such fw_cfg files in the guest, is also "stuff". I don't see any difference between a user controlling an ad-hoc application of his with "opt/blah1", and controlling OVMF behavior under "opt/ovmf/xizzy". If the user tries to control his own app by choosing the name "opt/ovmf/xizzy", that's *his* fault. "opt/ovmf/" is not a prefix someone comes up with by chance. We meant just two partitions of the namespace. "opt/" and non-"opt/". The latter belongs to QEMU, the former belongs to everything else, and the subdivision of everything else doesn't belong into QEMU. OVMF is part of everything else. If we're paranoid about it, we can make a recommendation that each user pick "opt/UUID/..." for himself, running "uuidgen" and then sticking with the output for his own stuff. That will guarantee that no conflicts will be seen. OVMF could be adapted to that as well. (The only snag with uuidgen is that it would leave only 56 - 4 - 36 - 1 == 15 characters for the actual knob name.) Choosing "etc/" for such files of OVMF that are passed in with the -fw_cfg switch would be the textbook example of violating the original idea, because etc/ is heavily populated by QEMU itself. > or "efi/" is useful (so we don't have > different names on ovmf and aavmf). I dislike efi/. It suggests that it controls some features that all UEFI implementations offer. > Possibly add "pcd/". Maybe even > support setting *any* pcd via "efi/pcd/$name" (just an idea, not sure > whenever that is useful and is possible without being too invasive). It's all of: too invasive, not always useful, and not flexible enough. Knobs should be exposed (with their different value domains) on a case-by-case basis. >> - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which > > Hmm. Leave as-is for now I'd say. Will do. > If we decide to keep it we should > probably rename it and place it below etc/, next to reserved-memory-end. > Export the size in bytes, like reserved-memory-end does. Add an option > to qemu to set it, so you can specify the size as "32G" using the > standard qemu size parser. I certainly don't disagree with this, but if it goes under etc/, and -- hence necessarily -- also gets its own command line option, then SeaBIOS should probably adhere to it as well (or the QEMU manual should state that this option is also firmware specific). Thanks Laszlo
I frankly think it's overengineered, but it's already much better and if it helps converging to a compromise why not. Alternatives to your proposals follow: On 17/03/2016 14:13, Michael S. Tsirkin wrote: > > QEMU command line: > A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts > with QEMU hardware Alternative: no need to prepend usr/, I think. > B. -fw-cfg org.qemu/unsupported/XXX as a hack, removes > org.qemu/unsupported/ and leaves just XXX, > for people who want to break^?^?^?^?^?debug QEMU hardware Alternative: fail on: - a blacklist of etc/* files including etc/system-states, etc/smbios/smbios-tables, etc/smbios/smbios-anchor, etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly etc/boot-menu-wait - on all org.qemu/* files - iff etc/boot-menu-wait is blacklisted, fail on org.seabios/boot-menu-wait too. Everything else is passed through. No hacks required. > C. -fw-cfg opt/FOO accepts any path, for backwards compatibility Implicit in my proposed alternative to A. > D. any other use fails Replaced by my alternative to B. RFQDN is just a best practice, and it is not enforced except as proposed in B. For the same reason, no changes are required in the Linux driver. > OVMF: > Can use the compatible opt/ovmf/ for now. [snip] > Long term: Gradually transition OVMF to look up paths in usr/org.uefi/: > if nothing is found there, look up in opt/ovmf/ for backwards > compatibility. Agreed except it would be org.tianocore.edk2.ovmf/ rather than usr/org.uefi. Likewise SeaBIOS would switch from etc/ to an org.seabios/ prefix (for stuff usable from both Coreboot and QEMU, e.g. org.seabios/bootsplash.bmp) or org.qemu/ (for stuff that is specific to QEMU). Files that could be moved from etc/ to org.qemu/ correspond to the ones that are blacklisted in (B), e.g. etc/system-states -> org.qemu/system-states. Paolo
On Thu, Mar 17, 2016 at 02:28:38PM +0100, Laszlo Ersek wrote: > On 03/17/16 11:22, Gerd Hoffmann wrote: > > Hi, > > > >> Occasionally, yes. > >> > >> - "opt/ovmf/PcdPropertiesTableEnable" controls whether the "properties > > > >> - "opt/ovmf/PcdSetNxForStack" controls whether the stack is made > > > >> - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which the > > > >> In downstream, we have two more (same purpose but separately for ARM and > >> x86): > >> - opt/aavmf/PcdResizeXterm > >> - opt/ovmf/PcdResizeXterm > >> > >> - Another flag I might expose later is "PcdHiiOsRuntimeSupport". This > > > > I think we should probably move those out of the opt/ namespace space, > > as this is reserved for user-defined files. > > > > I think either "etc/efi/" > > This is more or less what Paolo suggests. > > It confuses me. > > I have now re-read the last section of "docs/specs/fw_cfg.txt" to make > sure my current mental image of the original idea has not diverged from > the actual original idea. I don't think so. > > So, "user defined files" exist so that users can control "stuff" with > them, without QEMU's knowledge. OVMF is "stuff". Just because it's > firmware, it remains "stuff". Whatever Gabriel wants to control with > such fw_cfg files in the guest, is also "stuff". I don't see any > difference between a user controlling an ad-hoc application of his with > "opt/blah1", and controlling OVMF behavior under "opt/ovmf/xizzy". If > the user tries to control his own app by choosing the name > "opt/ovmf/xizzy", that's *his* fault. "opt/ovmf/" is not a prefix > someone comes up with by chance. > > We meant just two partitions of the namespace. "opt/" and non-"opt/". > The latter belongs to QEMU, the former belongs to everything else, and > the subdivision of everything else doesn't belong into QEMU. OVMF is > part of everything else. This is where we made a design mistake. There are 3 kinds of users adding entries: QEMU, QEMU firmware developers and QEMU users. And QEMU users could be further subdivided. > If we're paranoid about it, we can make a recommendation that each user > pick "opt/UUID/..." for himself, running "uuidgen" and then sticking > with the output for his own stuff. That will guarantee that no conflicts > will be seen. OVMF could be adapted to that as well. (The only snag with > uuidgen is that it would leave only 56 - 4 - 36 - 1 == 15 characters for > the actual knob name.) > > Choosing "etc/" for such files of OVMF that are passed in with the > -fw_cfg switch would be the textbook example of violating the original > idea, because etc/ is heavily populated by QEMU itself. > > > or "efi/" is useful (so we don't have > > different names on ovmf and aavmf). > > I dislike efi/. It suggests that it controls some features that all UEFI > implementations offer. > > > Possibly add "pcd/". Maybe even > > support setting *any* pcd via "efi/pcd/$name" (just an idea, not sure > > whenever that is useful and is possible without being too invasive). > > It's all of: too invasive, not always useful, and not flexible enough. > Knobs should be exposed (with their different value domains) on a > case-by-case basis. > > >> - "opt/ovmf/X-PciMmio64Mb" controls the size of the range from which > > > > Hmm. Leave as-is for now I'd say. > > Will do. > > > If we decide to keep it we should > > probably rename it and place it below etc/, next to reserved-memory-end. > > Export the size in bytes, like reserved-memory-end does. Add an option > > to qemu to set it, so you can specify the size as "32G" using the > > standard qemu size parser. > > I certainly don't disagree with this, but if it goes under etc/, and -- > hence necessarily -- also gets its own command line option, then SeaBIOS > should probably adhere to it as well (or the QEMU manual should state > that this option is also firmware specific). > > Thanks > Laszlo
On 17/03/2016 14:35, Michael S. Tsirkin wrote: > > We meant just two partitions of the namespace. "opt/" and non-"opt/". > > The latter belongs to QEMU, the former belongs to everything else, and > > the subdivision of everything else doesn't belong into QEMU. OVMF is > > part of everything else. > > This is where we made a design mistake. There are 3 kinds of users > adding entries: QEMU, QEMU firmware developers and QEMU users. I can definitely agree with this. Paolo > And QEMU users could be further subdivided.
On 03/17/16 14:30, Paolo Bonzini wrote: > On 17/03/2016 14:13, Michael S. Tsirkin wrote: >> OVMF: >> Can use the compatible opt/ovmf/ for now. [snip] >> Long term: Gradually transition OVMF to look up paths in usr/org.uefi/: >> if nothing is found there, look up in opt/ovmf/ for backwards >> compatibility. > > Agreed except it would be org.tianocore.edk2.ovmf/ rather than usr/org.uefi. > > Likewise SeaBIOS would switch from etc/ to an org.seabios/ prefix (for > stuff usable from both Coreboot and QEMU, e.g. > org.seabios/bootsplash.bmp) or org.qemu/ (for stuff that is specific to > QEMU). I think it's feasible (in the long term) to make OVMF look for "standard" fw_cfg files under "org.qemu/", and for the OVMF-specific knobs under "org.tianocore.edk2.ovmf/". (The longest knob name OVMF uses at the moment is 24 chars; it would fit.) It's just that I don't see how a user is any more likely to randomly pick "opt/ovmf/" for his own ad-hoc purposes than to pick "org.tianocore.edk2.ovmf/". Anyway, I agree that RFQDNs leave a few characters for the actual knob names, and they are mostly unique. Thanks Laszlo
On Thu, Mar 17, 2016 at 02:30:34PM +0100, Paolo Bonzini wrote: > I frankly think it's overengineered, but it's already much better and if > it helps converging to a compromise why not. Thanks, I'll think of your suggestions over the weekend. We might be able to simplify things a bit. > Alternatives to your proposals follow: I wonder what are the advantages of the alternative though? It is certainly more code, the rules for users are more complex to figure out and need more effort for user to follow. > On 17/03/2016 14:13, Michael S. Tsirkin wrote: > > > > QEMU command line: > > A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts > > with QEMU hardware > > Alternative: no need to prepend usr/, I think. I personally dislike telling user "do X". I don't see a reason not to be friendly and do X. The rare case where users do not want X can be easily enabled. > > B. -fw-cfg org.qemu/unsupported/XXX as a hack, removes > > org.qemu/unsupported/ and leaves just XXX, > > for people who want to break^?^?^?^?^?debug QEMU hardware > > Alternative: fail on: > > - a blacklist of etc/* files including etc/system-states, > etc/smbios/smbios-tables, etc/smbios/smbios-anchor, > etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly > etc/boot-menu-wait We can not predict the future. Future firmware will look for files under etc/mst. Users using this firmware with current QEMU will get a nasty surprise where it previously worked. Besides, it is way easier to maintain and understand a simple rule than a blacklist. > - on all org.qemu/* files > > - iff etc/boot-menu-wait is blacklisted, fail on > org.seabios/boot-menu-wait too. > > Everything else is passed through. No hacks required. > > > C. -fw-cfg opt/FOO accepts any path, for backwards compatibility > > Implicit in my proposed alternative to A. > > > D. any other use fails > > Replaced by my alternative to B. RFQDN is just a best practice, and it > is not enforced except as proposed in B. For the same reason, no > changes are required in the Linux driver. > > > OVMF: > > Can use the compatible opt/ovmf/ for now. [snip] > > Long term: Gradually transition OVMF to look up paths in usr/org.uefi/: > > if nothing is found there, look up in opt/ovmf/ for backwards > > compatibility. > > Agreed except it would be org.tianocore.edk2.ovmf/ rather than usr/org.uefi. > > Likewise SeaBIOS would switch from etc/ to an org.seabios/ prefix (for > stuff usable from both Coreboot and QEMU, e.g. > org.seabios/bootsplash.bmp) or org.qemu/ (for stuff that is specific to > QEMU). > > Files that could be moved from etc/ to org.qemu/ correspond to the ones > that are blacklisted in (B), e.g. etc/system-states -> > org.qemu/system-states. > > Paolo I am not sure about moving things into usr/org.qemu. These are system files, not user-provided ones. But we can argue about future plans down the road.
On 17/03/2016 14:49, Michael S. Tsirkin wrote: >> On 17/03/2016 14:13, Michael S. Tsirkin wrote: >>> >>> QEMU command line: >>> A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts >>> with QEMU hardware >> >> Alternative: no need to prepend usr/, I think. > > I personally dislike telling user "do X". I don't see a reason not to be > friendly and do X. The rare case where users do not want X can be > easily enabled. I wouldn't include usr/ at all in the paths. The RFQDN recommendation is enough to avoid clashes with etc/ and opt/. >>> B. -fw-cfg org.qemu/unsupported/XXX as a hack, removes >>> org.qemu/unsupported/ and leaves just XXX, >>> for people who want to break^?^?^?^?^?debug QEMU hardware >> >> Alternative: fail on: >> >> - a blacklist of etc/* files including etc/system-states, >> etc/smbios/smbios-tables, etc/smbios/smbios-anchor, >> etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly >> etc/boot-menu-wait > > We can not predict the future. Future firmware will look for > files under etc/mst. Users using this firmware with > current QEMU will get a nasty surprise where it previously > worked. > > Besides, it is way easier to maintain and understand a simple rule than > a blacklist. The reason for the blacklist is that these are files owned by QEMU but traditionally under etc/. The error can be simply "fw_cfg file %s is provided by QEMU". If a file is added in the future that is owned by QEMU, it will be under org.qemu/* so the blacklist will not grow. >> Likewise SeaBIOS would switch from etc/ to an org.seabios/ prefix (for >> stuff usable from both Coreboot and QEMU, e.g. >> org.seabios/bootsplash.bmp) or org.qemu/ (for stuff that is specific to >> QEMU). >> >> Files that could be moved from etc/ to org.qemu/ correspond to the ones >> that are blacklisted in (B), e.g. etc/system-states -> >> org.qemu/system-states. > > I am not sure about moving things into usr/org.qemu. > These are system files, not user-provided ones. > But we can argue about future plans down the road. Does it make more sense if it's just org.qemu, not usr/org.qemu? Thanks, Paolo
On Thu, Mar 17, 2016 at 02:55:52PM +0100, Paolo Bonzini wrote: > > > On 17/03/2016 14:49, Michael S. Tsirkin wrote: > >> On 17/03/2016 14:13, Michael S. Tsirkin wrote: > >>> > >>> QEMU command line: > >>> A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts > >>> with QEMU hardware > >> > >> Alternative: no need to prepend usr/, I think. > > > > I personally dislike telling user "do X". I don't see a reason not to be > > friendly and do X. The rare case where users do not want X can be > > easily enabled. > > I wouldn't include usr/ at all in the paths. The RFQDN recommendation > is enough to avoid clashes with etc/ and opt/. Yes but then we need a blacklist. And usr/ is not visible to users so I do not see a problem with it. > >>> B. -fw-cfg org.qemu/unsupported/XXX as a hack, removes > >>> org.qemu/unsupported/ and leaves just XXX, > >>> for people who want to break^?^?^?^?^?debug QEMU hardware > >> > >> Alternative: fail on: > >> > >> - a blacklist of etc/* files including etc/system-states, > >> etc/smbios/smbios-tables, etc/smbios/smbios-anchor, > >> etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly > >> etc/boot-menu-wait > > > > We can not predict the future. Future firmware will look for > > files under etc/mst. Users using this firmware with > > current QEMU will get a nasty surprise where it previously > > worked. > > > > Besides, it is way easier to maintain and understand a simple rule than > > a blacklist. > > The reason for the blacklist is that these are files owned by QEMU but > traditionally under etc/. The error can be simply "fw_cfg file %s is > provided by QEMU". If a file is added in the future that is owned by > QEMU, it will be under org.qemu/* so the blacklist will not grow. Yes, but a new prefix seems like a cleaner way. > >> Likewise SeaBIOS would switch from etc/ to an org.seabios/ prefix (for > >> stuff usable from both Coreboot and QEMU, e.g. > >> org.seabios/bootsplash.bmp) or org.qemu/ (for stuff that is specific to > >> QEMU). > >> > >> Files that could be moved from etc/ to org.qemu/ correspond to the ones > >> that are blacklisted in (B), e.g. etc/system-states -> > >> org.qemu/system-states. > > > > I am not sure about moving things into usr/org.qemu. > > These are system files, not user-provided ones. > > But we can argue about future plans down the road. > > Does it make more sense if it's just org.qemu, not usr/org.qemu? > > Thanks, > > Paolo I am not sure, let's discuss after 2.6.
On 17/03/2016 15:17, Michael S. Tsirkin wrote: > On Thu, Mar 17, 2016 at 02:55:52PM +0100, Paolo Bonzini wrote: >> >> >> On 17/03/2016 14:49, Michael S. Tsirkin wrote: >>>> On 17/03/2016 14:13, Michael S. Tsirkin wrote: >>>>> >>>>> QEMU command line: >>>>> A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts >>>>> with QEMU hardware >>>> >>>> Alternative: no need to prepend usr/, I think. >>> >>> I personally dislike telling user "do X". I don't see a reason not to be >>> friendly and do X. The rare case where users do not want X can be >>> easily enabled. >> >> I wouldn't include usr/ at all in the paths. The RFQDN recommendation >> is enough to avoid clashes with etc/ and opt/. > > Yes but then we need a blacklist. Can't the blacklist be as simple as "org.qemu/*", plus some handling of legacy "etc/*"? We'd need special handling of "etc/*" anyway because SeaBIOS is using it (Gerd's usecase) and you certainly don't want to use org.qemu/unsupported/etc/XYZ hacks for that. Paolo
On Thu, Mar 17, 2016 at 03:50:08PM +0100, Paolo Bonzini wrote: > > > On 17/03/2016 15:17, Michael S. Tsirkin wrote: > > On Thu, Mar 17, 2016 at 02:55:52PM +0100, Paolo Bonzini wrote: > >> > >> > >> On 17/03/2016 14:49, Michael S. Tsirkin wrote: > >>>> On 17/03/2016 14:13, Michael S. Tsirkin wrote: > >>>>> > >>>>> QEMU command line: > >>>>> A. -fw-cfg RFQDN/PATH prepends usr/. So users will not get conflicts > >>>>> with QEMU hardware > >>>> > >>>> Alternative: no need to prepend usr/, I think. > >>> > >>> I personally dislike telling user "do X". I don't see a reason not to be > >>> friendly and do X. The rare case where users do not want X can be > >>> easily enabled. > >> > >> I wouldn't include usr/ at all in the paths. The RFQDN recommendation > >> is enough to avoid clashes with etc/ and opt/. > > > > Yes but then we need a blacklist. > > Can't the blacklist be as simple as "org.qemu/*", plus some handling of > legacy "etc/*"? > > We'd need special handling of "etc/*" anyway because SeaBIOS is using it > (Gerd's usecase) and you certainly don't want to use > org.qemu/unsupported/etc/XYZ hacks for that. > > Paolo I would prefer a white-list, but I have an idea: look for "." in the name. If not there, it's not an RFQDN.
Hi, > So, "user defined files" exist so that users can control "stuff" with > them, without QEMU's knowledge. OVMF is "stuff". Just because it's > firmware, it remains "stuff". Whatever Gabriel wants to control with > such fw_cfg files in the guest, is also "stuff". Well, non-firmware stuff was my interpretation, i.e. user-specific stuff in both guest and host. cheers, Gerd
Hi, > Alternative: fail on: > > - a blacklist of etc/* files including etc/system-states, > etc/smbios/smbios-tables, etc/smbios/smbios-anchor, > etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly > etc/boot-menu-wait I think that fails already because qemu throws an error on duplicate firmware entries. > Likewise SeaBIOS would switch from etc/ to an org.seabios/ prefix (for > stuff usable from both Coreboot and QEMU, e.g. > org.seabios/bootsplash.bmp) or org.qemu/ (for stuff that is specific to > QEMU). Do we really want shuffle around stuff like this? It means we have to support both paths in both firmware and qemu for a quite a while for compatibility reasons. (moving around opt/ovmf/ has this issue too, even though to a lesser extend as there isn't stuff as critical as acpi tables or boot order). Also note that most stuff in etc/ is used by all firmwares, so moving this to org.seabios looks pointless to me, especially as things might change over time (i.e. ovmf starting to use cfg options it used to ignore first, which is the case for etc/boot-menu-wait IIRC, so it started as seabios only but isn't any more). Also note that the fw_cfg layout is modeled a bit after cbfs (coreboot filesystem) which is provided by coreboot and used by seabios. That'll cause trouble too if we try to redefine the paths. cheers, Gerd
On 17/03/2016 18:17, Gerd Hoffmann wrote: > Hi, > >> Alternative: fail on: >> >> - a blacklist of etc/* files including etc/system-states, >> etc/smbios/smbios-tables, etc/smbios/smbios-anchor, >> etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly >> etc/boot-menu-wait > > I think that fails already because qemu throws an error on duplicate > firmware entries. The problem is that some files are only added conditionally (e.g. etc/boot-menu-wait). > Do we really want shuffle around stuff like this? It means we have to > support both paths in both firmware and qemu for a quite a while for > compatibility reasons. > > Also note that most stuff in etc/ is used by all firmwares, so moving > this to org.seabios looks pointless to me, especially as things might > change over time (i.e. ovmf starting to use cfg options it used to > ignore first, which is the case for etc/boot-menu-wait IIRC, so it > started as seabios only but isn't any more). Stuff that is used by all firmwares could move to org.coreboot/ or org.qemu/, but keeping etc/ as a special case is certainly fine by me. Paolo
On Thu, Mar 17, 2016 at 08:35:25PM +0100, Paolo Bonzini wrote: > > > On 17/03/2016 18:17, Gerd Hoffmann wrote: > > Hi, > > > >> Alternative: fail on: > >> > >> - a blacklist of etc/* files including etc/system-states, > >> etc/smbios/smbios-tables, etc/smbios/smbios-anchor, > >> etc/reserved-memory-end, etc/pvpanic-port, etc/e820, and possibly > >> etc/boot-menu-wait > > > > I think that fails already because qemu throws an error on duplicate > > firmware entries. > > The problem is that some files are only added conditionally (e.g. > etc/boot-menu-wait). Another problem is that future firmware can run on an older QEMU. Any blacklist-only strategy fails because of that, we need a schema. > > Do we really want shuffle around stuff like this? It means we have to > > support both paths in both firmware and qemu for a quite a while for > > compatibility reasons. > > > > Also note that most stuff in etc/ is used by all firmwares, so moving > > this to org.seabios looks pointless to me, especially as things might > > change over time (i.e. ovmf starting to use cfg options it used to > > ignore first, which is the case for etc/boot-menu-wait IIRC, so it > > started as seabios only but isn't any more). > > Stuff that is used by all firmwares could move to org.coreboot/ or > org.qemu/, but keeping etc/ as a special case is certainly fine by me. > > Paolo
diff --git a/configure b/configure index 2b32876..e5a692f 100755 --- a/configure +++ b/configure @@ -256,6 +256,7 @@ sysconfdir="\${prefix}/etc" local_statedir="\${prefix}/var" confsuffix="/qemu" slirp="yes" +unsafe_fw_cfg="no" oss_lib="" bsd="no" linux="no" @@ -875,6 +876,8 @@ for opt do ;; --enable-vnc-png) vnc_png="yes" ;; + --enable-unsafe-fw-cfg) unsafe_fw_cfg="yes" + ;; --disable-slirp) slirp="no" ;; --disable-uuid) uuid="no" @@ -1286,6 +1289,7 @@ Advanced options (experts only): Available backends: $($python $source_path/scripts/tracetool.py --list-backends) --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace-<pid> + --enable-unsafe-fw-cfg Enable loading fw cfg entries at unsafe paths (broken! don't use in production!) --disable-slirp disable SLIRP userspace network connectivity --enable-tcg-interpreter enable TCG with bytecode interpreter (TCI) --oss-lib path to OSS library @@ -4911,6 +4915,9 @@ fi if test "$profiler" = "yes" ; then echo "CONFIG_PROFILER=y" >> $config_host_mak fi +if test "$unsafe_fw_cfg" = "yes" ; then + echo "CONFIG_UNSAFE_FW_CFG=y" >> $config_host_mak +fi if test "$slirp" = "yes" ; then echo "CONFIG_SLIRP=y" >> $config_host_mak echo "CONFIG_SMBD_COMMAND=\"$smbd\"" >> $config_host_mak diff --git a/vl.c b/vl.c index 7a28982..5adf217 100644 --- a/vl.c +++ b/vl.c @@ -2321,6 +2321,9 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) if (strncmp(name, "opt/", 4) != 0) { error_report("warning: externally provided fw_cfg item names " "should be prefixed with \"opt/\""); +#ifndef CONFIG_UNSAFE_FW_CFG + return -1; +#endif } if (nonempty_str(str)) { size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
Allowing arbitary file names on command line is setting us up for failure: future guests will look for a specific QEMU-specified name and will get confused finding a user file there. We do warn but people are conditioned to ignore warnings by now, so at best that will help users debug problem, not avoid it. Disable this by default, so distros don't get to deal with it, but leave an option for developers to configure this in, with scary warnings so people only do it if they know what they are doing. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- changes from v1: add configure option configure | 7 +++++++ vl.c | 3 +++ 2 files changed, 10 insertions(+)