Re: [PATCH v2 3/4] target-arm: kvm - support for single step

2015-04-21 Thread Alex Bennée

Peter Maydell peter.mayd...@linaro.org writes:

 On 31 March 2015 at 16:40, Alex Bennée alex.ben...@linaro.org wrote:
 This adds support for single-step. There isn't much to do on the QEMU
 side as after we set-up the request for single step via the debug ioctl
 it is all handled within the kernel.

 Signed-off-by: Alex Bennée alex.ben...@linaro.org

 ---
 v2
   - convert to using HSR_EC

 diff --git a/target-arm/kvm.c b/target-arm/kvm.c
 index 290c1fe..ae0f8b2 100644
 --- a/target-arm/kvm.c
 +++ b/target-arm/kvm.c
 @@ -475,6 +475,7 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
  */

  #define HSR_EC_SHIFT26
 +#define HSR_EC_SOFT_STEP0x32
  #define HSR_EC_SW_BKPT  0x3c

 We already include internals.h in this file, so can you just use
 the EC_* constants and ARM_EL_EC_SHIFT rather than defining
 new ones? (Applies for patch 1 as well.)

  static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
 @@ -483,6 +484,13 @@ static int kvm_handle_debug(CPUState *cs, struct 
 kvm_run *run)
  int hsr_ec = arch_info-hsr  HSR_EC_SHIFT;

  switch (hsr_ec) {
 +case HSR_EC_SOFT_STEP:
 +if (cs-singlestep_enabled) {
 +return true;
 +} else {
 +error_report(Came out of SINGLE STEP when not enabled);

 This can only happen if there's a kernel bug, right?

Sure. Should we report it differently? abort() out?


 +}
 +break;
  case HSR_EC_SW_BKPT:
  if (kvm_find_sw_breakpoint(cs, arch_info-pc)) {
  return true;
 @@ -542,6 +550,9 @@ int kvm_arch_on_sigbus(int code, void *addr)

  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
  {
 +if (cs-singlestep_enabled) {
 +dbg-control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
 +}
  if (kvm_sw_breakpoints_active(cs)) {
  dbg-control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
  }
 --
 2.3.4



 thanks
 -- PMM

-- 
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 3/4] target-arm: kvm - support for single step

2015-04-21 Thread Peter Maydell
On 21 April 2015 at 13:56, Alex Bennée alex.ben...@linaro.org wrote:

 Peter Maydell peter.mayd...@linaro.org writes:
  switch (hsr_ec) {
 +case HSR_EC_SOFT_STEP:
 +if (cs-singlestep_enabled) {
 +return true;
 +} else {
 +error_report(Came out of SINGLE STEP when not enabled);

 This can only happen if there's a kernel bug, right?

 Sure. Should we report it differently? abort() out?

Saying 'This would be a kernel bug' in a comment would do...

-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 06/10] KVM: arm64: guest debug, add SW break point support

2015-04-21 Thread Zhichao Huang
On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
 This adds support for SW breakpoints inserted by userspace.

 We do this by trapping all BKPT exceptions in the
 hypervisor (MDCR_EL2_TDE).

why should we trap all debug exceptions?

The trap for cp14 register r/w seems enough to record relevant
informations to context switch the dbg register while neccessary.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v12 4/9] hw/vfio/platform: add capability to start IRQ propagation

2015-04-21 Thread Eric Auger
Hi Alex,
On 04/17/2015 09:41 PM, Alex Williamson wrote:
 On Fri, 2015-04-17 at 17:31 +0200, Eric Auger wrote:
 Hi Alex,
 On 04/17/2015 12:04 AM, Alex Williamson wrote:
 On Thu, 2015-03-19 at 17:16 +, Eric Auger wrote:
 Add a reset notify function that enables to start the propagation of
 interrupts to the guest.

 Signed-off-by: Eric Auger eric.au...@linaro.org

 ---
 v10 - v11:
 - comment reword

 v8 - v9:
 - handle failure in vfio_irq_starter
 ---
  hw/vfio/platform.c  | 64 
 +
  include/hw/vfio/vfio-platform.h |  8 ++
  2 files changed, 72 insertions(+)

 diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
 index 31c2701..361e01b 100644
 --- a/hw/vfio/platform.c
 +++ b/hw/vfio/platform.c
 @@ -572,6 +572,70 @@ static void vfio_platform_realize(DeviceState *dev, 
 Error **errp)
  }
  }
  
 +/**
 + * vfio_irq_starter: Start IRQ forwarding for a specific VFIO device
 + * @sbdev: Sysbus device handle
 + * @opaque: DeviceState handle of the interrupt controller the device
 + *  is attached to
 + *
 + * The function retrieves the absolute GSI number each VFIO IRQ
 + * corresponds to and start forwarding.
 + */

 Using forwarding here is really making me look for your platform's EOI
 trick (IRQ Forwarding), which isn't here.
 sure

 +static int vfio_irq_starter(SysBusDevice *sbdev, void *opaque)
 +{
 +DeviceState *intc = (DeviceState *)opaque;
 +VFIOPlatformDevice *vdev;
 +VFIOINTp *intp;
 +qemu_irq irq;
 +int gsi, ret;
 +
 +if (object_dynamic_cast(OBJECT(sbdev), TYPE_VFIO_PLATFORM)) {
 +vdev = VFIO_PLATFORM_DEVICE(sbdev);
 +
 +QLIST_FOREACH(intp, vdev-intp_list, next) {
 +gsi = 0;
 +while (1) {
 +irq = qdev_get_gpio_in(intc, gsi);
 +if (irq == intp-qemuirq) {
 +break;
 +}
 +gsi++;
 +}

 A for() loop would be more compact here, but is there some other exit
 condition we can add?  gsi  INT_MAX?
 Currently I do not see any way to retrieve the number of input GPIOs.
 This is static in core/qdev.c. either I leave as is or introduce getters.
 
 Well, INT_MAX is always an option, right? 
yes it is.
 I prefer the way vfio-pci is
 structured where we can ask for the gsi routing via
 pci_device_route_intx_to_irq() rather than searching the entire address
 space.  Can we do something similar on platform to pass in a qemuirq and
 get back a gsi?
I launched a separate thread to find out a solution for qemu_irq to gsi
conversion.

As for the notification mechanism the intx_routing_notifier is part of
the PCIDevice. By analogy this would mean a similar notifier in the
SysBusDevice. I would add a notifier setter in SysbusDevice and call it
on sysbus_connect_irq. This is what I currently think about. I will send
a separate patch and see what happens ... if anyone already is opposed
to that, please let me know ...

  
 +intp-virtualID = gsi;
 +ret = vdev-start_irq_fn(intp);
 +if (ret) {
 +error_report(%s unable to start propagation of IRQ index 
 %d,
 + vdev-vbasedev.name, intp-pin);
 +exit(1);
 +}
 +}
 +}
 +return 0;
 +}
 +
 +/**
 + * vfio_kick_irqs: start IRQ forwarding for all the VFIO devices
 + * attached to the platform bus
 + * @data: Device state handle of the interrupt controller the VFIO IRQs
 + *must be bound to
 + *
 + * Binding to the platform bus IRQs happens on a machine init done
 + * notifier registered by the platform bus. Only at that time the
 + * absolute virtual IRQ = GSI is known and allows to setup IRQFD.
 + * This function typically can be called in a reset notifier registered
 + * by the machine file.
 + */

 So there's not actually an irqfd setup done here yet and what is
 currently done by the above starter function doesn't depend at all on
 the GSI.  Do you perhaps want to setup the vfio-eventfd signaling
 during your initfn and then only connect the eventfd-irqfd to KVM in
 the reset function?  Sort of how vfio-pci does the routing update.

 Not sure I get what you mean here. In above starter I call start_irq_fn.
 This latter starts the injection depending on the chosen technique, 1)
 user side handling, 2) standalone irqfd, 2) irqfd + ARM forwarding.
 For starting 2) and 3) I actually use the GSI set above by
 intp-virtualID = gsi;
 See vfio_start_irqfd_injection.

 Indeed I noticed in VFIO-PCI you first started eventfd and then irqfd
 but I thought I did not need to do that. What is the problem starting
 VFIO signaling on reset (notifier) only? Besides, moving back to 2-step
 settup would not fix my issue of being able to know the gsi to attach to
 my irqfd. I need to further investigate this PCI INTx routing notifier...
 
 vfio-pci effectively has the same issue.  PCI supports supports four
 legacy interrupt lines and we know which of