Patchwork [2/6] Add config space conversion function for uni_north

login
register
mail settings
Submitter Alexander Graf
Date Jan. 3, 2010, 1:50 a.m.
Message ID <1262483450-15206-3-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/42031/
State New
Headers show

Comments

Alexander Graf - Jan. 3, 2010, 1:50 a.m.
As stated in the previous patch, the Uninorth PCI bridge requires different
layouts in its PCI config space accessors.

This patch introduces a conversion function that makes it compatible with
the way Linux accesses it.

I also kept an OpenBIOS compatibility hack in. I think it'd be better to
take small steps here and do the config space access rework in OpenBIOS
later on. When that's done we can remove that hack.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Jan. 3, 2010, 3:32 p.m.
On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> As stated in the previous patch, the Uninorth PCI bridge requires different
> layouts in its PCI config space accessors.
> 
> This patch introduces a conversion function that makes it compatible with
> the way Linux accesses it.
> 
> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
> take small steps here and do the config space access rework in OpenBIOS
> later on. When that's done we can remove that hack.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index fdb9401..1c49008 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>  {
>  }
>  
> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> +{
> +    uint32_t retval;
> +    uint32_t reg = s->config_reg;
> +
> +    if (reg & (1u << 31)) {
> +        /* XXX OpenBIOS compatibility hack */
> +        retval = reg;
> +        addr |= reg & 7;
> +    } else if (reg & 1) {
> +        /* Set upper valid bit and remove lower one */
> +        retval = (reg & ~3u) | (1u << 31);
> +    } else {
> +        uint32_t slot, func;
> +        uint32_t devfn;
> +
> +        /* Grab CFA0 style values */
> +        slot = ffs(reg & 0xfffff800) - 1;
> +        func = (reg >> 8) & 7;
> +        devfn = PCI_DEVFN(slot, func);
> +
> +        /* ... and then convert them to x86 format */
> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);

Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
number?  This way this encoding can be changed down the road.

> +    }
> +
> +    retval &= ~3u;
> +    retval |= (addr & 7);
> +
> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
> +                 reg, addr, retval);
> +
> +    return retval;
> +}
> +
>  static int pci_unin_main_init_device(SysBusDevice *dev)
>  {
>      UNINState *s;
> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>      s = FROM_SYSBUS(UNINState, dev);
>  
>      pci_host_init(&s->host_state);
> +    s->host_state.get_config_reg = unin_get_config_reg;

This looks slightly ugly: let's make pci_host_init accept
the conversion function instead?

>      pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
>      pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> -- 
> 1.6.0.2
> 
>
Alexander Graf - Jan. 3, 2010, 3:40 p.m.
On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
>> As stated in the previous patch, the Uninorth PCI bridge requires different
>> layouts in its PCI config space accessors.
>> 
>> This patch introduces a conversion function that makes it compatible with
>> the way Linux accesses it.
>> 
>> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
>> take small steps here and do the config space access rework in OpenBIOS
>> later on. When that's done we can remove that hack.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>> 1 files changed, 35 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>> index fdb9401..1c49008 100644
>> --- a/hw/unin_pci.c
>> +++ b/hw/unin_pci.c
>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>> {
>> }
>> 
>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
>> +{
>> +    uint32_t retval;
>> +    uint32_t reg = s->config_reg;
>> +
>> +    if (reg & (1u << 31)) {
>> +        /* XXX OpenBIOS compatibility hack */
>> +        retval = reg;
>> +        addr |= reg & 7;
>> +    } else if (reg & 1) {
>> +        /* Set upper valid bit and remove lower one */
>> +        retval = (reg & ~3u) | (1u << 31);
>> +    } else {
>> +        uint32_t slot, func;
>> +        uint32_t devfn;
>> +
>> +        /* Grab CFA0 style values */
>> +        slot = ffs(reg & 0xfffff800) - 1;
>> +        func = (reg >> 8) & 7;
>> +        devfn = PCI_DEVFN(slot, func);
>> +
>> +        /* ... and then convert them to x86 format */
>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> 
> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> number?  This way this encoding can be changed down the road.

I don't think I understand this comment? :-)

> 
>> +    }
>> +
>> +    retval &= ~3u;
>> +    retval |= (addr & 7);
>> +
>> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
>> +                 reg, addr, retval);
>> +
>> +    return retval;
>> +}
>> +
>> static int pci_unin_main_init_device(SysBusDevice *dev)
>> {
>>     UNINState *s;
>> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>>     s = FROM_SYSBUS(UNINState, dev);
>> 
>>     pci_host_init(&s->host_state);
>> +    s->host_state.get_config_reg = unin_get_config_reg;
> 
> This looks slightly ugly: let's make pci_host_init accept
> the conversion function instead?

Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones.


Alex
Michael S. Tsirkin - Jan. 3, 2010, 3:47 p.m.
On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >> As stated in the previous patch, the Uninorth PCI bridge requires different
> >> layouts in its PCI config space accessors.
> >> 
> >> This patch introduces a conversion function that makes it compatible with
> >> the way Linux accesses it.
> >> 
> >> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
> >> take small steps here and do the config space access rework in OpenBIOS
> >> later on. When that's done we can remove that hack.
> >> 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >> 1 files changed, 35 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >> index fdb9401..1c49008 100644
> >> --- a/hw/unin_pci.c
> >> +++ b/hw/unin_pci.c
> >> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >> {
> >> }
> >> 
> >> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >> +{
> >> +    uint32_t retval;
> >> +    uint32_t reg = s->config_reg;
> >> +
> >> +    if (reg & (1u << 31)) {
> >> +        /* XXX OpenBIOS compatibility hack */
> >> +        retval = reg;
> >> +        addr |= reg & 7;
> >> +    } else if (reg & 1) {
> >> +        /* Set upper valid bit and remove lower one */
> >> +        retval = (reg & ~3u) | (1u << 31);
> >> +    } else {
> >> +        uint32_t slot, func;
> >> +        uint32_t devfn;
> >> +
> >> +        /* Grab CFA0 style values */
> >> +        slot = ffs(reg & 0xfffff800) - 1;
> >> +        func = (reg >> 8) & 7;
> >> +        devfn = PCI_DEVFN(slot, func);
> >> +
> >> +        /* ... and then convert them to x86 format */
> >> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> > 
> > Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> > number?  This way this encoding can be changed down the road.
> 
> I don't think I understand this comment? :-)

This puts reg+dev+fn in a format that pci_host can the understand
correct? So it would make sense to have an inline function
in pci host that gets 3 parameters and does the encoding.

> > 
> >> +    }
> >> +
> >> +    retval &= ~3u;
> >> +    retval |= (addr & 7);
> >> +
> >> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
> >> +                 reg, addr, retval);
> >> +
> >> +    return retval;
> >> +}
> >> +
> >> static int pci_unin_main_init_device(SysBusDevice *dev)
> >> {
> >>     UNINState *s;
> >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
> >>     s = FROM_SYSBUS(UNINState, dev);
> >> 
> >>     pci_host_init(&s->host_state);
> >> +    s->host_state.get_config_reg = unin_get_config_reg;
> > 
> > This looks slightly ugly: let's make pci_host_init accept
> > the conversion function instead?
> 
> Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones.
> 
> 
> Alex
Michael S. Tsirkin - Jan. 3, 2010, 3:48 p.m.
On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >> As stated in the previous patch, the Uninorth PCI bridge requires different
> >> layouts in its PCI config space accessors.
> >> 
> >> This patch introduces a conversion function that makes it compatible with
> >> the way Linux accesses it.
> >> 
> >> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
> >> take small steps here and do the config space access rework in OpenBIOS
> >> later on. When that's done we can remove that hack.
> >> 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >> 1 files changed, 35 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >> index fdb9401..1c49008 100644
> >> --- a/hw/unin_pci.c
> >> +++ b/hw/unin_pci.c
> >> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >> {
> >> }
> >> 
> >> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >> +{
> >> +    uint32_t retval;
> >> +    uint32_t reg = s->config_reg;
> >> +
> >> +    if (reg & (1u << 31)) {
> >> +        /* XXX OpenBIOS compatibility hack */
> >> +        retval = reg;
> >> +        addr |= reg & 7;
> >> +    } else if (reg & 1) {
> >> +        /* Set upper valid bit and remove lower one */
> >> +        retval = (reg & ~3u) | (1u << 31);
> >> +    } else {
> >> +        uint32_t slot, func;
> >> +        uint32_t devfn;
> >> +
> >> +        /* Grab CFA0 style values */
> >> +        slot = ffs(reg & 0xfffff800) - 1;
> >> +        func = (reg >> 8) & 7;
> >> +        devfn = PCI_DEVFN(slot, func);
> >> +
> >> +        /* ... and then convert them to x86 format */
> >> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> > 
> > Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> > number?  This way this encoding can be changed down the road.
> 
> I don't think I understand this comment? :-)
> 
> > 
> >> +    }
> >> +
> >> +    retval &= ~3u;
> >> +    retval |= (addr & 7);
> >> +
> >> +    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
> >> +                 reg, addr, retval);
> >> +
> >> +    return retval;
> >> +}
> >> +
> >> static int pci_unin_main_init_device(SysBusDevice *dev)
> >> {
> >>     UNINState *s;
> >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
> >>     s = FROM_SYSBUS(UNINState, dev);
> >> 
> >>     pci_host_init(&s->host_state);
> >> +    s->host_state.get_config_reg = unin_get_config_reg;
> > 
> > This looks slightly ugly: let's make pci_host_init accept
> > the conversion function instead?
> 
> Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones.
> 

OK.

> Alex
Alexander Graf - Jan. 3, 2010, 4:13 p.m.
On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
>>>> As stated in the previous patch, the Uninorth PCI bridge requires different
>>>> layouts in its PCI config space accessors.
>>>> 
>>>> This patch introduces a conversion function that makes it compatible with
>>>> the way Linux accesses it.
>>>> 
>>>> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
>>>> take small steps here and do the config space access rework in OpenBIOS
>>>> later on. When that's done we can remove that hack.
>>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>>>> 1 files changed, 35 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>>>> index fdb9401..1c49008 100644
>>>> --- a/hw/unin_pci.c
>>>> +++ b/hw/unin_pci.c
>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>>>> {
>>>> }
>>>> 
>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
>>>> +{
>>>> +    uint32_t retval;
>>>> +    uint32_t reg = s->config_reg;
>>>> +
>>>> +    if (reg & (1u << 31)) {
>>>> +        /* XXX OpenBIOS compatibility hack */
>>>> +        retval = reg;
>>>> +        addr |= reg & 7;
>>>> +    } else if (reg & 1) {
>>>> +        /* Set upper valid bit and remove lower one */
>>>> +        retval = (reg & ~3u) | (1u << 31);
>>>> +    } else {
>>>> +        uint32_t slot, func;
>>>> +        uint32_t devfn;
>>>> +
>>>> +        /* Grab CFA0 style values */
>>>> +        slot = ffs(reg & 0xfffff800) - 1;
>>>> +        func = (reg >> 8) & 7;
>>>> +        devfn = PCI_DEVFN(slot, func);
>>>> +
>>>> +        /* ... and then convert them to x86 format */
>>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
>>> 
>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
>>> number?  This way this encoding can be changed down the road.
>> 
>> I don't think I understand this comment? :-)
> 
> This puts reg+dev+fn in a format that pci_host can the understand
> correct? So it would make sense to have an inline function
> in pci host that gets 3 parameters and does the encoding.

We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it. We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.

I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).

Alex
Michael S. Tsirkin - Jan. 3, 2010, 4:20 p.m.
On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> >> 
> >>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >>>> As stated in the previous patch, the Uninorth PCI bridge requires different
> >>>> layouts in its PCI config space accessors.
> >>>> 
> >>>> This patch introduces a conversion function that makes it compatible with
> >>>> the way Linux accesses it.
> >>>> 
> >>>> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
> >>>> take small steps here and do the config space access rework in OpenBIOS
> >>>> later on. When that's done we can remove that hack.
> >>>> 
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>> ---
> >>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >>>> 1 files changed, 35 insertions(+), 0 deletions(-)
> >>>> 
> >>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >>>> index fdb9401..1c49008 100644
> >>>> --- a/hw/unin_pci.c
> >>>> +++ b/hw/unin_pci.c
> >>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >>>> {
> >>>> }
> >>>> 
> >>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >>>> +{
> >>>> +    uint32_t retval;
> >>>> +    uint32_t reg = s->config_reg;
> >>>> +
> >>>> +    if (reg & (1u << 31)) {
> >>>> +        /* XXX OpenBIOS compatibility hack */
> >>>> +        retval = reg;
> >>>> +        addr |= reg & 7;
> >>>> +    } else if (reg & 1) {
> >>>> +        /* Set upper valid bit and remove lower one */
> >>>> +        retval = (reg & ~3u) | (1u << 31);
> >>>> +    } else {
> >>>> +        uint32_t slot, func;
> >>>> +        uint32_t devfn;
> >>>> +
> >>>> +        /* Grab CFA0 style values */
> >>>> +        slot = ffs(reg & 0xfffff800) - 1;
> >>>> +        func = (reg >> 8) & 7;
> >>>> +        devfn = PCI_DEVFN(slot, func);
> >>>> +
> >>>> +        /* ... and then convert them to x86 format */
> >>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> >>> 
> >>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> >>> number?  This way this encoding can be changed down the road.
> >> 
> >> I don't think I understand this comment? :-)
> > 
> > This puts reg+dev+fn in a format that pci_host can the understand
> > correct? So it would make sense to have an inline function
> > in pci host that gets 3 parameters and does the encoding.
> 
> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it.
> 
> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.
> 
> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).
> 
> Alex

So what the comment above suggests, is adding helper routine
that gets register device, function and creates 32 bit value
in "default format".
Alexander Graf - Jan. 3, 2010, 4:35 p.m.
On 03.01.2010, at 17:20, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
>>>> 
>>>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
>>>>>> As stated in the previous patch, the Uninorth PCI bridge requires different
>>>>>> layouts in its PCI config space accessors.
>>>>>> 
>>>>>> This patch introduces a conversion function that makes it compatible with
>>>>>> the way Linux accesses it.
>>>>>> 
>>>>>> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
>>>>>> take small steps here and do the config space access rework in OpenBIOS
>>>>>> later on. When that's done we can remove that hack.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>> ---
>>>>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
>>>>>> 1 files changed, 35 insertions(+), 0 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>>>>>> index fdb9401..1c49008 100644
>>>>>> --- a/hw/unin_pci.c
>>>>>> +++ b/hw/unin_pci.c
>>>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
>>>>>> {
>>>>>> }
>>>>>> 
>>>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
>>>>>> +{
>>>>>> +    uint32_t retval;
>>>>>> +    uint32_t reg = s->config_reg;
>>>>>> +
>>>>>> +    if (reg & (1u << 31)) {
>>>>>> +        /* XXX OpenBIOS compatibility hack */
>>>>>> +        retval = reg;
>>>>>> +        addr |= reg & 7;
>>>>>> +    } else if (reg & 1) {
>>>>>> +        /* Set upper valid bit and remove lower one */
>>>>>> +        retval = (reg & ~3u) | (1u << 31);
>>>>>> +    } else {
>>>>>> +        uint32_t slot, func;
>>>>>> +        uint32_t devfn;
>>>>>> +
>>>>>> +        /* Grab CFA0 style values */
>>>>>> +        slot = ffs(reg & 0xfffff800) - 1;
>>>>>> +        func = (reg >> 8) & 7;
>>>>>> +        devfn = PCI_DEVFN(slot, func);
>>>>>> +
>>>>>> +        /* ... and then convert them to x86 format */
>>>>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
>>>>> 
>>>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
>>>>> number?  This way this encoding can be changed down the road.
>>>> 
>>>> I don't think I understand this comment? :-)
>>> 
>>> This puts reg+dev+fn in a format that pci_host can the understand
>>> correct? So it would make sense to have an inline function
>>> in pci host that gets 3 parameters and does the encoding.
>> 
>> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it.
>> 
>> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.
>> 
>> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).
>> 
>> Alex
> 
> So what the comment above suggests, is adding helper routine
> that gets register device, function and creates 32 bit value
> in "default format".

Oh, so you mean that instead of returning a uint32_t that magically gets converted inside the conversion function, we'd create another function like this:

uint32_t pci_host_config_address(int bus, int dev, int fn)
{
    return (1u << 31) | (bus << 11) | (dev << 3) | fn;
}

which then would be called at the end of the conversion function?


Alex
Michael S. Tsirkin - Jan. 3, 2010, 5:23 p.m.
On Sun, Jan 03, 2010 at 05:35:01PM +0100, Alexander Graf wrote:
> 
> On 03.01.2010, at 17:20, Michael S. Tsirkin wrote:
> 
> > On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote:
> >> 
> >> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote:
> >> 
> >>> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote:
> >>>> 
> >>>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote:
> >>>> 
> >>>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote:
> >>>>>> As stated in the previous patch, the Uninorth PCI bridge requires different
> >>>>>> layouts in its PCI config space accessors.
> >>>>>> 
> >>>>>> This patch introduces a conversion function that makes it compatible with
> >>>>>> the way Linux accesses it.
> >>>>>> 
> >>>>>> I also kept an OpenBIOS compatibility hack in. I think it'd be better to
> >>>>>> take small steps here and do the config space access rework in OpenBIOS
> >>>>>> later on. When that's done we can remove that hack.
> >>>>>> 
> >>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>>> ---
> >>>>>> hw/unin_pci.c |   35 +++++++++++++++++++++++++++++++++++
> >>>>>> 1 files changed, 35 insertions(+), 0 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> >>>>>> index fdb9401..1c49008 100644
> >>>>>> --- a/hw/unin_pci.c
> >>>>>> +++ b/hw/unin_pci.c
> >>>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque)
> >>>>>> {
> >>>>>> }
> >>>>>> 
> >>>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
> >>>>>> +{
> >>>>>> +    uint32_t retval;
> >>>>>> +    uint32_t reg = s->config_reg;
> >>>>>> +
> >>>>>> +    if (reg & (1u << 31)) {
> >>>>>> +        /* XXX OpenBIOS compatibility hack */
> >>>>>> +        retval = reg;
> >>>>>> +        addr |= reg & 7;
> >>>>>> +    } else if (reg & 1) {
> >>>>>> +        /* Set upper valid bit and remove lower one */
> >>>>>> +        retval = (reg & ~3u) | (1u << 31);
> >>>>>> +    } else {
> >>>>>> +        uint32_t slot, func;
> >>>>>> +        uint32_t devfn;
> >>>>>> +
> >>>>>> +        /* Grab CFA0 style values */
> >>>>>> +        slot = ffs(reg & 0xfffff800) - 1;
> >>>>>> +        func = (reg >> 8) & 7;
> >>>>>> +        devfn = PCI_DEVFN(slot, func);
> >>>>>> +
> >>>>>> +        /* ... and then convert them to x86 format */
> >>>>>> +        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
> >>>>> 
> >>>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit
> >>>>> number?  This way this encoding can be changed down the road.
> >>>> 
> >>>> I don't think I understand this comment? :-)
> >>> 
> >>> This puts reg+dev+fn in a format that pci_host can the understand
> >>> correct? So it would make sense to have an inline function
> >>> in pci host that gets 3 parameters and does the encoding.
> >> 
> >> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it.
> >> 
> >> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format.
> >> 
> >> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-).
> >> 
> >> Alex
> > 
> > So what the comment above suggests, is adding helper routine
> > that gets register device, function and creates 32 bit value
> > in "default format".
> 
> Oh, so you mean that instead of returning a uint32_t that magically gets converted inside the conversion function, we'd create another function like this:
> 
> uint32_t pci_host_config_address(int bus, int dev, int fn)
> {
>     return (1u << 31) | (bus << 11) | (dev << 3) | fn;
> }
> 
> which then would be called at the end of the conversion function?
> 
> 
> Alex

Yes.

Patch

diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index fdb9401..1c49008 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -75,6 +75,40 @@  static void pci_unin_reset(void *opaque)
 {
 }
 
+static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr)
+{
+    uint32_t retval;
+    uint32_t reg = s->config_reg;
+
+    if (reg & (1u << 31)) {
+        /* XXX OpenBIOS compatibility hack */
+        retval = reg;
+        addr |= reg & 7;
+    } else if (reg & 1) {
+        /* Set upper valid bit and remove lower one */
+        retval = (reg & ~3u) | (1u << 31);
+    } else {
+        uint32_t slot, func;
+        uint32_t devfn;
+
+        /* Grab CFA0 style values */
+        slot = ffs(reg & 0xfffff800) - 1;
+        func = (reg >> 8) & 7;
+        devfn = PCI_DEVFN(slot, func);
+
+        /* ... and then convert them to x86 format */
+        retval = (reg & 0xfc) | (devfn << 8) | (1u << 31);
+    }
+
+    retval &= ~3u;
+    retval |= (addr & 7);
+
+    UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n",
+                 reg, addr, retval);
+
+    return retval;
+}
+
 static int pci_unin_main_init_device(SysBusDevice *dev)
 {
     UNINState *s;
@@ -85,6 +119,7 @@  static int pci_unin_main_init_device(SysBusDevice *dev)
     s = FROM_SYSBUS(UNINState, dev);
 
     pci_host_init(&s->host_state);
+    s->host_state.get_config_reg = unin_get_config_reg;
     pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
     pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);