Re: [Qemu-devel] [PATCH 04/13] target-arm: Add secure qemu machine option

[Greg Bellows]

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 <address@hidden> 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 <address@hidden>
> wrote:
>         On Fri, 2014-12-05 at 15:39 +0000, Peter Maydell wrote:
>         > On 5 December 2014 at 15:33, Greg Bellows
>         <address@hidden> wrote:
>         > >
>         > >
>         > > On 5 December 2014 at 09:18, Peter Maydell
>         <address@hidden> wrote:
>         > >>
>         > >> On 3 December 2014 at 20:05, Greg Bellows
>         <address@hidden> wrote:
>         > >> > Added 'secure' qemu boolean option to
>         qemu_machine_opts[].
>         > >> >
>         > >> > Signed-off-by: Greg Bellows <address@hidden>
>         > >> > ---
>         > >> >  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
>
>
>
>
>