| Message ID | 20180611121655.19616-6-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-dimm: next bunch of cleanups | expand |
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 <david@redhat.com> Acked-by: David Gibson <david@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. "
On Mon, 11 Jun 2018 14:16:49 +0200 David Hildenbrand <david@redhat.com> 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. it's not duplicate, if a machine doesn't access to memory region in preplug time (hence doesn't check), then device itself would check it, check won't be missed by accident. (yep it's more code but more robust at the same time, so I'd leave it as is) > 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(). So I have to say no to this particular patch. It is indeed a step backwards and it looks like workaround for broken nvdimm impl. Firstly, memdev property must be set for dimm device and a user accessing memory region first must check for error. More details below. [...] > @@ -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) { here 2 bugs are colliding and leading to invalid code path 'if(!mr)' check happens to work for pc-dimm as it returns NULL on error and error is reported to user. however in nvdimm case, nvdimm_get_memory_region() unconditionally returns pointer to not initialized memory alias without any checks 1st issue here is that spapr_memory_pre_plug() should check for error like spapr_memory_plug() does when calling the same function. 2nd, nvdimm should (re)initialize nvdimm_mr alias whenever hostmem/label_size properties are set (it's doable but could be tricky. however device model shouldn't push its issues up to the stack). There are other places in nvdimm that access uninitialized nvdimm_mr during properties setting (I suppose all this sites should be fixed as part of 2nd bugfix). CCing author & co of nvdimm_mr, so that they could fix issue > - 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. "
On 13.06.2018 13:01, Igor Mammedov wrote: > On Mon, 11 Jun 2018 14:16:49 +0200 > David Hildenbrand <david@redhat.com> 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. > it's not duplicate, if a machine doesn't access to memory region > in preplug time (hence doesn't check), then device itself would check it, > check won't be missed by accident. > (yep it's more code but more robust at the same time, so I'd leave it as is) Checking at two places for the same thing == duplicate checks > >> 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(). > So I have to say no to this particular patch. > It is indeed a step backwards and it looks like workaround for broken nvdimm impl. > > Firstly, memdev property must be set for dimm device and > a user accessing memory region first must check for error. > More details below. > You assume that any class function can be called at any time. And I don't think this is the way to go.
On Wed, 13 Jun 2018 13:05:53 +0200 David Hildenbrand <david@redhat.com> wrote: > On 13.06.2018 13:01, Igor Mammedov wrote: > > On Mon, 11 Jun 2018 14:16:49 +0200 > > David Hildenbrand <david@redhat.com> 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. > > it's not duplicate, if a machine doesn't access to memory region > > in preplug time (hence doesn't check), then device itself would check it, > > check won't be missed by accident. > > (yep it's more code but more robust at the same time, so I'd leave it as is) > > Checking at two places for the same thing == duplicate checks device models and there users are separate entities hence I consider checks are separate. If user code can be written without adding extra checks it's fine. But if device model doesn't have its own checks when and is used in by new user code without checks also, it's going to break. So it would be hard to convince me that consolidating error handling between in-depended layers is a good idea in general and particularly in this case. I'd just drop this error cleanups altogether so that they won't get in the way of actual changes you are aiming for (unless you have to do it). > >> 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(). > > So I have to say no to this particular patch. > > It is indeed a step backwards and it looks like workaround for broken nvdimm impl. > > > > Firstly, memdev property must be set for dimm device and > > a user accessing memory region first must check for error. > > More details below. > > > > You assume that any class function can be called at any time. And I > don't think this is the way to go. Not any time, in the case of get_memory_region() it should work at pre_plug() time as all the pieces for it are already there. So we should make it work correctly for NVDIMM instead of succumbing to it and running wild. we should stick to canonical sequence object_new -> set props -> realize I don't see any reason to violate it in this case other than laziness.
On 13.06.2018 15:57, Igor Mammedov wrote: > On Wed, 13 Jun 2018 13:05:53 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 13.06.2018 13:01, Igor Mammedov wrote: >>> On Mon, 11 Jun 2018 14:16:49 +0200 >>> David Hildenbrand <david@redhat.com> 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. >>> it's not duplicate, if a machine doesn't access to memory region >>> in preplug time (hence doesn't check), then device itself would check it, >>> check won't be missed by accident. >>> (yep it's more code but more robust at the same time, so I'd leave it as is) >> >> Checking at two places for the same thing == duplicate checks > device models and there users are separate entities hence I consider > checks are separate. If user code can be written without adding extra checks > it's fine. But if device model doesn't have its own checks when and is > used in by new user code without checks also, it's going to break. > > So it would be hard to convince me that consolidating error handling > between in-depended layers is a good idea in general and particularly > in this case. > > I'd just drop this error cleanups altogether so that they won't get > in the way of actual changes you are aiming for (unless you have to do it). > >>>> 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(). >>> So I have to say no to this particular patch. >>> It is indeed a step backwards and it looks like workaround for broken nvdimm impl. >>> >>> Firstly, memdev property must be set for dimm device and >>> a user accessing memory region first must check for error. >>> More details below. >>> >> >> You assume that any class function can be called at any time. And I >> don't think this is the way to go. > Not any time, in the case of get_memory_region() it should work at > pre_plug() time as all the pieces for it are already there. > So we should make it work correctly for NVDIMM instead of > succumbing to it and running wild. > we should stick to canonical sequence > object_new -> set props -> realize > I don't see any reason to violate it in this case other than laziness. > See my replay to patch nr. 7. In contrast to what you suggest, I propose converting all nvdimm properties to static properties (because that's what they actually are). The validation/initialization of them should happen at a central place, not at various places (realize(), property setters, or even get_memory_region()) what you suggest. I propose a separate function for this (prepare() on DeviceClass), where we can split of the applicable parts of realize() that just perform property checks + basic initialization (e.g. of derived properties like the memory region in case of NVDIMM).
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. "
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 <david@redhat.com> --- hw/ppc/spapr.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)