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

[Marcel Apfelbaum <marcel.a@redhat.com>]

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
>         
>         
>         
> 
>