Re: ppb(4) related changes

["Jonathan A. Kollasch" <jakllsch%kollasch.net@localhost>]

On Mon, Jan 28, 2019 at 07:34:29PM +0900, Masanobu SAITOH wrote:
>  Hi.
> 
>  I'm now working to modify PCI bridge stuff. I'd like to do the following
> two modifications:
> 
>  0) ppbreg.h definitions
> 
>     All of PCI configuration space's register definitions are in pcireg.h
>     The ppbreg.h file also defines the same config registers. I think it's
>     good to remove ppbreg.hs' definitions an use pcireg.h's definitions.
>     Is it OK?

Seems reasonable, although be sure to fix everything that includes it.

> 
>  1) Mis-configuration of prefetchable memory in pciconf.c::configure_bridge()
>     (only for PCI_NETBSD_CONFIGURE)
> 
>     In configure_bridge():
> 
> >         mem = pci_conf_read(pb->pc, pd->tag, PCI_BRIDGE_PREFETCHMEM_REG);
> 
> 	Read register
> 
> > #if ULONG_MAX > 0xffffffff
> >         if (!PCI_BRIDGE_PREFETCHMEM_64BITS(mem) && mem_limit > 0xFFFFFFFFULL) {
> 
> 	Test if it's marked as 64bit (the maring is in lower 4bit)
> 
> >                 printf("Bus %d bridge does not support 64-bit PMEM.  ",
> >                     pb->busno);
> >                 printf("Disabling prefetchable-MEM accesses\n");
> >                 mem_base  = 0x100000;   /* 1M */
> >                 mem_limit = 0x000000;
> >         }
> > #endif
> >         mem = (((mem_base >> 20) & PCI_BRIDGE_PREFETCHMEM_BASE_MASK)
> >             << PCI_BRIDGE_PREFETCHMEM_BASE_SHIFT);
> >         mem |= (((mem_limit >> 20) & PCI_BRIDGE_PREFETCHMEM_LIMIT_MASK)
> >             << PCI_BRIDGE_PREFETCHMEM_LIMIT_SHIFT);
> >         pci_conf_write(pb->pc, pd->tag, PCI_BRIDGE_PREFETCHMEM_REG, mem);
> 
> 	Regenerate mem value from scratch _WITHOUT_SETTING_LOWER_4BITS_
> 	(It's OK because the lower 4bit is read only).
> 
> >         /*
> >          * XXX -- 64-bit systems need a lot more than just this...
> >          */
> >         if (PCI_BRIDGE_PREFETCHMEM_64BITS(mem)) {
> 
> 	Test if it's marked as 64bit or not. It must be wrong because "mem"
> 	is not the original value, newly generated and low 4bits are 0.
> 	It should always be false.
> 
> >                 mem_base  = (uint64_t) mem_base  >> 32;
> >                 mem_limit = (uint64_t) mem_limit >> 32;
> >                 pci_conf_write(pb->pc, pd->tag, PCI_BRIDGE_PREFETCHBASE32_REG,
> >                     mem_base & 0xffffffff);
> >                 pci_conf_write(pb->pc, pd->tag, PCI_BRIDGE_PREFETCHLIMIT32_REG,
> >                     mem_limit & 0xffffffff);
> >         }
> 
> 	So, proposed patch:
> --------------
> Index: pciconf.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/pci/pciconf.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 pciconf.c
> --- pciconf.c	5 Sep 2014 05:29:16 -0000	1.37
> +++ pciconf.c	28 Jan 2019 10:30:13 -0000
> @@ -855,6 +855,7 @@ configure_bridge(pciconf_dev_t *pd)
>  	pciconf_bus_t	*pb;
>  	pcireg_t	io, iohigh, mem, cmd;
>  	int		rv;
> +	bool		isprefetchmem64;
>  	pb = pd->ppb;
>  	/* Configure I/O base & limit*/
> @@ -919,8 +920,9 @@ configure_bridge(pciconf_dev_t *pd)
>  		mem_limit = 0x000000;
>  	}
>  	mem = pci_conf_read(pb->pc, pd->tag, PCI_BRIDGE_PREFETCHMEM_REG);
> +	isprefetchmem64 = PCI_BRIDGE_PREFETCHMEM_64BITS(mem);
>  #if ULONG_MAX > 0xffffffff
> -	if (!PCI_BRIDGE_PREFETCHMEM_64BITS(mem) && mem_limit > 0xFFFFFFFFULL) {
> +	if (!isprefetchmem64 && mem_limit > 0xFFFFFFFFULL) {
>  		printf("Bus %d bridge does not support 64-bit PMEM.  ",
>  		    pb->busno);
>  		printf("Disabling prefetchable-MEM accesses\n");
> @@ -936,7 +938,7 @@ configure_bridge(pciconf_dev_t *pd)
>  	/*
>  	 * XXX -- 64-bit systems need a lot more than just this...
>  	 */
> -	if (PCI_BRIDGE_PREFETCHMEM_64BITS(mem)) {
> +	if (isprefetchmem64) {
>  		mem_base  = (uint64_t) mem_base  >> 32;
>  		mem_limit = (uint64_t) mem_limit >> 32;
>  		pci_conf_write(pb->pc, pd->tag, PCI_BRIDGE_PREFETCHBASE32_REG,
> --------------
> 
> 	I can't test this diff because I have no any machine which can test
> 	PCI_NETBSD_CONFIGURE now. Is it OK to commit without test?
> 

This also seems reasonable.  This might be the sort of change that
could break video card console output.  However, I think on the vast
majority of machines where this could be a problem generally a serial
console is used, or PCI_NETBSD_CONFIGURE is not enabled.

	Jonathan Kollasch