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