From: Ben Widawsky <ben.widawsky@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: "Thomas Huth" <thuth@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
"Vishal Verma" <vishal.l.verma@intel.com>,
"Chris Browy" <cbrowy@avery-design.com>,
"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
qemu-devel@nongnu.org, "Prashant V Agarwal" <agpr123@gmail.com>,
"Dan Williams" <dan.j.williams@intel.com>
Subject: Re: [RFC PATCH v2 05/32] hw/cxl/device: Implement the CAP array (8.2.8.1-2)
Date: Wed, 6 Jan 2021 09:09:24 -0800 [thread overview]
Message-ID: <20210106170924.l3vxy3khysrnlvt3@intel.com> (raw)
In-Reply-To: <20210106170641.0000557c@Huawei.com>
On 21-01-06 17:06:41, Jonathan Cameron wrote:
> On Wed, 6 Jan 2021 08:49:48 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > On 21-01-06 13:28:05, Jonathan Cameron wrote:
> > > On Tue, 5 Jan 2021 08:52:56 -0800
> > > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > > This implements all device MMIO up to the first capability .That
> > > > includes the CXL Device Capabilities Array Register, as well as all of
> > > > the CXL Device Capability Header Registers. The latter are filled in as
> > > > they are implemented in the following patches.
> > > >
> > > > v2: Break out register alignment checks (Jonathan)
> > > >
> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > Hi Ben,
> > >
> > > One buglet / inconsistency inline that I spotted whilst chasing that issue
> > > with size of reads.
> > >
> > > Will get to a full review after messing around ('testing') this a bit more ;)
> > >
> > > Jonathan
> > >
> > > > ---
> > > > hw/cxl/cxl-device-utils.c | 72 +++++++++++++++++++++++++++++++++++++++
> > > > hw/cxl/meson.build | 1 +
> > > > 2 files changed, 73 insertions(+)
> > > > create mode 100644 hw/cxl/cxl-device-utils.c
> > > >
> > > > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > > > new file mode 100644
> > > > index 0000000000..d1b1371e66
> > > > --- /dev/null
> > > > +++ b/hw/cxl/cxl-device-utils.c
> > > > @@ -0,0 +1,72 @@
> > > > +/*
> > > > + * CXL Utility library for devices
> > > > + *
> > > > + * Copyright(C) 2020 Intel Corporation.
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2. See the
> > > > + * COPYING file in the top-level directory.
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qemu/log.h"
> > > > +#include "hw/cxl/cxl.h"
> > > > +
> > > > +static int cxl_device_check_register_alignment(hwaddr offset, unsigned size)
> > > > +{
> > > > + if (unlikely(offset & (size - 1))) {
> > > > + return 1;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
> > > > +{
> > > > + CXLDeviceState *cxl_dstate = opaque;
> > > > +
> > > > + if (cxl_device_check_register_alignment(offset, size)) {
> > > > + qemu_log_mask(LOG_UNIMP, "Unaligned register read\n");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return ldn_le_p(cxl_dstate->caps_reg_state + offset, size);
> > > > +}
> > > > +
> > > > +static const MemoryRegionOps caps_ops = {
> > > > + .read = caps_reg_read,
> > > > + .write = NULL,
> > > > + .endianness = DEVICE_LITTLE_ENDIAN,
> > > > + .valid = {
> > > > + .min_access_size = 4,
> > > > + .max_access_size = 8,
> > > > + },
> > > > + .impl = {
> > > > + .min_access_size = 4,
> > > > + .max_access_size = 8,
> > > > + },
> > > > +};
> > > > +
> > > > +void cxl_device_register_block_init(Object *obj, CXLDeviceState *cxl_dstate)
> > > > +{
> > > > + /* This will be a BAR, so needs to be rounded up to pow2 for PCI spec */
> > > > + memory_region_init(
> > > > + &cxl_dstate->device_registers, obj, "device-registers",
> > > > + pow2ceil(CXL_MAILBOX_REGISTERS_LENGTH + CXL_MAILBOX_REGISTERS_OFFSET));
> > >
> > > I can see why you jumped directly to sizing this for the whole region, but the snag
> > > is that means I think you missed the fact that patch 8 adds a region after the end
> > > of the mailbox. Doesn't result in an actual bug because the ceil above takes
> > > you way past the space needed though (the memory device region is only 8 bytes long).
> > >
> > >
> >
> > Maybe I misunderstand, but this is the intended behavior.
> > cxl_dstate->device_registers is the MemoryRegion container for all the
> > subregions that are the actual device MMIO.
> >
> > +------------------+
> > ^ | |
> > | | unused |
> > | --------------------
> > | | memdev regs |
> > | --------------------
> > | | |
> > | | +--------------+ |
> > cxl_dstate-> | | | | |
> > device_registers | | | | |
> > | | |payload | |
> > | | |(2k currently)| |
> > | | | | |
> > | | | | |
> > | | +--------------+ |
> > | | mailbox regs |
> > | --------------------
> > | | device regs |
> > v --------------------
> > | caps regs |
> > BAR ------> +------------------+
> >
> >
> > Perhaps I should add this as a comment in the code?
>
> I agree with intent but above it is setting limit to the top of
> the mailbox regs, not the memdev regs which I'd expect to see.
>
oops :D - you're correct
> >
> > > > +
> > > > + memory_region_init_io(&cxl_dstate->caps, obj, &caps_ops, cxl_dstate,
> > > > + "cap-array", CXL_DEVICE_REGISTERS_OFFSET - 0);
> > > > +
> > > > + memory_region_add_subregion(&cxl_dstate->device_registers, 0,
> > > > + &cxl_dstate->caps);
> > > > +}
> > > > +
> > > > +void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
> > > > +{
> > > > + uint32_t *cap_hdrs = cxl_dstate->caps_reg_state32;
> > > > + const int cap_count = 0;
> > > > +
> > > > + /* CXL Device Capabilities Array Register */
> > > > + ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_ID, 0);
> > > > + ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_VERSION, 1);
> > > > + ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY2, CAP_COUNT, cap_count);
> > > > +}
> > > > diff --git a/hw/cxl/meson.build b/hw/cxl/meson.build
> > > > index 00c3876a0f..47154d6850 100644
> > > > --- a/hw/cxl/meson.build
> > > > +++ b/hw/cxl/meson.build
> > > > @@ -1,3 +1,4 @@
> > > > softmmu_ss.add(when: 'CONFIG_CXL', if_true: files(
> > > > 'cxl-component-utils.c',
> > > > + 'cxl-device-utils.c',
> > > > ))
> > >
>
next prev parent reply other threads:[~2021-01-06 17:13 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-05 16:52 [RFC PATCH v2 00/32] CXL 2.0 Support Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 01/32] Temp: Add the PCI_EXT_ID_DVSEC definition to the qemu pci_regs.h copy Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 02/32] hw/pci/cxl: Add a CXL component type (interface) Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 03/32] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5) Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 04/32] hw/cxl/device: Introduce a CXL device (8.2.8) Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 05/32] hw/cxl/device: Implement the CAP array (8.2.8.1-2) Ben Widawsky
2021-01-06 13:28 ` Jonathan Cameron
2021-01-06 16:49 ` Ben Widawsky
2021-01-06 17:06 ` Jonathan Cameron
2021-01-06 17:09 ` Ben Widawsky [this message]
2021-01-05 16:52 ` [RFC PATCH v2 06/32] hw/cxl/device: Add device status (8.2.8.3) Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 07/32] hw/cxl/device: Implement basic mailbox (8.2.8.4) Ben Widawsky
2021-01-06 13:21 ` Jonathan Cameron
2021-01-06 16:31 ` Ben Widawsky
2021-01-06 17:40 ` [Linuxarm] " Jonathan Cameron
2021-01-06 18:05 ` Ben Widawsky
2021-01-06 19:08 ` Ben Widawsky
2021-01-08 5:36 ` Ben Widawsky
2021-01-05 16:52 ` [RFC PATCH v2 08/32] hw/cxl/device: Add memory devices (8.2.8.5) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 09/32] hw/cxl/device: Add cheap EVENTS implementation (8.2.9.1) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 10/32] hw/cxl/device: Placeholder for firmware commands Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 11/32] hw/cxl/device: Timestamp implementation (8.2.9.3) Ben Widawsky
2021-01-05 17:12 ` Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 12/32] hw/cxl/device: Add log commands (8.2.9.4) + CEL Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 13/32] hw/pxb: Use a type for realizing expanders Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 14/32] hw/pci/cxl: Create a CXL bus type Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 15/32] hw/pxb: Allow creation of a CXL PXB (host bridge) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 16/32] qtest: allow DSDT acpi table changes Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 17/32] acpi/pci: Consolidate host bridge setup Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 18/32] tests/acpi: remove stale allowed tables Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 19/32] hw/pci: Plumb _UID through host bridges Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 20/32] hw/cxl/component: Implement host bridge MMIO (8.2.5, table 142) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 21/32] acpi/pxb/cxl: Reserve host bridge MMIO Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 22/32] hw/pxb/cxl: Add "windows" for host bridges Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 23/32] hw/cxl/rp: Add a root port Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5) Ben Widawsky
2021-01-27 21:03 ` Igor Mammedov
2021-01-27 21:11 ` Ben Widawsky
2021-01-27 21:21 ` Igor Mammedov
2021-01-27 21:30 ` Ben Widawsky
2021-01-27 21:26 ` Ben Widawsky
2021-01-28 10:25 ` Jonathan Cameron
2021-01-28 15:03 ` Ben Widawsky
2021-01-28 15:14 ` Ben Widawsky
2021-01-28 16:51 ` Ben Widawsky
2021-01-28 16:58 ` Ben Widawsky
2021-01-28 17:40 ` Jonathan Cameron
2021-01-05 16:53 ` [RFC PATCH v2 25/32] hw/cxl/device: Implement MMIO HDM decoding (8.2.5.12) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 26/32] acpi/cxl: Add _OSC implementation (9.14.2) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 27/32] tests/acpi: allow CEDT table addition Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 28/32] acpi/cxl: Create the CEDT (9.14.1) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 29/32] Temp: acpi/cxl: Add ACPI0017 (CEDT awareness) Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 30/32] tests/acpi: Add new CEDT files Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 31/32] WIP: i386/cxl: Initialize a host bridge Ben Widawsky
2021-01-05 16:53 ` [RFC PATCH v2 32/32] qtest/cxl: Add very basic sanity tests Ben Widawsky
2021-01-08 18:44 ` [RFC PATCH v2 00/32] CXL 2.0 Support Jonathan Cameron
2021-01-08 18:51 ` Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210106170924.l3vxy3khysrnlvt3@intel.com \
--to=ben.widawsky@intel.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=agpr123@gmail.com \
--cc=cbrowy@avery-design.com \
--cc=dan.j.williams@intel.com \
--cc=f4bug@amsat.org \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=thuth@redhat.com \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).