PCI MSI questions

PCI MSI questions

[Jan Beulich]

Looking at the MSI implementation I have a couple of questions:

1) There currently seems to be a hidden requirement of NR_PIRQS in the
kernel needing to be no smaller than NR_IRQS in the hypervisor.
Otherwise, the pirq returned from PHYSDEVOP_map_pirq may collide
with the dynamic IRQs in the kernel or even be out of range altogether.
Therefore I think that NR_PIRQS has to become a variable defaulting
to 256 but getting initialized from a hypervisor reported value (perhaps
in start_info, or else from a new (sub-)hypercall).

2) While pci_restore_msi_state() properly checks the success of
msi_map_pirq_to_vector(), pci_restore_msix_state() doesn't. Is this
for a reason, or just because the code would get more complex if the
error needs to be handled?

3) The type parameter of xc_physdev_map_pirq{,_msi}() seems
superfluous, or is there any reason why these could be called with the
respectively reversed types?

4) The hypervisor option "msi_irq_enable" seems to be named pretty
oddly - both the "irq" and the "enable" in the name are more or less
redundant. So unless there's a reason for this long a name for an
option that generally I would expect most people want to set (at
least on bigger systems), I'd like to change it into "msi" or, if that's
considered prone for ambiguity, "pci-msi". Also, are there any plans
when to make have default be on rather than off?

Thanks, Jan

Re: PCI MSI questions

[Keir Fraser]

On 24/7/08 08:02, "Jan Beulich" <jbeulich@novell.com> wrote:

> 1) There currently seems to be a hidden requirement of NR_PIRQS in the
> kernel needing to be no smaller than NR_IRQS in the hypervisor.
> Otherwise, the pirq returned from PHYSDEVOP_map_pirq may collide
> with the dynamic IRQs in the kernel or even be out of range altogether.
> Therefore I think that NR_PIRQS has to become a variable defaulting
> to 256 but getting initialized from a hypervisor reported value (perhaps
> in start_info, or else from a new (sub-)hypercall).

Or have the kernel remap the return value of map_pirq into its own PIRQ
namespace, and maintain appropriate translation info? Although, it'd be nice
to have dynamic NR_IRQS sizing anyway -- people who want to run lots of
domUs currently may have to recompile dom0 with more DYNIRQS.

> 4) The hypervisor option "msi_irq_enable" seems to be named pretty
> oddly - both the "irq" and the "enable" in the name are more or less
> redundant. So unless there's a reason for this long a name for an
> option that generally I would expect most people want to set (at
> least on bigger systems), I'd like to change it into "msi" or, if that's
> considered prone for ambiguity, "pci-msi". Also, are there any plans
> when to make have default be on rather than off?

Renaming sounds sensible. I admit I forgot it was turned off by default. I
guess at this point we should turn it on by default immediately after 3.3
has branched.

 -- Keir

Re: PCI MSI questions

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 24.07.08 09:20 >>>
>On 24/7/08 08:02, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> 1) There currently seems to be a hidden requirement of NR_PIRQS in the
>> kernel needing to be no smaller than NR_IRQS in the hypervisor.
>> Otherwise, the pirq returned from PHYSDEVOP_map_pirq may collide
>> with the dynamic IRQs in the kernel or even be out of range altogether.
>> Therefore I think that NR_PIRQS has to become a variable defaulting
>> to 256 but getting initialized from a hypervisor reported value (perhaps
>> in start_info, or else from a new (sub-)hypercall).
>
>Or have the kernel remap the return value of map_pirq into its own PIRQ
>namespace, and maintain appropriate translation info? Although, it'd be nice
>to have dynamic NR_IRQS sizing anyway -- people who want to run lots of
>domUs currently may have to recompile dom0 with more DYNIRQS.

So what route would you prefer to go (so that if I'm able to get to it I
wouldn't end up submitting a patch you'd have wanted done the other
way around)?

Jan

Re: PCI MSI questions

[Keir Fraser]

On 8/8/08 11:09, "Jan Beulich" <jbeulich@novell.com> wrote:

>> Or have the kernel remap the return value of map_pirq into its own PIRQ
>> namespace, and maintain appropriate translation info? Although, it'd be nice
>> to have dynamic NR_IRQS sizing anyway -- people who want to run lots of
>> domUs currently may have to recompile dom0 with more DYNIRQS.
> 
> So what route would you prefer to go (so that if I'm able to get to it I
> wouldn't end up submitting a patch you'd have wanted done the other
> way around)?

I'd like the extra level of renaming. The guest shouldn't need to make
assumptions about the pirq handles handed out by Xen.

 -- Keir

Re: PCI MSI questions

[Jan Beulich]

