[OpenBIOS] Re: [PATCH v4] drivers/pci: Implement config access words

[BALATON Zoltan]

On Tue, 31 Jan 2023, Mark Cave-Ayland wrote:
> On 28/01/2023 22:42, BALATON Zoltan wrote:
> > Signed-off-by: BALATON Zoltan
> >
> > v4: Added call parent word for bridge devices
> >
> > drivers/pci.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 102 insertions(+)
> >
> > diff --git a/drivers/pci.c b/drivers/pci.c
> > index f30e427..084ebe9 100644
> > --- a/drivers/pci.c
> > +++ b/drivers/pci.c
> > @@ -424,6 +424,60 @@ ob_pci_bus_map_in(int *idx)
> >    PUSH(virt);
> >   }
> >   +static void
> > +ob_pci_config_read8(int *idx)
> > +{
> > cell hi = POP();
> > pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
> > uint8_t val = pci_config_read8(addr, hi & 0xff);
> > PUSH(val);
> >
> > +}
> >
> > +static void
> > +ob_pci_config_write8(int *idx)
> > +{
> > cell hi = POP();
> > pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
> > cell val = POP();
> > pci_config_write8(addr, hi & 0xff, val);
> >
> > +}
> >
> > +static void
> > +ob_pci_config_read16(int *idx)
> > +{
> > cell hi = POP();
> > pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
> > uint16_t val = pci_config_read16(addr, hi & 0xff);
> > PUSH(val);
> >
> > +}
> >
> > +static void
> > +ob_pci_config_write16(int *idx)
> > +{
> > cell hi = POP();
> > pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
> > cell val = POP();
> > pci_config_write16(addr, hi & 0xff, val);
> >
> > +}
> >
> > +static void
> > +ob_pci_config_read32(int *idx)
> > +{
> > cell hi = POP();
> > pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
> > uint32_t val = pci_config_read32(addr, hi & 0xff);
> > PUSH(val);
> >
> > +}
> >
> > +static void
> > +ob_pci_config_write32(int *idx)
> > +{
> > cell hi = POP();
> > pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
> > cell val = POP();
> > pci_config_write32(addr, hi & 0xff, val);
> >
> > +}
> > static void
> > ob_pci_dma_alloc(int *idx)
> > {
> >
> > @@ -460,6 +514,12 @@ NODE_METHODS(ob_pci_bus_node) = {
> >    { "decode-unit",	ob_pci_decode_unit	},
> >    { "encode-unit",	ob_pci_encode_unit	},
> >    { "pci-map-in",		ob_pci_bus_map_in	},
> > { "config-b@",		ob_pci_config_read8	},
> > { "config-b!",		ob_pci_config_write8	},
> > { "config-w@",		ob_pci_config_read16	},
> > { "config-w!",		ob_pci_config_write16	},
> > { "config-l@",		ob_pci_config_read32	},
> > { "config-l!",		ob_pci_config_write32	},
> > { "dma-alloc",		ob_pci_dma_alloc	},
> > { "dma-free",		ob_pci_dma_free		},
> > { "dma-map-in",		ob_pci_dma_map_in	},
> >
> > @@ -476,12 +536,54 @@ ob_pci_bridge_map_in(int *idx)
> >    call_parent_method("pci-map-in");
> >   }
> >   +static void
> > +ob_pci_bridge_cread8(void)
> > +{
> > call_parent_method("config-b@");
> >
> > +}
> >
> > +static void
> > +ob_pci_bridge_cwrite8(void)
> > +{
> > call_parent_method("config-b!");
> >
> > +}
> >
> > +static void
> > +ob_pci_bridge_cread16(void)
> > +{
> > call_parent_method("config-w@");
> >
> > +}
> >
> > +static void
> > +ob_pci_bridge_cwrite16(void)
> > +{
> > call_parent_method("config-w!");
> >
> > +}
> >
> > +static void
> > +ob_pci_bridge_cread32(void)
> > +{
> > call_parent_method("config-l@");
> >
> > +}
> >
> > +static void
> > +ob_pci_bridge_cwrite32(void)
> > +{
> > call_parent_method("config-l!");
> >
> > +}
> > NODE_METHODS(ob_pci_bridge_node) = {
> >  { "open",		ob_pci_open		},
> >  { "close",		ob_pci_close		},
> >  { "decode-unit",	ob_pci_decode_unit	},
> >  { "encode-unit",	ob_pci_encode_unit	},
> >  { "pci-map-in",		ob_pci_bridge_map_in	},
> > { "config-b@",		ob_pci_bridge_cread8	},
> > { "config-b!",		ob_pci_bridge_cwrite8	},
> > { "config-w@",		ob_pci_bridge_cread16	},
> > { "config-w!",		ob_pci_bridge_cwrite16	},
> > { "config-l@",		ob_pci_bridge_cread32	},
> > { "config-l!",		ob_pci_bridge_cwrite32	},
> > { "dma-alloc",		ob_pci_dma_alloc	},
> > { "dma-free",		ob_pci_dma_free		},
> > { "dma-map-in",		ob_pci_dma_map_in	},
>
> I've given this a try, and whilst the patch looks right to me, there is still 
> something wrong with regard to the PCI bridge:
>
> 0 > cd /pci@1fe,0/pci@1,1/QEMU,VGA@2  ok
> 0 > " /pci@1fe,0/pci@1,1/QEMU,VGA@2" open-dev to my-self  ok
> 0 > my-space " config-w@" $call-parent  ok
> 1 > u. ffff  ok
>
> ffff normally indicates a failed read/write to PCI configuration space so it 
> seems like the offset isn't being calculated correctly.

No idea what could be wrong with that but sent a new version with a 
different approach which adds these via Forth so we avoid going back and 
... well, forth :-) between C and Forth (Forth is even confusing just to 
talk about) so if that was causing a problem somhow corrupting the stack 
this should avoid that and also avoids the one line functions so if this 
works I'd add another patch to replace the other similar dma-* words too. 
Can you test this please?

Regards,
BALATON Zoltan