[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Alexander Graf]

On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:

> On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>> 
>> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>> 
>>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>> 
>>>>> On Sun, Jan 03, 2010 at 02:50:45AM +0100, Alexander Graf wrote:
>>>>>> Different host buses may have different layouts for config space 
>>>>>> accessors.
>>>>>> 
>>>>>> The Mac U3 for example uses the following define to access Type 0 
>>>>>> (directly
>>>>>> attached) devices:
>>>>>> 
>>>>>> #define MACRISC_CFA0(devfn, off)        \
>>>>>>      ((1 << (unsigned int)PCI_SLOT(dev_fn)) \
>>>>>>      | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
>>>>>>      | (((unsigned int)(off)) & 0xFCUL))
>>>>>> 
>>>>>> Obviously, this is different from the encoding we interpret in qemu. In
>>>>>> order to let host controllers take some influence there, we can just
>>>>>> introduce a converter function that takes whatever accessor we have and
>>>>>> makes a qemu understandable one out of it.
>>>>>> 
>>>>>> This patch is the groundwork for Uninorth PCI config space fixes.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf <address@hidden>
>>>>>> CC: Michael S. Tsirkin <address@hidden>
>>>>> 
>>>>> Thanks!
>>>>> 
>>>>> It always bothered me that the code in pci_host uses x86 encoding and
>>>>> other architectures are converted to x86.  As long as we are adding
>>>>> redirection, maybe we should get rid of this, and make get_config_reg
>>>>> return register and devfn separately, so we don't need to encode/decode
>>>>> back and forth?
>>>> 
>>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to 
>>>> build on stuff that is there already. That's exactly what happened here.
>>>> 
>>>> I'm personally not against defining the x86 format as qemu default. That 
>>>> way everyone knows what to deal with. I'm not sure if PCI defines anything 
>>>> that couldn't be represented by the x86 encoding in 32 bits. I actually 
>>>> doubt it. So it seems like a pretty good fit, especially given that all 
>>>> the other code is already in place.
>>>> 
>>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>> unin_pci simply use its own routines instead of 
>>>>> pci_host_conf_register_mmio
>>>>> etc?
>>>> 
>>>> Hm, I figured this is less work :-).
>>> 
>>> Let me explain the idea: we have:
>>> 
>>>     static void pci_host_config_writel_ioport(void *opaque,
>>>                                               uint32_t addr, uint32_t val)
>>>     {
>>>         PCIHostState *s = opaque;
>>> 
>>>         PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
>>>     val);
>>>         s->config_reg = val;
>>>     }
>>> 
>>>     static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
>>>     addr)
>>>     {
>>>         PCIHostState *s = opaque;
>>>         uint32_t val = s->config_reg;
>>> 
>>>         PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
>>>     val);
>>>         return val;
>>>     }
>>> 
>>>     void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>>>     {
>>>         register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
>>>     s);
>>>         register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
>>>     }
>>> 
>>> If instead of a simple config_reg = val we translate to pc format
>>> here, the rest will work. No?
>> 
>> Well, that'd mean I'd have to implement a config_reg read function and do 
>> the conversion backwards again there. Or I could just save it off in the 
>> unin state ... hmm ...
> 
> Right.
> 
>> Either way, let's better get this done right. I'd rather want to have a 
>> proper framework in place in case someone else stumbles across something 
>> similar.
>> 
>> Alex
> 
> There's already ugliness with swap/noswap versions in there.  Anything
> that claims to be a proper framework would need to address that as well
> IMO.

Hm, so you'd rather wait for 5 years to have an all-in-one rework than 
incrementally improving the existing code? I can do the hacky version then :-).


Alex