>>> "Jan Beulich" <jbeulich@novell.com> 24.07.08 09:02 >>>
>Looking at the MSI implementation I have a couple of questions:
>
>1) There currently seems to be a hidden requirement of NR_PIRQS in the
>kernel needing to be no smaller than NR_IRQS in the hypervisor.
>Otherwise, the pirq returned from PHYSDEVOP_map_pirq may collide
>with the dynamic IRQs in the kernel or even be out of range altogether.
>Therefore I think that NR_PIRQS has to become a variable defaulting
>to 256 but getting initialized from a hypervisor reported value (perhaps
>in start_info, or else from a new (sub-)hypercall).
>
>2) While pci_restore_msi_state() properly checks the success of
>msi_map_pirq_to_vector(), pci_restore_msix_state() doesn't. Is this
>for a reason, or just because the code would get more complex if the
>error needs to be handled?
>
>3) The type parameter of xc_physdev_map_pirq{,_msi}() seems
>superfluous, or is there any reason why these could be called with the
>respectively reversed types?
>
>4) The hypervisor option "msi_irq_enable" seems to be named pretty
>oddly - both the "irq" and the "enable" in the name are more or less
>redundant. So unless there's a reason for this long a name for an
>option that generally I would expect most people want to set (at
>least on bigger systems), I'd like to change it into "msi" or, if that's
>considered prone for ambiguity, "pci-msi". Also, are there any plans
>when to make have default be on rather than off?

5) Shouldn't most of the MSI/MSI-X capability structures be write-
protected even in permissive mode (which at once would suppress
the warning I'd expect from pciback_config_write() for HVM guests
which get an MSI capable device assigned)? Or shouldn't perhaps
writes of incorrect control/address/data values even render the
device non-functional? And shouldn't then reads return the values
written rather than the ones in hardware?

Jan

RE: PCI MSI questions

[Shan, Haitao]

Jan Beulich wrote:
> Looking at the MSI implementation I have a couple of questions:
> 
> 1) There currently seems to be a hidden requirement of NR_PIRQS in the
> kernel needing to be no smaller than NR_IRQS in the hypervisor.
> Otherwise, the pirq returned from PHYSDEVOP_map_pirq may collide
> with the dynamic IRQs in the kernel or even be out of range
> altogether. Therefore I think that NR_PIRQS has to become a variable
> defaulting 
> to 256 but getting initialized from a hypervisor reported value
> (perhaps in start_info, or else from a new (sub-)hypercall).
> 
> 2) While pci_restore_msi_state() properly checks the success of
> msi_map_pirq_to_vector(), pci_restore_msix_state() doesn't. Is this
> for a reason, or just because the code would get more complex if the
> error needs to be handled?
Yes. I do not know what is the proper action. If one of the MSI-X pirq
failed, should we return? Or unmap those already mapped and return? Or
continue processing other MSI-X entries?
Any comments on this? Jan.

> 
> 3) The type parameter of xc_physdev_map_pirq{,_msi}() seems
> superfluous, or is there any reason why these could be called with the
> respectively reversed types?
Yes. The type is not useful in current code.
I am not quite sure about the reason. I think at the beginning of
submitting the patches, we do not have two seperate wrap functions for
this hypercall (only xc_physdev_map_pirq). That's where the "type"
parameter comes. Later, with MSI capabilities owned by Xen, we need pass
down more information to Xen via this hypercall. Thus the second one was
born.
Agree that this may need to be cleaned up.

> 
> 4) The hypervisor option "msi_irq_enable" seems to be named pretty
> oddly - both the "irq" and the "enable" in the name are more or less
> redundant. So unless there's a reason for this long a name for an
> option that generally I would expect most people want to set (at
> least on bigger systems), I'd like to change it into "msi" or, if
> that's considered prone for ambiguity, "pci-msi". Also, are there any
> plans when to make have default be on rather than off?
> 
> Thanks, Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: PCI MSI questions

[Jan Beulich]

>> 2) While pci_restore_msi_state() properly checks the success of
>> msi_map_pirq_to_vector(), pci_restore_msix_state() doesn't. Is this
>> for a reason, or just because the code would get more complex if the
>> error needs to be handled?
>Yes. I do not know what is the proper action. If one of the MSI-X pirq
>failed, should we return? Or unmap those already mapped and return? Or
>continue processing other MSI-X entries?
>Any comments on this? Jan.

I would think this should follow the logic in msix_capabilities_init(), i.e.
unmap what was mapped (and hence leave the device in a semi-
consistent [interrupts non-functional] state).

>> 3) The type parameter of xc_physdev_map_pirq{,_msi}() seems
>> superfluous, or is there any reason why these could be called with the
> respectively reversed types?
>Yes. The type is not useful in current code.
>I am not quite sure about the reason. I think at the beginning of
>submitting the patches, we do not have two seperate wrap functions for
>this hypercall (only xc_physdev_map_pirq). That's where the "type"
>parameter comes. Later, with MSI capabilities owned by Xen, we need pass
>down more information to Xen via this hypercall. Thus the second one was
>born.
>Agree that this may need to be cleaned up.

That is what I suspected. I'll prepare a patch, unless you want to.

Jan