Re: [PATCH] KVM: arm/arm64: Keep mapped, disabled interrupts quiet
On Thu, Oct 08, 2015 at 01:36:09PM +0100, Marc Zyngier wrote: > If a mapped interrupt is disabled, we must make sure the > corresponding physical interrupt cannot fire, as we are not > injecting the interrupt, and not setting the active bit. > > For example, a guest disabling its timer interrupt at the GIC level > but leaving the timer firing would stop making progress as noone > would be able to prevent this timer from firing and interrupting > the host. Not quite what is expected. And if we're rebooting > or turning a vcpu off while the interrupt is about to fire, > we're exactly going to face this. > > In order to cope with this, parse the list of mapped interrupts, > and mark it as active if we're about to run the guest (or inactive > if we've exited). Hopefully, nobody is going to run with zillions > of disabled, mapped interrupts, right? > > Reported-by: Lorenzo Pieralisi> Signed-off-by: Marc Zyngier > --- > My gut feeling is that this vgic_dist_irq_set_pending should always > be done, but I'd like some other eyes to have a look at it. > > Tested on Seattle (running 4.3-rc4) with a script hammering CPU hotplug. > > virt/kvm/arm/vgic.c | 60 > + > 1 file changed, 60 insertions(+) > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 6bd1c9b..0ad3f7e 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1092,6 +1092,11 @@ static void vgic_retire_lr(int lr_nr, int irq, struct > kvm_vcpu *vcpu) > struct vgic_cpu *vgic_cpu = >arch.vgic_cpu; > struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr); > > + if (vlr.state & LR_HW) { > + vgic_dist_irq_set_pending(vcpu, irq); > + vlr.hwirq = 0; > + } > + I don't understand this logic? At least you should check if the LR state is active or pending, sort of what vgic_unqueue_irqs does. In fact, it looks like we can re-use the vgic_unqueue_irqs if we do some refactoring of that function. But why is it necessary? > vlr.state = 0; > vgic_set_lr(vcpu, lr_nr, vlr); > clear_bit(lr_nr, vgic_cpu->lr_used); > @@ -1232,6 +1237,57 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, > int irq) > } > > /* > + * If a mapped interrupt is disabled, we must make sure the > + * corresponding physical interrupt cannot fire, as we are not > + * injecting the interrupt, and not setting the active bit. > + * > + * Parse the list of mapped interrupts, and mark it as active if we're > + * about to run the guest (or inactive if we've exited). Hopefully, > + * nobody is going to run with zillions of disabled, mapped > + * interrupts... > + */ > +static void vgic_handle_disabled_mapped_irq(struct kvm_vcpu *vcpu, bool > enter) > +{ > + struct vgic_dist *dist = >kvm->arch.vgic; > + struct vgic_cpu *vgic_cpu = >arch.vgic_cpu; > + struct vgic_bitmap *spi_bitmap; > + struct list_head *root; > + struct irq_phys_map_entry *entry; > + struct irq_phys_map *map; > + int ret; > + > + rcu_read_lock(); > + > + /* Check for PPIs */ > + root = _cpu->irq_phys_map_list; > + list_for_each_entry_rcu(entry, root, entry) { > + map = >map; > + if (!vgic_irq_is_enabled(vcpu, map->virt_irq)) { > + ret = irq_set_irqchip_state(map->irq, > + IRQCHIP_STATE_ACTIVE, > + enter); > + WARN_ON(ret); > + } > + } > + > + /* Check for SPIs routed to this vcpu */ > + root = >irq_phys_map_list; > + spi_bitmap = >irq_spi_target[vcpu->vcpu_id]; > + list_for_each_entry_rcu(entry, root, entry) { > + map = >map; > + if (!vgic_irq_is_enabled(vcpu, map->virt_irq) && > + vgic_bitmap_get_irq_val(spi_bitmap, 0, map->virt_irq)) { why do we check the emulated target bit here? is the right thing not to see if we're running on the physical CPU which the physical distributor is configured to route this SPI to, and in that case set the IRQ active to prevent it from firting? > + ret = irq_set_irqchip_state(map->irq, > + IRQCHIP_STATE_ACTIVE, > + enter); > + WARN_ON(ret); > + } > + } > + > + rcu_read_unlock(); > +} > + > +/* > * Fill the list registers with pending interrupts before running the > * guest. > */ > @@ -1320,6 +1376,8 @@ epilog: > WARN_ON(ret); > } > } > + > + vgic_handle_disabled_mapped_irq(vcpu, true); > } > > static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) > @@ -1484,6 +1542,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu > *vcpu) > vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY; > } > > +
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thursday 15 October 2015 10:08:02 Eric Auger wrote: > Hi Arnd, > On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > >> --- a/drivers/vfio/platform/vfio_platform_common.c > >> +++ b/drivers/vfio/platform/vfio_platform_common.c > >> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo > >> reset_lookup_table[] = { > >> .reset_function_name = "vfio_platform_calxedaxgmac_reset", > >> .module_name = "vfio-platform-calxedaxgmac", > >> }, > >> + { > >> + .compat = "amd,xgbe-seattle-v1a", > >> + .reset_function_name = "vfio_platform_amdxgbe_reset", > >> + .module_name = "vfio-platform-amdxgbe", > >> + }, > >> }; > >> > >> static void vfio_platform_get_reset(struct vfio_platform_device *vdev, > >> > > > > This is causing build errors for me when CONFIG_MODULES is disabled. > Sorry about that and thanks for reporting the issue > > > > Could this please be restructured so vfio_platform_get_reset does > > not attempt to call __symbol_get() but instead has the drivers > > register themselves properly to a subsystem? > OK > > Could you elaborate about "has the drivers register themselves properly > to a subsystem". > > My first proposal when coping with this problematic of being able to add > reset plugins to the vfio-platform driver was to create new drivers per > device requiring reset. but this was considered painful for end-users, > who needed to be aware of the right driver to bind - and I think that > makes sense - (https://lkml.org/lkml/2015/4/17/568) . Having multiple drivers indeed sucks, but your current approach isn't that much better, as you still have two modules that are used to driver the same hardware. I would expect that the same driver that is used for the normal operation and that it calls a function to register itself to vfio by passing a structure with the device and reset function pointer. > A naive question I dare to ask, wouldn't it be acceptable to make > vfio_platform depend on CONFIG_MODULES? Don't we disable modules for > security purpose? In that context would we use VFIO? I think a lot of embedded systems turn off modules to save a little memory, speed up boot time and simplify their user space. Aside from that, the current method is highly unusual and looks a bit fragile to me, as you are relying on internals of the module loader code. It's also a layering violation as the generic code needs to be patched for each device specific module that is added, and we try to avoid that. A possible solution could be something inside the xgbe driver like static void xgbe_init_module(void) { int ret = 0; if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) ret = platform_driver_register(_driver); if (ret) return ret; if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) ret = vfio_platform_register_reset(_of_match, xgbe_platform_reset); return ret; } This way you have exactly one driver module that gets loaded for the device and you can use it either with the platform_driver or through vfio. A nicer way that would be a little more work would be to integrate the reset infrastructure into 'struct platform_driver' framework, by adding another callback to the it for doing the interaction with vfio, something like enum vfio_platform_op { VFIO_PLATFORM_BIND, VFIO_PLATFORM_UNBIND, VFIO_PLATFORM_RESET, }; struct platform_driver { int (*probe)(struct platform_device *); int (*remove)(struct platform_device *); ... int (*vfio_manage)(struct platform_device *, enum vfio_platform_op); struct device_driver driver; }; This would integrate much more closely into the platform driver framework, just like the regular vfio driver integrates into the PCI framework. Unlike PCI however, you can't just use the generic driver framework to unbind the driver, because you still need device specific code. Arnd ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 0/4] KVM: arm/arm64: guest synchronous halt/resume
On Fri, Sep 25, 2015 at 11:41:13PM +0200, Eric Auger wrote: > This series introduces the capability to synchronously exit the guest > and prevent it from being re-entered. This modality will be used by > IRQ forwarding series when changing the state of the IRQ. > > Former pause flag used when starting the vcpu in KVM_ARM_VCPU_POWER_OFF > state, in PSCI calls and in KVM_SET_MP_STATE ioctl is renamed into > power_off. A new pause flag is introduced. Both now are checked in > kvm_arch_vcpu_runnable and in the VCPU_RUN critical section, before > entering the vcpu. > Thanks - applied, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm: use GIC support unconditionally
On Mon, Oct 12, 2015 at 03:22:31PM +0200, Arnd Bergmann wrote: > The vgic code on ARM is built for all configurations that enable KVM, > but the parent_data field that it references is only present when > CONFIG_IRQ_DOMAIN_HIERARCHY is set: > > virt/kvm/arm/vgic.c: In function 'kvm_vgic_map_phys_irq': > virt/kvm/arm/vgic.c:1781:13: error: 'struct irq_data' has no member named > 'parent_data' > > This flag is implied by the GIC driver, and indeed the VGIC code only > makes sense if a GIC is present. This changes the CONFIG_KVM symbol > to always select GIC, which avoids the issue. > > Signed-off-by: Arnd Bergmann> Fixes: 662d9715840 ("arm/arm64: KVM: Kill CONFIG_KVM_ARM_{VGIC,TIMER}") > > diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig > index 210eccadb69a..356970f3b25e 100644 > --- a/arch/arm/kvm/Kconfig > +++ b/arch/arm/kvm/Kconfig > @@ -21,6 +21,7 @@ config KVM > depends on MMU && OF > select PREEMPT_NOTIFIERS > select ANON_INODES > + select ARM_GIC > select HAVE_KVM_CPU_RELAX_INTERCEPT > select HAVE_KVM_ARCH_TLB_FLUSH_ALL > select KVM_MMIO > Applied. Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
Hi Arnd, On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo >> reset_lookup_table[] = { >> .reset_function_name = "vfio_platform_calxedaxgmac_reset", >> .module_name = "vfio-platform-calxedaxgmac", >> }, >> + { >> + .compat = "amd,xgbe-seattle-v1a", >> + .reset_function_name = "vfio_platform_amdxgbe_reset", >> + .module_name = "vfio-platform-amdxgbe", >> + }, >> }; >> >> static void vfio_platform_get_reset(struct vfio_platform_device *vdev, >> > > This is causing build errors for me when CONFIG_MODULES is disabled. Sorry about that and thanks for reporting the issue > > Could this please be restructured so vfio_platform_get_reset does > not attempt to call __symbol_get() but instead has the drivers > register themselves properly to a subsystem? OK Could you elaborate about "has the drivers register themselves properly to a subsystem". My first proposal when coping with this problematic of being able to add reset plugins to the vfio-platform driver was to create new drivers per device requiring reset. but this was considered painful for end-users, who needed to be aware of the right driver to bind - and I think that makes sense - (https://lkml.org/lkml/2015/4/17/568) . A naive question I dare to ask, wouldn't it be acceptable to make vfio_platform depend on CONFIG_MODULES? Don't we disable modules for security purpose? In that context would we use VFIO? Best Regards Eric > > I don't see any way this could be fixed otherwise. The problem > of course showed up with calxedaxgmac already, but I'd prefer not > to see anything added there until the common code has been improved. > > Arnd > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: its_alloc_tables() - with all BASER marked none
On 15/10/15 02:30, Mario Smarduch wrote: > If its_init()/its_probe()/its_alloc_tables() finds all GITS_BASER type none, > it > continues with ITS initialization. > > Is there a reason to continue? Of course. There is nothing that *mandates* the ITS to request memory from the operating system. The HW could perfectly come with its own memory and not need anything at all. > Started to look through this code recently - little confused here. Only the beginning! ;-) M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thursday 15 October 2015 16:20:46 Eric Auger wrote: > On 10/15/2015 02:12 PM, Christoffer Dall wrote: > > On Thu, Oct 15, 2015 at 01:21:55PM +0200, Arnd Bergmann wrote: > >> On Thursday 15 October 2015 10:08:02 Eric Auger wrote: > >>> On 10/14/2015 05:38 PM, Arnd Bergmann wrote: > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > >> A possible solution could be something inside the xgbe driver like > >> > >> > >> static void xgbe_init_module(void) > >> { > >>int ret = 0; > >> > >>if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) > >>ret = platform_driver_register(_driver); > >>if (ret) > >>return ret; > >> > >>if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) > >>ret = vfio_platform_register_reset(_of_match, > >> xgbe_platform_reset); > >> > >>return ret; > >> } > >> > >> This way you have exactly one driver module that gets loaded for the > >> device and you can use it either with the platform_driver or through > >> vfio. > If I understand it correctly you still need 2 loaded modules (VFIO > driver & XGBE driver which implements the reset function) or am I > missing something? That is correct, yes. > I had a similar mechanism of registration in my PATCH v1 but I did the > registration from the reset module itself instead of in the native > driver, as you suggest here. Right. The main difference is that you don't have two modules fighting over the same device with the approach here. Arnd ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thursday 15 October 2015 16:46:09 Eric Auger wrote: > > > > This is where we'd need a little more changes for this approach. Instead > > of unbinding the device from its driver, the idea would be that the > > driver remains bound as far as the driver model is concerned, but > > it would be in a quiescent state where no other subsystem interacts with > > it (i.e. it gets unregistered from networking core or whichever it uses). > > Currently we use the same mechanism as for PCI, ie. unbind the native > driver and then bind VFIO platform driver in its place. Don't you think > changing this may be a pain for user-space tools that are designed to > work that way for PCI? > > My personal preference would be to start with your first proposal since > it looks (to me) less complex and "unknown" that the 2d approach. We certainly can't easily change from one approach to the other without breaking user expectations, so the decision needs to be made carefully. The main observation here is that platform devices are unlike PCI in this regard because they need extra per-device code. I have argued in the past that we should not reuse the "VFIO" name here because it's actually something else. On the other hand, there are a lot of commonalities, we just have to make sure we don't try to force the code into one model that doesn't really work just to make it look more like PCI VFIO. Arnd ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thu, Oct 15, 2015 at 04:55:13PM +0200, Arnd Bergmann wrote: > On Thursday 15 October 2015 16:46:09 Eric Auger wrote: > > > > > > This is where we'd need a little more changes for this approach. Instead > > > of unbinding the device from its driver, the idea would be that the > > > driver remains bound as far as the driver model is concerned, but > > > it would be in a quiescent state where no other subsystem interacts with > > > it (i.e. it gets unregistered from networking core or whichever it uses). > > > > Currently we use the same mechanism as for PCI, ie. unbind the native > > driver and then bind VFIO platform driver in its place. Don't you think > > changing this may be a pain for user-space tools that are designed to > > work that way for PCI? > > > > My personal preference would be to start with your first proposal since > > it looks (to me) less complex and "unknown" that the 2d approach. > > We certainly can't easily change from one approach to the other without > breaking user expectations, so the decision needs to be made carefully. > > The main observation here is that platform devices are unlike PCI in this > regard because they need extra per-device code. I have argued in the > past that we should not reuse the "VFIO" name here because it's actually > something else. I've adjusted to consider VFIO a general purpose framework for mapping device resources into userspace/VMs, and there are certainly a lot of commonality with both PCI, platform, and potentially other devices for that to make sense. > On the other hand, there are a lot of commonalities, > we just have to make sure we don't try to force the code into one model > that doesn't really work just to make it look more like PCI VFIO. > But given that we now have code for platform device passthrough that works in both QEMU and the kernel side and is actually useful for people, is there a clear technical advantage to go back and rework thaat at this point? Don't get me wrong, I like the idea of having a single driver bound to a platform device, and then that's it, but it just feels like that discussion doesn't necessarily belong in the context of a patch that 'just' seeks to add reset functionality for a specific device for VFIO? -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
Hi Arnd, On 10/15/2015 02:12 PM, Christoffer Dall wrote: > On Thu, Oct 15, 2015 at 01:21:55PM +0200, Arnd Bergmann wrote: >> On Thursday 15 October 2015 10:08:02 Eric Auger wrote: >>> Hi Arnd, >>> On 10/14/2015 05:38 PM, Arnd Bergmann wrote: On Wednesday 14 October 2015 15:33:12 Eric Auger wrote: > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo > reset_lookup_table[] = { > .reset_function_name = "vfio_platform_calxedaxgmac_reset", > .module_name = "vfio-platform-calxedaxgmac", > }, > + { > + .compat = "amd,xgbe-seattle-v1a", > + .reset_function_name = "vfio_platform_amdxgbe_reset", > + .module_name = "vfio-platform-amdxgbe", > + }, > }; > > static void vfio_platform_get_reset(struct vfio_platform_device *vdev, > This is causing build errors for me when CONFIG_MODULES is disabled. >>> Sorry about that and thanks for reporting the issue Could this please be restructured so vfio_platform_get_reset does not attempt to call __symbol_get() but instead has the drivers register themselves properly to a subsystem? >>> OK >>> >>> Could you elaborate about "has the drivers register themselves properly >>> to a subsystem". >>> >>> My first proposal when coping with this problematic of being able to add >>> reset plugins to the vfio-platform driver was to create new drivers per >>> device requiring reset. but this was considered painful for end-users, >>> who needed to be aware of the right driver to bind - and I think that >>> makes sense - (https://lkml.org/lkml/2015/4/17/568) . >> >> Having multiple drivers indeed sucks, but your current approach isn't >> that much better, as you still have two modules that are used to driver >> the same hardware. >> >> I would expect that the same driver that is used for the normal >> operation and that it calls a function to register itself to vfio >> by passing a structure with the device and reset function pointer. >> >>> A naive question I dare to ask, wouldn't it be acceptable to make >>> vfio_platform depend on CONFIG_MODULES? Don't we disable modules for >>> security purpose? In that context would we use VFIO? >> >> I think a lot of embedded systems turn off modules to save a little >> memory, speed up boot time and simplify their user space. >> >> Aside from that, the current method is highly unusual and looks a bit >> fragile to me, as you are relying on internals of the module loader >> code. It's also a layering violation as the generic code needs to be >> patched for each device specific module that is added, and we try >> to avoid that. Many thanks for taking the time to write this down >> >> A possible solution could be something inside the xgbe driver like >> >> >> static void xgbe_init_module(void) >> { >> int ret = 0; >> >> if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET) >> ret = platform_driver_register(_driver); >> if (ret) >> return ret; >> >> if (IS_ENABLED(CONFIG_VFIO_PLATFORM)) >> ret = vfio_platform_register_reset(_of_match, >> xgbe_platform_reset); >> >> return ret; >> } >> >> This way you have exactly one driver module that gets loaded for the >> device and you can use it either with the platform_driver or through >> vfio. If I understand it correctly you still need 2 loaded modules (VFIO driver & XGBE driver which implements the reset function) or am I missing something? I had a similar mechanism of registration in my PATCH v1 but I did the registration from the reset module itself instead of in the native driver, as you suggest here. Best Regards Eric >> >> A nicer way that would be a little more work would be to integrate >> the reset infrastructure into 'struct platform_driver' framework, >> by adding another callback to the it for doing the interaction with >> vfio, something like >> >> enum vfio_platform_op { >> VFIO_PLATFORM_BIND, >> VFIO_PLATFORM_UNBIND, >> VFIO_PLATFORM_RESET, >> }; >> >> struct platform_driver { >> int (*probe)(struct platform_device *); >> int (*remove)(struct platform_device *); >> ... >> int (*vfio_manage)(struct platform_device *, enum vfio_platform_op); >> struct device_driver driver; >> }; >> >> This would integrate much more closely into the platform driver framework, >> just like the regular vfio driver integrates into the PCI framework. >> Unlike PCI however, you can't just use the generic driver framework to >> unbind the driver, because you still need device specific code. >> > Thanks for these suggestions, really helpful. > > What I don't understand in the latter example is how VFIO knows which > struct platform_driver to interact with? > > Also, just so I'm sure I understand
[PATCH 2/9] KVM: arm: Do not indent the arguments of DECLARE_BITMAP
Besides being a coding style issue, it confuses make tags: ctags: Warning: include/kvm/arm_vgic.h:307: null expansion of name pattern "\1" ctags: Warning: include/kvm/arm_vgic.h:308: null expansion of name pattern "\1" ctags: Warning: include/kvm/arm_vgic.h:309: null expansion of name pattern "\1" ctags: Warning: include/kvm/arm_vgic.h:317: null expansion of name pattern "\1" Cc: kvmarm@lists.cs.columbia.edu Signed-off-by: Michal Marek--- include/kvm/arm_vgic.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index d901f1a47be6..58c5c0eb8d2d 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -304,9 +304,9 @@ struct vgic_cpu { u8 *vgic_irq_lr_map; /* Pending/active/both interrupts on this VCPU */ - DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS); - DECLARE_BITMAP( active_percpu, VGIC_NR_PRIVATE_IRQS); - DECLARE_BITMAP( pend_act_percpu, VGIC_NR_PRIVATE_IRQS); + DECLARE_BITMAP(pending_percpu, VGIC_NR_PRIVATE_IRQS); + DECLARE_BITMAP(active_percpu, VGIC_NR_PRIVATE_IRQS); + DECLARE_BITMAP(pend_act_percpu, VGIC_NR_PRIVATE_IRQS); /* Pending/active/both shared interrupts, dynamically sized */ unsigned long *pending_shared; @@ -314,7 +314,7 @@ struct vgic_cpu { unsigned long *pend_act_shared; /* Bitmap of used/free list registers */ - DECLARE_BITMAP( lr_used, VGIC_V2_MAX_LRS); + DECLARE_BITMAP(lr_used, VGIC_V2_MAX_LRS); /* Number of list registers on this CPU */ int nr_lr; -- 2.1.4 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thu, 2015-10-15 at 21:42 +0200, Christoffer Dall wrote: > On Thu, Oct 15, 2015 at 10:53:17AM -0600, Alex Williamson wrote: > > On Thu, 2015-10-15 at 16:46 +0200, Eric Auger wrote: > > > Hi Arnd, > > > On 10/15/2015 03:59 PM, Arnd Bergmann wrote: > > > > On Thursday 15 October 2015 14:12:28 Christoffer Dall wrote: > > > >>> > > > >>> enum vfio_platform_op { > > > >>> VFIO_PLATFORM_BIND, > > > >>> VFIO_PLATFORM_UNBIND, > > > >>> VFIO_PLATFORM_RESET, > > > >>> }; > > > >>> > > > >>> struct platform_driver { > > > >>> int (*probe)(struct platform_device *); > > > >>> int (*remove)(struct platform_device *); > > > >>> ... > > > >>> int (*vfio_manage)(struct platform_device *, enum > > > >>> vfio_platform_op); > > > >>> struct device_driver driver; > > > >>> }; > > > >>> > > > >>> This would integrate much more closely into the platform driver > > > >>> framework, > > > >>> just like the regular vfio driver integrates into the PCI framework. > > > >>> Unlike PCI however, you can't just use the generic driver framework to > > > >>> unbind the driver, because you still need device specific code. > > > >>> > > > >> Thanks for these suggestions, really helpful. > > > >> > > > >> What I don't understand in the latter example is how VFIO knows which > > > >> struct platform_driver to interact with? > > > > > > > > This would assume that the driver remains bound to the device, so VFIO > > > > gets a pointer to the device from somewhere (as it does today) and then > > > > follows the dev->driver pointer to get to the platform_driver. > > > > The complexity of managing a bi-modal driver seems like far more than a > > little bit of code duplication in a device specific reset module and > > extends into how userspace makes devices available through vfio, so I > > think it's too late for that discussion. > > > > I have had extremely limited exposure to the implementation details of > the drivers for devices relevant for VFIO platform, so apologies for > asking stupid questions. > > I'm sure that your point is valid, I just fully understand how the > complexities of a bi-modal driver arise? > > Is it simply that the reset function in a particular device driver may > not be self-contained so therefore the whole driver would need to be > refactored to be able to do a reset for the purpose of VFIO? Yes, I would expect that reset function in a driver is not typically self contained, probably referencing driver specific data structures for register offsets, relying on various mappings that are expected to be created from the driver probe() function, etc. It also creates a strange dependency on the host driver, how is the user to know they need the native host driver loaded for full functionality in a device assignment scenario? Are we going to need to do a module_request() of the host driver in vfio platform anyway? What if there are multiple devices and the host driver claims the others when loaded? In the case of PCI and SR-IOV virtual functions, I often blacklist the host VF driver because I shouldn't need it when I only intend to use the device in guests. Not to mention that we'd have to drop a little bit of vfio knowledge into each host driver that we intend to enlighten like this, and how do we resolve whether the host driver, potentially compiled from a separate source tree has this support. I really don't see the layering violation in having a set of reset functions and some lookup mechanism to pick the correct one. The vfio platform driver is a meta driver and sometimes we need to enlighten it a little bit more about the device it's operating on. For PCI we have all sorts of special functionality for reset, but we have a standard to work with there, so we may need to choose between a bus reset, PM reset, AF FLR, PCIe FLR, or device specific reset, but it's all buried in the PCI core code; where device specific resets are the exception on PCI, they are the norm on platform. > > > >> Also, just so I'm sure I understand correctly, VFIO_PLATFORM_UNBIND is > > > >> then called by VFIO before the VFIO driver unbinds from the device > > > >> (unbinding the platform driver from the device being a completely > > > >> separate thing)? > > > > > > > > This is where we'd need a little more changes for this approach. Instead > > > > of unbinding the device from its driver, the idea would be that the > > > > driver remains bound as far as the driver model is concerned, but > > > > it would be in a quiescent state where no other subsystem interacts with > > > > it (i.e. it gets unregistered from networking core or whichever it > > > > uses). > > > > > > Currently we use the same mechanism as for PCI, ie. unbind the native > > > driver and then bind VFIO platform driver in its place. Don't you think > > > changing this may be a pain for user-space tools that are designed to > > > work that way for PCI? > > > > > > My personal preference would be to start
Re: [PATCH v3 00/20] KVM: ARM64: Add guest PMU support
On 09/24/2015 05:31 PM, Shannon Zhao wrote: > This patchset adds guest PMU support for KVM on ARM64. It takes > trap-and-emulate approach. When guest wants to monitor one event, it > will be trapped by KVM and KVM will call perf_event API to create a perf > event and call relevant perf_event APIs to get the count value of event. > > Use perf to test this patchset in guest. When using "perf list", it > shows the list of the hardware events and hardware cache events perf > supports. Then use "perf stat -e EVENT" to monitor some event. For > example, use "perf stat -e cycles" to count cpu cycles and > "perf stat -e cache-misses" to count cache misses. > > Below are the outputs of "perf stat -r 5 sleep 5" when running in host > and guest. > > Host: > Performance counter stats for 'sleep 5' (5 runs): > > 0.551428 task-clock (msec) #0.000 CPUs utilized > ( +- 0.91% ) > 1 context-switches #0.002 M/sec > 0 cpu-migrations#0.000 K/sec > 48 page-faults #0.088 M/sec > ( +- 1.05% ) >1150265 cycles#2.086 GHz > ( +- 0.92% ) > stalled-cycles-frontend > stalled-cycles-backend > 526398 instructions #0.46 insns per cycle > ( +- 0.89% ) > branches > 9485 branch-misses # 17.201 M/sec > ( +- 2.35% ) > >5.000831616 seconds time elapsed >( +- 0.00% ) > > Guest: > Performance counter stats for 'sleep 5' (5 runs): > > 0.730868 task-clock (msec) #0.000 CPUs utilized > ( +- 1.13% ) > 1 context-switches #0.001 M/sec > 0 cpu-migrations#0.000 K/sec > 48 page-faults #0.065 M/sec > ( +- 0.42% ) >1642982 cycles#2.248 GHz > ( +- 1.04% ) > stalled-cycles-frontend > stalled-cycles-backend > 637964 instructions #0.39 insns per cycle > ( +- 0.65% ) > branches > 10377 branch-misses # 14.198 M/sec > ( +- 1.09% ) > >5.001289068 seconds time elapsed >( +- 0.00% ) > Thanks for V3. One suggestion is to run more perf stress tests, such as "perf test". So we know the corner cases are covered as much as possible. > This patchset can be fetched from [1] and the relevant QEMU version for > test can be fetched from [2]. > > Thanks, > Shannon > > [1] https://git.linaro.org/people/shannon.zhao/linux-mainline.git > KVM_ARM64_PMU_v3 > [2] https://git.linaro.org/people/shannon.zhao/qemu.git PMU_v2 > > Changes since v2->v3: > * Directly use perf raw event type to create perf_event in KVM > * Add a helper vcpu_sysreg_write > * remove unrelated header file > > Changes since v1->v2: > * Use switch...case for registers access handler instead of adding > alone handler for each register > * Try to use the sys_regs to store the register value instead of adding > new variables in struct kvm_pmc > * Fix the handle of cp15 regs > * Create a new kvm device vPMU, then userspace could choose whether to > create PMU > * Fix the handle of PMU overflow interrupt > > Shannon Zhao (20): > ARM64: Move PMU register related defines to asm/pmu.h > KVM: ARM64: Define PMU data structure for each vcpu > KVM: ARM64: Add offset defines for PMU registers > KVM: ARM64: Add reset and access handlers for PMCR_EL0 register > KVM: ARM64: Add reset and access handlers for PMSELR register > KVM: ARM64: Add reset and access handlers for PMCEID0 and PMCEID1 > register > KVM: ARM64: PMU: Add perf event map and introduce perf event creating > function > KVM: ARM64: Add reset and access handlers for PMXEVTYPER register > KVM: ARM64: Add reset and access handlers for PMXEVCNTR register > KVM: ARM64: Add reset and access handlers for PMCCNTR register > KVM: ARM64: Add reset and access handlers for PMCNTENSET and > PMCNTENCLR register > KVM: ARM64: Add reset and access handlers for PMINTENSET and > PMINTENCLR register > KVM: ARM64: Add reset and access handlers for PMOVSSET and PMOVSCLR > register > KVM: ARM64: Add reset and access handlers for PMUSERENR register > KVM: ARM64: Add reset and access handlers for PMSWINC register > KVM: ARM64: Add access handlers for PMEVCNTRn and PMEVTYPERn register > KVM: ARM64: Add PMU overflow interrupt routing > KVM: ARM64: Reset PMU state when resetting vcpu > KVM: ARM64: Free perf event of PMU when destroying vcpu > KVM: ARM64: Add a new kvm ARM PMU device > >
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thu, Oct 15, 2015 at 5:49 PM, Arnd Bergmannwrote: > On Thursday 15 October 2015 17:03:21 Christoffer Dall wrote: >> On Thu, Oct 15, 2015 at 04:55:13PM +0200, Arnd Bergmann wrote: >> > On Thursday 15 October 2015 16:46:09 Eric Auger wrote: >> > > > >> > > > This is where we'd need a little more changes for this approach. >> > > > Instead >> > > > of unbinding the device from its driver, the idea would be that the >> > > > driver remains bound as far as the driver model is concerned, but >> > > > it would be in a quiescent state where no other subsystem interacts >> > > > with >> > > > it (i.e. it gets unregistered from networking core or whichever it >> > > > uses). >> > > >> > > Currently we use the same mechanism as for PCI, ie. unbind the native >> > > driver and then bind VFIO platform driver in its place. Don't you think >> > > changing this may be a pain for user-space tools that are designed to >> > > work that way for PCI? >> > > >> > > My personal preference would be to start with your first proposal since >> > > it looks (to me) less complex and "unknown" that the 2d approach. >> > >> > We certainly can't easily change from one approach to the other without >> > breaking user expectations, so the decision needs to be made carefully. >> > >> > The main observation here is that platform devices are unlike PCI in this >> > regard because they need extra per-device code. I have argued in the >> > past that we should not reuse the "VFIO" name here because it's actually >> > something else. >> >> I've adjusted to consider VFIO a general purpose framework for mapping >> device resources into userspace/VMs, and there are certainly a lot of >> commonality with both PCI, platform, and potentially other devices for >> that to make sense. >> >> >> > On the other hand, there are a lot of commonalities, >> > we just have to make sure we don't try to force the code into one model >> > that doesn't really work just to make it look more like PCI VFIO. >> > >> >> But given that we now have code for platform device passthrough that >> works in both QEMU and the kernel side and is actually useful for >> people, is there a clear technical advantage to go back and rework thaat >> at this point? >> >> Don't get me wrong, I like the idea of having a single driver bound to a >> platform device, and then that's it, but it just feels like that >> discussion doesn't necessarily belong in the context of a patch that >> 'just' seeks to add reset functionality for a specific device for VFIO? > > Ah, this is for qemu/kvm? If there is already upstream qemu code that > finds it easier to use this approach, it's certainly more logical to > deal with it in my first approach than the second. yes, indeed. > > I was thinking of ODP as the primary user, and that wouldn't need > the interface to be consistent with PCI as much, because the code > is inherently device specific there anyway. > I didn't think about how this applies to ODP (haven't had time to look at ODP in any technical detail TBH), but it sounds like ODP could be adapted to use either approach. In any case we can always transition towards your 2nd approach in a userspace compatible way later, if we find good reasons to. Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thu, 2015-10-15 at 16:46 +0200, Eric Auger wrote: > Hi Arnd, > On 10/15/2015 03:59 PM, Arnd Bergmann wrote: > > On Thursday 15 October 2015 14:12:28 Christoffer Dall wrote: > >>> > >>> enum vfio_platform_op { > >>> VFIO_PLATFORM_BIND, > >>> VFIO_PLATFORM_UNBIND, > >>> VFIO_PLATFORM_RESET, > >>> }; > >>> > >>> struct platform_driver { > >>> int (*probe)(struct platform_device *); > >>> int (*remove)(struct platform_device *); > >>> ... > >>> int (*vfio_manage)(struct platform_device *, enum vfio_platform_op); > >>> struct device_driver driver; > >>> }; > >>> > >>> This would integrate much more closely into the platform driver framework, > >>> just like the regular vfio driver integrates into the PCI framework. > >>> Unlike PCI however, you can't just use the generic driver framework to > >>> unbind the driver, because you still need device specific code. > >>> > >> Thanks for these suggestions, really helpful. > >> > >> What I don't understand in the latter example is how VFIO knows which > >> struct platform_driver to interact with? > > > > This would assume that the driver remains bound to the device, so VFIO > > gets a pointer to the device from somewhere (as it does today) and then > > follows the dev->driver pointer to get to the platform_driver. The complexity of managing a bi-modal driver seems like far more than a little bit of code duplication in a device specific reset module and extends into how userspace makes devices available through vfio, so I think it's too late for that discussion. > >> Also, just so I'm sure I understand correctly, VFIO_PLATFORM_UNBIND is > >> then called by VFIO before the VFIO driver unbinds from the device > >> (unbinding the platform driver from the device being a completely > >> separate thing)? > > > > This is where we'd need a little more changes for this approach. Instead > > of unbinding the device from its driver, the idea would be that the > > driver remains bound as far as the driver model is concerned, but > > it would be in a quiescent state where no other subsystem interacts with > > it (i.e. it gets unregistered from networking core or whichever it uses). > > Currently we use the same mechanism as for PCI, ie. unbind the native > driver and then bind VFIO platform driver in its place. Don't you think > changing this may be a pain for user-space tools that are designed to > work that way for PCI? > > My personal preference would be to start with your first proposal since > it looks (to me) less complex and "unknown" that the 2d approach. > > Let's wait for Alex opinion too... I thought the reason we took the approach we have now is so that we don't have reset code loaded into the kernel unless we have a device that needs it. Therefore we don't really want to preemptively load all the reset drivers and have them do a registration. The unfortunate side-effect of that is the platform code needs to go looking for the driver. We do that via the __symbol_get() trick, which only fails without modules because the underscore variant isn't defined in that case. I remember asking Eric previously why we're using that rather than symbol_get(), I've since forgotten his answer, but the fact that __symbol_get() is only defined for modules makes it moot, we either need to make symbol_get() work or define __symbol_get() for non-module builds. Otherwise, we should probably abandon the idea of these reset functions being modules and build them into the vfio platform driver (which would still be less loaded, dead code than a bi-modal host driver). Thanks, Alex ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: its_alloc_tables() - with all BASER marked none
On 10/15/2015 12:50 AM, Marc Zyngier wrote: > On 15/10/15 02:30, Mario Smarduch wrote: >> If its_init()/its_probe()/its_alloc_tables() finds all GITS_BASER type none, >> it >> continues with ITS initialization. >> >> Is there a reason to continue? > > Of course. There is nothing that *mandates* the ITS to request memory > from the operating system. The HW could perfectly come with its own > memory and not need anything at all. > >> Started to look through this code recently - little confused here. > > Only the beginning! ;-) Well at this time the beginning of console freeze close to "ITS: using cache flushing for cmd queue". Poked around CBASER and noticed Inner cache bits are updated but not sticky so it got me wondering. Appears like I need to look elsewhere, like maybe my environment. Thanks for clarifying. - Mario > > M. > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: its_alloc_tables() - with all BASER marked none
On 10/15/2015 10:10 AM, Marc Zyngier wrote: > On 15/10/15 18:08, Mario Smarduch wrote: >> >> >> On 10/15/2015 12:50 AM, Marc Zyngier wrote: >>> On 15/10/15 02:30, Mario Smarduch wrote: If its_init()/its_probe()/its_alloc_tables() finds all GITS_BASER type none, it continues with ITS initialization. Is there a reason to continue? >>> >>> Of course. There is nothing that *mandates* the ITS to request memory >>> from the operating system. The HW could perfectly come with its own >>> memory and not need anything at all. >>> Started to look through this code recently - little confused here. >>> >>> Only the beginning! ;-) >> >> Well at this time the beginning of console freeze close to "ITS: using cache >> flushing for cmd queue". Poked around CBASER and noticed Inner cache bits are >> updated but not sticky so it got me wondering. >> >> Appears like I need to look elsewhere, like maybe my environment. > > Out of curiosity, is that on a FastModel? Or on some actual HW? FastModel, license ran out on FVP. > > M. > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] VFIO: platform: AMD xgbe reset module
On Thu, Oct 15, 2015 at 10:53:17AM -0600, Alex Williamson wrote: > On Thu, 2015-10-15 at 16:46 +0200, Eric Auger wrote: > > Hi Arnd, > > On 10/15/2015 03:59 PM, Arnd Bergmann wrote: > > > On Thursday 15 October 2015 14:12:28 Christoffer Dall wrote: > > >>> > > >>> enum vfio_platform_op { > > >>> VFIO_PLATFORM_BIND, > > >>> VFIO_PLATFORM_UNBIND, > > >>> VFIO_PLATFORM_RESET, > > >>> }; > > >>> > > >>> struct platform_driver { > > >>> int (*probe)(struct platform_device *); > > >>> int (*remove)(struct platform_device *); > > >>> ... > > >>> int (*vfio_manage)(struct platform_device *, enum > > >>> vfio_platform_op); > > >>> struct device_driver driver; > > >>> }; > > >>> > > >>> This would integrate much more closely into the platform driver > > >>> framework, > > >>> just like the regular vfio driver integrates into the PCI framework. > > >>> Unlike PCI however, you can't just use the generic driver framework to > > >>> unbind the driver, because you still need device specific code. > > >>> > > >> Thanks for these suggestions, really helpful. > > >> > > >> What I don't understand in the latter example is how VFIO knows which > > >> struct platform_driver to interact with? > > > > > > This would assume that the driver remains bound to the device, so VFIO > > > gets a pointer to the device from somewhere (as it does today) and then > > > follows the dev->driver pointer to get to the platform_driver. > > The complexity of managing a bi-modal driver seems like far more than a > little bit of code duplication in a device specific reset module and > extends into how userspace makes devices available through vfio, so I > think it's too late for that discussion. > I have had extremely limited exposure to the implementation details of the drivers for devices relevant for VFIO platform, so apologies for asking stupid questions. I'm sure that your point is valid, I just fully understand how the complexities of a bi-modal driver arise? Is it simply that the reset function in a particular device driver may not be self-contained so therefore the whole driver would need to be refactored to be able to do a reset for the purpose of VFIO? > > >> Also, just so I'm sure I understand correctly, VFIO_PLATFORM_UNBIND is > > >> then called by VFIO before the VFIO driver unbinds from the device > > >> (unbinding the platform driver from the device being a completely > > >> separate thing)? > > > > > > This is where we'd need a little more changes for this approach. Instead > > > of unbinding the device from its driver, the idea would be that the > > > driver remains bound as far as the driver model is concerned, but > > > it would be in a quiescent state where no other subsystem interacts with > > > it (i.e. it gets unregistered from networking core or whichever it uses). > > > > Currently we use the same mechanism as for PCI, ie. unbind the native > > driver and then bind VFIO platform driver in its place. Don't you think > > changing this may be a pain for user-space tools that are designed to > > work that way for PCI? > > > > My personal preference would be to start with your first proposal since > > it looks (to me) less complex and "unknown" that the 2d approach. > > > > Let's wait for Alex opinion too... > > I thought the reason we took the approach we have now is so that we > don't have reset code loaded into the kernel unless we have a device > that needs it. Therefore we don't really want to preemptively load all > the reset drivers and have them do a registration. The unfortunate > side-effect of that is the platform code needs to go looking for the > driver. Does the current approach have a separate driver for doing VFIO reset or does it reuse the existing device driver? Why does the driver registering itself instead of using symbol_get imply that we must load reset drivers that we don't need? Thanks, -Christoffer > We do that via the __symbol_get() trick, which only fails > without modules because the underscore variant isn't defined in that > case. I remember asking Eric previously why we're using that rather > than symbol_get(), I've since forgotten his answer, but the fact that > __symbol_get() is only defined for modules makes it moot, we either need > to make symbol_get() work or define __symbol_get() for non-module > builds. > > Otherwise, we should probably abandon the idea of these reset functions > being modules and build them into the vfio platform driver (which would > still be less loaded, dead code than a bi-modal host driver). Thanks, > > Alex > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm