[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Stefan Weil <weil@mail.berlios.de>]

Anthony Liguori schrieb:
> On 01/11/2010 02:30 PM, Michael S. Tsirkin wrote:
>> On Mon, Jan 11, 2010 at 09:18:51PM +0100, Stefan Weil wrote:
>>   
>>> Michael S. Tsirkin schrieb:
>>>     
>>>> On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote:
>>>>       
>>>>> Michael S. Tsirkin schrieb:
>>>>>         
>>>>>> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
>>>>>>           
>>>>>>> Michael S. Tsirkin schrieb:
>>>>>>>             
>>>>>>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
>>>>>>>> ...
>>>>>>>>               
>>>>>>>>> - PCI_CONFIG_16(PCI_STATUS,
>>>>>>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>>>>>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
>>>>>>>>> PCI_STATUS_FAST_BACK);
>>>>>>>>> /* PCI Revision ID */
>>>>>>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
>>>>>>>>>                  
>>>>>>>> BTW if you are not afraid of churn, there's no reason
>>>>>>>> for PCI_CONFIG_8 and friends anymore, because pci.h
>>>>>>>> has much nicer pci_set_byte etc.
>>>>>>>>                
>>>>>>> Hello Michael,
>>>>>>>
>>>>>>> I already noticed pci_set_byte, pci_set_word, pci_set_long and
>>>>>>> the corresponding pci_get_xxx functions and thought about using
>>>>>>> them.
>>>>>>>
>>>>>>> I did not start it because I want to suggest a different API
>>>>>>> for use in PCI device emulations:
>>>>>>>
>>>>>>> instead of
>>>>>>>
>>>>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>>>>>>
>>>>>>> it would be better to call
>>>>>>>
>>>>>>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>>>>
>>>>>>>
>>>>>>> The prototypes would look like this:
>>>>>>>
>>>>>>> /* Set PCI config value. */
>>>>>>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>>>>
>>>>>>> /* Set PCI cmask value. */
>>>>>>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t
>>>>>>> val);
>>>>>>>
>>>>>>> /* Set PCI wmask value. */
>>>>>>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t
>>>>>>> val);
>>>>>>>
>>>>>>> What are the advantages?
>>>>>>> * strict type checking (the old API takes any uint8_t *)
>>>>>>>              
>>>>>> So IMO it's easier to make mistakes with your proposed API
>>>>>> because if
>>>>>> you confuse offset and value compiler does not complain. More
>>>>>> importantly, if you want to pass some other uint8_t pointer to e.g.
>>>>>> pci_set_word, you can *and nothing unexpected will happen*
>>>>>> it will set the word to the given value. So the current
>>>>>> API is more type safe than what you propose.
>>>>>>
>>>>>>            
>>>>> No. The current API takes any uint8_t pointer to read or write
>>>>> a value. This is not safe.
>>>>>          
>>>> Why isn't it?
>>>>        
>>> It is not safe, because it allows programmers to write silly code
>>> like these examples:
>>>
>>> pci_set_word(&gen_opc_cc_op[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>>      
>> This might be valid and useful for all I know.
>>
>>   
>>> pci_set_word(&pci_conf[UINT32_MAX], PCI_STATUS_CAP_LIST);
>>>
>>> for (i = 0; i<  sizeof(pci_conf); i++) {
>>>      pci_set_long(&pci_conf[i], 0);
>>> }
>>>
>>> All three will result in runtime failures which can be very
>>> difficult to detect.
>>>      
>> In theory. In practice I expect pci_set_word(pci_dev, 0x0, PCI_STATUS)
>> to be much more common with your proposed API. Just look how
>> common swapping memset parameters is.
>>
>>   
>>>>       
>>>>> The proposed API only takes a PCIDevice pointer
>>>>> and reads or writes only configuration (or cmask or
>>>>> wmask) values. Yes, you can take offset for value.
>>>>> If you are lucky and value is an uint16_t or uint32_t,
>>>>> your compiler will complain.
>>>>>          
>>>> Such a compiler will also complain over most of qemu code.
>>>>        
>>> Yes, but it's good to see that QEMU's code is
>>> improving.
>>>
>>> By the way, such a compiler is gcc when called with
>>> -Wconversion, so it is easy to see how much code
>>> is affected.
>>>      
>> Didn't try it. So it will warn on
>> pci_set_word(pci_dev, 0x0, PCI_STATUS)
>> but not
>> pci_set_word(pci_dev, PCI_STATUS, 0x0)
>> ?

Only code which reduces precision would result in a warning:
pci_set_word(pci_dev, 0x4711, PCI_STATUS)

>>    
>
> I haven't read this whole thread, but I really prefer things like
>
> pci_set_vendor_id(pci_dev, XXXX);
>
> A close alternative, would be some refactoring to allow PCI config
> space to be represented as a C structure.  Gerd had some patches at
> one point for this.
>
> Regards,
>
> Anthony Liguori

This is a good solution for the standard configuration entries,
so most code could use such calls if they were complete.

For entries above offset 0x40, I'm afraid that it won't work
(neither with individual functions nor with a C structure).

Regards,

Stefan Weil