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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> "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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel