| Message ID | 1473920070-21938-5-git-send-email-dmitry@daynix.com |
|---|---|
| State | New |
| Headers | show |
On 2016年09月15日 14:14, Dmitry Fleytman wrote: > This patch fixes incorrect check for > interrypt type being used. > > PBSCLR register is valid for MSI-X only. > > See spec. 10.2.3.13 MSI—X PBA Clear > > Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> > --- > hw/net/e1000e_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index 22765cb..c38ed10 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val) > > core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK; > > - if (msix_enabled(core->owner)) { > + if (!msix_enabled(core->owner)) { > return; > } > Spec also said "writing 0b has no effect". So we'd better implement this behavior too?
> On 22 Sep 2016, at 09:40 AM, Jason Wang <jasowang@redhat.com> wrote: > > > > On 2016年09月15日 14:14, Dmitry Fleytman wrote: >> This patch fixes incorrect check for >> interrypt type being used. >> >> PBSCLR register is valid for MSI-X only. >> >> See spec. 10.2.3.13 MSI—X PBA Clear >> >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> >> --- >> hw/net/e1000e_core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >> index 22765cb..c38ed10 100644 >> --- a/hw/net/e1000e_core.c >> +++ b/hw/net/e1000e_core.c >> @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val) >> core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK; >> - if (msix_enabled(core->owner)) { >> + if (!msix_enabled(core->owner)) { >> return; >> } >> > > Spec also said "writing 0b has no effect". So we'd better implement this behavior too? Hi Jason, Not sure I understand you correctly. With current implementation, writing 0b does nothing except that it changes value of PBACLR being read. I just verified that physical device behaves exactly like this. Is this what you meant? ~Dmitry
On 2016年09月22日 17:01, Dmitry Fleytman wrote: >> On 22 Sep 2016, at 09:40 AM, Jason Wang <jasowang@redhat.com> wrote: >> >> >> >> On 2016年09月15日 14:14, Dmitry Fleytman wrote: >>> This patch fixes incorrect check for >>> interrypt type being used. >>> >>> PBSCLR register is valid for MSI-X only. >>> >>> See spec. 10.2.3.13 MSI—X PBA Clear >>> >>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> >>> --- >>> hw/net/e1000e_core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>> index 22765cb..c38ed10 100644 >>> --- a/hw/net/e1000e_core.c >>> +++ b/hw/net/e1000e_core.c >>> @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val) >>> core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK; >>> - if (msix_enabled(core->owner)) { >>> + if (!msix_enabled(core->owner)) { >>> return; >>> } >>> >> Spec also said "writing 0b has no effect". So we'd better implement this behavior too? > > Hi Jason, > > Not sure I understand you correctly. > > With current implementation, writing 0b does nothing > except that it changes value of PBACLR being read. > > I just verified that physical device behaves exactly like this. > > Is this what you meant? > > ~Dmitry > Yes, then it looks fine to me. Thanks
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index 22765cb..c38ed10 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -2347,7 +2347,7 @@ e1000e_set_pbaclr(E1000ECore *core, int index, uint32_t val) core->mac[PBACLR] = val & E1000_PBACLR_VALID_MASK; - if (msix_enabled(core->owner)) { + if (!msix_enabled(core->owner)) { return; }
This patch fixes incorrect check for interrypt type being used. PBSCLR register is valid for MSI-X only. See spec. 10.2.3.13 MSI—X PBA Clear Signed-off-by: Dmitry Fleytman <dmitry@daynix.com> --- hw/net/e1000e_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)