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

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3055 bytes --]

On 2018-10-08 02:57, David Gibson wrote:
> 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 <thuth@redhat.com>
>>>
>>> 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.

We could ... but stubs should IMHO only be used if there is no other
good solution. In this case, it's perfectly fine if we just move the
spapr_rng_populate_dt() function to another location. So I'll have a try
with spapr.c instead.

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]