Message-ID: <20080611185754.GA14442@sgi.com>
Date:	Wed, 11 Jun 2008 13:57:54 -0500
From:	Jack Steiner <steiner@....com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, tglx@...utronix.de,
	holt@....com, andrea@...ranet.com
Subject: Re: [patch 01/11] GRU Driver - hardware data structures

On Mon, Jun 09, 2008 at 03:52:17PM -0700, Andrew Morton wrote:
> On Mon, 09 Jun 2008 16:10:29 -0500
> steiner@....com wrote:
> 
> > This patch contains the definitions of the hardware GRU data structures that are used
> > by the driver to manage the GRU.
> > 
> 
> oh goody, more code to review.

Glad to oblige ....    :-)


> 
> >
> > ...
> >
> > +/* Convert resource counts to the number of AU */
> > +#define GRU_DS_BYTES_TO_AU(n)	(((n) + GRU_DSR_AU_BYTES - 1) / \
> > +				 GRU_DSR_AU_BYTES)
> > +#define GRU_CB_COUNT_TO_AU(n)	(((n) + GRU_CBR_AU_SIZE - 1) / 	\
> > +				 GRU_CBR_AU_SIZE)
> 
> These are open-coded ROUND_UP()s

Fixed.


> 
> > +/* UV limits */
> > +#define GRU_CHIPLETS_PER_HUB	2
> > +#define GRU_HUBS_PER_BLADE	1
> > +#define GRU_CHIPLETS_PER_BLADE	(GRU_HUBS_PER_BLADE * GRU_CHIPLETS_PER_HUB)
> > +
> > +/* User GRU Gseg offsets */
> > +#define GRU_CB_BASE		0
> > +#define GRU_CB_LIMIT		(GRU_CB_BASE + GRU_HANDLE_STRIDE * GRU_NUM_CBE)
> > +#define GRU_DS_BASE		0x20000
> > +#define GRU_DS_LIMIT		(GRU_DS_BASE + GRU_NUM_DSR_BYTES)
> > +
> > +/* General addressing macros. b=grubase, c=ctxnum, i=cbnum, cl=cacheline#  */
> > +#define GRU_GSEG(b, c)		((void *)((b) + GRU_GSEG0_BASE +	\
> > +		GRU_GSEG_STRIDE * (c)))
> > +#define GRU_GSEG_CB(b, c, i)	((void *)(GRU_GSEG((b), (c)) +		\
> > +		GRU_CB_BASE + GRU_HANDLE_STRIDE * (i)))
> > +#define GRU_GSEG_DS(b, c, cl)	((void *)(GRU_GSEG((b), (c)) +		\
> > +		GRU_DS_BASE + GRU_CACHE_LINE_BYTES * (cl)))
> > +#define GRU_TFM(b, c)		((struct gru_tlb_fault_map *)		\
> > +		((unsigned long)(b) + GRU_TFM_BASE + (c) * GRU_HANDLE_STRIDE))
> > +#define GRU_TGH(b, c)		((struct gru_tlb_global_handle *)	\
> > +		((unsigned long)(b) + GRU_TGH_BASE + (c) * GRU_HANDLE_STRIDE))
> > +#define GRU_CBE(b, n)		((struct gru_control_block_extended *)	\
> > +		((unsigned long)(b) + GRU_CBE_BASE + (n) * GRU_HANDLE_STRIDE))
> > +#define GRU_TFH(b, n)		((struct gru_tlb_fault_handle *)	\
> > +		((unsigned long)(b) + GRU_TFH_BASE + (n) * GRU_HANDLE_STRIDE))
> > +#define GRU_CCH(b, n)		((struct gru_context_configuration_handle *) \
> > +		((unsigned long)(b) + GRU_CCH_BASE + (n) * GRU_HANDLE_STRIDE))
> > +#define GRU_GSH(b)		((struct gru_global_status_handle *)	\
> > +		((unsigned long)(b) + GRU_GSH_BASE))
> 
> Is there any particular reason why these had to be implemented via macros?

Not particularily. I can switch them to inline functions. The added type checking will help.


