[Qemu-devel] [RFC] API change for pci_set_word and related functions (was Re: [PATCH] eepro100: Fix initial value for PCI_STATUS)

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

Michael S. Tsirkin schrieb:
> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
> ...
>> ---
>> hw/eepro100.c | 4 +---
>> 1 files changed, 1 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 336ca49..a21c984 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
>> /* TODO: this is the default, do not override. */
>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>> /* PCI Status */
>> - /* TODO: this seems to make no sense. */
>> /* TODO: Value at RST# should be 0. */
>
> So this second todo can go too. I've removed it in my tree.
>
>> - 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 *)
* many other pci_* functions also have a first parameter of type PCIDevice
* calls look nicer (at least in my opinion)
* strict range checking (offset is limited to 0...255, additional
  assertions possible - the old API is unsafe because it just takes
  a pointer)

The functions are inline, so the resulting code won't differ.

Instead of _byte, _word and _long I personally prefer something
like _8, _16, _32 because _word and _long need interpretation.
But this is only a matter of taste - the API change is more important.


Regards,

Stefan Weil