[Qemu-devel] [PATCH] pci: initialize header type register.

[Qemu-devel] [PATCH] pci: initialize header type register.

[Isaku Yamahata]

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(-)

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

[Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Gerd Hoffmann]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Gerd Hoffmann]

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

[Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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?


-- 
MT

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Gerd Hoffmann]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Gerd Hoffmann]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Gerd Hoffmann]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Blue Swirl]

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
>

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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
> >

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Blue Swirl]

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.

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Gerd Hoffmann]

   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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Anthony Liguori]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Anthony Liguori]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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
>

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Anthony Liguori]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Isaku Yamahata]

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.
-- 
yamahata

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Markus Armbruster]

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

Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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
>