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

[Marcel Apfelbaum]

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