Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
Dear Jason,
You were right both about the AIT and the FFMT! On a physical card,
the reserved bits in both of them read as zeros, even if they were set
to ones right before the reading.
V2 of the patches, with all the remarks addressed, is on the way.
Thanks again for your review.
Leonid.
___
On Fri, Oct 23, 2015 at 6:10 AM, Jason Wang wrote:
>
>
> On 10/22/2015 10:05 PM, Leonid Bloch wrote:
>> On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang wrote:
>>>
>>>
>>> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
Hi Jason, thanks for the review!
On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote:
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>> These registers appear in Intel's specs, but were not implemented.
>> These registers are now implemented trivially, i.e. they are initiated
>> with zero values, and if they are RW, they can be written or read by the
>> driver, or read only if they are R (essentially retaining their zero
>> values). For these registers no other procedures are performed.
>>
>> The registers implemented here are:
>>
>> Transmit:
>> RW: AIT
>>
>> Management:
>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
> My version of DSM (Revision) said WUS is read only.
This seems to be a typo in the specs. We also have the specs where it
is said that WUS's read only, but exactly two lines below it - writing
to it is mentioned. Additionally, in the specs for newer Intel's
devices, where the offset and the functionality of WUS are exactly the
same, it is written that WUS is RW.
>>> Ok.
>>>
>> Diagnostic:
>> RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
> For those diagnostic register, isn't it better to warn the incomplete
> emulation instead of trying to give all zero values silently? I suspect
> this can make diagnostic software think the device is malfunction?
That's a good point. What do you think about keeping the zero values,
but printing out a warning (via DBGOUT) on each read/write attempt?
>>> This is fine for me.
>>>
>> Statistic:
>> RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
> TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
Yes, they are. Thanks, I'll reword.
>> R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
>> LATECOL MCC COLCDC TNCRS SEC CEXTERR RLEC
>> XONRXC
>> XONTXC XOFFRXC XOFFTXC
>>
>> Signed-off-by: Leonid Bloch
>> Signed-off-by: Dmitry Fleytman
>> ---
>> hw/net/e1000.c | 52
>> +---
>> hw/net/e1000_regs.h | 6 ++
>> 2 files changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 6d57663..6f754ac 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -168,7 +168,17 @@ enum {
>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
>> defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
>> -defreg(ITR),
>> +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
>> +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
>> +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
>> +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
>> +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
>> +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
>> +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
>> +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
>> +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
>> +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
>> +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
>> };
>>
>> static void
>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
>> }
>>
>> static uint32_t
>> +mac_low11_read(E1000State *s, int index)
>> +{
>> +return s->mac_reg[index] & 0x7ff;
>> +}
>> +
>> +static uint32_t
>> +mac_low13_read(E1000State *s, int index)
>> +{
>> +return s->mac_reg[index] & 0x1fff;
>> +}
>> +
>> +static uint32_t
>> mac_icr_read(E1000State *s, int index)
>> {
>> uint32_t ret = s->mac_reg[ICR];
>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State
>> *, int) = {
>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
>> getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
>> getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
>> -getreg(
Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
On 10/22/2015 10:05 PM, Leonid Bloch wrote:
> On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang wrote:
>>
>>
>> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
>>> Hi Jason, thanks for the review!
>>>
>>> On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote:
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> These registers appear in Intel's specs, but were not implemented.
> These registers are now implemented trivially, i.e. they are initiated
> with zero values, and if they are RW, they can be written or read by the
> driver, or read only if they are R (essentially retaining their zero
> values). For these registers no other procedures are performed.
>
> The registers implemented here are:
>
> Transmit:
> RW: AIT
>
> Management:
> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
My version of DSM (Revision) said WUS is read only.
>>> This seems to be a typo in the specs. We also have the specs where it
>>> is said that WUS's read only, but exactly two lines below it - writing
>>> to it is mentioned. Additionally, in the specs for newer Intel's
>>> devices, where the offset and the functionality of WUS are exactly the
>>> same, it is written that WUS is RW.
>> Ok.
>>
> Diagnostic:
> RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
For those diagnostic register, isn't it better to warn the incomplete
emulation instead of trying to give all zero values silently? I suspect
this can make diagnostic software think the device is malfunction?
>>> That's a good point. What do you think about keeping the zero values,
>>> but printing out a warning (via DBGOUT) on each read/write attempt?
>> This is fine for me.
>>
> Statistic:
> RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
>>> Yes, they are. Thanks, I'll reword.
> R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
> XONTXC XOFFRXC XOFFTXC
>
> Signed-off-by: Leonid Bloch
> Signed-off-by: Dmitry Fleytman
> ---
> hw/net/e1000.c | 52
> +---
> hw/net/e1000_regs.h | 6 ++
> 2 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 6d57663..6f754ac 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -168,7 +168,17 @@ enum {
> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
> -defreg(ITR),
> +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
> +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
> +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
> +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
> +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
> +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> };
>
> static void
> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> }
>
> static uint32_t
> +mac_low11_read(E1000State *s, int index)
> +{
> +return s->mac_reg[index] & 0x7ff;
> +}
> +
> +static uint32_t
> +mac_low13_read(E1000State *s, int index)
> +{
> +return s->mac_reg[index] & 0x1fff;
> +}
> +
> +static uint32_t
> mac_icr_read(E1000State *s, int index)
> {
> uint32_t ret = s->mac_reg[ICR];
> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *,
> int) = {
> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
> getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
> -getreg(TADV), getreg(ITR),
> +getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
> +getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
For AIT should we use low16_read() ?
>>> Contrary to registers where lowXX_read() is used, for AIT the specs
>>> say that the higher bits should be written with 0b, and not "read as
>>> 0b". That's my reasoning for that. What do you think?
>> I think it's better to test this be
Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang wrote:
>
>
>
> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
> > Hi Jason, thanks for the review!
> >
> > On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote:
> >>
> >>
> >> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> >>> These registers appear in Intel's specs, but were not implemented.
> >>> These registers are now implemented trivially, i.e. they are initiated
> >>> with zero values, and if they are RW, they can be written or read by the
> >>> driver, or read only if they are R (essentially retaining their zero
> >>> values). For these registers no other procedures are performed.
> >>>
> >>> The registers implemented here are:
> >>>
> >>> Transmit:
> >>> RW: AIT
> >>>
> >>> Management:
> >>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
> >> My version of DSM (Revision) said WUS is read only.
> > This seems to be a typo in the specs. We also have the specs where it
> > is said that WUS's read only, but exactly two lines below it - writing
> > to it is mentioned. Additionally, in the specs for newer Intel's
> > devices, where the offset and the functionality of WUS are exactly the
> > same, it is written that WUS is RW.
>
> Ok.
>
> >>> Diagnostic:
> >>> RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
> >> For those diagnostic register, isn't it better to warn the incomplete
> >> emulation instead of trying to give all zero values silently? I suspect
> >> this can make diagnostic software think the device is malfunction?
> > That's a good point. What do you think about keeping the zero values,
> > but printing out a warning (via DBGOUT) on each read/write attempt?
>
> This is fine for me.
>
> >>> Statistic:
> >>> RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
> >> TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
> > Yes, they are. Thanks, I'll reword.
> >>> R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> >>> LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
> >>> XONTXC XOFFRXC XOFFTXC
> >>>
> >>> Signed-off-by: Leonid Bloch
> >>> Signed-off-by: Dmitry Fleytman
> >>> ---
> >>> hw/net/e1000.c | 52
> >>> +---
> >>> hw/net/e1000_regs.h | 6 ++
> >>> 2 files changed, 55 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> >>> index 6d57663..6f754ac 100644
> >>> --- a/hw/net/e1000.c
> >>> +++ b/hw/net/e1000.c
> >>> @@ -168,7 +168,17 @@ enum {
> >>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> >>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> >>> defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
> >>> -defreg(ITR),
> >>> +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
> >>> +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> >>> +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> >>> +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
> >>> +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
> >>> +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
> >>> +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
> >>> +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> >>> +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> >>> +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> >>> +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> >>> };
> >>>
> >>> static void
> >>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> >>> }
> >>>
> >>> static uint32_t
> >>> +mac_low11_read(E1000State *s, int index)
> >>> +{
> >>> +return s->mac_reg[index] & 0x7ff;
> >>> +}
> >>> +
> >>> +static uint32_t
> >>> +mac_low13_read(E1000State *s, int index)
> >>> +{
> >>> +return s->mac_reg[index] & 0x1fff;
> >>> +}
> >>> +
> >>> +static uint32_t
> >>> mac_icr_read(E1000State *s, int index)
> >>> {
> >>> uint32_t ret = s->mac_reg[ICR];
> >>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *,
> >>> int) = {
> >>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> >>> getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
> >>> getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
> >>> -getreg(TADV), getreg(ITR),
> >>> +getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
> >>> +getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
> >> For AIT should we use low16_read() ?
> > Contrary to registers where lowXX_read() is used, for AIT the specs
> > say that the higher bits should be written with 0b, and not "read as
> > 0b". That's my reasoning for that. What do you think?
>
> I think it's better to test this behavior on real card.
What can be tested ex
Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
On 10/21/2015 05:13 PM, Leonid Bloch wrote:
> Hi Jason, thanks for the review!
>
> On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote:
>>
>>
>> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>>> These registers appear in Intel's specs, but were not implemented.
>>> These registers are now implemented trivially, i.e. they are initiated
>>> with zero values, and if they are RW, they can be written or read by the
>>> driver, or read only if they are R (essentially retaining their zero
>>> values). For these registers no other procedures are performed.
>>>
>>> The registers implemented here are:
>>>
>>> Transmit:
>>> RW: AIT
>>>
>>> Management:
>>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
>> My version of DSM (Revision) said WUS is read only.
> This seems to be a typo in the specs. We also have the specs where it
> is said that WUS's read only, but exactly two lines below it - writing
> to it is mentioned. Additionally, in the specs for newer Intel's
> devices, where the offset and the functionality of WUS are exactly the
> same, it is written that WUS is RW.
Ok.
>>> Diagnostic:
>>> RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
>> For those diagnostic register, isn't it better to warn the incomplete
>> emulation instead of trying to give all zero values silently? I suspect
>> this can make diagnostic software think the device is malfunction?
> That's a good point. What do you think about keeping the zero values,
> but printing out a warning (via DBGOUT) on each read/write attempt?
This is fine for me.
>>> Statistic:
>>> RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
>> TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
> Yes, they are. Thanks, I'll reword.
>>> R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
>>> LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
>>> XONTXC XOFFRXC XOFFTXC
>>>
>>> Signed-off-by: Leonid Bloch
>>> Signed-off-by: Dmitry Fleytman
>>> ---
>>> hw/net/e1000.c | 52
>>> +---
>>> hw/net/e1000_regs.h | 6 ++
>>> 2 files changed, 55 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>> index 6d57663..6f754ac 100644
>>> --- a/hw/net/e1000.c
>>> +++ b/hw/net/e1000.c
>>> @@ -168,7 +168,17 @@ enum {
>>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
>>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
>>> defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
>>> -defreg(ITR),
>>> +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
>>> +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
>>> +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
>>> +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
>>> +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
>>> +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
>>> +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
>>> +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
>>> +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
>>> +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
>>> +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
>>> };
>>>
>>> static void
>>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
>>> }
>>>
>>> static uint32_t
>>> +mac_low11_read(E1000State *s, int index)
>>> +{
>>> +return s->mac_reg[index] & 0x7ff;
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low13_read(E1000State *s, int index)
>>> +{
>>> +return s->mac_reg[index] & 0x1fff;
>>> +}
>>> +
>>> +static uint32_t
>>> mac_icr_read(E1000State *s, int index)
>>> {
>>> uint32_t ret = s->mac_reg[ICR];
>>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *,
>>> int) = {
>>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
>>> getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
>>> getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
>>> -getreg(TADV), getreg(ITR),
>>> +getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
>>> +getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
>> For AIT should we use low16_read() ?
> Contrary to registers where lowXX_read() is used, for AIT the specs
> say that the higher bits should be written with 0b, and not "read as
> 0b". That's my reasoning for that. What do you think?
I think it's better to test this behavior on real card.
>> And low4_read() for FFMT?
> Why? The specs say nothing about the reserved bits there...
If I read the spec (Revision 3.7 13.6.10) correctly, only low 4 bits
were used for byte mask.
[...]
Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
Hi Jason, thanks for the review!
On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote:
>
>
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> > These registers appear in Intel's specs, but were not implemented.
> > These registers are now implemented trivially, i.e. they are initiated
> > with zero values, and if they are RW, they can be written or read by the
> > driver, or read only if they are R (essentially retaining their zero
> > values). For these registers no other procedures are performed.
> >
> > The registers implemented here are:
> >
> > Transmit:
> > RW: AIT
> >
> > Management:
> > RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
>
> My version of DSM (Revision) said WUS is read only.
This seems to be a typo in the specs. We also have the specs where it
is said that WUS's read only, but exactly two lines below it - writing
to it is mentioned. Additionally, in the specs for newer Intel's
devices, where the offset and the functionality of WUS are exactly the
same, it is written that WUS is RW.
>
> >
> > Diagnostic:
> > RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
>
> For those diagnostic register, isn't it better to warn the incomplete
> emulation instead of trying to give all zero values silently? I suspect
> this can make diagnostic software think the device is malfunction?
That's a good point. What do you think about keeping the zero values,
but printing out a warning (via DBGOUT) on each read/write attempt?
>
> >
> > Statistic:
> > RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
>
> TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
Yes, they are. Thanks, I'll reword.
>
> > R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> > LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
> > XONTXC XOFFRXC XOFFTXC
> >
> > Signed-off-by: Leonid Bloch
> > Signed-off-by: Dmitry Fleytman
> > ---
> > hw/net/e1000.c | 52
> > +---
> > hw/net/e1000_regs.h | 6 ++
> > 2 files changed, 55 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 6d57663..6f754ac 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -168,7 +168,17 @@ enum {
> > defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> > defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> > defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
> > -defreg(ITR),
> > +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
> > +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> > +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> > +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
> > +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
> > +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
> > +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
> > +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> > +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> > +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> > +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> > };
> >
> > static void
> > @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> > }
> >
> > static uint32_t
> > +mac_low11_read(E1000State *s, int index)
> > +{
> > +return s->mac_reg[index] & 0x7ff;
> > +}
> > +
> > +static uint32_t
> > +mac_low13_read(E1000State *s, int index)
> > +{
> > +return s->mac_reg[index] & 0x1fff;
> > +}
> > +
> > +static uint32_t
> > mac_icr_read(E1000State *s, int index)
> > {
> > uint32_t ret = s->mac_reg[ICR];
> > @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *,
> > int) = {
> > getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> > getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
> > getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
> > -getreg(TADV), getreg(ITR),
> > +getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
> > +getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
>
> For AIT should we use low16_read() ?
Contrary to registers where lowXX_read() is used, for AIT the specs
say that the higher bits should be written with 0b, and not "read as
0b". That's my reasoning for that. What do you think?
>
> And low4_read() for FFMT?
Why? The specs say nothing about the reserved bits there...
>
> > +getreg(ECOL), getreg(MCC), getreg(LATECOL), getreg(COLC),
> > +getreg(DC), getreg(TNCRS),getreg(SEC), getreg(CEXTERR),
> > +getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
> > +getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
> > +getreg(TSCTFC), ge
Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> These registers appear in Intel's specs, but were not implemented.
> These registers are now implemented trivially, i.e. they are initiated
> with zero values, and if they are RW, they can be written or read by the
> driver, or read only if they are R (essentially retaining their zero
> values). For these registers no other procedures are performed.
>
> The registers implemented here are:
>
> Transmit:
> RW: AIT
>
> Management:
> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
My version of DSM (Revision) said WUS is read only.
>
> Diagnostic:
> RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
For those diagnostic register, isn't it better to warn the incomplete
emulation instead of trying to give all zero values silently? I suspect
this can make diagnostic software think the device is malfunction?
>
> Statistic:
> RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
> R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
> XONTXC XOFFRXC XOFFTXC
>
> Signed-off-by: Leonid Bloch
> Signed-off-by: Dmitry Fleytman
> ---
> hw/net/e1000.c | 52 +---
> hw/net/e1000_regs.h | 6 ++
> 2 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 6d57663..6f754ac 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -168,7 +168,17 @@ enum {
> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
> -defreg(ITR),
> +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
> +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
> +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
> +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
> +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
> +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> };
>
> static void
> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> }
>
> static uint32_t
> +mac_low11_read(E1000State *s, int index)
> +{
> +return s->mac_reg[index] & 0x7ff;
> +}
> +
> +static uint32_t
> +mac_low13_read(E1000State *s, int index)
> +{
> +return s->mac_reg[index] & 0x1fff;
> +}
> +
> +static uint32_t
> mac_icr_read(E1000State *s, int index)
> {
> uint32_t ret = s->mac_reg[ICR];
> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *,
> int) = {
> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
> getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
> -getreg(TADV), getreg(ITR),
> +getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
> +getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
For AIT should we use low16_read() ?
And low4_read() for FFMT?
> +getreg(ECOL), getreg(MCC), getreg(LATECOL), getreg(COLC),
> +getreg(DC), getreg(TNCRS),getreg(SEC), getreg(CEXTERR),
> +getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
> +getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
> +getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
>
> [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
> [GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] =
> mac_read_clr4,
> [TPT] = mac_read_clr4,
> [ICR] = mac_icr_read, [EECD] = get_eecd,[EERD] =
> flash_eerd_read,
> +[RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
> +[RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] =
> mac_low13_read,
> +[TDFH] = mac_low11_read, [TDFT] = mac_low11_read,
> +[TDFHS] = mac_low13_read, [TDFTS] = mac_low13_read, [TDFPC] =
> mac_low13_read,
> [CRCERRS ... MPC] = &mac_readreg,
> +[IP6AT ... IP6AT+3] = &mac_readreg, [IP4AT ... IP4AT+6] = &mac_readreg,
> +[FFLT ... FFLT+6] = &mac_low11_read,
> [RA ... RA+31] = &mac_readreg,
> +[WUPM ... WUPM+31] = &mac_readreg,
> [MTA ... MTA+127] = &mac_readreg,
> [VFTA ... VFTA+127] = &mac_readreg,
> +[FFMT ... FFMT+254] = &mac_readreg, [FFVT ... FFVT+254] = &mac_readreg,
> +[PBM ...
[Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
These registers appear in Intel's specs, but were not implemented.
These registers are now implemented trivially, i.e. they are initiated
with zero values, and if they are RW, they can be written or read by the
driver, or read only if they are R (essentially retaining their zero
values). For these registers no other procedures are performed.
The registers implemented here are:
Transmit:
RW: AIT
Management:
RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
Diagnostic:
RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
Statistic:
RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
XONTXC XOFFRXC XOFFTXC
Signed-off-by: Leonid Bloch
Signed-off-by: Dmitry Fleytman
---
hw/net/e1000.c | 52 +---
hw/net/e1000_regs.h | 6 ++
2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 6d57663..6f754ac 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -168,7 +168,17 @@ enum {
defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
-defreg(ITR),
+defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
+defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
+defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
+defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
+defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
+defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
+defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
+defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
+defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
+defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
+defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
};
static void
@@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
}
static uint32_t
+mac_low11_read(E1000State *s, int index)
+{
+return s->mac_reg[index] & 0x7ff;
+}
+
+static uint32_t
+mac_low13_read(E1000State *s, int index)
+{
+return s->mac_reg[index] & 0x1fff;
+}
+
+static uint32_t
mac_icr_read(E1000State *s, int index)
{
uint32_t ret = s->mac_reg[ICR];
@@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int)
= {
getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
-getreg(TADV), getreg(ITR),
+getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
+getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
+getreg(ECOL), getreg(MCC), getreg(LATECOL), getreg(COLC),
+getreg(DC), getreg(TNCRS),getreg(SEC), getreg(CEXTERR),
+getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
+getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
+getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
[TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
[GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
[TPT] = mac_read_clr4,
[ICR] = mac_icr_read, [EECD] = get_eecd,[EERD] =
flash_eerd_read,
+[RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
+[RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] =
mac_low13_read,
+[TDFH] = mac_low11_read, [TDFT] = mac_low11_read,
+[TDFHS] = mac_low13_read, [TDFTS] = mac_low13_read, [TDFPC] =
mac_low13_read,
[CRCERRS ... MPC] = &mac_readreg,
+[IP6AT ... IP6AT+3] = &mac_readreg, [IP4AT ... IP4AT+6] = &mac_readreg,
+[FFLT ... FFLT+6] = &mac_low11_read,
[RA ... RA+31] = &mac_readreg,
+[WUPM ... WUPM+31] = &mac_readreg,
[MTA ... MTA+127] = &mac_readreg,
[VFTA ... VFTA+127] = &mac_readreg,
+[FFMT ... FFMT+254] = &mac_readreg, [FFVT ... FFVT+254] = &mac_readreg,
+[PBM ... PBM+16383] = &mac_readreg,
};
enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
@@ -1232,7 +1269,11 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
putreg(PBA), putreg(EERD), putreg(SWSM), putreg(WUFC),
putreg(TDBAL),putreg(TDBAH),putreg(TXDCTL), putreg(RDBAH),
-putreg(RDBAL),putreg(LEDCTL), putreg(VET),
+putreg(RDBAL),putreg(LEDCTL), putreg(VET), putreg(FCRUC),
+putreg(TDFH), putreg(TDFT), putreg(TDFHS),putreg(TDFTS),
+putreg(TDFPC),putreg(RDFH), putreg(RDFT), putreg(RDFHS),
+
