On Mon, Jun 11, 2018 at 02:16:49PM +0200, David Hildenbrand wrote: > This might look like a step backwards, but it is not. get_memory_region() > should not be called on uninititalized devices. In general, only > properties should be access, but no "derived" satte like the memory > region. > > 1. We need duplicate error checks if memdev is actually already set. > realize() performs these checks, no need to duplicate. > 2. This is bad practise as one can see when looking at the NVDIMM > implemetation. The call does not return sane data before the device > is realized. Although spapr does not use NVDIMM, conceptually it is > wrong. > > So let's just move this call to the right place. We can then cleanup > get_memory_region(). > > Signed-off-by: David Hildenbrand <da...@redhat.com>
Acked-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..a5f1bbd58a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3153,6 +3153,12 @@ static void spapr_memory_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> align = memory_region_get_alignment(mr);
> size = memory_region_size(mr);
>
> + if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> + error_setg(&local_err, "Hotplugged memory size must be a multiple of
> "
> + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> + goto out;
> + }
> +
> pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
> if (local_err) {
> goto out;
> @@ -3186,9 +3192,6 @@ static void spapr_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> {
> const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr;
> - uint64_t size;
> char *mem_dev;
>
> if (!smc->dr_lmb_enabled) {
> @@ -3196,18 +3199,6 @@ static void spapr_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> return;
> }
>
> - mr = ddc->get_memory_region(dimm, errp);
> - if (!mr) {
> - return;
> - }
> - size = memory_region_size(mr);
> -
> - if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> - error_setg(errp, "Hotplugged memory size must be a multiple of "
> - "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> - return;
> - }
> -
> mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
> NULL);
> if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
> error_setg(errp, "Memory backend has bad page size. "
--
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
