| Message ID | 20100208064147.GD22624@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: > initialize header type register in pci generic code. > > Cc: Blue Swirl <blauwirbel@gmail.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> No objections here, I am assuming this will be followed by patches removing header type init from bridges? From qdev perspective, it is probably cleaner to make multifunction bit a separate qdev property though, right? I queued this one, let's wait a bit for comments from interested people. > --- > hw/pci.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index eb2043e..7b055b4 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -603,6 +603,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > pci_dev->irq_state = 0; > pci_config_alloc(pci_dev); > > + pci_dev->config[PCI_HEADER_TYPE] = header_type; > header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION; > if (header_type == PCI_HEADER_TYPE_NORMAL) { > pci_set_default_subsystem_id(pci_dev); > -- > 1.6.6.1
On 02/08/10 11:17, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >> initialize header type register in pci generic code. >> >> Cc: Blue Swirl<blauwirbel@gmail.com> >> Cc: "Michael S. Tsirkin"<mst@redhat.com> >> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> > > No objections here, I am assuming this will be followed > by patches removing header type init from bridges? > From qdev perspective, it is probably cleaner to make > multifunction bit a separate qdev property though, right? From a qdev perspective it would make *alot* of sense to move a bunch of pci config stuff (including, but not limited to header type) into PCIDeviceInfo. cheers, Gerd
On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: > On 02/08/10 11:17, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>> initialize header type register in pci generic code. >>> >>> Cc: Blue Swirl<blauwirbel@gmail.com> >>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >> >> No objections here, I am assuming this will be followed >> by patches removing header type init from bridges? >> From qdev perspective, it is probably cleaner to make >> multifunction bit a separate qdev property though, right? > > From a qdev perspective it would make *alot* of sense to move a bunch of > pci config stuff (including, but not limited to header type) into > PCIDeviceInfo. > > cheers, > Gerd It's an Ack then?
On 02/08/10 12:16, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>> initialize header type register in pci generic code. >>>> >>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>> >>> No objections here, I am assuming this will be followed >>> by patches removing header type init from bridges? >>> From qdev perspective, it is probably cleaner to make >>> multifunction bit a separate qdev property though, right? >> >> From a qdev perspective it would make *alot* of sense to move a bunch of >> pci config stuff (including, but not limited to header type) into >> PCIDeviceInfo. >> >> cheers, >> Gerd > > It's an Ack then? Yes, count it as ack. It is a move into the right direction, and the fact that more cleanups can/should be done here shouldn't hold it up. cheers, Gerd
On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: > initialize header type register in pci generic code. > > Cc: Blue Swirl <blauwirbel@gmail.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> Applied, thanks. > --- > hw/pci.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index eb2043e..7b055b4 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -603,6 +603,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > pci_dev->irq_state = 0; > pci_config_alloc(pci_dev); > > + pci_dev->config[PCI_HEADER_TYPE] = header_type; > header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION; > if (header_type == PCI_HEADER_TYPE_NORMAL) { > pci_set_default_subsystem_id(pci_dev); > -- > 1.6.6.1
On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: > On 02/08/10 11:17, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>> initialize header type register in pci generic code. >>> >>> Cc: Blue Swirl<blauwirbel@gmail.com> >>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >> >> No objections here, I am assuming this will be followed >> by patches removing header type init from bridges? >> From qdev perspective, it is probably cleaner to make >> multifunction bit a separate qdev property though, right? > > From a qdev perspective it would make *alot* of sense to move a bunch of > pci config stuff (including, but not limited to header type) into > PCIDeviceInfo. > > cheers, > Gerd Actually - won't this make it possible to create broken configurations by tweaking properties from command-line? And generally, it sounds bad to have header type duplicated in qdev and in config. Why do we want it in qdev? Isaku Yamahata, could you please clarify?
On 02/08/10 17:27, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>> initialize header type register in pci generic code. >>>> >>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>> >>> No objections here, I am assuming this will be followed >>> by patches removing header type init from bridges? >>> From qdev perspective, it is probably cleaner to make >>> multifunction bit a separate qdev property though, right? >> >> From a qdev perspective it would make *alot* of sense to move a bunch of >> pci config stuff (including, but not limited to header type) into >> PCIDeviceInfo. >> >> cheers, >> Gerd > > Actually - won't this make it possible to create broken configurations > by tweaking properties from command-line? Not as property, as struct element in PCIDeviceInfo. i.e. static PCIDeviceInfo e1000_info = { [ stuff which is here right now ] .vendor_id = PCI_VENDOR_ID_INTEL, .device_id = E1000_DEVID, .class = PCI_CLASS_NETWORK_ETHERNET, [ probably more stuff which makes sense ] } Then setup this in generic pci code instead of having each driver doing a bunch of pci_config_set_*() calls. cheers, Gerd
On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: > On 02/08/10 17:27, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>>> initialize header type register in pci generic code. >>>>> >>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>> >>>> No objections here, I am assuming this will be followed >>>> by patches removing header type init from bridges? >>>> From qdev perspective, it is probably cleaner to make >>>> multifunction bit a separate qdev property though, right? >>> >>> From a qdev perspective it would make *alot* of sense to move a bunch of >>> pci config stuff (including, but not limited to header type) into >>> PCIDeviceInfo. >>> >>> cheers, >>> Gerd >> >> Actually - won't this make it possible to create broken configurations >> by tweaking properties from command-line? > > Not as property, as struct element in PCIDeviceInfo. i.e. > > static PCIDeviceInfo e1000_info = { > [ stuff which is here right now ] > .vendor_id = PCI_VENDOR_ID_INTEL, > .device_id = E1000_DEVID, > .class = PCI_CLASS_NETWORK_ETHERNET, > [ probably more stuff which makes sense ] > } > > Then setup this in generic pci code instead of having each driver doing > a bunch of pci_config_set_*() calls. > > cheers, > Gerd We still end up with class, vendor etc duplicated in 2 places. Why do we want stuff like vendor id in PCIDeviceInfo at all? Why can't everyone just use pci_config_set/get calls?
On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: > On 02/08/10 18:32, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: >>> On 02/08/10 17:27, Michael S. Tsirkin wrote: >>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>>>>> initialize header type register in pci generic code. >>>>>>> >>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>>>> >>>>>> No objections here, I am assuming this will be followed >>>>>> by patches removing header type init from bridges? >>>>>> From qdev perspective, it is probably cleaner to make >>>>>> multifunction bit a separate qdev property though, right? >>>>> >>>>> From a qdev perspective it would make *alot* of sense to move a bunch of >>>>> pci config stuff (including, but not limited to header type) into >>>>> PCIDeviceInfo. >>>>> >>>>> cheers, >>>>> Gerd >>>> >>>> Actually - won't this make it possible to create broken configurations >>>> by tweaking properties from command-line? >>> >>> Not as property, as struct element in PCIDeviceInfo. i.e. >>> >>> static PCIDeviceInfo e1000_info = { >>> [ stuff which is here right now ] >>> .vendor_id = PCI_VENDOR_ID_INTEL, >>> .device_id = E1000_DEVID, >>> .class = PCI_CLASS_NETWORK_ETHERNET, >>> [ probably more stuff which makes sense ] >>> } >>> >>> Then setup this in generic pci code instead of having each driver doing >>> a bunch of pci_config_set_*() calls. >>> >>> cheers, >>> Gerd >> >> We still end up with class, vendor etc duplicated in 2 places. > > No. The info should be *only* in PCIDeviceInfo then. That would put a lot of code in pci config cycle path. A single array mirroring the whole config space is much cleaner. >> Why do >> we want stuff like vendor id in PCIDeviceInfo at all? Why can't >> everyone just use pci_config_set/get calls? > > You can do nice stuff like printing vendor/device IDs in the '-device ?' > list then. That should use pci functions as well. > cheers, > Gerd
On 02/08/10 18:32, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: >> On 02/08/10 17:27, Michael S. Tsirkin wrote: >>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>>>> initialize header type register in pci generic code. >>>>>> >>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>>> >>>>> No objections here, I am assuming this will be followed >>>>> by patches removing header type init from bridges? >>>>> From qdev perspective, it is probably cleaner to make >>>>> multifunction bit a separate qdev property though, right? >>>> >>>> From a qdev perspective it would make *alot* of sense to move a bunch of >>>> pci config stuff (including, but not limited to header type) into >>>> PCIDeviceInfo. >>>> >>>> cheers, >>>> Gerd >>> >>> Actually - won't this make it possible to create broken configurations >>> by tweaking properties from command-line? >> >> Not as property, as struct element in PCIDeviceInfo. i.e. >> >> static PCIDeviceInfo e1000_info = { >> [ stuff which is here right now ] >> .vendor_id = PCI_VENDOR_ID_INTEL, >> .device_id = E1000_DEVID, >> .class = PCI_CLASS_NETWORK_ETHERNET, >> [ probably more stuff which makes sense ] >> } >> >> Then setup this in generic pci code instead of having each driver doing >> a bunch of pci_config_set_*() calls. >> >> cheers, >> Gerd > > We still end up with class, vendor etc duplicated in 2 places. No. The info should be *only* in PCIDeviceInfo then. > Why do > we want stuff like vendor id in PCIDeviceInfo at all? Why can't > everyone just use pci_config_set/get calls? You can do nice stuff like printing vendor/device IDs in the '-device ?' list then. cheers, Gerd
On 02/08/10 18:37, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: >> On 02/08/10 18:32, Michael S. Tsirkin wrote: >>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: >>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: >>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>>>>>> initialize header type register in pci generic code. >>>>>>>> >>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>>>>> >>>>>>> No objections here, I am assuming this will be followed >>>>>>> by patches removing header type init from bridges? >>>>>>> From qdev perspective, it is probably cleaner to make >>>>>>> multifunction bit a separate qdev property though, right? >>>>>> >>>>>> From a qdev perspective it would make *alot* of sense to move a bunch of >>>>>> pci config stuff (including, but not limited to header type) into >>>>>> PCIDeviceInfo. >>>>>> >>>>>> cheers, >>>>>> Gerd >>>>> >>>>> Actually - won't this make it possible to create broken configurations >>>>> by tweaking properties from command-line? >>>> >>>> Not as property, as struct element in PCIDeviceInfo. i.e. >>>> >>>> static PCIDeviceInfo e1000_info = { >>>> [ stuff which is here right now ] >>>> .vendor_id = PCI_VENDOR_ID_INTEL, >>>> .device_id = E1000_DEVID, >>>> .class = PCI_CLASS_NETWORK_ETHERNET, >>>> [ probably more stuff which makes sense ] >>>> } >>>> >>>> Then setup this in generic pci code instead of having each driver doing >>>> a bunch of pci_config_set_*() calls. >>>> >>>> cheers, >>>> Gerd >>> >>> We still end up with class, vendor etc duplicated in 2 places. >> >> No. The info should be *only* in PCIDeviceInfo then. > > That would put a lot of code in pci config cycle path. A single array > mirroring the whole config space is much cleaner. > >>> Why do >>> we want stuff like vendor id in PCIDeviceInfo at all? Why can't >>> everyone just use pci_config_set/get calls? >> >> You can do nice stuff like printing vendor/device IDs in the '-device ?' >> list then. > > That should use pci functions as well. Hmm, do you mix up PCIDevice and PCIDeviceInfo structs? cheers, Gerd
On Mon, Feb 8, 2010 at 7:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: >> On 02/08/10 18:32, Michael S. Tsirkin wrote: >>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: >>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: >>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>>>>>> initialize header type register in pci generic code. >>>>>>>> >>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>>>>> >>>>>>> No objections here, I am assuming this will be followed >>>>>>> by patches removing header type init from bridges? >>>>>>> From qdev perspective, it is probably cleaner to make >>>>>>> multifunction bit a separate qdev property though, right? >>>>>> >>>>>> From a qdev perspective it would make *alot* of sense to move a bunch of >>>>>> pci config stuff (including, but not limited to header type) into >>>>>> PCIDeviceInfo. >>>>>> >>>>>> cheers, >>>>>> Gerd >>>>> >>>>> Actually - won't this make it possible to create broken configurations >>>>> by tweaking properties from command-line? >>>> >>>> Not as property, as struct element in PCIDeviceInfo. i.e. >>>> >>>> static PCIDeviceInfo e1000_info = { >>>> [ stuff which is here right now ] >>>> .vendor_id = PCI_VENDOR_ID_INTEL, >>>> .device_id = E1000_DEVID, >>>> .class = PCI_CLASS_NETWORK_ETHERNET, >>>> [ probably more stuff which makes sense ] >>>> } >>>> >>>> Then setup this in generic pci code instead of having each driver doing >>>> a bunch of pci_config_set_*() calls. >>>> >>>> cheers, >>>> Gerd >>> >>> We still end up with class, vendor etc duplicated in 2 places. >> >> No. The info should be *only* in PCIDeviceInfo then. > > That would put a lot of code in pci config cycle path. A single array > mirroring the whole config space is much cleaner. I'd suppose the arrays would remain as they are now, they just would be initialized (using the pci functions) based on PCIDeviceInfo structure. This would simplify the device code a lot. >>> Why do >>> we want stuff like vendor id in PCIDeviceInfo at all? Why can't >>> everyone just use pci_config_set/get calls? >> >> You can do nice stuff like printing vendor/device IDs in the '-device ?' >> list then. > > That should use pci functions as well. > >> cheers, >> Gerd >
On Mon, Feb 08, 2010 at 06:43:51PM +0100, Gerd Hoffmann wrote: > On 02/08/10 18:37, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: >>> On 02/08/10 18:32, Michael S. Tsirkin wrote: >>>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: >>>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: >>>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >>>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >>>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >>>>>>>>> initialize header type register in pci generic code. >>>>>>>>> >>>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >>>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >>>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>>>>>> >>>>>>>> No objections here, I am assuming this will be followed >>>>>>>> by patches removing header type init from bridges? >>>>>>>> From qdev perspective, it is probably cleaner to make >>>>>>>> multifunction bit a separate qdev property though, right? >>>>>>> >>>>>>> From a qdev perspective it would make *alot* of sense to move a bunch of >>>>>>> pci config stuff (including, but not limited to header type) into >>>>>>> PCIDeviceInfo. >>>>>>> >>>>>>> cheers, >>>>>>> Gerd >>>>>> >>>>>> Actually - won't this make it possible to create broken configurations >>>>>> by tweaking properties from command-line? >>>>> >>>>> Not as property, as struct element in PCIDeviceInfo. i.e. >>>>> >>>>> static PCIDeviceInfo e1000_info = { >>>>> [ stuff which is here right now ] >>>>> .vendor_id = PCI_VENDOR_ID_INTEL, >>>>> .device_id = E1000_DEVID, >>>>> .class = PCI_CLASS_NETWORK_ETHERNET, >>>>> [ probably more stuff which makes sense ] >>>>> } >>>>> >>>>> Then setup this in generic pci code instead of having each driver doing >>>>> a bunch of pci_config_set_*() calls. >>>>> >>>>> cheers, >>>>> Gerd >>>> >>>> We still end up with class, vendor etc duplicated in 2 places. >>> >>> No. The info should be *only* in PCIDeviceInfo then. >> >> That would put a lot of code in pci config cycle path. A single array >> mirroring the whole config space is much cleaner. >> >>>> Why do >>>> we want stuff like vendor id in PCIDeviceInfo at all? Why can't >>>> everyone just use pci_config_set/get calls? >>> >>> You can do nice stuff like printing vendor/device IDs in the '-device ?' >>> list then. >> >> That should use pci functions as well. > > Hmm, do you mix up PCIDevice and PCIDeviceInfo structs? > > cheers, > Gerd OK. OTOH, I don't think we need to out print header type in -device ? so what good is it there?
On Mon, Feb 08, 2010 at 07:56:27PM +0200, Blue Swirl wrote: > On Mon, Feb 8, 2010 at 7:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: > >> On 02/08/10 18:32, Michael S. Tsirkin wrote: > >>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: > >>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: > >>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: > >>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: > >>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: > >>>>>>>> initialize header type register in pci generic code. > >>>>>>>> > >>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> > >>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> > >>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> > >>>>>>> > >>>>>>> No objections here, I am assuming this will be followed > >>>>>>> by patches removing header type init from bridges? > >>>>>>> From qdev perspective, it is probably cleaner to make > >>>>>>> multifunction bit a separate qdev property though, right? > >>>>>> > >>>>>> From a qdev perspective it would make *alot* of sense to move a bunch of > >>>>>> pci config stuff (including, but not limited to header type) into > >>>>>> PCIDeviceInfo. > >>>>>> > >>>>>> cheers, > >>>>>> Gerd > >>>>> > >>>>> Actually - won't this make it possible to create broken configurations > >>>>> by tweaking properties from command-line? > >>>> > >>>> Not as property, as struct element in PCIDeviceInfo. i.e. > >>>> > >>>> static PCIDeviceInfo e1000_info = { > >>>> [ stuff which is here right now ] > >>>> .vendor_id = PCI_VENDOR_ID_INTEL, > >>>> .device_id = E1000_DEVID, > >>>> .class = PCI_CLASS_NETWORK_ETHERNET, > >>>> [ probably more stuff which makes sense ] > >>>> } > >>>> > >>>> Then setup this in generic pci code instead of having each driver doing > >>>> a bunch of pci_config_set_*() calls. > >>>> > >>>> cheers, > >>>> Gerd > >>> > >>> We still end up with class, vendor etc duplicated in 2 places. > >> > >> No. The info should be *only* in PCIDeviceInfo then. > > > > That would put a lot of code in pci config cycle path. A single array > > mirroring the whole config space is much cleaner. > > I'd suppose the arrays would remain as they are now, they just would > be initialized (using the pci functions) based on PCIDeviceInfo > structure. This still means we have two copies of same data and need to maintain code that keeps them in sync, even if that is called just at init time. > This would simplify the device code a lot. Well, I think pci_set_class(pci_dev, PCI_CLASS_NETWORK_ETHERNET) is simpler than .class = PCI_CLASS_NETWORK_ETHERNET and some magic that copies that to pci config. > >>> Why do > >>> we want stuff like vendor id in PCIDeviceInfo at all? Why can't > >>> everyone just use pci_config_set/get calls? > >> > >> You can do nice stuff like printing vendor/device IDs in the '-device ?' > >> list then. > > > > That should use pci functions as well. > > > >> cheers, > >> Gerd > >
On Mon, Feb 8, 2010 at 8:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Feb 08, 2010 at 07:56:27PM +0200, Blue Swirl wrote: >> On Mon, Feb 8, 2010 at 7:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: >> >> On 02/08/10 18:32, Michael S. Tsirkin wrote: >> >>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: >> >>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: >> >>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: >> >>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: >> >>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: >> >>>>>>>> initialize header type register in pci generic code. >> >>>>>>>> >> >>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> >> >>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> >> >>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >> >>>>>>> >> >>>>>>> No objections here, I am assuming this will be followed >> >>>>>>> by patches removing header type init from bridges? >> >>>>>>> From qdev perspective, it is probably cleaner to make >> >>>>>>> multifunction bit a separate qdev property though, right? >> >>>>>> >> >>>>>> From a qdev perspective it would make *alot* of sense to move a bunch of >> >>>>>> pci config stuff (including, but not limited to header type) into >> >>>>>> PCIDeviceInfo. >> >>>>>> >> >>>>>> cheers, >> >>>>>> Gerd >> >>>>> >> >>>>> Actually - won't this make it possible to create broken configurations >> >>>>> by tweaking properties from command-line? >> >>>> >> >>>> Not as property, as struct element in PCIDeviceInfo. i.e. >> >>>> >> >>>> static PCIDeviceInfo e1000_info = { >> >>>> [ stuff which is here right now ] >> >>>> .vendor_id = PCI_VENDOR_ID_INTEL, >> >>>> .device_id = E1000_DEVID, >> >>>> .class = PCI_CLASS_NETWORK_ETHERNET, >> >>>> [ probably more stuff which makes sense ] >> >>>> } >> >>>> >> >>>> Then setup this in generic pci code instead of having each driver doing >> >>>> a bunch of pci_config_set_*() calls. >> >>>> >> >>>> cheers, >> >>>> Gerd >> >>> >> >>> We still end up with class, vendor etc duplicated in 2 places. >> >> >> >> No. The info should be *only* in PCIDeviceInfo then. >> > >> > That would put a lot of code in pci config cycle path. A single array >> > mirroring the whole config space is much cleaner. >> >> I'd suppose the arrays would remain as they are now, they just would >> be initialized (using the pci functions) based on PCIDeviceInfo >> structure. > > This still means we have two copies of same data > and need to maintain code that keeps them in sync, > even if that is called just at init time. > >> This would simplify the device code a lot. > > Well, I think > > pci_set_class(pci_dev, PCI_CLASS_NETWORK_ETHERNET) > > is simpler than > > .class = PCI_CLASS_NETWORK_ETHERNET > > and some magic that copies that to pci config. The advantage is that if the code happens to change, only the magic (which is in one place) needs to be changed. Similar process has happened with savevm.
On Mon, Feb 08, 2010 at 09:32:54PM +0200, Blue Swirl wrote: > On Mon, Feb 8, 2010 at 8:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Feb 08, 2010 at 07:56:27PM +0200, Blue Swirl wrote: > >> On Mon, Feb 8, 2010 at 7:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: > >> >> On 02/08/10 18:32, Michael S. Tsirkin wrote: > >> >>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: > >> >>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: > >> >>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: > >> >>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: > >> >>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: > >> >>>>>>>> initialize header type register in pci generic code. > >> >>>>>>>> > >> >>>>>>>> Cc: Blue Swirl<blauwirbel@gmail.com> > >> >>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com> > >> >>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> > >> >>>>>>> > >> >>>>>>> No objections here, I am assuming this will be followed > >> >>>>>>> by patches removing header type init from bridges? > >> >>>>>>> From qdev perspective, it is probably cleaner to make > >> >>>>>>> multifunction bit a separate qdev property though, right? > >> >>>>>> > >> >>>>>> From a qdev perspective it would make *alot* of sense to move a bunch of > >> >>>>>> pci config stuff (including, but not limited to header type) into > >> >>>>>> PCIDeviceInfo. > >> >>>>>> > >> >>>>>> cheers, > >> >>>>>> Gerd > >> >>>>> > >> >>>>> Actually - won't this make it possible to create broken configurations > >> >>>>> by tweaking properties from command-line? > >> >>>> > >> >>>> Not as property, as struct element in PCIDeviceInfo. i.e. > >> >>>> > >> >>>> static PCIDeviceInfo e1000_info = { > >> >>>> [ stuff which is here right now ] > >> >>>> .vendor_id = PCI_VENDOR_ID_INTEL, > >> >>>> .device_id = E1000_DEVID, > >> >>>> .class = PCI_CLASS_NETWORK_ETHERNET, > >> >>>> [ probably more stuff which makes sense ] > >> >>>> } > >> >>>> > >> >>>> Then setup this in generic pci code instead of having each driver doing > >> >>>> a bunch of pci_config_set_*() calls. > >> >>>> > >> >>>> cheers, > >> >>>> Gerd > >> >>> > >> >>> We still end up with class, vendor etc duplicated in 2 places. > >> >> > >> >> No. The info should be *only* in PCIDeviceInfo then. > >> > > >> > That would put a lot of code in pci config cycle path. A single array > >> > mirroring the whole config space is much cleaner. > >> > >> I'd suppose the arrays would remain as they are now, they just would > >> be initialized (using the pci functions) based on PCIDeviceInfo > >> structure. > > > > This still means we have two copies of same data > > and need to maintain code that keeps them in sync, > > even if that is called just at init time. > > > >> This would simplify the device code a lot. > > > > Well, I think > > > > pci_set_class(pci_dev, PCI_CLASS_NETWORK_ETHERNET) > > > > is simpler than > > > > .class = PCI_CLASS_NETWORK_ETHERNET > > > > and some magic that copies that to pci config. > > The advantage is that if the code happens to change, only the magic > (which is in one place) needs to be changed. Similar process has > happened with savevm. Yes, one place is good. But magic is bad. What's wrong with pci_set_header type or something like that? Why do we need header type in qdev?
Hi, > This still means we have two copies of same data > and need to maintain code that keeps them in sync, > even if that is called just at init time. No. There is nothing to keep in sync. And there is no extra copy of data. Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). I'd like to see them replaced with PCIDeviceInfo->$field + setup in common code. The information that device $foo has vendor id 42 and device id 4711 (and other properties) just moves from code to data. It is static information, it should be static data. And having the information in a well defined place in a data structure instead of hidden somewhere in the ->init() code makes it alot easier to reuse the information for something else. That is the whole point. cheers, Gerd
On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote: > Hi, > >> This still means we have two copies of same data >> and need to maintain code that keeps them in sync, >> even if that is called just at init time. > > No. There is nothing to keep in sync. And there is no extra copy of data. > > Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). > I'd like to see them replaced with PCIDeviceInfo->$field + setup in > common code. The information that device $foo has vendor id 42 and > device id 4711 (and other properties) just moves from code to data. We still need it in config array which is read by guest. So that is two places. > It is static information, it should be static data. And having the > information in a well defined place in a data structure instead of > hidden somewhere in the ->init() code makes it alot easier to reuse the > information for something else. That is the whole point. > > cheers, > Gerd more important IMO is making code easier to follow.
On 02/08/2010 02:19 PM, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote: > >> Hi, >> >> >>> This still means we have two copies of same data >>> and need to maintain code that keeps them in sync, >>> even if that is called just at init time. >>> >> No. There is nothing to keep in sync. And there is no extra copy of data. >> >> Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). >> I'd like to see them replaced with PCIDeviceInfo->$field + setup in >> common code. The information that device $foo has vendor id 42 and >> device id 4711 (and other properties) just moves from code to data. >> > We still need it in config array which is read by guest. > So that is two places. > There's no reason that we couldn't make the config space read like all of the other spaces we support. IOW, instead of using an array to store the data, store each element in a structure, and have a big switch(). I'm not sure one's better than the other though TBH. I think just universally moving to a set of accessors that took a PCIDevice as an argument in the form of pci_device_set_vendor() would be a big improvement. Regards, Anthony Liguori
On Mon, Feb 08, 2010 at 02:32:26PM -0600, Anthony Liguori wrote: > On 02/08/2010 02:19 PM, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote: >> >>> Hi, >>> >>> >>>> This still means we have two copies of same data >>>> and need to maintain code that keeps them in sync, >>>> even if that is called just at init time. >>>> >>> No. There is nothing to keep in sync. And there is no extra copy of data. >>> >>> Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). >>> I'd like to see them replaced with PCIDeviceInfo->$field + setup in >>> common code. The information that device $foo has vendor id 42 and >>> device id 4711 (and other properties) just moves from code to data. >>> >> We still need it in config array which is read by guest. >> So that is two places. >> > > There's no reason that we couldn't make the config space read like all > of the other spaces we support. IOW, instead of using an array to store > the data, store each element in a structure, and have a big switch(). > > I'm not sure one's better than the other though TBH. Yea. So the solution that needs less code is better. > I think just universally moving to a set of accessors that took a > PCIDevice as an argument in the form of pci_device_set_vendor() would be > a big improvement. > > Regards, > > Anthony Liguori Not sure it's such a *big* improvement, but I won't object to that.
On 02/08/2010 02:34 PM, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 02:32:26PM -0600, Anthony Liguori wrote: > >> On 02/08/2010 02:19 PM, Michael S. Tsirkin wrote: >> >>> On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote: >>> >>> >>>> Hi, >>>> >>>> >>>> >>>>> This still means we have two copies of same data >>>>> and need to maintain code that keeps them in sync, >>>>> even if that is called just at init time. >>>>> >>>>> >>>> No. There is nothing to keep in sync. And there is no extra copy of data. >>>> >>>> Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). >>>> I'd like to see them replaced with PCIDeviceInfo->$field + setup in >>>> common code. The information that device $foo has vendor id 42 and >>>> device id 4711 (and other properties) just moves from code to data. >>>> >>>> >>> We still need it in config array which is read by guest. >>> So that is two places. >>> >>> >> There's no reason that we couldn't make the config space read like all >> of the other spaces we support. IOW, instead of using an array to store >> the data, store each element in a structure, and have a big switch(). >> >> I'm not sure one's better than the other though TBH. >> > Yea. So the solution that needs less code is better. > > >> I think just universally moving to a set of accessors that took a >> PCIDevice as an argument in the form of pci_device_set_vendor() would be >> a big improvement. >> >> Regards, >> >> Anthony Liguori >> > Not sure it's such a *big* improvement, but I won't object to that. > Sorry, but: versatile_pci.c: d->config[0x04] = 0x00; versatile_pci.c: d->config[0x05] = 0x00; versatile_pci.c: d->config[0x06] = 0x20; versatile_pci.c: d->config[0x07] = 0x02; To: pci_config_set_command(d, 0); pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); Is a huge improvement. I'm staring at a PCI config space diagram right now and I'm *still* not even sure I'm interpreting the versatile_pci code correctly :-) Having a nice structure with: { .command = 0, .status = PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ, } would be nice but I can live without it. Regards, Anthony Liguori
On Mon, Feb 08, 2010 at 02:54:24PM -0600, Anthony Liguori wrote: > On 02/08/2010 02:34 PM, Michael S. Tsirkin wrote: >> On Mon, Feb 08, 2010 at 02:32:26PM -0600, Anthony Liguori wrote: >> >>> On 02/08/2010 02:19 PM, Michael S. Tsirkin wrote: >>> >>>> On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote: >>>> >>>> >>>>> Hi, >>>>> >>>>> >>>>> >>>>>> This still means we have two copies of same data >>>>>> and need to maintain code that keeps them in sync, >>>>>> even if that is called just at init time. >>>>>> >>>>>> >>>>> No. There is nothing to keep in sync. And there is no extra copy of data. >>>>> >>>>> Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). >>>>> I'd like to see them replaced with PCIDeviceInfo->$field + setup in >>>>> common code. The information that device $foo has vendor id 42 and >>>>> device id 4711 (and other properties) just moves from code to data. >>>>> >>>>> >>>> We still need it in config array which is read by guest. >>>> So that is two places. >>>> >>>> >>> There's no reason that we couldn't make the config space read like all >>> of the other spaces we support. IOW, instead of using an array to store >>> the data, store each element in a structure, and have a big switch(). >>> >>> I'm not sure one's better than the other though TBH. >>> >> Yea. So the solution that needs less code is better. >> >> >>> I think just universally moving to a set of accessors that took a >>> PCIDevice as an argument in the form of pci_device_set_vendor() would be >>> a big improvement. >>> >>> Regards, >>> >>> Anthony Liguori >>> >> Not sure it's such a *big* improvement, but I won't object to that. >> > > Sorry, but: > > versatile_pci.c: d->config[0x04] = 0x00; > versatile_pci.c: d->config[0x05] = 0x00; > versatile_pci.c: d->config[0x06] = 0x20; > versatile_pci.c: d->config[0x07] = 0x02; > > To: > > pci_config_set_command(d, 0); > pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); > > Is a huge improvement. Yes but pci_set_word(d->config + PCI_COMMAND, 0); pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); is not much worse, and that API is already there. And advatage is it uses macros from linux which have higher chance to be correct than what we come up with. > I'm staring at a PCI config space diagram right > now and I'm *still* not even sure I'm interpreting the versatile_pci > code correctly :-) I spent time cleaning up devices, just did not get to bridges. What I did is write patches and verify that compiled code did not change at all. This guarantees no breakage. Care to volunteer to complete that work? Separately people that are familiar with device can clean it up. > Having a nice structure with: > > { .command = 0, > .status = PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ, > } > > would be nice but I can live without it. > > Regards, > > Anthony Liguori >
On 02/08/2010 03:01 PM, Michael S. Tsirkin wrote: >> Sorry, but: >> >> versatile_pci.c: d->config[0x04] = 0x00; >> versatile_pci.c: d->config[0x05] = 0x00; >> versatile_pci.c: d->config[0x06] = 0x20; >> versatile_pci.c: d->config[0x07] = 0x02; >> >> To: >> >> pci_config_set_command(d, 0); >> pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); >> >> Is a huge improvement. >> > > Yes but > > pci_set_word(d->config + PCI_COMMAND, 0); > pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); > > is not much worse, and that API is already there. > And advatage is it uses macros from linux which have > higher chance to be correct than what we come up with. > Oh, pci_set_word() is certainly an improvement. Personally, I prefer passing the PCIDevice as a context and adding individual accessors but anything is better than open coded config. >> I'm staring at a PCI config space diagram right >> now and I'm *still* not even sure I'm interpreting the versatile_pci >> code correctly :-) >> > I spent time cleaning up devices, just did not get to bridges. > What I did is write patches and verify that compiled code > did not change at all. This guarantees no breakage. > Care to volunteer to complete that work? > Separately people that are familiar with device can clean it up. > It's on my radar but I've got another PCI series in flight. I've got a branch pci-cleanup on staging that's a work in progress for adding a proper region API along with PCI memory accessors. Once I find a little more time to finish converting VGA devices, I'll post. Regards, Anthony Liguori
On Mon, Feb 08, 2010 at 03:56:29PM -0600, Anthony Liguori wrote: > On 02/08/2010 03:01 PM, Michael S. Tsirkin wrote: >>> Sorry, but: >>> >>> versatile_pci.c: d->config[0x04] = 0x00; >>> versatile_pci.c: d->config[0x05] = 0x00; >>> versatile_pci.c: d->config[0x06] = 0x20; >>> versatile_pci.c: d->config[0x07] = 0x02; >>> >>> To: >>> >>> pci_config_set_command(d, 0); >>> pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); >>> >>> Is a huge improvement. >>> >> >> Yes but >> >> pci_set_word(d->config + PCI_COMMAND, 0); >> pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); >> >> is not much worse, and that API is already there. >> And advatage is it uses macros from linux which have >> higher chance to be correct than what we come up with. >> > > Oh, pci_set_word() is certainly an improvement. Personally, I prefer > passing the PCIDevice as a context and adding individual accessors but > anything is better than open coded config. > >>> I'm staring at a PCI config space diagram right >>> now and I'm *still* not even sure I'm interpreting the versatile_pci >>> code correctly :-) >>> >> I spent time cleaning up devices, just did not get to bridges. >> What I did is write patches and verify that compiled code >> did not change at all. This guarantees no breakage. >> Care to volunteer to complete that work? >> Separately people that are familiar with device can clean it up. >> > > It's on my radar Just converted versatile_pci, with some nudging I might do others :) > but I've got another PCI series in flight. I've got a > branch pci-cleanup on staging that's a work in progress for adding a > proper region API along with PCI memory accessors. > > Once I find a little more time to finish converting VGA devices, I'll post. > > Regards, > > Anthony Liguori Great! That's required for proper spec compliance. VGA devices are definitely the main pain point here.
On 02/08/2010 03:58 PM, Michael S. Tsirkin wrote: > > Just converted versatile_pci, with some nudging I might do others :) > *nudge* :-) >> but I've got another PCI series in flight. I've got a >> branch pci-cleanup on staging that's a work in progress for adding a >> proper region API along with PCI memory accessors. >> >> Once I find a little more time to finish converting VGA devices, I'll post. >> >> Regards, >> >> Anthony Liguori >> > Great! That's required for proper spec compliance. > VGA devices are definitely the main pain point here. > The KVM LFB optimization makes it challenging. Need to find some time to figure out the best way to do it. Suggestions are certainly appreciated. Regards, Anthony Liguori
On Mon, Feb 08, 2010 at 06:27:53PM +0200, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: > > On 02/08/10 11:17, Michael S. Tsirkin wrote: > >> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wrote: > >>> initialize header type register in pci generic code. > >>> > >>> Cc: Blue Swirl<blauwirbel@gmail.com> > >>> Cc: "Michael S. Tsirkin"<mst@redhat.com> > >>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> > >> > >> No objections here, I am assuming this will be followed > >> by patches removing header type init from bridges? > >> From qdev perspective, it is probably cleaner to make > >> multifunction bit a separate qdev property though, right? > > > > From a qdev perspective it would make *alot* of sense to move a bunch of > > pci config stuff (including, but not limited to header type) into > > PCIDeviceInfo. > > > > cheers, > > Gerd > > Actually - won't this make it possible to create broken configurations > by tweaking properties from command-line? > And generally, it sounds bad to have header type duplicated in qdev > and in config. Why do we want it in qdev? > > Isaku Yamahata, could you please clarify? My idea behind the patch is that making configuration space initialization a sort of table driven (in long term?) like Gerd and Blue already stated it. If there is a consensus to go for it, I'm willing to create patches. However it seems that we haven't reached the conclusion yet. I'm not sure it's worth while to allow user to tweak config space.
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >> This still means we have two copies of same data >> and need to maintain code that keeps them in sync, >> even if that is called just at init time. > > No. There is nothing to keep in sync. And there is no extra copy of data. > > Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). > I'd like to see them replaced with PCIDeviceInfo->$field + setup in > common code. The information that device $foo has vendor id 42 and > device id 4711 (and other properties) just moves from code to data. > > It is static information, it should be static data. And having the > information in a well defined place in a data structure instead of > hidden somewhere in the ->init() code makes it alot easier to reuse > the information for something else. That is the whole point. ACK
On Mon, Feb 08, 2010 at 04:25:51PM -0600, Anthony Liguori wrote: > On 02/08/2010 03:58 PM, Michael S. Tsirkin wrote: >> >> Just converted versatile_pci, with some nudging I might do others :) >> > > *nudge* :-) which files do you care most about? >>> but I've got another PCI series in flight. I've got a >>> branch pci-cleanup on staging that's a work in progress for adding a >>> proper region API along with PCI memory accessors. >>> >>> Once I find a little more time to finish converting VGA devices, I'll post. >>> >>> Regards, >>> >>> Anthony Liguori >>> >> Great! That's required for proper spec compliance. >> VGA devices are definitely the main pain point here. >> > > The KVM LFB optimization makes it challenging. Need to find some time > to figure out the best way to do it. Suggestions are certainly > appreciated. > > Regards, > > Anthony Liguori >
diff --git a/hw/pci.c b/hw/pci.c index eb2043e..7b055b4 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -603,6 +603,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, pci_dev->irq_state = 0; pci_config_alloc(pci_dev); + pci_dev->config[PCI_HEADER_TYPE] = header_type; header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION; if (header_type == PCI_HEADER_TYPE_NORMAL) { pci_set_default_subsystem_id(pci_dev);
initialize header type register in pci generic code. Cc: Blue Swirl <blauwirbel@gmail.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)