qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] e1000e: remove internal interrupt flag


From: Dmitry Fleytman
Subject: Re: [Qemu-devel] [PATCH] e1000e: remove internal interrupt flag
Date: Thu, 18 Aug 2016 23:27:46 +0300


> On 18 Aug 2016, at 19:41 PM, Markus Armbruster <address@hidden> wrote:
> 
> Dmitry Fleytman <address@hidden> writes:
> 
>>> On 18 Aug 2016, at 17:15, Cao jin <address@hidden> wrote:
>>> 
>>> Commit 66bf7d58 removed internal msi state flag E1000E_USE_MSI, 
>>> E1000E_USE_MSIX
>>> is not necessary too, remove it now. And interrupt flag field intr_state 
>>> also
>>> can be removed now.
>>> 
>>> CC: Dmitry Fleytman <address@hidden>
>>> CC: Jason Wang <address@hidden>
>>> CC: Markus Armbruster <address@hidden>
>>> CC: Marcel Apfelbaum <address@hidden>
>>> CC: Michael S. Tsirkin <address@hidden>
>>> CC: Paolo Bonzini <address@hidden>
>>> Signed-off-by: Cao jin <address@hidden>
>>> ---
>>> hw/net/e1000e.c | 8 +-------
>>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>> 
>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>>> index 82a7be1..72aad21 100644
>>> --- a/hw/net/e1000e.c
>>> +++ b/hw/net/e1000e.c
>>> @@ -69,7 +69,6 @@ typedef struct E1000EState {
>>>    uint16_t subsys_ven_used;
>>>    uint16_t subsys_used;
>>> 
>>> -    uint32_t intr_state;
>>>    bool disable_vnet;
>>> 
>>>    E1000ECore core;
>>> @@ -89,8 +88,6 @@ typedef struct E1000EState {
>>> #define E1000E_MSIX_TABLE   (0x0000)
>>> #define E1000E_MSIX_PBA     (0x2000)
>>> 
>>> -#define E1000E_USE_MSIX    BIT(0)
>>> -
>>> static uint64_t
>>> e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
>>> {
>>> @@ -302,8 +299,6 @@ e1000e_init_msix(E1000EState *s)
>>>    } else {
>>>        if (!e1000e_use_msix_vectors(s, E1000E_MSIX_VEC_NUM)) {
>>>            msix_uninit(d, &s->msix, &s->msix);
>>> -        } else {
>>> -            s->intr_state |= E1000E_USE_MSIX;
>> 
>> So you are removing error handling here. But what if e1000e_use_msix_vectors 
>> fails?
> 
> Before the patch, E1000E_USE_MSIX is set exactly when MSI-X was enabled
> successfully.  It's only use is in MSI-X cleanup (next hunk): cleanup is
> skipped when the flag isn't set.
> 
> The flag is pointless, because we can just as well test whether MSI-X is
> enabled directly.  That's what the patch does.

I see.

Acked-by: Dmitry Fleytman <address@hidden>

> 
>> 
>>>        }
>>>    }
>>> }
>>> @@ -311,7 +306,7 @@ e1000e_init_msix(E1000EState *s)
>>> static void
>>> e1000e_cleanup_msix(E1000EState *s)
>>> {
>>> -    if (s->intr_state & E1000E_USE_MSIX) {
>>> +    if (msix_enabled(PCI_DEVICE(s))) {
>>>        e1000e_unuse_msix_vectors(s, E1000E_MSIX_VEC_NUM);
>>>        msix_uninit(PCI_DEVICE(s), &s->msix, &s->msix);
>>>    }
>>> @@ -601,7 +596,6 @@ static const VMStateDescription e1000e_vmstate = {
>>>        VMSTATE_MSIX(parent_obj, E1000EState),
>>> 
>>>        VMSTATE_UINT32(ioaddr, E1000EState),
>>> -       VMSTATE_UINT32(intr_state, E1000EState),
> 
> We can still do this, because as Paolo pointed out, no released version
> of QEMU had this device.
> 
>>>        VMSTATE_UINT32(core.rxbuf_min_shift, E1000EState),
>>>        VMSTATE_UINT8(core.rx_desc_len, E1000EState),
>>>        VMSTATE_UINT32_ARRAY(core.rxbuf_sizes, E1000EState,
>>> -- 
>>> 2.1.0
> 
> Reviewed-by: Markus Armbruster <address@hidden>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]