| Message ID | 1417637167-20640-5-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 3 December 2014 at 20:05, Greg Bellows <greg.bellows@linaro.org> wrote: > Added 'secure' qemu boolean option to qemu_machine_opts[]. > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > --- > vl.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/vl.c b/vl.c > index eb89d62..5d640f7 100644 > --- a/vl.c > +++ b/vl.c > @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = { > .name = "iommu", > .type = QEMU_OPT_BOOL, > .help = "Set on/off to enable/disable Intel IOMMU (VT-d)", > + },{ > + .name = "secure", > + .type = QEMU_OPT_BOOL, > + .help = "Set on/off to enable/disable secure state", > }, If patch 5 adds 'secure' as a machine property for only those boards where it makes sense, why do we need this global switch? thanks -- PMM
On 5 December 2014 at 09:18, Peter Maydell <peter.maydell@linaro.org> wrote: > On 3 December 2014 at 20:05, Greg Bellows <greg.bellows@linaro.org> wrote: > > Added 'secure' qemu boolean option to qemu_machine_opts[]. > > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > vl.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/vl.c b/vl.c > > index eb89d62..5d640f7 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = { > > .name = "iommu", > > .type = QEMU_OPT_BOOL, > > .help = "Set on/off to enable/disable Intel IOMMU (VT-d)", > > + },{ > > + .name = "secure", > > + .type = QEMU_OPT_BOOL, > > + .help = "Set on/off to enable/disable secure state", > > }, > > If patch 5 adds 'secure' as a machine property for only those > boards where it makes sense, why do we need this global switch? > > That is what I thought as well, but this is apparently needed as we get an invalid machine property otherwise. Below is the error, I'll look again, but it appeared all machine properties were defined here. *qemu-system-aarch64: -machine type=vexpress-a15,secure=off: Invalid parameter 'secure'* > thanks > -- PMM >
On 5 December 2014 at 15:33, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On 5 December 2014 at 09:18, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On 3 December 2014 at 20:05, Greg Bellows <greg.bellows@linaro.org> wrote: >> > Added 'secure' qemu boolean option to qemu_machine_opts[]. >> > >> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> >> > --- >> > vl.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/vl.c b/vl.c >> > index eb89d62..5d640f7 100644 >> > --- a/vl.c >> > +++ b/vl.c >> > @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = { >> > .name = "iommu", >> > .type = QEMU_OPT_BOOL, >> > .help = "Set on/off to enable/disable Intel IOMMU (VT-d)", >> > + },{ >> > + .name = "secure", >> > + .type = QEMU_OPT_BOOL, >> > + .help = "Set on/off to enable/disable secure state", >> > }, >> >> If patch 5 adds 'secure' as a machine property for only those >> boards where it makes sense, why do we need this global switch? >> > > That is what I thought as well, but this is apparently needed as we get an > invalid machine property otherwise. Below is the error, I'll look again, > but it appeared all machine properties were defined here. > > qemu-system-aarch64: -machine type=vexpress-a15,secure=off: Invalid > parameter 'secure' That would seem to defeat the point of the machine opts design, so it looks a bit strange. Marcel: how is this supposed to work for board-specific -machine options? thanks -- PMM
On Fri, 2014-12-05 at 15:39 +0000, Peter Maydell wrote: > On 5 December 2014 at 15:33, Greg Bellows <greg.bellows@linaro.org> wrote: > > > > > > On 5 December 2014 at 09:18, Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >> On 3 December 2014 at 20:05, Greg Bellows <greg.bellows@linaro.org> wrote: > >> > Added 'secure' qemu boolean option to qemu_machine_opts[]. > >> > > >> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > >> > --- > >> > vl.c | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > > >> > diff --git a/vl.c b/vl.c > >> > index eb89d62..5d640f7 100644 > >> > --- a/vl.c > >> > +++ b/vl.c > >> > @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = { > >> > .name = "iommu", > >> > .type = QEMU_OPT_BOOL, > >> > .help = "Set on/off to enable/disable Intel IOMMU (VT-d)", > >> > + },{ > >> > + .name = "secure", > >> > + .type = QEMU_OPT_BOOL, > >> > + .help = "Set on/off to enable/disable secure state", > >> > }, > >> > >> If patch 5 adds 'secure' as a machine property for only those > >> boards where it makes sense, why do we need this global switch? > >> > > > > That is what I thought as well, but this is apparently needed as we get an > > invalid machine property otherwise. Below is the error, I'll look again, > > but it appeared all machine properties were defined here. > > > > qemu-system-aarch64: -machine type=vexpress-a15,secure=off: Invalid > > parameter 'secure' > > That would seem to defeat the point of the machine opts design, > so it looks a bit strange. Marcel: how is this supposed to work > for board-specific -machine options? Hi Peter, We have indeed properties per machine type and it works like this: 1. You add a QOM property in the specific machine init code. (Exactly as in [PATCH 05/13] target-arm: Add vexpress machine secure property) 2. In vl.c the following code should automatically fill in the per machine properties. machine_opts = qemu_get_machine_opts(); if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine, 1) < 0) { object_unref(OBJECT(current_machine)); exit(1); } 3. machine_set_property should handle the "per machine" properties. That being said, we do have a problem in the way the machine_opts are built. In order for the property to be "visible", we need to add it to a global qemu_machine_opts list. The reason (I think) is the parsing code that checks it against a predefined list: The correct way to to this is to build the machine option list dynamically and not from the predefined global list and check them against the specific machine instance. Andreas, does it seems right to you? Thanks for bringing this to my attention! I'll fix this and submit a patch shortly. Thanks, Marcel > > thanks > -- PMM
Thanks Marcel. Just to make sure I understand, at this point do to limitations in the existing functionality, there is nothing that can be done other than adding the option to the global qemu_machine_opts list. Once you add a fix then it will be possible to add it dynamically. If I missed anything please let me know. Regards, Greg On 5 December 2014 at 13:40, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Fri, 2014-12-05 at 15:39 +0000, Peter Maydell wrote: > > On 5 December 2014 at 15:33, Greg Bellows <greg.bellows@linaro.org> > wrote: > > > > > > > > > On 5 December 2014 at 09:18, Peter Maydell <peter.maydell@linaro.org> > wrote: > > >> > > >> On 3 December 2014 at 20:05, Greg Bellows <greg.bellows@linaro.org> > wrote: > > >> > Added 'secure' qemu boolean option to qemu_machine_opts[]. > > >> > > > >> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > >> > --- > > >> > vl.c | 4 ++++ > > >> > 1 file changed, 4 insertions(+) > > >> > > > >> > diff --git a/vl.c b/vl.c > > >> > index eb89d62..5d640f7 100644 > > >> > --- a/vl.c > > >> > +++ b/vl.c > > >> > @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = { > > >> > .name = "iommu", > > >> > .type = QEMU_OPT_BOOL, > > >> > .help = "Set on/off to enable/disable Intel IOMMU > (VT-d)", > > >> > + },{ > > >> > + .name = "secure", > > >> > + .type = QEMU_OPT_BOOL, > > >> > + .help = "Set on/off to enable/disable secure state", > > >> > }, > > >> > > >> If patch 5 adds 'secure' as a machine property for only those > > >> boards where it makes sense, why do we need this global switch? > > >> > > > > > > That is what I thought as well, but this is apparently needed as we > get an > > > invalid machine property otherwise. Below is the error, I'll look > again, > > > but it appeared all machine properties were defined here. > > > > > > qemu-system-aarch64: -machine type=vexpress-a15,secure=off: Invalid > > > parameter 'secure' > > > > That would seem to defeat the point of the machine opts design, > > so it looks a bit strange. Marcel: how is this supposed to work > > for board-specific -machine options? > Hi Peter, > > We have indeed properties per machine type and it works like this: > 1. You add a QOM property in the specific machine init code. > (Exactly as in [PATCH 05/13] target-arm: Add vexpress machine secure > property) > > 2. In vl.c the following code should automatically fill in the per machine > properties. > > machine_opts = qemu_get_machine_opts(); > if (qemu_opt_foreach(machine_opts, machine_set_property, > current_machine, > 1) < 0) { > object_unref(OBJECT(current_machine)); > exit(1); > } > > 3. machine_set_property should handle the "per machine" properties. > > That being said, we do have a problem in the way the machine_opts are > built. > In order for the property to be "visible", we need to add it to a global > qemu_machine_opts list. > The reason (I think) is the parsing code that checks it against a > predefined list: > > The correct way to to this is to build the machine option list dynamically > and not from the predefined global list and check them against the > specific machine instance. > Andreas, does it seems right to you? > > Thanks for bringing this to my attention! > I'll fix this and submit a patch shortly. > > Thanks, > Marcel > > > > > > > > thanks > > -- PMM > > > >
On Fri, 2014-12-05 at 14:40 -0600, Greg Bellows wrote: > Thanks Marcel. > > > Just to make sure I understand, at this point do to limitations in the > existing functionality, there is nothing that can be done other than > adding the option to the global qemu_machine_opts list. Once you add > a fix then it will be possible to add it dynamically. Yes, this is correct. What we have now is a way to determine if an option belongs to a specific machine, for example trying to use your "secure" option with a PC machine will fail since PC machines do not have this property. But you still need to define that option in the global qemu_machine_opts in order to use it. This is of course not good enough and we will take care of it. We have two options here: 1. You add the "secure" option to the machines opts and I'll remove it once I'll fix the above limitation. 2. You wait until I fix this and you'll not need it at all. I am OK with it other way, but the decision is not only mine :) I'll try to come up with something next week, but it will need reviews and it may postpone your series. However I suppose the series is for 2.3, so we have plenty of time to do it properly. Thanks, Marcel > > > If I missed anything please let me know. > > > Regards, > > > Greg > > On 5 December 2014 at 13:40, Marcel Apfelbaum <marcel.a@redhat.com> > wrote: > On Fri, 2014-12-05 at 15:39 +0000, Peter Maydell wrote: > > On 5 December 2014 at 15:33, Greg Bellows > <greg.bellows@linaro.org> wrote: > > > > > > > > > On 5 December 2014 at 09:18, Peter Maydell > <peter.maydell@linaro.org> wrote: > > >> > > >> On 3 December 2014 at 20:05, Greg Bellows > <greg.bellows@linaro.org> wrote: > > >> > Added 'secure' qemu boolean option to > qemu_machine_opts[]. > > >> > > > >> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > >> > --- > > >> > vl.c | 4 ++++ > > >> > 1 file changed, 4 insertions(+) > > >> > > > >> > diff --git a/vl.c b/vl.c > > >> > index eb89d62..5d640f7 100644 > > >> > --- a/vl.c > > >> > +++ b/vl.c > > >> > @@ -387,6 +387,10 @@ static QemuOptsList > qemu_machine_opts = { > > >> > .name = "iommu", > > >> > .type = QEMU_OPT_BOOL, > > >> > .help = "Set on/off to enable/disable > Intel IOMMU (VT-d)", > > >> > + },{ > > >> > + .name = "secure", > > >> > + .type = QEMU_OPT_BOOL, > > >> > + .help = "Set on/off to enable/disable > secure state", > > >> > }, > > >> > > >> If patch 5 adds 'secure' as a machine property for only > those > > >> boards where it makes sense, why do we need this global > switch? > > >> > > > > > > That is what I thought as well, but this is apparently > needed as we get an > > > invalid machine property otherwise. Below is the error, > I'll look again, > > > but it appeared all machine properties were defined here. > > > > > > qemu-system-aarch64: -machine > type=vexpress-a15,secure=off: Invalid > > > parameter 'secure' > > > > That would seem to defeat the point of the machine opts > design, > > so it looks a bit strange. Marcel: how is this supposed to > work > > for board-specific -machine options? > > Hi Peter, > > We have indeed properties per machine type and it works like > this: > 1. You add a QOM property in the specific machine init code. > (Exactly as in [PATCH 05/13] target-arm: Add vexpress > machine secure property) > > 2. In vl.c the following code should automatically fill in the > per machine properties. > > machine_opts = qemu_get_machine_opts(); > if (qemu_opt_foreach(machine_opts, machine_set_property, > current_machine, > 1) < 0) { > object_unref(OBJECT(current_machine)); > exit(1); > } > > 3. machine_set_property should handle the "per machine" > properties. > > That being said, we do have a problem in the way the > machine_opts are built. > In order for the property to be "visible", we need to add it > to a global > qemu_machine_opts list. > The reason (I think) is the parsing code that checks it > against a predefined list: > > The correct way to to this is to build the machine option list > dynamically > and not from the predefined global list and check them against > the > specific machine instance. > Andreas, does it seems right to you? > > Thanks for bringing this to my attention! > I'll fix this and submit a patch shortly. > > Thanks, > Marcel > > > > > > > > thanks > > -- PMM > > > > >
Thanks for the clarification Marcel. I'm not sure my stuff is quite ready to go in either, so why don't we both move ahead and we can address it when we have a better idea of who might make it in first. Yes, 2.3 would be the target and we have plenty of time. Greg On 5 December 2014 at 16:44, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Fri, 2014-12-05 at 14:40 -0600, Greg Bellows wrote: > > Thanks Marcel. > > > > > > Just to make sure I understand, at this point do to limitations in the > > existing functionality, there is nothing that can be done other than > > adding the option to the global qemu_machine_opts list. Once you add > > a fix then it will be possible to add it dynamically. > > Yes, this is correct. > > What we have now is a way to determine if an option belongs to a specific > machine, > for example trying to use your "secure" option with a PC machine will fail > since PC machines do not have this property. > > But you still need to define that option in the global qemu_machine_opts > in order to use it. This is of course not good enough and we will take > care of it. > > We have two options here: > 1. You add the "secure" option to the machines opts and I'll remove it once > I'll fix the above limitation. > 2. You wait until I fix this and you'll not need it at all. > > I am OK with it other way, but the decision is not only mine :) > I'll try to come up with something next week, but it will need reviews > and it may postpone your series. However I suppose the series is for 2.3, > so we have plenty of time to do it properly. > > Thanks, > Marcel > > > > > > > If I missed anything please let me know. > > > > > > Regards, > > > > > > Greg > > > > On 5 December 2014 at 13:40, Marcel Apfelbaum <marcel.a@redhat.com> > > wrote: > > On Fri, 2014-12-05 at 15:39 +0000, Peter Maydell wrote: > > > On 5 December 2014 at 15:33, Greg Bellows > > <greg.bellows@linaro.org> wrote: > > > > > > > > > > > > On 5 December 2014 at 09:18, Peter Maydell > > <peter.maydell@linaro.org> wrote: > > > >> > > > >> On 3 December 2014 at 20:05, Greg Bellows > > <greg.bellows@linaro.org> wrote: > > > >> > Added 'secure' qemu boolean option to > > qemu_machine_opts[]. > > > >> > > > > >> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > >> > --- > > > >> > vl.c | 4 ++++ > > > >> > 1 file changed, 4 insertions(+) > > > >> > > > > >> > diff --git a/vl.c b/vl.c > > > >> > index eb89d62..5d640f7 100644 > > > >> > --- a/vl.c > > > >> > +++ b/vl.c > > > >> > @@ -387,6 +387,10 @@ static QemuOptsList > > qemu_machine_opts = { > > > >> > .name = "iommu", > > > >> > .type = QEMU_OPT_BOOL, > > > >> > .help = "Set on/off to enable/disable > > Intel IOMMU (VT-d)", > > > >> > + },{ > > > >> > + .name = "secure", > > > >> > + .type = QEMU_OPT_BOOL, > > > >> > + .help = "Set on/off to enable/disable > > secure state", > > > >> > }, > > > >> > > > >> If patch 5 adds 'secure' as a machine property for only > > those > > > >> boards where it makes sense, why do we need this global > > switch? > > > >> > > > > > > > > That is what I thought as well, but this is apparently > > needed as we get an > > > > invalid machine property otherwise. Below is the error, > > I'll look again, > > > > but it appeared all machine properties were defined here. > > > > > > > > qemu-system-aarch64: -machine > > type=vexpress-a15,secure=off: Invalid > > > > parameter 'secure' > > > > > > That would seem to defeat the point of the machine opts > > design, > > > so it looks a bit strange. Marcel: how is this supposed to > > work > > > for board-specific -machine options? > > > > Hi Peter, > > > > We have indeed properties per machine type and it works like > > this: > > 1. You add a QOM property in the specific machine init code. > > (Exactly as in [PATCH 05/13] target-arm: Add vexpress > > machine secure property) > > > > 2. In vl.c the following code should automatically fill in the > > per machine properties. > > > > machine_opts = qemu_get_machine_opts(); > > if (qemu_opt_foreach(machine_opts, machine_set_property, > > current_machine, > > 1) < 0) { > > object_unref(OBJECT(current_machine)); > > exit(1); > > } > > > > 3. machine_set_property should handle the "per machine" > > properties. > > > > That being said, we do have a problem in the way the > > machine_opts are built. > > In order for the property to be "visible", we need to add it > > to a global > > qemu_machine_opts list. > > The reason (I think) is the parsing code that checks it > > against a predefined list: > > > > The correct way to to this is to build the machine option list > > dynamically > > and not from the predefined global list and check them against > > the > > specific machine instance. > > Andreas, does it seems right to you? > > > > Thanks for bringing this to my attention! > > I'll fix this and submit a patch shortly. > > > > Thanks, > > Marcel > > > > > > > > > > > > > > thanks > > > -- PMM > > > > > > > > > > > > > >
diff --git a/vl.c b/vl.c index eb89d62..5d640f7 100644 --- a/vl.c +++ b/vl.c @@ -387,6 +387,10 @@ static QemuOptsList qemu_machine_opts = { .name = "iommu", .type = QEMU_OPT_BOOL, .help = "Set on/off to enable/disable Intel IOMMU (VT-d)", + },{ + .name = "secure", + .type = QEMU_OPT_BOOL, + .help = "Set on/off to enable/disable secure state", }, { /* End of list */ } },
Added 'secure' qemu boolean option to qemu_machine_opts[]. Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- vl.c | 4 ++++ 1 file changed, 4 insertions(+)