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

[Michael S. Tsirkin]

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:
> > ...
> >> ---
> >> 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 *)

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.

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

> * calls look nicer (at least in my opinion)

What I value is the fact that it's obvious which
data is changed.

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

> 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



-- 
MST