Re: [PATCH 3/4] cxl/type3: minimum MHD cci support
On Thu, 31 Aug 2023 12:59:24 -0400
Gregory Price wrote:
> On Mon, Aug 07, 2023 at 03:56:09PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Jul 2023 12:35:08 -0400
> > Gregory Price wrote:
> >
> > > Implement the MHD GET_INFO cci command and add a shared memory
> > > region to the type3 device to host the information.
> > >
> > > Add a helper program to initialize this shared memory region.
> > >
> > > Add a function pointer to type3 devices for future work that
> > > will allow an mhd device to provide a hook to validate whether
> > > a memory access is valid or not.
> > >
> > > For now, limit the number of LD's to the number of heads. Later,
> > > this limitation will need to be lifted for MH-MLDs.
> > >
> > > Intended use case:
> > >
> > > 1. Create the shared memory region
> > > 2. Format the shared memory region
> > > 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
> >
> > I'd definitely like some feedback from experienced QEMU folk on
> > how to model this sort of cross QEMU instance sharing.
> >
> > My instinct is a socket would be more flexible as it lets us
> > potentially emulate the machines on multiple hosts (assuming they
> > can see some shared backend storage).
> >
>
> I'd considered a socket, but after mulling it over, I realized the
> operations I was trying to implement amounted to an atomic compare
> and swap. I wanted to avoid locks and serialization if at all
> possible to avoid introducing that into the hot-path of memory
> accesses. Maybe there is a known pattern for this kind of
> multi-instance QEMU coherency stuff, but that seemed the simplest path.
I'd definitely like some input from others on this if possible as I've
no idea if this problem has really been tackled in the past.
>
> This obviously has the downside of limiting emulation of MHD system to
> a local machine, but the use of common files/memory to back CXL memory
> already does that (i think).
Files should be fine I think on a suitable sharing aware file system.
>
> ¯\_(ツ)_/¯ feedback welcome. I'll leave it for now unless there are
> strong opinions.
>
> > else not needed if we returned in previous leg.
> >
> > if (ct3d->is_mhd && ...
> >
> > > + (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0 {
> >
> > How does mhd_head equal that? Default is 0. I'm not sure there is a reason
> > to require it.
> >
>
> Good catch, the default should be ~(0). Updated in the field
> initialization section.
>
> > > +/* For now, limit the number of heads to the number of LD's (SLD) */
> > >
> >
> > Feels backwards. Number of heads never going to be bigger than numbre of
> > LDs. Other way around is possible of course.
> >
> > > +if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {
> >
> > mhd head needs to be out of range? Confused.
> >
>
> Woops, yes this should be mhd_head >= nr_heads. Wording is backwards and
> so is the code. fixed.
>
> > > +if (ct3d->mhd_state) {
> > > +shmdt(ct3d->mhd_state);
> > > +}
> >
> > Reverse order of realize - so I think this wants to be earlier.
> >
> > > }
>
> Just to be clear you *don't* want the reverse order, correct? If so
> i'll swap it.
I do want reverse order. Generally speaking it's much easier to
review precisely matching reverse order as then mostly the ordering is
fine in a way that it isn't in any other ordering.
>
> > >
> > > +if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> > > +if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
> > > +return MEMTX_ERROR;
> >
> > Brackets for inner block.
> > Arguably could just add the ct3d->is_mhd check in the place where
> > mhd_access_valid is set and hence only need to check that here.
> > Maybe that makes it slightly harder to follow though.
> >
> > Also could just unset is_mhd if mhd_access_valid not present..
> >
>
> I'm going to change the next patch to fully inherit CXLType3Dev into
> Niagara as you suggested. I will have to see what falls out when i do
> that, but my feeling was being more explicit when checking pointers is
> better. If you prefer to to drop the is_mhd check i'm ok with that
> simply because mhd_access_valid should always be NULL when is_mhd is
> false. Either way the result is the same.
Minimizing redundant checks is always good as long as a reviewer can
easily see they are redundant. I think that's true here.
Jonathan
>
> ~Gregory
Re: [PATCH 3/4] cxl/type3: minimum MHD cci support
On Mon, Aug 07, 2023 at 03:56:09PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Jul 2023 12:35:08 -0400
> Gregory Price wrote:
>
> > Implement the MHD GET_INFO cci command and add a shared memory
> > region to the type3 device to host the information.
> >
> > Add a helper program to initialize this shared memory region.
> >
> > Add a function pointer to type3 devices for future work that
> > will allow an mhd device to provide a hook to validate whether
> > a memory access is valid or not.
> >
> > For now, limit the number of LD's to the number of heads. Later,
> > this limitation will need to be lifted for MH-MLDs.
> >
> > Intended use case:
> >
> > 1. Create the shared memory region
> > 2. Format the shared memory region
> > 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
>
> I'd definitely like some feedback from experienced QEMU folk on
> how to model this sort of cross QEMU instance sharing.
>
> My instinct is a socket would be more flexible as it lets us
> potentially emulate the machines on multiple hosts (assuming they
> can see some shared backend storage).
>
I'd considered a socket, but after mulling it over, I realized the
operations I was trying to implement amounted to an atomic compare
and swap. I wanted to avoid locks and serialization if at all
possible to avoid introducing that into the hot-path of memory
accesses. Maybe there is a known pattern for this kind of
multi-instance QEMU coherency stuff, but that seemed the simplest path.
This obviously has the downside of limiting emulation of MHD system to
a local machine, but the use of common files/memory to back CXL memory
already does that (i think).
¯\_(ツ)_/¯ feedback welcome. I'll leave it for now unless there are
strong opinions.
> else not needed if we returned in previous leg.
>
> if (ct3d->is_mhd && ...
>
> > + (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0 {
>
> How does mhd_head equal that? Default is 0. I'm not sure there is a reason
> to require it.
>
Good catch, the default should be ~(0). Updated in the field
initialization section.
> > +/* For now, limit the number of heads to the number of LD's (SLD) */
>
> Feels backwards. Number of heads never going to be bigger than numbre of
> LDs. Other way around is possible of course.
>
> > +if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {
>
> mhd head needs to be out of range? Confused.
>
Woops, yes this should be mhd_head >= nr_heads. Wording is backwards and
so is the code. fixed.
> > +if (ct3d->mhd_state) {
> > +shmdt(ct3d->mhd_state);
> > +}
>
> Reverse order of realize - so I think this wants to be earlier.
>
> > }
Just to be clear you *don't* want the reverse order, correct? If so
i'll swap it.
> >
> > +if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> > +if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
> > +return MEMTX_ERROR;
>
> Brackets for inner block.
> Arguably could just add the ct3d->is_mhd check in the place where
> mhd_access_valid is set and hence only need to check that here.
> Maybe that makes it slightly harder to follow though.
>
> Also could just unset is_mhd if mhd_access_valid not present..
>
I'm going to change the next patch to fully inherit CXLType3Dev into
Niagara as you suggested. I will have to see what falls out when i do
that, but my feeling was being more explicit when checking pointers is
better. If you prefer to to drop the is_mhd check i'm ok with that
simply because mhd_access_valid should always be NULL when is_mhd is
false. Either way the result is the same.
~Gregory
Re: [PATCH 3/4] cxl/type3: minimum MHD cci support
On Fri, 21 Jul 2023 12:35:08 -0400
Gregory Price wrote:
> Implement the MHD GET_INFO cci command and add a shared memory
> region to the type3 device to host the information.
>
> Add a helper program to initialize this shared memory region.
>
> Add a function pointer to type3 devices for future work that
> will allow an mhd device to provide a hook to validate whether
> a memory access is valid or not.
>
> For now, limit the number of LD's to the number of heads. Later,
> this limitation will need to be lifted for MH-MLDs.
>
> Intended use case:
>
> 1. Create the shared memory region
> 2. Format the shared memory region
> 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
I'd definitely like some feedback from experienced QEMU folk on
how to model this sort of cross QEMU instance sharing.
My instinct is a socket would be more flexible as it lets us
potentially emulate the machines on multiple hosts (assuming they
can see some shared backend storage).
Anyhow, some superficial comments inline.
What you have here looks good if we think shared memory is the
way to do this! Some bits are good anyway of course :)
Jonathan
>
> shmid=`ipcmk -M 4096 | grep -o -E '[0-9]+' | head -1`
> cxl_mhd_init 4 $shmid
> qemu-system-x86_64 \
> -nographic \
> -accel kvm \
> -drive file=./mhd.qcow2,format=qcow2,index=0,media=disk,id=hd \
> -m 4G,slots=4,maxmem=8G \
> -smp 4 \
> -machine type=q35,cxl=on,hmat=on \
> -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 \
> -device cxl-rp,id=rp0,bus=cxl.0,chassis=0,port=0,slot=0 \
> -object memory-backend-file,id=mem0,mem-path=/tmp/mem0,size=4G,share=true \
> -device
> cxl-type3,bus=rp0,volatile-memdev=mem0,id=cxl-mem0,sn=6,is_mhd=true,mhd_head=0,mhd_shmid=$shmid
> \
> -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G
>
> Signed-off-by: Gregory Price
> ---
> hw/cxl/cxl-mailbox-utils.c | 53 +
> hw/mem/cxl_type3.c | 67 +
> include/hw/cxl/cxl_device.h | 14
> tools/cxl/cxl_mhd_init.c| 63 ++
> tools/cxl/meson.build | 3 ++
> tools/meson.build | 1 +
> 6 files changed, 201 insertions(+)
> create mode 100644 tools/cxl/cxl_mhd_init.c
> create mode 100644 tools/cxl/meson.build
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index cad0cd0adb..57b8da4376 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -84,6 +84,8 @@ enum {
> #define GET_PHYSICAL_PORT_STATE 0x1
> TUNNEL = 0x53,
> #define MANAGEMENT_COMMAND 0x0
> +MHD = 0x55,
> +#define GET_MHD_INFO 0x0
> };
>
> /* CCI Message Format CXL r3.0 Figure 7-19 */
> @@ -1155,6 +1157,56 @@ static CXLRetCode cmd_media_clear_poison(const struct
> cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static CXLRetCode cmd_mhd_get_info(const struct cxl_cmd *cmd,
Reference would be good (for some reason it took me quite a bit
of typing related search terms in to actually find it in
CXL r3.0 section 7.6.7.5.1: Get Multi-Headed Info (Opcode 5500h)
(I'm trying to standardize on that format - just need to fix
all the existing references!)
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> +CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +struct {
> +uint8_t start_ld;
> +uint8_t ldmap_len;
> +} QEMU_PACKED *input = (void *)payload_in;
> +
> +struct {
> +uint8_t nr_lds;
> +uint8_t nr_heads;
> +uint16_t resv1;
> +uint8_t start_ld;
> +uint8_t ldmap_len;
> +uint16_t resv2;
> +uint8_t ldmap[];
> +} QEMU_PACKED *output = (void *)payload_out;
> +
> +uint8_t start_ld = input->start_ld;
> +uint8_t ldmap_len = input->ldmap_len;
> +uint8_t i;
> +
> +if (!ct3d->is_mhd) {
> +return CXL_MBOX_UNSUPPORTED;
> +}
> +
> +if (start_ld >= ct3d->mhd_state->nr_lds) {
> +return CXL_MBOX_INVALID_INPUT;
> +}
> +
> +output->nr_lds = ct3d->mhd_state->nr_lds;
> +output->nr_heads = ct3d->mhd_state->nr_heads;
> +output->resv1 = 0;
> +output->start_ld = start_ld;
> +output->resv2 = 0;
> +
> +for (i = 0; i < ldmap_len && (start_ld + i) < output->nr_lds; i++) {
> +output->ldmap[i] = ct3d->mhd_state->ldmap[start_ld + i];
> +}
> +output->ldmap_len = i;
> +
> +*len_out = sizeof(*output) + output->ldmap_len;
> +return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1195,6 +1247,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> cmd_media_inject_poison, 8, 0 },
>
