Re: [PATCH 13/21] MSI: Do MSI masking in the arch code

Re: [PATCH 13/21] MSI: Do MSI masking in the arch code

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> It's an arch detail whether MSI irqs need to be masked using the PCI
> MSI registers.

Agreed.  It isn't an arch detail that they need to be unmasked in
the pci configuration space.

I assume this patch is motivated just to make arch support easier
and not for RTAS compatibility reason.

> This changes behaviour in that previously we unconditionally masked
> all MSIs, eventhough we only ever enable one, whereas now we only
> mask the irqs we're using.
>
> To be super paranoid I have the archs mask the irq before they write
> the msi message, just in case the device doesn't respect the MSI enable
> bit or MSI is already enabled or something else crazy.
>
> For MSI-X this might mean we mask the already masked MSI-X on the device,
> but that should be harmless.

I don't think this patch really makes sense.  I think we should mask
all possible vectors for msi and msi-x initially in the generic code
and then unmask them.

If we were trying to support Dave Miller's example of hypervisors
that don't let us touch the msi registers I think there would be a
point.  As it is I think this just makes the code more brittle.

Eric

Re: [PATCH 13/21] MSI: Do MSI masking in the arch code

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]

On Wed, 2007-03-28 at 00:39 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > It's an arch detail whether MSI irqs need to be masked using the PCI
> > MSI registers.
> 
> Agreed.  It isn't an arch detail that they need to be unmasked in
> the pci configuration space.
> 
> I assume this patch is motivated just to make arch support easier
> and not for RTAS compatibility reason.
> 
> > This changes behaviour in that previously we unconditionally masked
> > all MSIs, eventhough we only ever enable one, whereas now we only
> > mask the irqs we're using.
> >
> > To be super paranoid I have the archs mask the irq before they write
> > the msi message, just in case the device doesn't respect the MSI enable
> > bit or MSI is already enabled or something else crazy.
> >
> > For MSI-X this might mean we mask the already masked MSI-X on the device,
> > but that should be harmless.
> 
> I don't think this patch really makes sense.  I think we should mask
> all possible vectors for msi and msi-x initially in the generic code
> and then unmask them.
> 
> If we were trying to support Dave Miller's example of hypervisors
> that don't let us touch the msi registers I think there would be a
> point.  As it is I think this just makes the code more brittle.

The main motivation was to have the arch in control of as much direct
register writing as possible. Even though our HV does allow us to write
to config space, it's not obviously safe for Linux to be flipping bits
and also calling the HV to configure things. What's worse, I don't have
hardware that supports the mask bits, so I can't test it.

I also thought this would save me from having custom MSI irq_chips for
the powerpc backends. But I think I need them now anyway, so I guess
that's not a concern.

So I guess we'll drop this one, although I might try and tidy up the
implementation of "mask all MSIs" in msi_capability_init() as another
patch.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 13/21] MSI: Do MSI masking in the arch code

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

>
> The main motivation was to have the arch in control of as much direct
> register writing as possible. Even though our HV does allow us to write
> to config space, it's not obviously safe for Linux to be flipping bits
> and also calling the HV to configure things. What's worse, I don't have
> hardware that supports the mask bits, so I can't test it.

I agree that there are limits here, on what we can feed into a hypervisor
and not confuse it.  Mostly my thinking is to assume someone has been
scribbling with the msi capability before we got there  and to place
everything in a know good state and not assume that device reset has
already done the work for us.  Since we aren't the pci  device reset
we can choose the defaults that make sense for us.

Worse case we can just put the capability in the state that pci device
reset would have put it in.

> I also thought this would save me from having custom MSI irq_chips for
> the powerpc backends. But I think I need them now anyway, so I guess
> that's not a concern.

Well we could still do the basic setup in arch_setup_msi_irq so I don't think
it has ever been an irq_chips style operations.

> So I guess we'll drop this one, although I might try and tidy up the
> implementation of "mask all MSIs" in msi_capability_init() as another
> patch.

I have no problem with that.  The msix version of this still needs
to be written as well.

Eric