On 2023/04/24 20:23, Sriram Yagnaraman wrote:
>
>> -----Original Message-----
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Sent: Monday, 24 April 2023 12:52
>> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>> Cc: Jason Wang <jasowang@redhat.com>; Dmitry Fleytman
>> <dmitry.fleytman@gmail.com>; Michael S . Tsirkin <mst@redhat.com>; Alex
>> Bennée <alex.bennee@linaro.org>; Philippe Mathieu-Daudé
>> <philmd@linaro.org>; Thomas Huth <thuth@redhat.com>; Wainer dos Santos
>> Moschetta <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>;
>> Cleber Rosa <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>; Paolo
>> Bonzini <pbonzini@redhat.com>; qemu-devel@nongnu.org; Tomasz Dzieciol
>> <t.dzieciol@partner.samsung.com>
>> Subject: Re: [PATCH v3 06/47] igb: Clear IMS bits when committing ICR access
>>
>> On 2023/04/24 18:29, Sriram Yagnaraman wrote:
>>>> -----Original Message-----
>>>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> Sent: Sunday, 23 April 2023 06:18
>>>> Cc: Sriram Yagnaraman <sriram.yagnaraman@est.tech>; Jason Wang
>>>> <jasowang@redhat.com>; Dmitry Fleytman <dmitry.fleytman@gmail.com>;
>>>> Michael S . Tsirkin <mst@redhat.com>; Alex Bennée
>>>> <alex.bennee@linaro.org>; Philippe Mathieu-Daudé <philmd@linaro.org>;
>>>> Thomas Huth <thuth@redhat.com>; Wainer dos Santos Moschetta
>>>> <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>; Cleber Rosa
>>>> <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>; Paolo
>>>> Bonzini <pbonzini@redhat.com>; qemu-devel@nongnu.org; Tomasz
>> Dzieciol
>>>> <t.dzieciol@partner.samsung.com>; Akihiko Odaki
>>>> <akihiko.odaki@daynix.com>
>>>> Subject: [PATCH v3 06/47] igb: Clear IMS bits when committing ICR
>>>> access
>>>>
>>>> The datasheet says contradicting statements regarding ICR accesses so
>>>> it is not reliable to determine the behavior of ICR accesses.
>>>> However, e1000e does clear IMS bits when reading ICR accesses and
>>>> Linux also expects ICR accesses will clear IMS bits according to:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tr
>>>> ee/drivers/
>>>> net/ethernet/intel/igb/igb_main.c?h=v6.2#n8048
>>>>
>>>> Fixes: 3a977deebe ("Intrdocue igb device emulation")
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>> hw/net/igb_core.c | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
>>>> 96a118b6c1..eaca5bd2b6 100644
>>>> --- a/hw/net/igb_core.c
>>>> +++ b/hw/net/igb_core.c
>>>> @@ -2452,16 +2452,16 @@ igb_set_ims(IGBCore *core, int index,
>>>> uint32_t
>>>> val) static void igb_commit_icr(IGBCore *core) {
>>>> /*
>>>> - * If GPIE.NSICR = 0, then the copy of IAM to IMS will occur only if at
>>>> + * If GPIE.NSICR = 0, then the clear of IMS will occur only if
>>>> + at
>>>> * least one bit is set in the IMS and there is a true interrupt as
>>>> * reflected in ICR.INTA.
>>>> */
>>>> if ((core->mac[GPIE] & E1000_GPIE_NSICR) ||
>>>> (core->mac[IMS] && (core->mac[ICR] & E1000_ICR_INT_ASSERTED))) {
>>>> - igb_set_ims(core, IMS, core->mac[IAM]);
>>>> - } else {
>>>> - igb_update_interrupt_state(core);
>>>> + igb_clear_ims_bits(core, core->mac[IAM]);
>>>> }
>>>> +
>>>> + igb_update_interrupt_state(core);
>>>> }
>>>>
>>>> static void igb_set_icr(IGBCore *core, int index, uint32_t val)
>>>> --
>>>> 2.40.0
>>>
>>> Reviewed-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>>>
>>> An additional question, should ICR be cleared if an actual interrupt was
>> asserted?
>>> (According to 7.3.2.11 GPIE: Non Selective Interrupt clear on read:
>>> When set, every read of ICR clears it. When this bit is cleared, an ICR read
>> causes it to be cleared only if an actual interrupt was asserted or IMS = 0b.)
>> Something like this?
>>
>> That is handled in igb_commit_icr(), which is renamed to igb_nsicr() in patch
>> "igb: Notify only new interrupts".
>>
>
> Mm, I must be missing something, but I still don't see the ICR bits being cleared igb_commit_icr/igb_nsicr().
> For e.g. e1000e_mac_icr_read does this explicitly:
> if ((core->mac[ICR] & E1000_ICR_ASSERTED) &&
> (core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
> trace_e1000e_irq_icr_clear_iame();
> core->mac[ICR] = 0;
> trace_e1000e_irq_icr_process_iame();
> e1000e_clear_ims_bits(core, core->mac[IAM]);
> }
Now I get it. This is indeed missing and needs to be handled, just as
you suggested.
>
>
>>>
>>> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
>>> eaca5bd2b6..aaaf80e4a3 100644
>>> --- a/hw/net/igb_core.c
>>> +++ b/hw/net/igb_core.c
>>> @@ -2529,6 +2529,9 @@ igb_mac_icr_read(IGBCore *core, int index)
>>> } else if (core->mac[IMS] == 0) {
>>> trace_e1000e_irq_icr_clear_zero_ims();
>>> core->mac[ICR] = 0;
>>> + } else if (core->mac[ICR] & E1000_ICR_INT_ASSERTED) {
>>> + e1000e_irq_icr_clear_iame();
>>> + core->mac[ICR] = 0;
>>> } else if (!msix_enabled(core->owner)) {
>>> trace_e1000e_irq_icr_clear_nonmsix_icr_read();
>>> core->mac[ICR] = 0;