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

Thomas Huth posted 1 patch 5 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1538647621-9376-1-git-send-email-thuth@redhat.com
Test docker-clang@ubuntu failed
Test checkpatch passed
There is a newer version of this series
default-configs/ppc64-softmmu.mak |  1 +
hw/ppc/Makefile.objs              |  3 ++-
hw/ppc/spapr.c                    |  3 ++-
hw/ppc/spapr_rng.c                | 24 +-------------------
hw/ppc/spapr_rng.h                | 48 +++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr.h            |  4 ----
6 files changed, 54 insertions(+), 29 deletions(-)
create mode 100644 hw/ppc/spapr_rng.h
[Qemu-devel] [PATCH] hw/ppc/spapr_rng: Introduce CONFIG_SPAPR_RNG switch for spapr_rng.c
Posted by Thomas Huth 5 years, 5 months ago
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>
---
 default-configs/ppc64-softmmu.mak |  1 +
 hw/ppc/Makefile.objs              |  3 ++-
 hw/ppc/spapr.c                    |  3 ++-
 hw/ppc/spapr_rng.c                | 24 +-------------------
 hw/ppc/spapr_rng.h                | 48 +++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h            |  4 ----
 6 files changed, 54 insertions(+), 29 deletions(-)
 create mode 100644 hw/ppc/spapr_rng.h

diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index b94af6c..24d4717 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -17,3 +17,4 @@ CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
 CONFIG_MEM_HOTPLUG=y
+CONFIG_SPAPR_RNG=y
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 4ab5564..4e0c1c0 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -3,8 +3,9 @@ obj-y += ppc.o ppc_booke.o fdt.o
 # IBM pSeries (sPAPR)
 obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
-obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
+obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
 obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
+obj-$(CONFIG_SPAPR_RNG) +=  spapr_rng.o
 # IBM PowerNV
 obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 98868d8..ba40c85 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -53,9 +53,10 @@
 #include "hw/ppc/fdt.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
+#include "spapr_rng.h"
+
 #include "hw/pci-host/spapr.h"
 #include "hw/pci/msi.h"
-
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "hw/virtio/virtio-scsi.h"
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;
-}
-
 static Property spapr_rng_properties[] = {
     DEFINE_PROP_BOOL("use-kvm", sPAPRRngState, use_kvm, false),
     DEFINE_PROP_LINK("rng", sPAPRRngState, backend, TYPE_RNG_BACKEND,
diff --git a/hw/ppc/spapr_rng.h b/hw/ppc/spapr_rng.h
new file mode 100644
index 0000000..1d5fdcb
--- /dev/null
+++ b/hw/ppc/spapr_rng.h
@@ -0,0 +1,48 @@
+/*
+ * QEMU sPAPR random number generator "device" for H_RANDOM hypercall
+ *
+ * Copyright 2015 Thomas Huth, Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef SPAPR_RNG_H
+#define SPAPR_RNG_H
+
+#define TYPE_SPAPR_RNG "spapr-rng"
+
+static inline 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;
+}
+
+#endif
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ad4d7cf..9460c46 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -743,10 +743,6 @@ void spapr_lmb_release(DeviceState *dev);
 void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
 int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
 
-#define TYPE_SPAPR_RNG "spapr-rng"
-
-int spapr_rng_populate_dt(void *fdt);
-
 #define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
 
 /*
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH] hw/ppc/spapr_rng: Introduce CONFIG_SPAPR_RNG switch for spapr_rng.c
Posted by David Gibson 5 years, 5 months ago
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.



> ---
>  default-configs/ppc64-softmmu.mak |  1 +
>  hw/ppc/Makefile.objs              |  3 ++-
>  hw/ppc/spapr.c                    |  3 ++-
>  hw/ppc/spapr_rng.c                | 24 +-------------------
>  hw/ppc/spapr_rng.h                | 48 +++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h            |  4 ----
>  6 files changed, 54 insertions(+), 29 deletions(-)
>  create mode 100644 hw/ppc/spapr_rng.h
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index b94af6c..24d4717 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -17,3 +17,4 @@ CONFIG_XICS=$(CONFIG_PSERIES)
>  CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
>  CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
>  CONFIG_MEM_HOTPLUG=y
> +CONFIG_SPAPR_RNG=y
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 4ab5564..4e0c1c0 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -3,8 +3,9 @@ obj-y += ppc.o ppc_booke.o fdt.o
>  # IBM pSeries (sPAPR)
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> -obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> +obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> +obj-$(CONFIG_SPAPR_RNG) +=  spapr_rng.o
>  # IBM PowerNV
>  obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 98868d8..ba40c85 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -53,9 +53,10 @@
>  #include "hw/ppc/fdt.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
> +#include "spapr_rng.h"
> +
>  #include "hw/pci-host/spapr.h"
>  #include "hw/pci/msi.h"
> -
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
>  #include "hw/virtio/virtio-scsi.h"
> 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).

>  static Property spapr_rng_properties[] = {
>      DEFINE_PROP_BOOL("use-kvm", sPAPRRngState, use_kvm, false),
>      DEFINE_PROP_LINK("rng", sPAPRRngState, backend, TYPE_RNG_BACKEND,
> diff --git a/hw/ppc/spapr_rng.h b/hw/ppc/spapr_rng.h
> new file mode 100644
> index 0000000..1d5fdcb
> --- /dev/null
> +++ b/hw/ppc/spapr_rng.h
> @@ -0,0 +1,48 @@
> +/*
> + * QEMU sPAPR random number generator "device" for H_RANDOM hypercall
> + *
> + * Copyright 2015 Thomas Huth, Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License,
> + * or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef SPAPR_RNG_H
> +#define SPAPR_RNG_H
> +
> +#define TYPE_SPAPR_RNG "spapr-rng"
> +
> +static inline 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;
> +}
> +
> +#endif
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ad4d7cf..9460c46 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -743,10 +743,6 @@ void spapr_lmb_release(DeviceState *dev);
>  void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
>  int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>  
> -#define TYPE_SPAPR_RNG "spapr-rng"
> -
> -int spapr_rng_populate_dt(void *fdt);
> -
>  #define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
>  
>  /*

-- 
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
Re: [Qemu-devel] [PATCH] hw/ppc/spapr_rng: Introduce CONFIG_SPAPR_RNG switch for spapr_rng.c
Posted by Thomas Huth 5 years, 5 months ago
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. 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.

 Thomas

Re: [Qemu-devel] [PATCH] hw/ppc/spapr_rng: Introduce CONFIG_SPAPR_RNG switch for spapr_rng.c
Posted by David Gibson 5 years, 5 months ago
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.

-- 
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
Re: [Qemu-devel] [PATCH] hw/ppc/spapr_rng: Introduce CONFIG_SPAPR_RNG switch for spapr_rng.c
Posted by Thomas Huth 5 years, 5 months ago
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