[PATCH] mem/cxl_type3: fix GPF DVSEC
The structure is for device dvsec not port dvsec. Change type to fix this issue. Signed-off-by: Tong Zhang --- hw/mem/cxl_type3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 3bf2869573..ada2108fac 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -49,7 +49,7 @@ static void build_dvsecs(CXLType3Dev *ct3d) .phase2_power = 0x33, /* 0x33 miliwatts */ }; cxl_component_create_dvsec(cxl_cstate, CXL2_TYPE3_DEVICE, - GPF_DEVICE_DVSEC_LENGTH, GPF_PORT_DVSEC, + GPF_DEVICE_DVSEC_LENGTH, GPF_DEVICE_DVSEC, GPF_DEVICE_DVSEC_REVID, dvsec); } -- 2.25.1
Re: [RESEND PATCH] hw/dma: fix crash caused by race condition
On Wed, Jun 29, 2022 at 12:29 AM David Hildenbrand wrote:
> On 06.05.22 18:31, Tong Zhang wrote:
> > assert(dbs->acb) is meant to check the return value of io_func per
> > documented in commit 6bee44ea34 ("dma: the passed io_func does not
> > return NULL"). However, there is a chance that after calling
> > aio_context_release(dbs->ctx); the dma_blk_cb function is called before
> > the assertion and dbs->acb is set to NULL again at line 121. Thus when
> > we run assert at line 181 it will fail.
> >
> > softmmu/dma-helpers.c:181: dma_blk_cb: Assertion `dbs->acb' failed.
> >
> > Reported-by: Francisco Londono
> > Signed-off-by: Tong Zhang
> > ---
> > softmmu/dma-helpers.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
> > index 7820fec54c..cb81017928 100644
> > --- a/softmmu/dma-helpers.c
> > +++ b/softmmu/dma-helpers.c
> > @@ -177,8 +177,8 @@ static void dma_blk_cb(void *opaque, int ret)
> > aio_context_acquire(dbs->ctx);
> > dbs->acb = dbs->io_func(dbs->offset, >iov,
> > dma_blk_cb, dbs, dbs->io_func_opaque);
> > -aio_context_release(dbs->ctx);
> > assert(dbs->acb);
> > +aio_context_release(dbs->ctx);
> > }
> >
> > static void dma_aio_cancel(BlockAIOCB *acb)
>
> Please don't resend patches if the previous submission came to the
> conclusion that it's unclear how this should help.
>
>
> https://lkml.kernel.org/r/cajsp0qw396ry_g8ls1mncdzcov5gamury+xv+s8zmcdq03o...@mail.gmail.com
>
>
> I *still* don't understand the interaction between the lock and the
> assertion and so far nobody was able to clarify.
>
> --
> Thanks,
>
> David / dhildenb
>
hello
This message is sent way before the discussion
>
Re: [RESEND PATCH] hw/dma: fix crash caused by race condition
Hi Stefan, On Wed, Jun 1, 2022 at 6:56 AM Stefan Hajnoczi wrote: > > > > This patch makes sense to me. Can you rephrase your concern? > > > > The locking is around dbs->io_func(). > > > > aio_context_acquire(dbs->ctx); > > dbs->acb = dbs->io_func() > > aio_context_release(dbs->ctx); > > > > > > So where exactly would the lock that's now still held stop someone from > > modifying dbs->acb = NULL at the beginning of the function, which seems > > to be not protected by that lock? > > > > Maybe I'm missing some locking magic due to the lock being a recursive lock. > > Tong Zhang: Can you share a backtrace of all threads when the > assertion failure occurs? > Sorry I couldn't get the trace now -- but I can tell that we have some internal code uses this dma related code and will grab dbs->ctx lock in another thread and could overwrite dbs->acb. >From my understanding, one of the reasons that the lock is required here is to protect dbs->acb, we could not reliably test io_func()'s return value after releasing the lock here. Since this code affects our internal code base and I did not reproduce on master branch, feel free to ignore it. - Tong > Stefan
Re: [RESEND PATCH] hw/dma: fix crash caused by race condition
Hi David,
On Mon, May 30, 2022 at 9:19 AM David Hildenbrand wrote:
>
> On 27.04.22 22:51, Tong Zhang wrote:
> > assert(dbs->acb) is meant to check the return value of io_func per
> > documented in commit 6bee44ea34 ("dma: the passed io_func does not
> > return NULL"). However, there is a chance that after calling
> > aio_context_release(dbs->ctx); the dma_blk_cb function is called before
> > the assertion and dbs->acb is set to NULL again at line 121. Thus when
> > we run assert at line 181 it will fail.
> >
> > softmmu/dma-helpers.c:181: dma_blk_cb: Assertion `dbs->acb' failed.
> >
> > Reported-by: Francisco Londono
> > Signed-off-by: Tong Zhang
> > ---
> > softmmu/dma-helpers.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
> > index 7820fec54c..cb81017928 100644
> > --- a/softmmu/dma-helpers.c
> > +++ b/softmmu/dma-helpers.c
> > @@ -177,8 +177,8 @@ static void dma_blk_cb(void *opaque, int ret)
> > aio_context_acquire(dbs->ctx);
> > dbs->acb = dbs->io_func(dbs->offset, >iov,
> > dma_blk_cb, dbs, dbs->io_func_opaque);
> > -aio_context_release(dbs->ctx);
> > assert(dbs->acb);
> > +aio_context_release(dbs->ctx);
> > }
> >
> > static void dma_aio_cancel(BlockAIOCB *acb)
>
> I'm fairly new to that code, but I wonder what prevents dma_blk_cb() to
> run after you reshuffled the code?
>
IMO if the assert is to test whether io_func returns a non-NULL value
shouldn't it be immediately after calling io_func.
Also... as suggested by commit 6bee44ea346aed24e12d525daf10542d695508db
> dma: the passed io_func does not return NULL
Thanks,
- Tong
> After all, acquire/release is only around the dbs->io_func() call, so I
> don't immediately see how it interacts with re-entrance?
>
> --
> Thanks,
>
> David / dhildenb
>
[RESEND PATCH] hw/dma: fix crash caused by race condition
assert(dbs->acb) is meant to check the return value of io_func per
documented in commit 6bee44ea34 ("dma: the passed io_func does not
return NULL"). However, there is a chance that after calling
aio_context_release(dbs->ctx); the dma_blk_cb function is called before
the assertion and dbs->acb is set to NULL again at line 121. Thus when
we run assert at line 181 it will fail.
softmmu/dma-helpers.c:181: dma_blk_cb: Assertion `dbs->acb' failed.
Reported-by: Francisco Londono
Signed-off-by: Tong Zhang
---
softmmu/dma-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
index 7820fec54c..cb81017928 100644
--- a/softmmu/dma-helpers.c
+++ b/softmmu/dma-helpers.c
@@ -177,8 +177,8 @@ static void dma_blk_cb(void *opaque, int ret)
aio_context_acquire(dbs->ctx);
dbs->acb = dbs->io_func(dbs->offset, >iov,
dma_blk_cb, dbs, dbs->io_func_opaque);
-aio_context_release(dbs->ctx);
assert(dbs->acb);
+aio_context_release(dbs->ctx);
}
static void dma_aio_cancel(BlockAIOCB *acb)
--
2.25.1
[RESEND PATCH] hw/dma: fix crash caused by race condition
assert(dbs->acb) is meant to check the return value of io_func per
documented in commit 6bee44ea34 ("dma: the passed io_func does not
return NULL"). However, there is a chance that after calling
aio_context_release(dbs->ctx); the dma_blk_cb function is called before
the assertion and dbs->acb is set to NULL again at line 121. Thus when
we run assert at line 181 it will fail.
softmmu/dma-helpers.c:181: dma_blk_cb: Assertion `dbs->acb' failed.
Reported-by: Francisco Londono
Signed-off-by: Tong Zhang
---
softmmu/dma-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
index 7820fec54c..cb81017928 100644
--- a/softmmu/dma-helpers.c
+++ b/softmmu/dma-helpers.c
@@ -177,8 +177,8 @@ static void dma_blk_cb(void *opaque, int ret)
aio_context_acquire(dbs->ctx);
dbs->acb = dbs->io_func(dbs->offset, >iov,
dma_blk_cb, dbs, dbs->io_func_opaque);
-aio_context_release(dbs->ctx);
assert(dbs->acb);
+aio_context_release(dbs->ctx);
}
static void dma_aio_cancel(BlockAIOCB *acb)
--
2.25.1
[PATCH] hw/dma: fix crash caused by race condition
assert(dbs->acb) is meant to check the return value of io_func per
documented in commit 6bee44ea34 ("dma: the passed io_func does not
return NULL"). However, there is a chance that after calling
aio_context_release(dbs->ctx); the dma_blk_cb function is called before
the assertion and dbs->acb is set to NULL again at line 121. Thus when
we run assert at line 181 it will fail.
softmmu/dma-helpers.c:181: dma_blk_cb: Assertion `dbs->acb' failed.
Reported-by: Francisco Londono
Signed-off-by: Tong Zhang
---
softmmu/dma-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
index 7820fec54c..cb81017928 100644
--- a/softmmu/dma-helpers.c
+++ b/softmmu/dma-helpers.c
@@ -177,8 +177,8 @@ static void dma_blk_cb(void *opaque, int ret)
aio_context_acquire(dbs->ctx);
dbs->acb = dbs->io_func(dbs->offset, >iov,
dma_blk_cb, dbs, dbs->io_func_opaque);
-aio_context_release(dbs->ctx);
assert(dbs->acb);
+aio_context_release(dbs->ctx);
}
static void dma_aio_cancel(BlockAIOCB *acb)
--
2.25.1
Re: [PATCH v9 33/45] cxl/cxl-host: Add memops for CFMWS region.
On 4/4/22 08:14, Jonathan Cameron wrote:
> From: Jonathan Cameron
>
>
> +static MemTxResult cxl_read_cfmws(void *opaque, hwaddr addr, uint64_t *data,
> + unsigned size, MemTxAttrs attrs)
> +{
> +CXLFixedWindow *fw = opaque;
> +PCIDevice *d;
> +
> +d = cxl_cfmws_find_device(fw, addr);
> +if (d == NULL) {
> +*data = 0;
I'm looking at this code and comparing it to CXL2.0 spec 8.2.5.12.2 CXL HDM
Decoder Global Control Register (Offset 04h) table. It seems that we should
check POSION_ON_ERR_EN bit, if this bit is set, we return poison, otherwise
should return all 1's data.
Also, from the spec, this bit is implementation specific and hard
wired(RO) to either 1 or 0,
but for type3 device looks like we are currently allowing it to be
overwritten in ct3d_reg_write()
function. We probably also need more sanitation in ct3d_reg_write. (Also
for HDM
range/interleaving settings.)
> +/* Reads to invalid address return poison */
> +return MEMTX_ERROR;
> +}
> +
> +return cxl_type3_read(d, addr + fw->base, data, size, attrs);
> +}
> +
- Tong
Re: [PATCH v9 18/45] hw/cxl/device: Implement MMIO HDM decoding (8.2.5.12)
> On Apr 4, 2022, at 8:14 AM, Jonathan Cameron via
> wrote:
>
> From: Ben Widawsky
>
> A device's volatile and persistent memory are known Host Defined Memory
> (HDM) regions. The mechanism by which the device is programmed to claim
> the addresses associated with those regions is through dedicated logic
> known as the HDM decoder. In order to allow the OS to properly program
> the HDMs, the HDM decoders must be modeled.
>
> There are two ways the HDM decoders can be implemented, the legacy
> mechanism is through the PCIe DVSEC programming from CXL 1.1 (8.1.3.8),
> and MMIO is found in 8.2.5.12 of the spec. For now, 8.1.3.8 is not
> implemented.
>
> Much of CXL device logic is implemented in cxl-utils. The HDM decoder
> however is implemented directly by the device implementation.
> Whilst the implementation currently does no validity checks on the
> encoder set up, future work will add sanity checking specific to
> the type of cxl component.
>
> Signed-off-by: Ben Widawsky
> Co-developed-by: Jonathan Cameron
> Signed-off-by: Jonathan Cameron
> Reviewed-by: Alex Bennée
> ---
> hw/mem/cxl_type3.c | 55 ++
> 1 file changed, 55 insertions(+)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 329a6ea2a9..5c93fbbd9b 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -50,6 +50,48 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>GPF_DEVICE_DVSEC_REVID, dvsec);
> }
>
> +static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
> +{
> +ComponentRegisters *cregs = >cxl_cstate.crb;
> +uint32_t *cache_mem = cregs->cache_mem_registers;
> +
> +assert(which == 0);
> +
> +/* TODO: Sanity checks that the decoder is possible */
> +ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> +ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +
> +ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +}
> +
> +static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned size)
> +{
> +CXLComponentState *cxl_cstate = opaque;
> +ComponentRegisters *cregs = _cstate->crb;
> +CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
> +uint32_t *cache_mem = cregs->cache_mem_registers;
> +bool should_commit = false;
> +int which_hdm = -1;
> +
> +assert(size == 4);
> +g_assert(offset <= CXL2_COMPONENT_CM_REGION_SIZE);
> +
Looks like this will allow offset == CXL2_COMPONENT_CM_REGION_SIZE to pass the
check, and cause a buffer overrun.
Shouldn’t this be g_assert(offset< CXL2_COMPONENT_CM_REGION_SIZE)?
We also need to make sure (offset + 4<= CXL2_COMPONENT_CM_REGION_SIZE).
Or maybe we just need offset +4 <= CXL2_COMPONENT_CM_REGION_SIZE here, if
offset < CXL2_COMPONENT_CM_REGION_SIZE is already checked somewhere else.
> +switch (offset) {
> +case A_CXL_HDM_DECODER0_CTRL:
> +should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +which_hdm = 0;
> +break;
> +default:
> +break;
> +}
> +
> +stl_le_p((uint8_t *)cache_mem + offset, value);
> +if (should_commit) {
> +hdm_decoder_commit(ct3d, which_hdm);
> +}
> +}
> +
> static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> {
> MemoryRegion *mr;
> @@ -93,6 +135,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> ct3d->cxl_cstate.pdev = pci_dev;
> build_dvsecs(ct3d);
>
> +regs->special_ops = g_new0(MemoryRegionOps, 1);
> +regs->special_ops->write = ct3d_reg_write;
> +
> cxl_component_register_block_init(OBJECT(pci_dev), cxl_cstate,
> TYPE_CXL_TYPE3);
>
> @@ -107,6 +152,15 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> >cxl_dstate.device_registers);
> }
>
> +static void ct3_exit(PCIDevice *pci_dev)
> +{
> +CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
> +CXLComponentState *cxl_cstate = >cxl_cstate;
> +ComponentRegisters *regs = _cstate->crb;
> +
> +g_free(regs->special_ops);
> +}
> +
> static void ct3d_reset(DeviceState *dev)
> {
> CXLType3Dev *ct3d = CXL_TYPE3(dev);
> @@ -128,6 +182,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
> PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
>
> pc->realize = ct3_realize;
> +pc->exit = ct3_exit;
> pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> pc->vendor_id = PCI_VENDOR_ID_INTEL;
> pc->device_id = 0xd93; /* LVF for now */
> --
> 2.32.0
>
>
>
[Qemu-devel] [PATCH] applesmc: add error mem region handling for MacOS 10.12.4
This patch fixes a problem in applesmc emulation which prevent recent MacOS(10.12.4) from booing. Zhang Tong (1): applesmc: add error mem region handling for MacOS 10.12.4 hw/misc/applesmc.c | 152 + 1 file changed, 130 insertions(+), 22 deletions(-) -- 2.10.2
[Qemu-devel] [PATCH] applesmc: add error mem region handling for MacOS 10.12.4
From: Zhang Tong <zt...@vt.edu>
Add error memory region handling for applesmc, which is required
to boot MacOS 10.12.4, because the updated SMC driver checks that
error memory region and returns it as error code to
Dont_steal_macos.kext,
and Dont_steal_macos.kext checks the returned error code for good.
Signed-off-by: Tong Zhang<zt...@vt.edu>
---
hw/misc/applesmc.c | 152 +
1 file changed, 130 insertions(+), 22 deletions(-)
diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 77fab5b..28bbea8 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -2,9 +2,11 @@
* Apple SMC controller
*
* Copyright (c) 2007 Alexander Graf
+ * 2017 Tong Zhang
*
* Authors: Alexander Graf <ag...@suse.de>
* Susanne Graf <s...@csgraf.de>
+ * Tong Zhang <zt...@vt.edu>
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -43,6 +45,8 @@
#define APPLESMC_DATA_PORT 0x0
/* command/status port used by Apple SMC */
#define APPLESMC_CMD_PORT 0x4
+#define APPLESMC_ERROR_CODE_PORT 0x1e
+#define APPLESMC_INT_PORT 0x1f
#define APPLESMC_NR_PORTS 32
#define APPLESMC_READ_CMD 0x10
@@ -74,6 +78,7 @@ struct AppleSMCState {
MemoryRegion io_data;
MemoryRegion io_cmd;
+MemoryRegion io_err;
uint32_t iobase;
uint8_t cmd;
uint8_t status;
@@ -83,6 +88,7 @@ struct AppleSMCState {
uint8_t data_pos;
uint8_t data[255];
uint8_t charactic[4];
+uint8_t status_error;
char *osk;
QLIST_HEAD(, AppleSMCData) data_def;
};
@@ -91,12 +97,28 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr
addr, uint64_t val,
unsigned size)
{
AppleSMCState *s = opaque;
-
-smc_debug("CMD Write B: %#x = %#x\n", addr, val);
+smc_debug("CMD Write B: %lx = %lx\n", addr, val);
switch(val) {
-case APPLESMC_READ_CMD:
-s->status = 0x0c;
-break;
+case APPLESMC_READ_CMD:
+s->status_error = 0x00;
+s->status = 0x0c;
+break;
+case APPLESMC_WRITE_CMD:
+s->status_error = 0x00;
+s->status = 0x0c;
+break;
+case APPLESMC_GET_KEY_BY_INDEX_CMD:
+s->status_error = 0x00;
+s->status = 0x0c;
+break;
+case APPLESMC_GET_KEY_TYPE_CMD:
+s->status_error = 0x00;
+s->status = 0x0c;
+break;
+default:
+{
+smc_debug("applesmc_io_cmd_write, unhandled cmd %lx\n", val);
+}
}
s->cmd = val;
s->read_pos = 0;
@@ -112,9 +134,12 @@ static void applesmc_fill_data(AppleSMCState *s)
smc_debug("Key matched (%s Len=%d Data=%s)\n", d->key,
d->len, d->data);
memcpy(s->data, d->data, d->len);
+s->status_error = 0x00;
return;
}
}
+/* not found */
+s->status_error = 0x84;
}
static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
@@ -122,22 +147,69 @@ static void applesmc_io_data_write(void *opaque, hwaddr
addr, uint64_t val,
{
AppleSMCState *s = opaque;
-smc_debug("DATA Write B: %#x = %#x\n", addr, val);
+smc_debug("DATA Write B: 0x%lx\n", val);
switch(s->cmd) {
-case APPLESMC_READ_CMD:
-if(s->read_pos < 4) {
-s->key[s->read_pos] = val;
-s->status = 0x04;
-} else if(s->read_pos == 4) {
-s->data_len = val;
-s->status = 0x05;
-s->data_pos = 0;
-smc_debug("Key = %c%c%c%c Len = %d\n", s->key[0],
- s->key[1], s->key[2], s->key[3], val);
-applesmc_fill_data(s);
+case APPLESMC_READ_CMD:
+if (s->read_pos < 4) {
+s->key[s->read_pos] = val;
+s->status = 0x04;
+} else if (s->read_pos == 4) {
+s->data_len = val;
+s->status = 0x05;
+s->data_pos = 0;
+smc_debug("DRCMD Key = %c%c%c%c Len = %ld\n", s->key[0],
+ s->key[1], s->key[2], s->key[3], val);
+applesmc_fill_data(s);
+}
+s->read_pos++;
+break;
+case APPLESMC_WRITE_CMD:
+if (s->read_pos < 4) {
+s->key[s->read_pos] = val;
+s->status = 0x04;
+} else if (s->read_pos == 4) {
+s->status = 0x05;
+s->data_pos = 0;
+s->data_len = val;
+} else if (s->data_pos < s->data_
