Re: [Qemu-ppc] [PATCH] hw/ppc/spapr_rng: Introduce CONFIG_SPAPR_RNG switch for spapr_rng.c

[David Gibson]

On Fri, Oct 05, 2018 at 08:12:12AM +0200, Thomas Huth wrote:
> On 2018-10-05 06:25, David Gibson wrote:
> > On Thu, Oct 04, 2018 at 12:07:01PM +0200, Thomas Huth wrote:
> >> The spapr-rng device is suboptimal when compared to virtio-rng, so
> >> users might want to disable it in their builds. Thus let's introduce
> >> a proper CONFIG switch to allow us to compile QEMU without this device.
> >>
> >> Signed-off-by: Thomas Huth <address@hidden>
> > 
> > Uh, sure, I guess so.
> 
> Yes, we definitely want this for the QEMU in RHEL :-)
> 
> >> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
> >> index d2acd61..dec8434 100644
> >> --- a/hw/ppc/spapr_rng.c
> >> +++ b/hw/ppc/spapr_rng.c
> >> @@ -27,6 +27,7 @@
> >>  #include "sysemu/rng.h"
> >>  #include "hw/ppc/spapr.h"
> >>  #include "kvm_ppc.h"
> >> +#include "spapr_rng.h"
> >>  
> >>  #define SPAPR_RNG(obj) \
> >>      OBJECT_CHECK(sPAPRRngState, (obj), TYPE_SPAPR_RNG)
> >> @@ -132,29 +133,6 @@ static void spapr_rng_realize(DeviceState *dev, Error 
> >> **errp)
> >>      }
> >>  }
> >>  
> >> -int spapr_rng_populate_dt(void *fdt)
> >> -{
> >> -    int node;
> >> -    int ret;
> >> -
> >> -    node = qemu_fdt_add_subnode(fdt, "/ibm,platform-facilities");
> >> -    if (node <= 0) {
> >> -        return -1;
> >> -    }
> >> -    ret = fdt_setprop_string(fdt, node, "device_type",
> >> -                             "ibm,platform-facilities");
> >> -    ret |= fdt_setprop_cell(fdt, node, "#address-cells", 0x1);
> >> -    ret |= fdt_setprop_cell(fdt, node, "#size-cells", 0x0);
> >> -
> >> -    node = fdt_add_subnode(fdt, node, "ibm,random-v1");
> >> -    if (node <= 0) {
> >> -        return -1;
> >> -    }
> >> -    ret |= fdt_setprop_string(fdt, node, "compatible", "ibm,random");
> >> -
> >> -    return ret ? -1 : 0;
> >> -}
> >> -
> > 
> > Moving this to an inline doesn't seem right to me though - it's a more
> > complex function that we usually want in a .h inline, and I don't
> > really see a good reason for it to be there (rather than an #ifdeffed
> > stub).
> 
> An #ifdef is not possible here - the CONFIG switches for the targets are
> *not* turned into pre-processor macros, only the CONFIG switches for the
> host settings.

Ah, right.

> So putting this function as static inline into a separate
> header seems to be the best option to me right now. Alternatively, I
> could also put it directly into spapr.c directly, but that file is
> already very big... well, I don't mind, let me know what you prefer.

I'd prefer spapr.c to the inline.

But.. couldn't you put a stub version in stubs?  That would make a
weak symbol that would be overridden when SPAPR_RNG is compiled in.

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