qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael S. Tsirkin
Subject: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable
Date: Sun, 3 Jan 2010 20:06:09 +0200
User-agent: Mutt/1.5.19 (2009-01-05)

On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
> 
> 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?

Not really, incremental improvements are good.  But what you posted does
not seem to clean up most code, what it really does is fixes ppc
unin_pci.  My feeling was since there's only one user for now maybe it
is better to just have device specific code rather than complicate
generic code. Makes sense?

We need to see several users before we add yet another level of
indirection.  If there is a level of indirection that solves the
swap/noswap ugliness as well that would show this is a good abstraction.
I think I even know what a good abstraction would be: decode address on
write and keep it in decoded form.

> I can do the hacky version then :-).
> 
> 
> Alex


I haven't seen the hacky version one so it's hard to evaluate which way
of fixing unin_pci is better. What is your opinion?

-- 
MST




reply via email to

[Prev in Thread] Current Thread [Next in Thread]