Haren Myneni <ha...@linux.ibm.com> writes:
> On Thu, 2021-06-03 at 14:38 +1000, Nicholas Piggin wrote:
>> Excerpts from Haren Myneni's message of May 21, 2021 7:33 pm:
>> > Same vas_window struct is used on powerNV and pseries. So this
>> > patch
>> > changes in struct vas_window to support both platforms and also the
>> > corresponding modifications in powerNV vas code.
>> >
>> > On powerNV, vas_window is used for both TX and RX windows, whereas
>> > only for TX windows on powerVM. So some elements are specific to
>> > these platforms.
>> >
>> > Signed-off-by: Haren Myneni <ha...@linux.ibm.com>
>> > ---
>> > arch/powerpc/include/asm/vas.h | 50 +++++++-
>> > arch/powerpc/platforms/powernv/vas-debug.c | 12 +-
>> > arch/powerpc/platforms/powernv/vas-fault.c | 4 +-
>> > arch/powerpc/platforms/powernv/vas-trace.h | 6 +-
>> > arch/powerpc/platforms/powernv/vas-window.c | 129 +++++++++++-----
>> > ----
>> > arch/powerpc/platforms/powernv/vas.h | 38 +-----
>> > 6 files changed, 135 insertions(+), 104 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/vas.h
>> > b/arch/powerpc/include/asm/vas.h
>> > index 2c1040f399d9..49bfb5be896d 100644
>> > --- a/arch/powerpc/include/asm/vas.h
>> > +++ b/arch/powerpc/include/asm/vas.h
>> > @@ -10,8 +10,6 @@
>> > #include <asm/icswx.h>
>> > #include <uapi/asm/vas-api.h>
>> >
>> > -struct vas_window;
>> > -
>> > /*
>> > * Min and max FIFO sizes are based on Version 1.05 Section
>> > 3.1.4.25
>> > * (Local FIFO Size Register) of the VAS workbook.
>> > @@ -63,6 +61,54 @@ struct vas_user_win_ref {
>> > struct mm_struct *mm; /* Linux process mm_struct */
>> > };
>> >
>> > +/*
>> > + * In-kernel state a VAS window. One per window.
>> > + * powerVM: Used only for Tx windows.
>> > + * powerNV: Used for both Tx and Rx windows.
>> > + */
>> > +struct vas_window {
>> > + u32 winid;
>> > + u32 wcreds_max; /* Window credits */
>> > + enum vas_cop_type cop;
>> > + struct vas_user_win_ref task_ref;
>> > + char *dbgname;
>> > + struct dentry *dbgdir;
>> > + union {
>> > + /* powerNV specific data */
>> > + struct {
>> > + void *vinst; /* points to VAS instance
>> > */
>> > + bool tx_win; /* True if send window */
>> > + bool nx_win; /* True if NX window */
>> > + bool user_win; /* True if user space
>> > window */
>> > + void *hvwc_map; /* HV window context */
>> > + void *uwc_map; /* OS/User window context
>> > */
>> > +
>> > + /* Fields applicable only to send windows */
>> > + void *paste_kaddr;
>> > + char *paste_addr_name;
>> > + struct vas_window *rxwin;
>> > +
>> > + atomic_t num_txwins; /* Only for receive
>> > windows */
>> > + } pnv;
>> > + struct {
>> > + u64 win_addr; /* Physical paste address
>> > */
>> > + u8 win_type; /* QoS or Default window */
>> > + u8 status;
>> > + u32 complete_irq; /* Completion interrupt */
>> > + u32 fault_irq; /* Fault interrupt */
>> > + u64 domain[6]; /* Associativity domain Ids
>> > */
>> > + /* this window is allocated */
>> > + u64 util;
>> > +
>> > + /* List of windows opened which is used for LPM
>> > */
>> > + struct list_head win_list;
>> > + u64 flags;
>> > + char *name;
>> > + int fault_virq;
>> > + } lpar;
>> > + };
>> > +};
>>
>> The more typical way to do code like this is have the common bit as
>> its own struct, and then have the users embed it into their own structs.
>>
>>
>> struct vas_window {
>> u32 winid;
>> u32 wcreds_max; /* Window credits */
>> enum vas_cop_type cop;
>> struct vas_user_win_ref task_ref;
>> char *dbgname;
>> struct dentry *dbgdir;
>> };
>>
>> struct pnv_vas_window {
>> struct vas_window vas_window;
>>
>> void *vinst; /* points to VAS instance */
>> bool tx_win; /* True if send window */
>> bool nx_win; /* True if NX window */
>> bool user_win; /* True if user space window */
>> void *hvwc_map; /* HV window context */
>> void *uwc_map; /* OS/User window context */
>>
>> /* Fields applicable only to send windows */
>> void *paste_kaddr;
>> char *paste_addr_name;
>> struct vas_window *rxwin;
>>
>> atomic_t num_txwins; /* Only for receive windows */
>> };
>>
>> Which helps reusability / avoids churn (don't have to update the
>> same
>> structure to add new functionality), slightly helps naming and union
>> size mismatches, helps with type checking, etc.
>>
>> Maybe not a great benefit for your code as is which may not grow more
>> users, but unless there are some good reasons for the unions I would
>> really consider changing to this style.
>
> Defined platform specific data as union for the following reasons:
> - vas_window address is saved for each file descriptor
> (fp-private_data). If we define separate structs for PowerNV and
> PowerVM, 'struct vas_window' has to be the first element which can add
> confusion.
I don't follow.
I think what you're saying is you want to return a struct vas_window *
to the drive code, ie. you don't want the driver code to know if it's a
pnv window or a pseries one.
ie. you get a vas_window in open and stash it in fp->private_data:
static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg)
{
...
struct coproc_instance *cp_inst;
struct vas_window *txwin;
int rc;
cp_inst = fp->private_data;
...
txwin = cp_inst->coproc->vops->open_win(&uattr,
cp_inst->coproc->cop_type);
...
cp_inst->txwin = txwin;
return 0;
}
And then you want to pass it back to the backend (powernv/pseries) code
in eg. mmap:
static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
{
struct coproc_instance *cp_inst = fp->private_data;
struct vas_window *txwin;
...
txwin = cp_inst->txwin;
...
paste_addr = cp_inst->coproc->vops->paste_addr(txwin);
But that can work perfectly fine with Nick's suggestion. You just pass
the vas_window * in and out and the backend code is responsible for
using container_of() to get access to its backend-specific struct.
eg. for powernv it would be something like:
static u64 vas_user_win_paste_addr(struct vas_window *win)
{
struct pnv_vas_window *pnv_win;
u64 paste_addr;
pnv_win = container_of(win, struct pnv_vas_window, vas_window);
vas_win_paste_addr(pnv_win, &paste_addr, NULL);
return paste_addr;
}
Another advantage which I don't think Nick mentioned is that you can
have the powernv specific parts private to the powernv code, and
similarly for pseries.
cheers
