Re: [PATCH v4 05/42] hw/cxl/device: Implement the CAP array (8.2.8.1-2)

[Alex Bennée]

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> On Wed, 26 Jan 2022 18:17:12 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
>
>> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
>> 
>> > From: Ben Widawsky <ben.widawsky@intel.com>
>> >
>> > 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.
>> >
>> > Endianness and alignment are managed by softmmu memory core.
>> >
>> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> > ---
>> >  hw/cxl/cxl-device-utils.c   | 105 ++++++++++++++++++++++++++++++++++++
>> >  hw/cxl/meson.build          |   1 +
>> >  include/hw/cxl/cxl_device.h |  28 +++++++++-
>> >  3 files changed, 133 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
>> > new file mode 100644
>> > index 0000000000..cb1b0a8217
>> > --- /dev/null
>> > +++ b/hw/cxl/cxl-device-utils.c
>> > @@ -0,0 +1,105 @@
>> > +/*
>> > + * 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"
>> > +
>> > +/*
>> > + * Device registers have no restrictions per the spec, and so fall back 
>> > to the
>> > + * default memory mapped register rules in 8.2:
>> > + *   Software shall use CXL.io Memory Read and Write to access memory 
>> > mapped
>> > + *   register defined in this section. Unless otherwise specified, 
>> > software
>> > + *   shall restrict the accesses width based on the following:
>> > + *   • A 32 bit register shall be accessed as a 1 Byte, 2 Bytes or 4 Bytes
>> > + *     quantity.
>> > + *   • A 64 bit register shall be accessed as a 1 Byte, 2 Bytes, 4 Bytes 
>> > or 8
>> > + *     Bytes
>> > + *   • The address shall be a multiple of the access width, e.g. when
>> > + *     accessing a register as a 4 Byte quantity, the address shall be
>> > + *     multiple of 4.
>> > + *   • The accesses shall map to contiguous bytes.If these rules are not
>> > + *     followed, the behavior is undefined
>> > + */
>> > +
>> > +static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
>> > +{
>> > +    CXLDeviceState *cxl_dstate = opaque;
>> > +
>> > +    return cxl_dstate->caps_reg_state32[offset / 4];
>> > +}
>> > +
>> > +static uint64_t dev_reg_read(void *opaque, hwaddr offset, unsigned size)
>> > +{
>> > +    return 0;
>> > +}
>> > +
>> > +static const MemoryRegionOps dev_ops = {
>> > +    .read = dev_reg_read,
>> > +    .write = NULL, /* status register is read only */
>> > +    .endianness = DEVICE_LITTLE_ENDIAN,
>> > +    .valid = {
>> > +        .min_access_size = 1,
>> > +        .max_access_size = 8,
>> > +        .unaligned = false,
>> > +    },
>> > +    .impl = {
>> > +        .min_access_size = 1,
>> > +        .max_access_size = 8,
>> > +    },
>> > +};  
>> 
>> I think for >64 bit registers you need to use the read_with_attrs 
>
> I don't follow this comment.  Max access to registers is 64 bits.
> A few are documented as 128 bit or indeed larger in the spec, but the
> access is as if they were multiple 64 bit registers accesses.
> It's not permissible to do a single 128bit access for example.

No that was my brain fart - of course 8 bytes = 64 bit which is fine for
the current accesses functions (unless you want bus faults).

>
> The F4 errata clarified that - previously it was rather unclear what
> the restrictions on access to the larger registers were.
>
> I've updated a few comments on this to reflect the errata.
>
> Thanks,
>
> Jonathan


-- 
Alex Bennée