Verma, Vishal L wrote:
[..]
> > > +-m::
> > > +--memdevs::
> > > + Indicate that the non-option arguments for 'target(s)' refer to
> > > memdev
> > > + names.
> >
> > Are they names or filter parameters ala 'cxl list -m'? I.e. do you
> > foresee being able to do something like:
>
> Hm, in the current implementation they really are just names - I just
> use the remaining argv[] as the targets array, but..
>
> >
> > "cxl create-region -p $port -m all"
> >
> > ...to just select all the memdevs that are descendants of $port in the
> > future? More of a curiosity about future possibilities then a request
> > for change.
>
> This sounds like a useful thing to do - perhaps with the next bit of
> porcelain additions.
Sure.
>
> >
> > > +
> > > +-e::
> > > +--ep-decoders::
> > > + Indicate that the non-option arguments for 'target(s)' refer to
> > > endpoint
> > > + decoder names.
> >
> > I wonder if this should have a note about it being for test-only
> > purposes? Given the strict CXL decoder allocation rules I wonder if
> > anyone can use this in practice? There might be some synergy with 'cxl
> > reserve-dpa' where this option could be used to say "do not allocate new
> > decoders, and do not reserve more DPA, just try to use the DPA that was
> > already reserved in the following decoders".
> >
> > We might even delete this option for now unless I am missing the marquee
> > use case for it? Because unless someone knows what they are doing they
> > are almost always going to be wrong.
>
> Yeah I agree - it is definitely not straightforward to use, and I don't
> see a practical use case. I think I only had it because the ABI wanted
> decoders, and very early implementations had me explicitly asking for
> /everything/.
>
> I'm okay adding a note that this shouldn't normally be used, or
> removing it entirely.
Hey, if there's a choice, you can never go wrong with red-diff.
>
> >
> > > +
> > > +-s::
> > > +--size=::
> > > + Specify the total size for the new region. This is optional, and
> > > by
> > > + default, the maximum possible size will be used.
> >
> > How about add:
> >
> > "The maximum possible size is gated by both the contiguous free HPA
> > space remaining in the root decoder, and the available DPA space in the
> > component memdevs."
>
> Yep.
>
> >
> > > +
> > > +-t::
> > > +--type=::
> > > + Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> > > +
> > > +-U::
> > > +--uuid=::
> > > + Specify a UUID for the new region. This shouldn't usually need to
> > > be
> > > + specified, as one will be generated by default.
> > > +
> > > +-w::
> > > +--ways=::
> > > + The number of interleave ways for the new region's interleave.
> > > This
> > > + should be equal to the number of memdevs specified in --memdevs,
> > > if
> > > + --memdevs is being supplied. If --memdevs is not specified, an
> > > + appropriate number of memdevs will be chosen based on the number
> > > of
> > > + ways specified.
> >
> > The reverse is also true, right? That if -w is not specified then the
> > number of ways is determined by the number of targets specified, or by
> > other default target searches. I guess notes about those enhanced
> > default modes can wait until more 'create-region' porcelain arrives.
>
> Hm actually in the current state, /only/ the reverse is true, so this
> description was certainly a bit forward looking. I'll fix to say what
> it actually does today.
Cool.
>
> >
> > > +
> > > +-g::
> > > +--granularity=::
> > > + The interleave granularity for the new region. Must match the
> > > selected
> > > + root decoder's (if provided) granularity.
> >
> > This just has me thinking that the kernel needs to up-level its code
> > comments and changelogs on the "why" for this restriction to somewhere
> > this man page can reference.
> >
> > However second sentence should be:
> >
> > "If the root decoder is interleaved across more than one host-bridge
> > then this value must match that granularity. Otherwise, for
> > non-interleaved decode windows, any granularity can be
> > specified as long as all devices support that setting."
> >
> > As I type that it raises 2 questions:
> >
> > 1/ If someone does "cxl create-region -g 1024" with no other arguments,
> > will it fallback to a decoder that can support that setting if its first
> > choice can not?
>
> Well there's no automatic root decoder selection at all in this series,
> but for future porcelain, I'd imagine it should try to match exactly
> any args that were specified, and fail if /all/ of those can't be
> matched.
>
> e.g.:
> decoder1 - 2-way - IG:256, and
> decoder2 - 1-way - IG:1024
>
> and we get 'cxl create-region -g 1024', we should pick decoder 2,
> create a x1 interleave under it. Does that make sense?
Yup.
[..]
> > > + } else if (argc) {
> > > + p->ways = argc;
> >
> > This is where:
> >
> > cxl create-region -p $port -m all
> >
> > ...would not work, but maybe requiring explicit targets is ok. There's
> > so many potential moving pieces in a CXL topology maybe we do not want
> > to go there with this flexibility.
>
> Hm yeah - true. It was certainly convenient (ab)using argc and argv for
> this, but if we did add the flexibility of specifying a filter, or even
> multiple filters in the future, the targets array would need proper
> reconstruction anyway, and counting the objects in it would come
> alongwith it.
Agree.
