| Message ID | 1262862925-5205-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote: > The numerical value was wrong (0x2800 instead of 0x0280) > which indeed did not make sense. Aha! conversion to symbolic names paying off. > Signed-off-by: Stefan Weil <weil@mail.berlios.de> Applied, thanks. > --- > 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. > /* TODO: this is the default, do not override. */ > -- > 1.6.5
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
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
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: >>> ... >>>> --- >>>> 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. > No. The current API takes any uint8_t pointer to read or write a value. This is not safe. 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. And even if your compiler does not complain, it is wrong but still safe, because the code will only access the PCI configuration data. >> * 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); > >> * 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. 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
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: > >>> ... > >>>> --- > >>>> 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. > > > > No. The current API takes any uint8_t pointer to read or write > a value. This is not safe. Why isn't it? > 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. > 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. > >> * 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. > > > >> * 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. > 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 > >
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 >>
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) ? > > > >> 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. Example above tries to show that usng pointers in C is bad, switching to indexes all over is proposed as a solution. Oh well .. but I am surpised you bring up type safety as an argument, is is exactly the reason to use pointers. > > > >>>> * 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, ... Oh no. > > > >> 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 Sorry, I dislike the API you propose and I do not think it will help avoid bugs.
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
Anthony Liguori schrieb: > 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) >> ? Only code which reduces precision would result in a warning: pci_set_word(pci_dev, 0x4711, PCI_STATUS) >> > > 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 This is a good solution for the standard configuration entries, so most code could use such calls if they were complete. For entries above offset 0x40, I'm afraid that it won't work (neither with individual functions nor with a C structure). Regards, Stefan Weil
On 01/11/2010 04:10 PM, Stefan Weil wrote: >> 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 >> > This is a good solution for the standard configuration entries, > so most code could use such calls if they were complete. > > For entries above offset 0x40, I'm afraid that it won't work > (neither with individual functions nor with a C structure). > Which is fine. Device specific entries in the config space are uncommon compared to the standard entries. The same model could apply (devices write their own wrapper functions). The problem with the set_word interface is that quite a lot of important config space fields are stored in a byte or less. Regards, Anthony Liguori > Regards, > > Stefan Weil > >
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. */ - 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); /* TODO: this is the default, do not override. */
The numerical value was wrong (0x2800 instead of 0x0280) which indeed did not make sense. Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- hw/eepro100.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)