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

[Alexey Kardashevskiy]

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() but eventually called address_space_rw() 
returns an error code, should I switch to it?


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




--
Alexey