Re: [PATCH qemu v23] spapr: Fix implementation of Open Firmware client interface

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.

===


-- 
Alexey