> 
> > +/*
> > + * Test if an offset is a valid kernel handle address.
> > + * 	Ex:  TYPE_IS(CBE, chiplet_offset)
> > + */
> > +#define TYPE_IS(hn, h)		((h) >= GRU_##hn##_BASE && (h) < 	\
> > +	GRU_##hn##_BASE + GRU_NUM_##hn * GRU_HANDLE_STRIDE && 		\
> > +				 (((h) & (GRU_HANDLE_STRIDE - 1)) == 0))
> 
> That one will misbehave if passed an `h' whcih hasside-effects.  I
> guess that's hard to fix if you need to retain the pasting thing.

Hmmm. It turns out that this is used exclusively in the hardware simulator. I
will move this definition to the simulator so that it won't be part
of the kernel.


> 
> > +/*
> > + * Test a GRU physical address to determine the type of address range (does
> > + * NOT validate holes)
> > + */
> > +#define IS_MCS_PADDR(h)		(((h) & (GRU_SIZE - 1)) >= GRU_MCS_BASE)
> > +#define IS_CBR_PADDR(h)		(((h) & (GRU_SIZE - 1)) < 		\
> > +		GRU_MCS_BASE && (((h) & (GRU_GSEG_STRIDE - 1)) < GRU_DS_BASE))
> 
> has the same bug, but doesn't do pasting.

Will switch to inline functions.


> 
> > +#define IS_DSR_PADDR(h)		(((h) & (GRU_SIZE - 1)) < GRU_MCS_BASE && \
> > +		(((h) & (GRU_GSEG_STRIDE - 1)) >= GRU_DS_BASE))
> 
> ditto.

Ditto


> 
> > +/* Convert an arbitrary handle address to the beginning of the GRU segment */
> > +#ifndef __PLUGIN__
> > +#define GRUBASE(h)		((void *)((unsigned long)(h) & ~(GRU_SIZE - 1)))
> > +#else
> > +/* Emulator hack */
> > +extern void *gmu_grubase(void *h);
> > +#define GRUBASE(h)		gmu_grubase(h)
> > +#endif
> > +
> > +/* Convert a GRU physical address to the chiplet offset */
> > +#define GSEGPOFF(h) ((h) & (GRU_SIZE - 1))
> > +
> > +/* Convert a GSEG CB address to the relative CB number within the context */
> > +#define CBNUM(cb) ((((unsigned long)(cb) - GRU_CB_BASE) % GRU_GSEG_PAGESIZE) / \
> > +			GRU_HANDLE_STRIDE)
> > +
> > +/* Convert a TFH address to the relative TFH number within the GRU*/
> > +#define TFHNUM(tfh) ((((unsigned long)(tfh) - GRU_TFH_BASE) % GRU_SIZE) / \
> > +			GRU_HANDLE_STRIDE)
> > +
> > +/* Convert a CCH address to the relative context number within the GRU*/
> > +#define CCHNUM(cch) ((((unsigned long)(cch) - GRU_CCH_BASE) % GRU_SIZE) / \
> > +			GRU_HANDLE_STRIDE)
> > +
> > +/* Convert a CBE address to the relative context number within the GRU*/
> > +#define CBENUM(cbe) ((((unsigned long)(cbe) - GRU_CBE_BASE) % GRU_SIZE) / \
> > +			GRU_HANDLE_STRIDE)
> > +
> > +/* Convert a TFM address to the relative context number within the GRU*/
> > +#define TFMNUM(tfm) ((((unsigned long)(tfm) - GRU_TFM_BASE) % GRU_SIZE) / \
> > +			GRU_HANDLE_STRIDE)
> > +
> > +/* byte offset to a specific GRU chiplet. (p=pnode, c=chiplet (0 or 1)*/
> > +#define GRUCHIPOFFSET(p, c) (GRU_SIZE * ((p) * 2 + (c)))
> 
> etc.

Ditto


> 
> > +#ifndef BITS_TO_LONGS
> > +#define BITS_TO_LONGS(bits)     (((bits)+64-1)/64)
> > +#endif
> 
> BITS_TO_LONGS is defined in include/linux/bitops.h.  Is this here just
> for userspace inclusion?  If not, it can go.  If so, don't we have a
> 32-bit problem?  Or does this code have the suitable 64-bit Kconfig
> dependencies?

Yes - the header is also included in user test programs (mostly diagnostics) that
are used to dump the GRU state after an error.  If necessary, I can make these
lines go away from the kernel version of the file.


> 
> 
> > +/*
> > + * GSH - GRU Status Handle
> > + *	Shows status of each CBR/CBR resources
> > + */
> > +struct gru_global_status_handle {
> > +	unsigned long bits[BITS_TO_LONGS(GRU_NUM_CBE) * 2];
> 
> That's an open-coded DECLARE_BITMAP.
> 
> I'm assuming that we're about to see large amounts of code which
> reimplements the functions which the bitmap library offers, so I'll
> just ask: should this code be using the bitmap library?

I think I could use a bitmap here. However, I may still have an issue with
the use of this header in user programs - not sure yet.

However, we don't current use this GRU structure and am not sure if we will.
For now, I think I will just delete the structure. When/if we find a use,
I'll re-address the issue.


> 
> > +	unsigned int opc:1;
> > +	unsigned int fill1:5;
> > +
> > +	unsigned int fill2:8;
> > +
> >
> > ...
> >
> > +#ifdef __KERNEL__
> > +#include "gru_instructions.h"
> > +
> > +/* Extract the status field from a kernel handle */
> > +#define GET_MSEG_HANDLE_STATUS(h)	(((*(unsigned long *)(h)) >> 16) & 3)
> > +
> 
> > +#if defined __ia64__
> > +#elif defined __x86_64__
> > +#endif
> 
> CONFIG_IA64 and CONFIG_X86_64 would be more fashionable.

Agree. Fixed.


> 
> > +static inline void start_instruction(void *h)
> > +static inline int wait_instruction_complete(void *h)
> > +static inline void cch_allocate_set_asids(
> > +static inline void cch_allocate_set_asids(
> > +static inline int cch_allocate(struct gru_context_configuration_handle *cch,
> > +static inline int cch_start(struct gru_context_configuration_handle *cch)
> > +static inline int cch_interrupt(struct gru_context_configuration_handle *cch)
> > +static inline int cch_deallocate(struct gru_context_configuration_handle *cch)
> > +static inline int cch_interrupt_sync(struct gru_context_configuration_handle
> > +static inline int tgh_invalidate(struct gru_tlb_global_handle *tgh,
> > +static inline void tfh_write_only(struct gru_tlb_fault_handle *tfh,
> > +static inline void tfh_write_restart(struct gru_tlb_fault_handle *tfh,
> > +static inline void tfh_restart(struct gru_tlb_fault_handle *tfh)
> > +static inline void tfh_user_polling_mode(struct gru_tlb_fault_handle *tfh)
> > +static inline void tfh_exception(struct gru_tlb_fault_handle *tfh)
> 
> wow.  Most of these are way too large to be inlined.  And inlining is
> so unweildy and namespace-polluty.  How come?

I'll take another look at these. My "todo" list has an item to review the
use of inline functions and make sure they are correctly used.


--- jack

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists