Re: [PATCH qemu v13] spapr: Implement Open Firmware client interface

[David Gibson]

On Tue, Feb 23, 2021 at 11:19:38PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 23/02/2021 14:07, David Gibson wrote:
> > On Tue, Feb 09, 2021 at 10:02:52PM +1100, Alexey Kardashevskiy wrote:
> > > The PAPR platform which describes an OS environment that's presented by
> > > a combination of a hypervisor and firmware. The features it specifies
> > > require collaboration between the firmware and the hypervisor.
> > > 
> 
> [...]
> 
> > > +target_ulong spapr_h_vof_client(PowerPCCPU *cpu, SpaprMachineState 
> > > *spapr,
> > > +                                target_ulong opcode, target_ulong *args)
> > > +{
> > > +    target_ulong of_client_args = ppc64_phys_to_real(args[0]);
> > > +    struct prom_args pargs = { 0 };
> > > +    char service[64];
> > > +    unsigned nargs, nret, i;
> > > +
> > > +    cpu_physical_memory_read(of_client_args, &pargs, sizeof(pargs));
> > 
> > Need to check for read errors in case an out of bounds address is passed.
> 
> 
> cpu_physical_memory_read() returns void and so does
> cpu_physical_memory_rw()

Sorry, I'd forgotten that was the case.

> but eventually called address_space_rw() returns an error code, should I
> switch to it?

Yes, I think that would be best.

> > > +    nargs = be32_to_cpu(pargs.nargs);
> > > +    if (nargs >= ARRAY_SIZE(pargs.args)) {
> > > +        return H_PARAMETER;
> > > +    }
> > > +
> > > +    cpu_physical_memory_read(be32_to_cpu(pargs.service), service,
> > > +                             sizeof(service));
> > > +    if (strnlen(service, sizeof(service)) == sizeof(service)) {
> > > +        /* Too long service name */
> > > +        return H_PARAMETER;
> > > +    }
> > > +
> > > +    for (i = 0; i < nargs; ++i) {
> > > +        pargs.args[i] = be32_to_cpu(pargs.args[i]);
> > 
> > In general I dislike in-place endian conversion of structs, since I
> > think it's less confusing to think of the endianness as a property of
> > the type.
> 
> The type is uint32_t and there is no be32 in QEMU. I can have 2 copies of
> pargs if this makes reviewing easier, should I?

Even having 2 copies of the struct I don't really like.  Encoding the
endianness down to the individual field level is great when the tools
are available, but as you note qemu doesn't really have that.

But even without that, I like the endianness of structs to be fixed by
convention.  Otherwise when you see a struct instance it's not very
easy to tell if it's a pre-conversion or post-conversion version at
any point in the code.  That means later changes - even just simple
looking code motions can become very fragile, because they move things
to a point where the struct doesn't have the previously expected
endianness.

By preferred solution here when using a struct which needs to map
directly onto in-memory information with a specific endianness is to
*always* leave the struct in that endianness, and only convert when we
actually take things in or out of the struct to use them in
calculations.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature