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

[Stefan Weil]

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);
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.

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

>
>> And even if your compiler
>> does not complain, it is wrong but still safe, because
>> the code will only access the PCI configuration data.
>>
>
> Correct and safe beats wrong and safe every time.

See example above.

>
>>>> * many other pci_* functions also have a first parameter of type
>>>> PCIDevice
>>> So what would make sense IMO is higer level abstraction,
>>> for example similar to what we have with capabilities
>>> and msix, I think we could have something like this
>>> for e.g. power management.
>>>
>>> For low-level bit tweaking, the advantages of current API is that same
>>> thing can be used to set wmask, cmask, config itself, and whatever else
>>> we will come up with.
>> The low level API can be used where low level is
>> adequate: in pci.c for example.
>>
>> To implement emulated PCI devices, a more robust API
>> would be better. Think of the number of devices which
>> are still missing, think of people who want to write
>> a new PCI device emulation for QEMU without being
>> a QEMU expert.
>>
>>>> * calls look nicer (at least in my opinion)
>>> What I value is the fact that it's obvious which
>>> data is changed.
>> Here there is no difference between current and
>> proposed API:
>>
>> old: pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>> new: pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>
>> Every function call hides what happens. If you really wanted
>> to see which data is changed, you would have to write
>>
>> *(uint16_t *)&pci_conf[PCI_STATUS] = cpu_to_le16(PCI_STATUS_CAP_LIST);
>
> That's what we used to have, and it's not all bad, but very verbose and
> ugly.

Yes. I also prefer a function API.

>
>>>> * strict range checking (offset is limited to 0...255, additional
>>>> assertions possible - the old API is unsafe because it just takes
>>>> a pointer)
>>> I don't think we want to add return status, so there wouldn't
>>> be a benefit to range checking as we can't act on it.
>>> Anyway, it's very unusual to use anything but a constant
>>> as an offset, so range errors are very uncommon.
>> There is an implicit range checking in the proposed
>> API because the offset is uint8_t, so it cannot
>> exceed the range which is valid for configuration
>> offsets.
>
> Oh, btw, this is wrong on pci express.

Great, so PCI express devices should have their own
set of functions with the correct runtime checks:

pci_e_set_config, ...

>
>> A more elaborated check could require that
>> configuration byte values are only addressed
>> using pci_set_byte (not pci_set_long)
>> and raise a fatal runtime error otherwise.
>>
>> Runtime checks without return values
>> are well established in QEMU's code,
>> and they are very useful for code writers.
>>
>>>> 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
>>