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

[Anthony Liguori <anthony@codemonkey.ws>]

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

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