On Thu, Jul 08, 2021 at 03:00:22PM +0200, BALATON Zoltan wrote:
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
On 08/07/2021 20:39, BALATON Zoltan wrote:
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
On 08/07/2021 20:18, BALATON Zoltan wrote:
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
This addresses the comments from v22.
The functional changes are (the VOF ones need retesting with Pegasos2):
(VOF) setprop will start failing if the machine class callback
did not handle it;
I'll try this later but I think I've seen guests using
setprop (Linux also does that for some property). How should
I allow that? Do I need a new callback for this? Could it be
allower unless there's a callback that could deby it? But
that was the previous way I think.
A simple defined callback which always returns "true" should do.
Yes but what's the point? That would just effectiverly disable this
change so if we need that, we could just as well keep the previous
behaviour which is to allow setprop unless there's a callback that
can decide otherwise. The spapr machine has such a callback so it
already does not allow all setprop and if I'll have a callback in
pegasos2 returning true that will allow what's allowed now so this
part of this patch does nothing indeed.
Since guests could do all kinds of things that we don't know without
trying them restricting setprop is a good way to run into problems
with guests that were not tested that could otherwise just work.
Then we'll need another patch to enable that guest adding some more
properties to the list of allowed ones. Why it it a problem to allow
this by default in the first place and only reject changes for
machines that have a callback? Then I would not need more empty
callbacks in pegasos2.
From here:
https://patchwork.ozlabs.org/project/qemu-devel/patch/20210625055155.2252896-1-aik@ozlabs.ru/#2714158
===
+ if (vmo) {
+ VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
+
+ if (vmc->setprop &&
+ !vmc->setprop(ms, nodepath, propname, val, vallen)) {
+ goto trace_exit;
This defaults to allowing the setprop if the machine doesn't provide a
setprop callback. I think it would be safer to default to prohibiting
all setprops except those the machine explicitly allows.
Mmmm... I can imagine the client using the device tree as a temporary
storage. I'd rather add a trace for such cases.
If they do, I think that's something we'll need to consider and
account for that platform, rather than something we want to allow to
begin with.
I've seen that, yet I don't understand why. If I'll just add an empty
callback in pegasos2 to disable it then we're back to where we were before.
So my question is why do we want to explicitely enable setprop for every
guest when we encounter it one by one (especially if this works on other OF
implementations so guests are free to change the device tree therefore we
don't know in advance what are allowable properties). If you don't want it
for spapr I think you already have the callback for it that disallows it for
all but at a few properties but why change the default for other machines
that don't have a callback? If I can still add an empty callback that could
well be the default just to avoid more boilerplate in board code.
Because I think hitting the failure and deciphering that we need to
add setprop logic is likely to be less pain in the long run, than
allowing setprop by default, then discovering things break because the
guest expected that setprop to have some semantic effect beyond just
changing the dt, and we never even realized it was doing it.