Re: [PATCH v4 06/16] powerpc/vas: Define and use common vas_window struct
On Fri, 2021-06-04 at 21:52 +1000, Michael Ellerman wrote:
> Haren Myneni 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
> > > > ---
> > > > 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
> > > > #include
> > > >
> > > > -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 */
Re: [PATCH v4 06/16] powerpc/vas: Define and use common vas_window struct
Haren Myneni 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
>> > ---
>> > 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
>> > #include
>> >
>> > -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
Re: [PATCH v4 06/16] powerpc/vas: Define and use common vas_window struct
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
> > ---
> > 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
> > #include
> >
> > -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:
-
Re: [PATCH v4 06/16] powerpc/vas: Define and use common vas_window struct
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
> ---
> 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
> #include
>
> -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.
Thanks,
Nick
> + struct {
> + u64 win_addr; /* Physical paste address */
> + u8 win_type;/* QoS or Default window */
> + u8 status;
> + u32 complete_irq; /* Completion interrupt */
> +
