| Message ID | 20170210123351.73526-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.02.17 at 13:33, <roger.pau@citrix.com> wrote: > --- a/docs/misc/hvmlite.markdown > +++ b/docs/misc/hvmlite.markdown > @@ -75,3 +75,23 @@ info structure that's passed at boot time (field rsdp_paddr). > > Description of paravirtualized devices will come from XenStore, just as it's > done for HVM guests. > + > +## Interrupts ## > + > +### Interrupts from physical devices ### > + > +Interrupts from physical devices are delivered using native methods, this is > +done in order to take advantage of new hardware assisted virtualization > +functions, like posted interrupts. This implies that PVHv2 guests with physical > +devices will also have the necessary interrupt controllers in order to manage > +the delivery of interrupts from those devices, using the same interfaces that > +are available on native hardware. > + > +### Interrupts from paravirtualized devices ### > + > +Interrupts from paravirtualized devices are delivered using event channels, see > +[Event Channel Internals][event_channels] for more detailed information about > +event channels. Delivery of those interrupts can be configured in the same way > +as HVM guests, check xen/include/public/hvm/params.h and > +xen/include/public/hvm/hvm_op.h for more information about available delivery > +methods. Just FTR - my comments on v4 still stand; I especially don't buy your argument [1] of posted interrupts becoming more widely available, since - as pointed out before - we continue to not fully enable their use by default, due to unresolved problems with the implementation. Furthermore emulation_flags_ok() allows EMU_LAPIC to be clear (together with all other bits being clear), in which case posted interrupts can't be used at all afaict. Jan [1] https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg02092.html
On Mon, Feb 13, 2017 at 06:09:27AM -0700, Jan Beulich wrote: > >>> On 10.02.17 at 13:33, <roger.pau@citrix.com> wrote: > > --- a/docs/misc/hvmlite.markdown > > +++ b/docs/misc/hvmlite.markdown > > @@ -75,3 +75,23 @@ info structure that's passed at boot time (field rsdp_paddr). > > > > Description of paravirtualized devices will come from XenStore, just as it's > > done for HVM guests. > > + > > +## Interrupts ## > > + > > +### Interrupts from physical devices ### > > + > > +Interrupts from physical devices are delivered using native methods, this is > > +done in order to take advantage of new hardware assisted virtualization > > +functions, like posted interrupts. This implies that PVHv2 guests with physical > > +devices will also have the necessary interrupt controllers in order to manage > > +the delivery of interrupts from those devices, using the same interfaces that > > +are available on native hardware. > > + > > +### Interrupts from paravirtualized devices ### > > + > > +Interrupts from paravirtualized devices are delivered using event channels, see > > +[Event Channel Internals][event_channels] for more detailed information about > > +event channels. Delivery of those interrupts can be configured in the same way > > +as HVM guests, check xen/include/public/hvm/params.h and > > +xen/include/public/hvm/hvm_op.h for more information about available delivery > > +methods. > > Just FTR - my comments on v4 still stand; I especially don't buy > your argument [1] of posted interrupts becoming more widely > available, since - as pointed out before - we continue to not fully > enable their use by default, due to unresolved problems with > the implementation. Furthermore emulation_flags_ok() allows > EMU_LAPIC to be clear (together with all other bits being clear), > in which case posted interrupts can't be used at all afaict. It only allows it for DomUs, which don't support passthrough at all. If a device is passed thought, a LAPIC will be required. Also note that the current set of physdev ops available to PVHv2 is completely wrong. Since PVHv2 is considered a HVM guest from Xen PoV, it will take the is_hvm_domain branch at the top of physdev_map_pirq, which will certainly lead to all kinds of fun, because that's the weird path used in (PV)HVM guests that allows them to remap interrupts from emulated or physical devices into event channels. So at the moment, I think this should even be considered a bug-fix. As I think I've said before, I don't think we should initially follow this route for PVHv2, but in any case this can always be added afterwards by re-enabling the XENFEAT_hvm_pirqs flag and fixing the physdev ops so they can work properly in this context. Roger.
>>> On 13.02.17 at 15:22, <roger.pau@citrix.com> wrote: > On Mon, Feb 13, 2017 at 06:09:27AM -0700, Jan Beulich wrote: >> >>> On 10.02.17 at 13:33, <roger.pau@citrix.com> wrote: >> > --- a/docs/misc/hvmlite.markdown >> > +++ b/docs/misc/hvmlite.markdown >> > @@ -75,3 +75,23 @@ info structure that's passed at boot time (field rsdp_paddr). >> > >> > Description of paravirtualized devices will come from XenStore, just as it's >> > done for HVM guests. >> > + >> > +## Interrupts ## >> > + >> > +### Interrupts from physical devices ### >> > + >> > +Interrupts from physical devices are delivered using native methods, this is >> > +done in order to take advantage of new hardware assisted virtualization >> > +functions, like posted interrupts. This implies that PVHv2 guests with physical >> > +devices will also have the necessary interrupt controllers in order to manage >> > +the delivery of interrupts from those devices, using the same interfaces that >> > +are available on native hardware. >> > + >> > +### Interrupts from paravirtualized devices ### >> > + >> > +Interrupts from paravirtualized devices are delivered using event channels, see >> > +[Event Channel Internals][event_channels] for more detailed information about >> > +event channels. Delivery of those interrupts can be configured in the same way >> > +as HVM guests, check xen/include/public/hvm/params.h and >> > +xen/include/public/hvm/hvm_op.h for more information about available delivery >> > +methods. >> >> Just FTR - my comments on v4 still stand; I especially don't buy >> your argument [1] of posted interrupts becoming more widely >> available, since - as pointed out before - we continue to not fully >> enable their use by default, due to unresolved problems with >> the implementation. Furthermore emulation_flags_ok() allows >> EMU_LAPIC to be clear (together with all other bits being clear), >> in which case posted interrupts can't be used at all afaict. > > It only allows it for DomUs, which don't support passthrough at all. If a > device is passed thought, a LAPIC will be required. Is that spelled out anywhere? And I don't recall any fixme comments regarding pass-through not working. > Also note that the current set of physdev ops available to PVHv2 is completely > wrong. Since PVHv2 is considered a HVM guest from Xen PoV, it will take the > is_hvm_domain branch at the top of physdev_map_pirq, which will certainly lead > to all kinds of fun, because that's the weird path used in (PV)HVM guests that > allows them to remap interrupts from emulated or physical devices into event > channels. So at the moment, I think this should even be considered a bug-fix. I think I had indicated that I'm okay with the physdev ops change for now; i.e. ... > As I think I've said before, I don't think we should initially follow this > route for PVHv2, but in any case this can always be added afterwards by > re-enabling the XENFEAT_hvm_pirqs flag and fixing the physdev ops so they can > work properly in this context. ... please note that my comments are specifically against the document, not the code (anymore). And the document should spell out options, even if their implementation may be deferred. Jan
On Mon, Feb 13, 2017 at 07:33:17AM -0700, Jan Beulich wrote: > >>> On 13.02.17 at 15:22, <roger.pau@citrix.com> wrote: > > On Mon, Feb 13, 2017 at 06:09:27AM -0700, Jan Beulich wrote: > >> >>> On 10.02.17 at 13:33, <roger.pau@citrix.com> wrote: > >> > --- a/docs/misc/hvmlite.markdown > >> > +++ b/docs/misc/hvmlite.markdown > >> > @@ -75,3 +75,23 @@ info structure that's passed at boot time (field rsdp_paddr). > >> > > >> > Description of paravirtualized devices will come from XenStore, just as it's > >> > done for HVM guests. > >> > + > >> > +## Interrupts ## > >> > + > >> > +### Interrupts from physical devices ### > >> > + > >> > +Interrupts from physical devices are delivered using native methods, this is > >> > +done in order to take advantage of new hardware assisted virtualization > >> > +functions, like posted interrupts. This implies that PVHv2 guests with physical > >> > +devices will also have the necessary interrupt controllers in order to manage > >> > +the delivery of interrupts from those devices, using the same interfaces that > >> > +are available on native hardware. > >> > + > >> > +### Interrupts from paravirtualized devices ### > >> > + > >> > +Interrupts from paravirtualized devices are delivered using event channels, see > >> > +[Event Channel Internals][event_channels] for more detailed information about > >> > +event channels. Delivery of those interrupts can be configured in the same way > >> > +as HVM guests, check xen/include/public/hvm/params.h and > >> > +xen/include/public/hvm/hvm_op.h for more information about available delivery > >> > +methods. > >> > >> Just FTR - my comments on v4 still stand; I especially don't buy > >> your argument [1] of posted interrupts becoming more widely > >> available, since - as pointed out before - we continue to not fully > >> enable their use by default, due to unresolved problems with > >> the implementation. Furthermore emulation_flags_ok() allows > >> EMU_LAPIC to be clear (together with all other bits being clear), > >> in which case posted interrupts can't be used at all afaict. > > > > It only allows it for DomUs, which don't support passthrough at all. If a > > device is passed thought, a LAPIC will be required. > > Is that spelled out anywhere? And I don't recall any fixme > comments regarding pass-through not working. I'm not sure there's anywhere were a FIXME would be appropriate, this is not something that needs fixes, it's something that simply doesn't yet exist. Maybe libxl should be slightly adjusted to print a nice error message if someone attempts adding a PCI device to a PVHv2 guest. It is currently being discussed with the ARM guys also [0], in order to try to come up with a similar model for both ARM and PVHv2 (except of course for the arch-specific bits). > > Also note that the current set of physdev ops available to PVHv2 is completely > > wrong. Since PVHv2 is considered a HVM guest from Xen PoV, it will take the > > is_hvm_domain branch at the top of physdev_map_pirq, which will certainly lead > > to all kinds of fun, because that's the weird path used in (PV)HVM guests that > > allows them to remap interrupts from emulated or physical devices into event > > channels. So at the moment, I think this should even be considered a bug-fix. > > I think I had indicated that I'm okay with the physdev ops change > for now; i.e. ... > > > As I think I've said before, I don't think we should initially follow this > > route for PVHv2, but in any case this can always be added afterwards by > > re-enabling the XENFEAT_hvm_pirqs flag and fixing the physdev ops so they can > > work properly in this context. > > ... please note that my comments are specifically against the > document, not the code (anymore). And the document should > spell out options, even if their implementation may be deferred. The document simply describes what I plan to implement myself, so if someone wants to work on those bits (physdev ops), he/she is free to do it and expand the document appropriately. I simply don't plan to implement them myself, because I don't see much value in it and it's an interface that I would rather get rid of. Roger. [0] https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg03032.html
diff --git a/docs/misc/hvmlite.markdown b/docs/misc/hvmlite.markdown index 898b8ee..b2557f7 100644 --- a/docs/misc/hvmlite.markdown +++ b/docs/misc/hvmlite.markdown @@ -75,3 +75,23 @@ info structure that's passed at boot time (field rsdp_paddr). Description of paravirtualized devices will come from XenStore, just as it's done for HVM guests. + +## Interrupts ## + +### Interrupts from physical devices ### + +Interrupts from physical devices are delivered using native methods, this is +done in order to take advantage of new hardware assisted virtualization +functions, like posted interrupts. This implies that PVHv2 guests with physical +devices will also have the necessary interrupt controllers in order to manage +the delivery of interrupts from those devices, using the same interfaces that +are available on native hardware. + +### Interrupts from paravirtualized devices ### + +Interrupts from paravirtualized devices are delivered using event channels, see +[Event Channel Internals][event_channels] for more detailed information about +event channels. Delivery of those interrupts can be configured in the same way +as HVM guests, check xen/include/public/hvm/params.h and +xen/include/public/hvm/hvm_op.h for more information about available delivery +methods. diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 5f72758..9e40865 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3764,10 +3764,12 @@ static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { + struct domain *currd = current->domain; + switch ( cmd ) { default: - if ( !is_pvh_vcpu(current) || !is_hardware_domain(current->domain) ) + if ( !is_pvh_domain(currd) || !is_hardware_domain(currd) ) return -ENOSYS; /* fall through */ case PHYSDEVOP_map_pirq: @@ -3775,7 +3777,8 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case PHYSDEVOP_eoi: case PHYSDEVOP_irq_status_query: case PHYSDEVOP_get_free_pirq: - return do_physdev_op(cmd, arg); + return (has_pirq(currd) || is_pvh_domain(currd)) ? + do_physdev_op(cmd, arg) : -ENOSYS; } } @@ -3808,17 +3811,19 @@ static long hvm_memory_op_compat32(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) static long hvm_physdev_op_compat32( int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { + struct domain *d = current->domain; + switch ( cmd ) { - case PHYSDEVOP_map_pirq: - case PHYSDEVOP_unmap_pirq: - case PHYSDEVOP_eoi: - case PHYSDEVOP_irq_status_query: - case PHYSDEVOP_get_free_pirq: - return compat_physdev_op(cmd, arg); + case PHYSDEVOP_map_pirq: + case PHYSDEVOP_unmap_pirq: + case PHYSDEVOP_eoi: + case PHYSDEVOP_irq_status_query: + case PHYSDEVOP_get_free_pirq: + return has_pirq(d) ? compat_physdev_op(cmd, arg) : -ENOSYS; break; default: - return -ENOSYS; + return -ENOSYS; break; } } diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c index 5a49796..b4cc6a8 100644 --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -94,7 +94,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p, int pirq, irq, ret = 0; void *map_data = NULL; - if ( domid == DOMID_SELF && is_hvm_domain(d) ) + if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) ) { /* * Only makes sense for vector-based callback, else HVM-IRQ logic @@ -265,7 +265,7 @@ int physdev_unmap_pirq(domid_t domid, int pirq) if ( ret ) goto free_domain; - if ( is_hvm_domain(d) ) + if ( is_hvm_domain(d) && has_pirq(d) ) { spin_lock(&d->event_lock); if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND ) diff --git a/xen/common/kernel.c b/xen/common/kernel.c index d0edb13..4b87c60 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -332,7 +332,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case guest_type_hvm: fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | (1U << XENFEAT_hvm_callback_vector) | - (1U << XENFEAT_hvm_pirqs); + (has_pirq(d) ? (1U << XENFEAT_hvm_pirqs) : 0); break; } #endif diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index e6c7e13..7226ab3 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -419,6 +419,8 @@ struct arch_domain #define has_vvga(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_VGA)) #define has_viommu(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_IOMMU)) #define has_vpit(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_PIT)) +#define has_pirq(d) (!!((d)->arch.emulation_flags & \ + XEN_X86_EMU_USE_PIRQ)) #define has_arch_pdevs(d) (!list_empty(&(d)->arch.pdev_list)) diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index 363c8cc..73c829a 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -293,12 +293,14 @@ struct xen_arch_domainconfig { #define XEN_X86_EMU_IOMMU (1U<<_XEN_X86_EMU_IOMMU) #define _XEN_X86_EMU_PIT 8 #define XEN_X86_EMU_PIT (1U<<_XEN_X86_EMU_PIT) +#define _XEN_X86_EMU_USE_PIRQ 9 +#define XEN_X86_EMU_USE_PIRQ (1U<<_XEN_X86_EMU_USE_PIRQ) #define XEN_X86_EMU_ALL (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET | \ XEN_X86_EMU_PM | XEN_X86_EMU_RTC | \ XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC | \ XEN_X86_EMU_VGA | XEN_X86_EMU_IOMMU | \ - XEN_X86_EMU_PIT) + XEN_X86_EMU_PIT | XEN_X86_EMU_USE_PIRQ) uint32_t emulation_flags; };