hw/i386/pc.c | 14 ++++++++++++-- hw/mem/nvdimm.c | 2 +- hw/mem/pc-dimm.c | 14 +++++++++++--- hw/ppc/spapr.c | 42 ++++++++++++++++++++++++++++++------------ include/hw/mem/pc-dimm.h | 2 +- 5 files changed, 55 insertions(+), 19 deletions(-)
QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
machine without specifying its 'memdev' property. This happens because
pc_dimm_get_memory_region() does not check whether the 'memdev' property
has properly been set by the user. Looking closer at this function, it's
also obvious that it is using &error_abort to call another function - and
this is bad in a function that is used in the hot-plugging calling chain
since this can also cause QEMU to exit unexpectedly.
So let's fix these issues in a proper way now: Add a "Error **errp"
parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
property has not been set by the user, and which we can use instead of
the &error_abort, and change the callers of get_memory_region() to make
use of this "errp" parameter for proper error checking.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/i386/pc.c | 14 ++++++++++++--
hw/mem/nvdimm.c | 2 +-
hw/mem/pc-dimm.c | 14 +++++++++++---
hw/ppc/spapr.c | 42 ++++++++++++++++++++++++++++++------------
include/hw/mem/pc-dimm.h | 2 +-
5 files changed, 55 insertions(+), 19 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5943539..2108104 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
+ MemoryRegion *mr;
uint64_t align = TARGET_PAGE_SIZE;
bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+ mr = ddc->get_memory_region(dimm, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
align = memory_region_get_alignment(mr);
}
@@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
PCMachineState *pcms = PC_MACHINE(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
+ MemoryRegion *mr;
HotplugHandlerClass *hhc;
Error *local_err = NULL;
+ mr = ddc->get_memory_region(dimm, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index db896b0..952fce5 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
NULL, NULL);
}
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
{
NVDIMMDevice *nvdimm = NVDIMM(dimm);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index ea67b46..bdf6649 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
PCDIMMDevice *dimm = PC_DIMM(obj);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
- mr = ddc->get_memory_region(dimm);
+ mr = ddc->get_memory_region(dimm, errp);
+ if (!mr) {
+ return;
+ }
value = memory_region_size(mr);
visit_type_uint64(v, name, &value, errp);
@@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
host_memory_backend_set_mapped(dimm->hostmem, false);
}
-static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
+static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
{
- return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+ if (!dimm->hostmem) {
+ error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+ return NULL;
+ }
+
+ return host_memory_backend_get_memory(dimm->hostmem, errp);
}
static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a1972..cec441c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
- uint64_t align = memory_region_get_alignment(mr);
- uint64_t size = memory_region_size(mr);
- uint64_t addr;
+ MemoryRegion *mr;
+ uint64_t align, size, addr;
+
+ mr = ddc->get_memory_region(dimm, &local_err);
+ if (local_err) {
+ goto out;
+ }
+ align = memory_region_get_alignment(mr);
+ size = memory_region_size(mr);
pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
if (local_err) {
@@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
{
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
- uint64_t size = memory_region_size(mr);
+ MemoryRegion *mr;
+ uint64_t size;
char *mem_dev;
+ 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);
@@ -2882,7 +2893,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
{
sPAPRDRConnector *drc;
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
+ MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
uint64_t size = memory_region_size(mr);
uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
uint32_t avail_lmbs = 0;
@@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
+ MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
/* This information will get lost if a migration occurs
@@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
Error *local_err = NULL;
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
- uint64_t size = memory_region_size(mr);
- uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
- uint64_t addr_start, addr;
+ MemoryRegion *mr;
+ uint32_t nr_lmbs;
+ uint64_t size, addr_start, addr;
int i;
sPAPRDRConnector *drc;
+
+ mr = ddc->get_memory_region(dimm, &local_err);
+ if (local_err) {
+ goto out;
+ }
+ size = memory_region_size(mr);
+ nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
+
addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
&local_err);
if (local_err) {
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 1e483f2..6f8c3eb 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
/* public */
void (*realize)(PCDIMMDevice *dimm, Error **errp);
- MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
+ MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
} PCDIMMDeviceClass;
--
1.8.3.1
Hello Thomas,
>
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. This happens because
> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> has properly been set by the user. Looking closer at this function, it's
> also obvious that it is using &error_abort to call another function - and
> this is bad in a function that is used in the hot-plugging calling chain
> since this can also cause QEMU to exit unexpectedly.
In place of checking if 'memdev' is already allocated/present while fetching
'get_functions' every place. If 'memdev' is required for memory hotplug operation
can we just put a check at some common place and don't start or instantiate if
'memdev' is not present.
I can see:
'pc_dimm_realize' function checks for : in my case prints warning
and avoid Qemu to start.
...
...
if (!dimm->hostmem) {
error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
return;
}
...
Thanks,
Pankaj
>
> So let's fix these issues in a proper way now: Add a "Error **errp"
> parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> property has not been set by the user, and which we can use instead of
> the &error_abort, and change the callers of get_memory_region() to make
> use of this "errp" parameter for proper error checking.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/i386/pc.c | 14 ++++++++++++--
> hw/mem/nvdimm.c | 2 +-
> hw/mem/pc-dimm.c | 14 +++++++++++---
> hw/ppc/spapr.c | 42 ++++++++++++++++++++++++++++++------------
> include/hw/mem/pc-dimm.h | 2 +-
> 5 files changed, 55 insertions(+), 19 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5943539..2108104 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr;
> uint64_t align = TARGET_PAGE_SIZE;
> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> +
> if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> align = memory_region_get_alignment(mr);
> }
> @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler
> *hotplug_dev,
> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr;
> HotplugHandlerClass *hhc;
> Error *local_err = NULL;
>
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index db896b0..952fce5 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
> NULL, NULL);
> }
>
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error
> **errp)
> {
> NVDIMMDevice *nvdimm = NVDIMM(dimm);
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b46..bdf6649 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
> const char *name,
> PCDIMMDevice *dimm = PC_DIMM(obj);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>
> - mr = ddc->get_memory_region(dimm);
> + mr = ddc->get_memory_region(dimm, errp);
> + if (!mr) {
> + return;
> + }
> value = memory_region_size(mr);
>
> visit_type_uint64(v, name, &value, errp);
> @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error
> **errp)
> host_memory_backend_set_mapped(dimm->hostmem, false);
> }
>
> -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error
> **errp)
> {
> - return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> + if (!dimm->hostmem) {
> + error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> + return NULL;
> + }
> +
> + return host_memory_backend_get_memory(dimm->hostmem, errp);
> }
>
> static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a1972..cec441c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> - uint64_t align = memory_region_get_alignment(mr);
> - uint64_t size = memory_region_size(mr);
> - uint64_t addr;
> + MemoryRegion *mr;
> + uint64_t align, size, addr;
> +
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> + align = memory_region_get_alignment(mr);
> + size = memory_region_size(mr);
>
> pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
> if (local_err) {
> @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> {
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> - uint64_t size = memory_region_size(mr);
> + MemoryRegion *mr;
> + uint64_t size;
> char *mem_dev;
>
> + 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);
> @@ -2882,7 +2893,7 @@ static sPAPRDIMMState
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> {
> sPAPRDRConnector *drc;
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> uint64_t size = memory_region_size(mr);
> uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> uint32_t avail_lmbs = 0;
> @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr,
> PC_DIMM(dev));
>
> /* This information will get lost if a migration occurs
> @@ -2945,12 +2956,19 @@ static void
> spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> Error *local_err = NULL;
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> - uint64_t size = memory_region_size(mr);
> - uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> - uint64_t addr_start, addr;
> + MemoryRegion *mr;
> + uint32_t nr_lmbs;
> + uint64_t size, addr_start, addr;
> int i;
> sPAPRDRConnector *drc;
> +
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> + size = memory_region_size(mr);
> + nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> +
> addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> if (local_err) {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2..6f8c3eb 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
>
> /* public */
> void (*realize)(PCDIMMDevice *dimm, Error **errp);
> - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
> + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> } PCDIMMDeviceClass;
>
> --
> 1.8.3.1
>
>
>
Hi,
On 21.08.2017 09:36, Pankaj Gupta wrote:
>
> Hello Thomas,
>>
>> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
>> machine without specifying its 'memdev' property. This happens because
>> pc_dimm_get_memory_region() does not check whether the 'memdev' property
>> has properly been set by the user. Looking closer at this function, it's
>> also obvious that it is using &error_abort to call another function - and
>> this is bad in a function that is used in the hot-plugging calling chain
>> since this can also cause QEMU to exit unexpectedly.
>
> In place of checking if 'memdev' is already allocated/present while fetching
> 'get_functions' every place. If 'memdev' is required for memory hotplug operation
> can we just put a check at some common place and don't start or instantiate if
> 'memdev' is not present.
>
> I can see:
>
> 'pc_dimm_realize' function checks for : in my case prints warning
> and avoid Qemu to start.
> ...
> ...
> if (!dimm->hostmem) {
> error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
> return;
> }
> ...
That check unfortunately does not apply here: The crash happens in the
pre_plug() handler of the spapr machine, and this is called *before* the
realize function of the pc-dimm is called!
But even if we could find another common place where we could check
dimm->hostmem first, you still have the problem that
host_memory_backend_get_memory() could return an error and thus
pc_dimm_get_memory_region() still needs a way to fail gracefully during
the hotplug (or also hot-unplug) operation. So I think my patch is the
right way to go here.
Thomas
> >
> > Hello Thomas,
> >>
> >> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> >> machine without specifying its 'memdev' property. This happens because
> >> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> >> has properly been set by the user. Looking closer at this function, it's
> >> also obvious that it is using &error_abort to call another function - and
> >> this is bad in a function that is used in the hot-plugging calling chain
> >> since this can also cause QEMU to exit unexpectedly.
> >
> > In place of checking if 'memdev' is already allocated/present while
> > fetching
> > 'get_functions' every place. If 'memdev' is required for memory hotplug
> > operation
> > can we just put a check at some common place and don't start or instantiate
> > if
> > 'memdev' is not present.
> >
> > I can see:
> >
> > 'pc_dimm_realize' function checks for : in my case prints warning
> > and avoid Qemu to start.
> > ...
> > ...
> > if (!dimm->hostmem) {
> > error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
> > return;
> > }
> > ...
>
> That check unfortunately does not apply here: The crash happens in the
> pre_plug() handler of the spapr machine, and this is called *before* the
> realize function of the pc-dimm is called!
o.k I guessed so.
>
> But even if we could find another common place where we could check
> dimm->hostmem first, you still have the problem that
> host_memory_backend_get_memory() could return an error and thus
> pc_dimm_get_memory_region() still needs a way to fail gracefully during
> the hotplug (or also hot-unplug) operation. So I think my patch is the
> right way to go here.
Right. Thanks for the explanation.
Pankaj
>
> Thomas
>
On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote:
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. This happens because
> pc_dimm_get_memory_region() does not check whether the 'memdev' property
> has properly been set by the user. Looking closer at this function, it's
> also obvious that it is using &error_abort to call another function - and
> this is bad in a function that is used in the hot-plugging calling chain
> since this can also cause QEMU to exit unexpectedly.
>
> So let's fix these issues in a proper way now: Add a "Error **errp"
> parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> property has not been set by the user, and which we can use instead of
> the &error_abort, and change the callers of get_memory_region() to make
> use of this "errp" parameter for proper error checking.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
ppc portions
Acked-by: David Gibson <david@gibson.dropbear.id.au>
and the rest
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
I'm happy to queue this or have someone else queue it.
> ---
> hw/i386/pc.c | 14 ++++++++++++--
> hw/mem/nvdimm.c | 2 +-
> hw/mem/pc-dimm.c | 14 +++++++++++---
> hw/ppc/spapr.c | 42 ++++++++++++++++++++++++++++++------------
> include/hw/mem/pc-dimm.h | 2 +-
> 5 files changed, 55 insertions(+), 19 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5943539..2108104 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr;
> uint64_t align = TARGET_PAGE_SIZE;
> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> +
> if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> align = memory_region_get_alignment(mr);
> }
> @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr;
> HotplugHandlerClass *hhc;
> Error *local_err = NULL;
>
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index db896b0..952fce5 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
> NULL, NULL);
> }
>
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> {
> NVDIMMDevice *nvdimm = NVDIMM(dimm);
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b46..bdf6649 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> PCDIMMDevice *dimm = PC_DIMM(obj);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>
> - mr = ddc->get_memory_region(dimm);
> + mr = ddc->get_memory_region(dimm, errp);
> + if (!mr) {
> + return;
> + }
> value = memory_region_size(mr);
>
> visit_type_uint64(v, name, &value, errp);
> @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> host_memory_backend_set_mapped(dimm->hostmem, false);
> }
>
> -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> {
> - return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> + if (!dimm->hostmem) {
> + error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> + return NULL;
> + }
> +
> + return host_memory_backend_get_memory(dimm->hostmem, errp);
> }
>
> static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a1972..cec441c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> - uint64_t align = memory_region_get_alignment(mr);
> - uint64_t size = memory_region_size(mr);
> - uint64_t addr;
> + MemoryRegion *mr;
> + uint64_t align, size, addr;
> +
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> + align = memory_region_get_alignment(mr);
> + size = memory_region_size(mr);
>
> pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
> if (local_err) {
> @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> - uint64_t size = memory_region_size(mr);
> + MemoryRegion *mr;
> + uint64_t size;
> char *mem_dev;
>
> + 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);
> @@ -2882,7 +2893,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> {
> sPAPRDRConnector *drc;
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> uint64_t size = memory_region_size(mr);
> uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> uint32_t avail_lmbs = 0;
> @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
>
> /* This information will get lost if a migration occurs
> @@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> Error *local_err = NULL;
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> - uint64_t size = memory_region_size(mr);
> - uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> - uint64_t addr_start, addr;
> + MemoryRegion *mr;
> + uint32_t nr_lmbs;
> + uint64_t size, addr_start, addr;
> int i;
> sPAPRDRConnector *drc;
> +
> + mr = ddc->get_memory_region(dimm, &local_err);
> + if (local_err) {
> + goto out;
> + }
> + size = memory_region_size(mr);
> + nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> +
> addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> if (local_err) {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2..6f8c3eb 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
>
> /* public */
> void (*realize)(PCDIMMDevice *dimm, Error **errp);
> - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
> + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> } PCDIMMDeviceClass;
>
--
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
On Mon, 21 Aug 2017 17:55:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote:
> > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> > machine without specifying its 'memdev' property. This happens because
> > pc_dimm_get_memory_region() does not check whether the 'memdev' property
> > has properly been set by the user. Looking closer at this function, it's
> > also obvious that it is using &error_abort to call another function - and
> > this is bad in a function that is used in the hot-plugging calling chain
> > since this can also cause QEMU to exit unexpectedly.
> >
> > So let's fix these issues in a proper way now: Add a "Error **errp"
> > parameter to pc_dimm_get_memory_region() which we use in case the 'memdev'
> > property has not been set by the user, and which we can use instead of
> > the &error_abort, and change the callers of get_memory_region() to make
> > use of this "errp" parameter for proper error checking.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> ppc portions
>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
>
> and the rest
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> I'm happy to queue this or have someone else queue it.
Pls, go ahead and queue it via your tree
>
> > ---
> > hw/i386/pc.c | 14 ++++++++++++--
> > hw/mem/nvdimm.c | 2 +-
> > hw/mem/pc-dimm.c | 14 +++++++++++---
> > hw/ppc/spapr.c | 42 ++++++++++++++++++++++++++++++------------
> > include/hw/mem/pc-dimm.h | 2 +-
> > 5 files changed, 55 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 5943539..2108104 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > PCDIMMDevice *dimm = PC_DIMM(dev);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > + MemoryRegion *mr;
> > uint64_t align = TARGET_PAGE_SIZE;
> > bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >
> > + mr = ddc->get_memory_region(dimm, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > +
> > if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> > align = memory_region_get_alignment(mr);
> > }
> > @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> > PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > PCDIMMDevice *dimm = PC_DIMM(dev);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > + MemoryRegion *mr;
> > HotplugHandlerClass *hhc;
> > Error *local_err = NULL;
> >
> > + mr = ddc->get_memory_region(dimm, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > +
> > hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> > hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> >
> > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> > index db896b0..952fce5 100644
> > --- a/hw/mem/nvdimm.c
> > +++ b/hw/mem/nvdimm.c
> > @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj)
> > NULL, NULL);
> > }
> >
> > -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> > +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> > {
> > NVDIMMDevice *nvdimm = NVDIMM(dimm);
> >
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index ea67b46..bdf6649 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> > PCDIMMDevice *dimm = PC_DIMM(obj);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
> >
> > - mr = ddc->get_memory_region(dimm);
> > + mr = ddc->get_memory_region(dimm, errp);
> > + if (!mr) {
> > + return;
> > + }
> > value = memory_region_size(mr);
> >
> > visit_type_uint64(v, name, &value, errp);
> > @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> > host_memory_backend_set_mapped(dimm->hostmem, false);
> > }
> >
> > -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> > +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> > {
> > - return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> > + if (!dimm->hostmem) {
> > + error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> > + return NULL;
> > + }
> > +
> > + return host_memory_backend_get_memory(dimm->hostmem, errp);
> > }
> >
> > static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f7a1972..cec441c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> > PCDIMMDevice *dimm = PC_DIMM(dev);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > - uint64_t align = memory_region_get_alignment(mr);
> > - uint64_t size = memory_region_size(mr);
> > - uint64_t addr;
> > + MemoryRegion *mr;
> > + uint64_t align, size, addr;
> > +
> > + mr = ddc->get_memory_region(dimm, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > + align = memory_region_get_alignment(mr);
> > + size = memory_region_size(mr);
> >
> > pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
> > if (local_err) {
> > @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > {
> > PCDIMMDevice *dimm = PC_DIMM(dev);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > - uint64_t size = memory_region_size(mr);
> > + MemoryRegion *mr;
> > + uint64_t size;
> > char *mem_dev;
> >
> > + 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);
> > @@ -2882,7 +2893,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> > {
> > sPAPRDRConnector *drc;
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> > uint64_t size = memory_region_size(mr);
> > uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > uint32_t avail_lmbs = 0;
> > @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev)
> > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> > PCDIMMDevice *dimm = PC_DIMM(dev);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> > sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
> >
> > /* This information will get lost if a migration occurs
> > @@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> > Error *local_err = NULL;
> > PCDIMMDevice *dimm = PC_DIMM(dev);
> > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > - uint64_t size = memory_region_size(mr);
> > - uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > - uint64_t addr_start, addr;
> > + MemoryRegion *mr;
> > + uint32_t nr_lmbs;
> > + uint64_t size, addr_start, addr;
> > int i;
> > sPAPRDRConnector *drc;
> > +
> > + mr = ddc->get_memory_region(dimm, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > + size = memory_region_size(mr);
> > + nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > +
> > addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> > &local_err);
> > if (local_err) {
> > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > index 1e483f2..6f8c3eb 100644
> > --- a/include/hw/mem/pc-dimm.h
> > +++ b/include/hw/mem/pc-dimm.h
> > @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass {
> >
> > /* public */
> > void (*realize)(PCDIMMDevice *dimm, Error **errp);
> > - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
> > + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> > MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> > } PCDIMMDeviceClass;
> >
>
On Mon, Aug 21, 2017 at 10:45:02AM +0200, Igor Mammedov wrote: > On Mon, 21 Aug 2017 17:55:20 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote: > > > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries > > > machine without specifying its 'memdev' property. This happens because > > > pc_dimm_get_memory_region() does not check whether the 'memdev' property > > > has properly been set by the user. Looking closer at this function, it's > > > also obvious that it is using &error_abort to call another function - and > > > this is bad in a function that is used in the hot-plugging calling chain > > > since this can also cause QEMU to exit unexpectedly. > > > > > > So let's fix these issues in a proper way now: Add a "Error **errp" > > > parameter to pc_dimm_get_memory_region() which we use in case the 'memdev' > > > property has not been set by the user, and which we can use instead of > > > the &error_abort, and change the callers of get_memory_region() to make > > > use of this "errp" parameter for proper error checking. > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > ppc portions > > > > Acked-by: David Gibson <david@gibson.dropbear.id.au> > > > > and the rest > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > I'm happy to queue this or have someone else queue it. > Pls, go ahead and queue it via your tree Done. -- 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
© 2016 - 2024 Red Hat, Inc.