Re: [Xen-devel] [PATCH v6 for-xen-4.5 1/3] dpci: Move from domain centric model to hvm_dirq_dpci model.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 16:48, <konrad.wilk@xxxxxxxxxx> wrote:
> On Thu, Sep 25, 2014 at 03:24:53PM +0100, Jan Beulich wrote:
>> >>> On 23.09.14 at 04:10, <konrad.wilk@xxxxxxxxxx> wrote:
>> > @@ -130,6 +146,7 @@ int pt_irq_create_bind(
>> >          return -ENOMEM;
>> >      }
>> >      pirq_dpci = pirq_dpci(info);
>> > +    pt_pirq_reset(d, pirq_dpci);
>> >  
>> >      switch ( pt_irq_bind->irq_type )
>> >      {
>> > @@ -232,7 +249,6 @@ int pt_irq_create_bind(
>> >          {
>> >              unsigned int share;
>> >  
>> > -            pirq_dpci->dom = d;
>> >              if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
>> >              {
>> >                  pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
>> > @@ -258,7 +274,6 @@ int pt_irq_create_bind(
>> >              {
>> >                  if ( pt_irq_need_timer(pirq_dpci->flags) )
>> >                      kill_timer(&pirq_dpci->timer);
>> > -                pirq_dpci->dom = NULL;
>> >                  list_del(&girq->list);
>> >                  list_del(&digl->list);
>> >                  hvm_irq_dpci->link_cnt[link]--;
>> > @@ -391,7 +406,6 @@ int pt_irq_destroy_bind(
>> >          msixtbl_pt_unregister(d, pirq);
>> >          if ( pt_irq_need_timer(pirq_dpci->flags) )
>> >              kill_timer(&pirq_dpci->timer);
>> > -        pirq_dpci->dom   = NULL;
>> >          pirq_dpci->flags = 0;
>> >          pirq_cleanup_check(pirq, d);
>> >      }
>> 
>> Is all of the above really necessary? I.e. I can neither see why setting
>> ->dom earlier is needed, nor why clearing it on the error paths should
>> be dropped.
> 
> Yes. We need the ->dom so that the hvm_dirq_assist can run without
> hitting an NULL pointer exception. Please keep in mind that the moment
> we setup the IRQ action handler, we are "live" - [...]

But all you need is that this happens before respective
pirq_guest_bind() calls. I.e. in the PT_IRQ_TYPE_PCI and
PT_IRQ_TYPE_MSI_TRANSLATE cases it was already done early enough
(avoiding it remaining set on error paths), so all you'd need is adding
it for the PT_IRQ_TYPE_MSI path too.

I agree that the clearing of the field in error paths might need a
little care, but otoh you could equally well have hvm_dirq_assist()
bail when it finds it to be NULL?

> The extra '->dom = d' in the pt_irq_create_bind was an extra one since the
> pt_pirq_reset does it now.

Yeah, effectively pt_pirq_reset() is just replacing that assignment.
It's not even clear wrapping this in a function is really worthwhile.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel