RE: [PATCH v9 03/19] vfio: platform: add the VFIO PLATFORM module to Kconfig
> -Original Message- > From: Hongbo Zhang [mailto:hongbo.zh...@linaro.org] > Sent: Wednesday, November 12, 2014 4:09 PM > To: Bhushan Bharat-R65777 > Cc: Antonios Motakis; open list:VFIO DRIVER; will.dea...@arm.com; > alex.william...@redhat.com; open list; io...@lists.linux-foundation.org; > t...@virtualopensystems.com; kvm...@lists.cs.columbia.edu > Subject: Re: [PATCH v9 03/19] vfio: platform: add the VFIO PLATFORM module to > Kconfig > > On 12 November 2014 18:05, bharat.bhus...@freescale.com > wrote: > > Hi, > > > > > > > > This is not yet supported on Freescale PowerPC. I am still in process > > of upstreaming the FSL PAMU specific patches for same. > > > > Initial plan is to test with PCIe devices and then with Platform devices. > > > > I see there is already driver/iommu/fsl_pamu.c, doesn't it work? We need VFIO iommu driver for same. > Could you explain briefly what is wrong? I've heard that the vfio pci works on > powerpc platforms. Yes, patches are in Freescale internal git repository. But those patches are yet to be upstreamed. I have started working on same. Thanks -Bharat > > > > > > > Thanks > > > > -Bharat > > > > > > > > From: kvmarm-boun...@lists.cs.columbia.edu > > [mailto:kvmarm-boun...@lists.cs.columbia.edu] On Behalf Of Hongbo > > Zhang > > Sent: Wednesday, November 12, 2014 3:08 PM > > To: Antonios Motakis > > Cc: open list:VFIO DRIVER; will.dea...@arm.com; > > alex.william...@redhat.com; open list; > > io...@lists.linux-foundation.org; t...@virtualopensystems.com; > > kvm...@lists.cs.columbia.edu > > Subject: Re: [PATCH v9 03/19] vfio: platform: add the VFIO PLATFORM > > module to Kconfig > > > > > > > > > > > > > > > > On 28 October 2014 02:07, Antonios Motakis > > wrote: > > > > Enable building the VFIO PLATFORM driver that allows to use Linux > > platform devices with VFIO. > > > > Signed-off-by: Antonios Motakis > > --- > > drivers/vfio/Kconfig | 1 + > > drivers/vfio/Makefile | 1 + > > drivers/vfio/platform/Kconfig | 9 + > > drivers/vfio/platform/Makefile | 4 > > 4 files changed, 15 insertions(+) > > create mode 100644 drivers/vfio/platform/Kconfig create mode 100644 > > drivers/vfio/platform/Makefile > > > > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig index > > a0abe04..962fb80 100644 > > --- a/drivers/vfio/Kconfig > > +++ b/drivers/vfio/Kconfig > > @@ -27,3 +27,4 @@ menuconfig VFIO > > If you don't know what to do here, say N. > > > > source "drivers/vfio/pci/Kconfig" > > +source "drivers/vfio/platform/Kconfig" > > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index > > 0b035b1..dadf0ca 100644 > > --- a/drivers/vfio/Makefile > > +++ b/drivers/vfio/Makefile > > @@ -3,3 +3,4 @@ obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o > > obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o > > obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o > > obj-$(CONFIG_VFIO_PCI) += pci/ > > +obj-$(CONFIG_VFIO_PLATFORM) += platform/ > > diff --git a/drivers/vfio/platform/Kconfig > > b/drivers/vfio/platform/Kconfig new file mode 100644 index > > 000..c51af17 > > --- /dev/null > > +++ b/drivers/vfio/platform/Kconfig > > @@ -0,0 +1,9 @@ > > +config VFIO_PLATFORM > > + tristate "VFIO support for platform devices" > > + depends on VFIO && EVENTFD && ARM > > > > > > > > Hi Antonios, > > > > Is this only for ARM? how about X86 and PowerPC? > > > > On Freescale's PowerPC platform, the IOMMU is called PAMU (Peripheral > > Access Management Unit), and I am trying to use this VFIO framework on it. > > > > > > > > + help > > + Support for platform devices with VFIO. This is required to make > > + use of platform devices present on the system using the VFIO > > + framework. > > + > > + If you don't know what to do here, say N. > > diff --git a/drivers/vfio/platform/Makefile > > b/drivers/vfio/platform/Makefile new file mode 100644 index > > 000..279862b > > --- /dev/null > > +++ b/drivers/vfio/platform/Makefile > > @@ -0,0 +1,4 @@ > > + > > +vfio-platform-y := vfio_platform.o vfio_platform_common.o > > + > > +obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o > > -- > > 2.1.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe > > linux-kernel" in the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > >
RE: [PATCH v8 07/18] vfio/platform: return info for device memory mapped IO regions
> -Original Message-
> From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
> boun...@lists.linux-foundation.org] On Behalf Of Alex Williamson
> Sent: Tuesday, October 21, 2014 10:05 PM
> To: Antonios Motakis
> Cc: open list:VFIO DRIVER; eric.au...@linaro.org; marc.zyng...@arm.com;
> will.dea...@arm.com; open list; io...@lists.linux-foundation.org;
> t...@virtualopensystems.com; kvm...@lists.cs.columbia.edu;
> christoffer.d...@linaro.org
> Subject: Re: [PATCH v8 07/18] vfio/platform: return info for device memory
> mapped IO regions
>
> On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
> > This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
> > which allows the user to learn about the available MMIO resources of a
> > device.
> >
> > Signed-off-by: Antonios Motakis
> > ---
> > drivers/vfio/platform/vfio_platform_common.c | 93
> > +--
> > drivers/vfio/platform/vfio_platform_private.h | 22 +++
> > 2 files changed, 111 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vfio/platform/vfio_platform_common.c
> > b/drivers/vfio/platform/vfio_platform_common.c
> > index 1e4073f..8a7e474 100644
> > --- a/drivers/vfio/platform/vfio_platform_common.c
> > +++ b/drivers/vfio/platform/vfio_platform_common.c
> > @@ -27,17 +27,84 @@
> >
> > #include "vfio_platform_private.h"
> >
> > +static int vfio_platform_regions_init(struct vfio_platform_device
> > +*vdev) {
> > + int cnt = 0, i;
> > +
> > + while (vdev->get_resource(vdev, cnt))
> > + cnt++;
> > +
> > + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
> > + GFP_KERNEL);
> > + if (!vdev->regions)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < cnt; i++) {
> > + struct resource *res =
> > + vdev->get_resource(vdev, i);
> > +
> > + if (!res)
> > + goto err;
> > +
> > + vdev->regions[i].addr = res->start;
> > + vdev->regions[i].size = resource_size(res);
> > + vdev->regions[i].flags = 0;
> > +
> > + switch (resource_type(res)) {
> > + case IORESOURCE_MEM:
> > + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
> > + break;
> > + case IORESOURCE_IO:
> > + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
> > + break;
>
> Ok, we have support for PIO in platform now (thanks!), does the user know what
> type of region they're dealing with? Do they care? For PCI the user tests
> the
> PCI BAR in config space to determine which type it is. I'm guessing that
> platform would do something similar against the device tree or ACPI, right?
Interested in knowing what type of PIO region in a platform device, is this for
I2C/SPI type of device?
Thanks
-Bharat
>
> > + default:
> > + goto err;
> > + }
> > + }
> > +
> > + vdev->num_regions = cnt;
> > +
> > + return 0;
> > +err:
> > + kfree(vdev->regions);
> > + return -EINVAL;
> > +}
> > +
> > +static void vfio_platform_regions_cleanup(struct vfio_platform_device
> > +*vdev) {
> > + vdev->num_regions = 0;
> > + kfree(vdev->regions);
> > +}
> > +
> > static void vfio_platform_release(void *device_data) {
> > + struct vfio_platform_device *vdev = device_data;
> > +
> > + if (atomic_dec_and_test(&vdev->refcnt))
> > + vfio_platform_regions_cleanup(vdev);
> > +
> > module_put(THIS_MODULE);
> > }
> >
> > static int vfio_platform_open(void *device_data) {
> > + struct vfio_platform_device *vdev = device_data;
> > + int ret;
> > +
> > if (!try_module_get(THIS_MODULE))
> > return -ENODEV;
> >
> > + if (atomic_inc_return(&vdev->refcnt) == 1) {
> > + ret = vfio_platform_regions_init(vdev);
> > + if (ret)
> > + goto err_reg;
> > + }
> > +
> > return 0;
> > +
> > +err_reg:
> > + module_put(THIS_MODULE);
> > + return ret;
>
> Note that if vfio_platform_regions_init() fails then your refcnt is wrong. We
> switched to a mutex in vfio-pci to avoid this. See 61d792562b53.
>
> > }
> >
> > static long vfio_platform_ioctl(void *device_data, @@ -58,15 +125,33
> > @@ static long vfio_platform_ioctl(void *device_data,
> > return -EINVAL;
> >
> > info.flags = vdev->flags;
> > - info.num_regions = 0;
> > + info.num_regions = vdev->num_regions;
> > info.num_irqs = 0;
> >
> > return copy_to_user((void __user *)arg, &info, minsz);
> >
> > - } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> > - return -EINVAL;
> > + } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> > + struct vfio_region_info info;
> > +
> > + minsz = offsetofend(struct vfio_region_info, offset);
> > +
> > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > +
RE: [PATCH 0/7 v3] Guest debug emulation
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Tuesday, August 12, 2014 3:55 PM > To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248 > Subject: Re: [PATCH 0/7 v3] Guest debug emulation > > > On 06.08.14 08:38, Bharat Bhushan wrote: > > This patchset adds debug register and interrupt emulation support for > > guest, which enables running gdb/kgdb etc in guest. > > > > v2->v3 > > - Added One-reg interface for DBSR > > - removed arch->shadow_dbg_reg > > - Addressed some more comments on v2 (detail in individual patch) > > Thanks, applied patches 1-6 to kvm-ppc-queue. That way you only need to respin > patch 7 and add the documentation patch. Thanks Alex, I will add the documentation patch with next version on 7/7 patch. Regards -Bharat > > > Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, August 12, 2014 5:30 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
> On Wed, 2014-08-06 at 12:08 +0530, Bharat Bhushan wrote:
> > @@ -1249,6 +1284,7 @@ int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
> > setup_timer(&vcpu->arch.wdt_timer, kvmppc_watchdog_func,
> > (unsigned long)vcpu);
> >
> > + kvmppc_clear_dbsr();
> > return 0;
>
> This could use a comment for why we're doing this. Also, I'm a bit uneasy
> about
> clearing the whole DBSR here, where we haven't yet switched the debug
> registers
> to guest context.
I think we wanted MRR to not cause debug event to guest, So should we only
clear MRR ?
> It shouldn't actually matter except for deferred debug
> exceptions which are not actually useful (in fact e6500 removed support for
> them),
Exactly, that's why I was clearing complete DBSR. Probably we can have a comment
" Do not let previously set debug events visible to guest. As deferred debug
events
are not supported, so it is ok to clear complete DBSR.
"
Thanks
-Bharat
> but still...
>
> -Scott
>
N�r��yb�X��ǧv�^�){.n�+h����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
RE: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, August 05, 2014 4:23 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
> On Mon, 2014-08-04 at 13:32 +0530, Bharat Bhushan wrote:
> > @@ -735,7 +745,27 @@ static int kvmppc_handle_debug(struct kvm_run *run,
> struct kvm_vcpu *vcpu)
> > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> > u32 dbsr = vcpu->arch.dbsr;
> >
> > - /* Clear guest dbsr (vcpu->arch.dbsr).
> > + if (vcpu->guest_debug == 0) {
> > + /*
> > +* Debug resources belong to Guest.
> > +* Imprecise debug event are not injected
> > +*/
> > + if (dbsr & DBSR_IDE)
> > + return RESUME_GUEST;
>
> This is incorrect. DBSR_IDE shouldn't *cause* an injection, but it shouldn't
> inhibit it either.
Will this work ?
If ((dbsr & DBSR_IDE) && !(dbsr & ~DBSR_IDE))
Return RESUME_GUEST;
>
> > @@ -828,6 +858,8 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu
> *vcpu,
> > case BOOKE_INTERRUPT_DEBUG:
> > /* Save DBSR before preemption is enabled */
> > vcpu->arch.dbsr = mfspr(SPRN_DBSR);
> > + /* MASK out DBSR_MRR */
> > + vcpu->arch.dbsr &= ~DBSR_MRR;
> > kvmppc_clear_dbsr();
> > break;
> > }
>
> DBSR[MRR] can only be set once per host system reset. There's no need to
> filter
> it out here; just make sure the host clears it at some point before this
> point.
Can you please suggest where ? somewhere in KVM initialization ?
> The MRR value doesn't currently survive past kvmppc_clear_dbsr(), so this
> isn't
> helping to preserve it for the host's benefit...
>
> > @@ -1858,6 +1890,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
> > kvm_vcpu *vcpu,
> >
> > if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> > vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> > + vcpu->arch.dbg_reg.dbcr0 = 0;
>
> Again, it's not clear why we need shadow debug registers here. "Just in case
> we
> implement something that can't be implemented" isn't a good reason to keep
> complexity around.
One reason was that setting EDM in guest visible register, For this we need
shadow_reg is used to save/restore state in h/w register (which does not have
DBCR0_EDM) but debug_reg have DBCR0_EDM.
Thanks
-Bharat
>
> -Scott
>
RE: [PATCH 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit KVM_EXIT_DEBUG
> -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, August 05, 2014 4:17 AM > To: Bhushan Bharat-R65777 > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart- > B08248 > Subject: Re: [PATCH 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit > KVM_EXIT_DEBUG > > On Mon, 2014-08-04 at 13:22 +0530, Bharat Bhushan wrote: > > Dbsr is not visible to userspace and we do not think any need to > > expose this to userspace because: > > Userspace cannot inject debug interrupt to guest (as this > > does not know guest ability to handle debug interrupt), so > > userspace will always clear DBSR. > > Now if userspace has to always clear DBSR in KVM_EXIT_DEBUG > > handling then clearing dbsr in kernel looks simple as this > > avoid doing SET_SREGS/set_one_reg() to clear DBSR > > > > Signed-off-by: Bharat Bhushan > > --- > > arch/powerpc/kvm/booke.c | 11 +++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > 322da7d..5c2e26a 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -735,6 +735,17 @@ static int kvmppc_handle_debug(struct kvm_run *run, > struct kvm_vcpu *vcpu) > > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg); > > u32 dbsr = vcpu->arch.dbsr; > > > > + /* Clear guest dbsr (vcpu->arch.dbsr). > > +* dbsr is not visible to userspace and we do not think any > > +* need to expose this to userspace because: > > +* Userspace cannot inject debug interrupt to guest (as this does > > +* not know guest ability to handle debug interrupt), so userspace > > +* will always clear DBSR. > > +* Now if userspace has to always clear DBSR in KVM_EXIT_DEBUG > > +* handling then clearing here looks simple as this > > +* avoid doing SET_SREGS/set_one_reg() to clear DBSR > > +*/ > > + vcpu->arch.dbsr = 0; > > run->debug.arch.status = 0; > > run->debug.arch.address = vcpu->arch.pc; > > > > I think the changelog is adequate -- I don't think we need to be so verbose in > the code itself. The question was just whether this was a userspace-visible > change, and it isn't. Ok, will make a small comment. > > FWIW, I think dbsr should be visible to userspace in general (regardless of > whether it's cleared here), because all guest registers should be visible to > userspace. I may be debugging a guest through means that don't require owning > debug resources, such as stopping and inspecting a guest that has hung or > crashed. Do you mean that we should also add a one-reg interface for DBSR ? Thanks -Bharat > > -Scott >
RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, August 01, 2014 2:16 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
> On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Thursday, July 31, 2014 8:18 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder
> Stuart-
> > > B08248
> > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
> > >
> > > On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, July 29, 2014 3:58 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org;
> > > > > Yoder Stuart-
> > > > > B08248
> > > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
> > > > > and exception
> > > > >
> > > > > Userspace might be interested in
> > > > > the raw value,
> > > >
> > > > With the current design, If userspace is interested then it will not
> > > > get the DBSR.
> > >
> > > Oh, because DBSR isn't currently implemented in sregs or one reg?
> >
> > That is one reason. Another is that if we give dbsr visibility to
> > userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG.
>
> Right -- since I didn't realize DBSR wasn't already exposed, I thought
> userspace already had this responsibility.
>
> > > It looked like it was removing dbsr visibility and the requirement for
> userspace
> > > to clear dbsr. I guess the old way was that the value in
> > > vcpu->arch.dbsr didn't matter until the next debug exception, when it
> > > would be overwritten by the new SPRN_DBSR?
> >
> > But that means old dbsr will be visibility to userspace, which is even bad
> than not visible, no?
> >
> > Also this can lead to old dbsr visible to guest once userspace releases
> > debug resources, but this can be solved by clearing dbsr in
> > kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control &
> > KVM_GUESTDBG_ENABLE)) { }".
>
> I wasn't suggesting that you keep it that way, just clarifying my
> understanding of the current code.
>
>
> >
> > >
> > > > > > + case SPRN_DBCR2:
> > > > > > + /*
> > > > > > +* If userspace is debugging guest then guest
> > > > > > +* can not access debug registers.
> > > > > > +*/
> > > > > > + if (vcpu->guest_debug)
> > > > > > + break;
> > > > > > +
> > > > > > + debug_inst = true;
> > > > > > + vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > > > > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > > > > > break;
> > > > >
> > > > > In what circumstances can the architected and shadow registers differ?
> > > >
> > > > As of now they are same. But I think that if we want to implement other
> > > features like "Freeze Timer (FT)" then they can be different.
> > >
> > > I don't think we can possibly implement Freeze Timer.
> >
> > May be, but in my opinion we should keep this open.
>
> We're not talking about API here -- the implementation should be kept
> simple if there's no imminent need for shadow registers.
>
> > > > I am not sure what we should in that case ?
> > > >
> > > > As we are currently emulating a subset of debug events (IAC, DAC, IC,
> > > > BT and TIE --- DBCR0 emulation) then we should expose status of those
> > > > events in guest dbsr and rest should be cleared ?
> > >
> > > I'm not saying they need to be exposed to the guest, but I don't see where
> you
> > > filter out bits like these.
> >
> > I am trying to get what all bits should be filtered out, all bits
> > except IACx, DACx, IC, BT and TIE (same as event set filtering done
> > when setting DBCR0) ?
> >
> > i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?
>
> Bits like IRPT and RET don't really matter, as you shouldn't see them
> happen. Likewise MRR if you're sure you've cleared it since boot.
We can clear MRR bits when update vcpu->arch->dbsr with SPRM_DBSR in kvm debug
handler
> But
> IDE could be set any time an asynchronous exception happens. I don't
> think you should filter it out, but instead make sure that it doesn't
> cause an exception to be delivered.
So this means that in kvmpp_handle_debug() if DBSR_IDE is set then do not
inject debug interrupt
and
on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set.
case SPRN_DBSR:
vcpu->arch.dbsr &= ~spr_val;
if (!(vcpu->arch.dbsr & ~DBSR_IDE))
kvmppc_core_dequeue_debug(vcpu);
break;
or
vcpu->arch.dbsr &= ~(spr_val | DBSR_IDE);
RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Wood Scott-B07421
> Sent: Thursday, July 31, 2014 8:18 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
> On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, July 29, 2014 3:58 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org;
> > > Yoder Stuart-
> > > B08248
> > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
> > > and exception
> > >
> > > Userspace might be interested in
> > > the raw value,
> >
> > With the current design, If userspace is interested then it will not
> > get the DBSR.
>
> Oh, because DBSR isn't currently implemented in sregs or one reg?
That is one reason. Another is that if we give dbsr visibility to userspace
then userspace have to clear dbsr in handling KVM_EXIT_DEBUG. And we think
there is no gain in doing that because
"
- QEMU cannot inject debug interrupt to guest (as this does not know guest
ability to handle debug interrupt; MSR_DE), so will always clear DBSR.
- If QEMU has to always clear DBSR in handling KVM_EXIT_DEBUG then this
(clearing dbsr in kernel) avoid doing in SET_SREGS/set_one_reg()
" This makes dbsr not visible to userspace.
Also this (clearing of dbsr) should not be part of this patch, this should be a
separate patch. I will do that in next version.
>
> > But why userspace will be interested?
>
> Do you expose all of the hardware's debugging features in your high-level
> interface?
We support h/w breakpoint, watchpoint and IC (single stepping) and status in
userspace exit provide all required information to userspace.
>
> > > plus it's a change from the current API semantics.
> >
> > Can you please let us know how ?
>
> It looked like it was removing dbsr visibility and the requirement for
> userspace
> to clear dbsr. I guess the old way was that the value in
> vcpu->arch.dbsr didn't matter until the next debug exception, when it
> would be overwritten by the new SPRN_DBSR?
But that means old dbsr will be visibility to userspace, which is even bad than
not visible, no?
Also this can lead to old dbsr visible to guest once userspace releases debug
resources, but this can be solved by clearing dbsr in
kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control &
KVM_GUESTDBG_ENABLE)) { }".
>
> > > > + case SPRN_DBCR2:
> > > > + /*
> > > > +* If userspace is debugging guest then guest
> > > > +* can not access debug registers.
> > > > +*/
> > > > + if (vcpu->guest_debug)
> > > > + break;
> > > > +
> > > > + debug_inst = true;
> > > > + vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > > > break;
> > >
> > > In what circumstances can the architected and shadow registers differ?
> >
> > As of now they are same. But I think that if we want to implement other
> features like "Freeze Timer (FT)" then they can be different.
>
> I don't think we can possibly implement Freeze Timer.
May be, but in my opinion we should keep this open.
>
> > > > case SPRN_DBSR:
> > > > + /*
> > > > +* If userspace is debugging guest then guest
> > > > +* can not access debug registers.
> > > > +*/
> > > > + if (vcpu->guest_debug)
> > > > + break;
> > > > +
> > > > vcpu->arch.dbsr &= ~spr_val;
> > > > + if (vcpu->arch.dbsr == 0)
> > > > + kvmppc_core_dequeue_debug(vcpu);
> > > > break;
> > >
> > > Not all DBSR bits cause an exception, e.g. IDE and MRR.
> >
> > I am not sure what we should in that case ?
> >
> > As we are currently emulating a subset of debug events (IAC, DAC, IC,
> > BT and TIE --- DBCR0 emulation) then we should expose status of those
> > events in guest dbsr and rest should be cleared ?
>
> I'm not saying they need to be exposed to the guest, but I don't see where you
> filter out bits like these.
I am trying to get what all bits should be filtered out, all bits except IACx,
DACx, IC, BT and TIE (same as event set filtering done when setting DBCR0) ?
i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?
>
> > > > @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct
> > > > kvm_vcpu *vcpu, int
> > > sprn, ulong spr_val)
> > > > emulated = EMULATE_FAIL;
> > > > }
> > > >
> > > > + if (debug_inst) {
> > > > + switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg);
> > > > + current->thread.debug = vcpu->arch.shadow_dbg_reg;
> >
RE: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register
> -Original Message- > From: Wood Scott-B07421 > Sent: Wednesday, July 30, 2014 11:18 PM > To: Bhushan Bharat-R65777 > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart- > B08248 > Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest > visible register > > On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote: > > > > > -Original Message- > > > From: Wood Scott-B07421 > > > Sent: Tuesday, July 29, 2014 3:22 AM > > > To: Bhushan Bharat-R65777 > > > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; > > > Yoder Stuart- > > > B08248 > > > Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM > > > in guest visible register > > > > > > On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote: > > > > This is not used and even I do not remember why this was added in > > > > first place. > > > > > > > > Signed-off-by: Bharat Bhushan > > > > --- > > > > arch/powerpc/kvm/booke.c | 2 -- > > > > 1 file changed, 2 deletions(-) > > > > > > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > > > > index ab62109..a5ee42c 100644 > > > > --- a/arch/powerpc/kvm/booke.c > > > > +++ b/arch/powerpc/kvm/booke.c > > > > @@ -1804,8 +1804,6 @@ int > > > > kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu > > > *vcpu, > > > > kvm_guest_protect_msr(vcpu, MSR_DE, true); > > > > vcpu->guest_debug = dbg->control; > > > > vcpu->arch.shadow_dbg_reg.dbcr0 = 0; > > > > - /* Set DBCR0_EDM in guest visible DBCR0 register. */ > > > > - vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM; > > > > > > > > if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) > > > > vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC; > > > > > > This was intended to let the guest know that the host owns the debug > > > resources, by analogy to what a JTAG debugger would do. > > > > > > The Power ISA has this "Virtualized Implementation Note": > > > > > > It is the responsibility of the hypervisor to ensure that > > > DBCR0[EDM] is consistent with usage of DEP. > > > > Ok, That means that if MSRP_DEP is set then set DBCR0_EDM and if MSRP_DEP > > is > clear then clear DBCR0_EDM, right? > > We need to implement above mentioned this. > > We should probably clear EDM only when guest debug emulation is working and > enabled (i.e. not until at least patch 6/6). But if EDM is set then guest debug emulation will not start/allowed. Thanks -Bharat > > -Scott >
RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Monday, July 28, 2014 7:35 PM
> To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org
> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
>
> On 11.07.14 10:39, Bharat Bhushan wrote:
> > This patch emulates debug registers and debug exception to support
> > guest using debug resource. This enables running gdb/kgdb etc in
> > guest.
> >
> > On BOOKE architecture we cannot share debug resources between QEMU and
> > guest because:
> > When QEMU is using debug resources then debug exception must
> > be always enabled. To achieve this we set MSR_DE and also set
> > MSRP_DEP so guest cannot change MSR_DE.
> >
> > When emulating debug resource for guest we want guest
> > to control MSR_DE (enable/disable debug interrupt on need).
> >
> > So above mentioned two configuration cannot be supported
> > at the same time. So the result is that we cannot share
> > debug resources between QEMU and Guest on BOOKE architecture.
> >
> > In the current design QEMU gets priority over guest, this means that
> > if QEMU is using debug resources then guest cannot use them and if
> > guest is using debug resource then QEMU can overwrite them.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > Hi Alex,
> >
> > I thought of having some print in register emulation if QEMU is using
> > debug resource, Also when QEMU overwrites guest written values but
> > that looks excessive. If I uses some variable which get set when guest
> > starts using debug registers and check in debug set ioctl then that
> > look ugly. Looking for suggestions
>
> Whatever you do, have QEMU do the print, not the kernel.
>
> >
> > arch/powerpc/include/asm/kvm_ppc.h | 3 +
> > arch/powerpc/kvm/booke.c | 27 +++
> > arch/powerpc/kvm/booke_emulate.c | 157
> +
> > 3 files changed, 187 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> > b/arch/powerpc/include/asm/kvm_ppc.h
> > index e2fd5a1..f3f7611 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32
> > irq,
> u32 *server,
> > extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
> > extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
> >
> > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu); void
> > +kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
> > +
> > union kvmppc_one_reg {
> > u32 wval;
> > u64 dval;
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > fadfe76..c2471ed 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct
> > kvm_vcpu
> *vcpu)
> > clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions);
> > }
> >
> > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) {
> > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); }
> > +
> > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) {
> > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); }
> > +
> > static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32
> srr1)
> > {
> > #ifdef CONFIG_KVM_BOOKE_HV
> > @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run,
> struct kvm_vcpu *vcpu)
> > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> > u32 dbsr = vcpu->arch.dbsr;
> >
> > + if (vcpu->guest_debug == 0) {
> > + /* Debug resources belong to Guest */
> > + if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> > + kvmppc_core_queue_debug(vcpu);
> > +
> > + /* Inject a program interrupt if trap debug is not allowed */
> > + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE))
> > + kvmppc_core_queue_program(vcpu, ESR_PTR);
>
> In that case we would've received a program interrupt and never entered this
> code path, no?
Yes for HV.
But for PR we can be here, MSR_DE is set in h/w msr and guest MSR_DE is not set.
Having a ifdef does not look good but we can have a comment here.
Thanks
-Bharat
>
>
> Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 3:58 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
> On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> > This patch emulates debug registers and debug exception
> > to support guest using debug resource. This enables running
> > gdb/kgdb etc in guest.
> >
> > On BOOKE architecture we cannot share debug resources between QEMU and
> > guest because:
> > When QEMU is using debug resources then debug exception must
> > be always enabled. To achieve this we set MSR_DE and also set
> > MSRP_DEP so guest cannot change MSR_DE.
> >
> > When emulating debug resource for guest we want guest
> > to control MSR_DE (enable/disable debug interrupt on need).
> >
> > So above mentioned two configuration cannot be supported
> > at the same time. So the result is that we cannot share
> > debug resources between QEMU and Guest on BOOKE architecture.
> >
> > In the current design QEMU gets priority over guest, this means that if
> > QEMU is using debug resources then guest cannot use them and if guest is
> > using debug resource then QEMU can overwrite them.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > Hi Alex,
> >
> > I thought of having some print in register emulation if QEMU
> > is using debug resource, Also when QEMU overwrites guest written
> > values but that looks excessive. If I uses some variable which
> > get set when guest starts using debug registers and check in
> > debug set ioctl then that look ugly. Looking for suggestions
> >
> > arch/powerpc/include/asm/kvm_ppc.h | 3 +
> > arch/powerpc/kvm/booke.c | 27 +++
> > arch/powerpc/kvm/booke_emulate.c | 157
> +
> > 3 files changed, 187 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> b/arch/powerpc/include/asm/kvm_ppc.h
> > index e2fd5a1..f3f7611 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32
> > irq,
> u32 *server,
> > extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
> > extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
> >
> > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu);
> > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
> > +
> > union kvmppc_one_reg {
> > u32 wval;
> > u64 dval;
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index fadfe76..c2471ed 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct
> > kvm_vcpu
> *vcpu)
> > clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions);
> > }
> >
> > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
> > +{
> > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
> > +}
> > +
> > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
> > +{
> > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions);
> > +}
> > +
> > static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32
> srr1)
> > {
> > #ifdef CONFIG_KVM_BOOKE_HV
> > @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run,
> struct kvm_vcpu *vcpu)
> > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> > u32 dbsr = vcpu->arch.dbsr;
> >
> > + if (vcpu->guest_debug == 0) {
> > + /* Debug resources belong to Guest */
> > + if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> > + kvmppc_core_queue_debug(vcpu);
>
> Should also check for DBCR0[IDM].
Ok
>
> > +
> > + /* Inject a program interrupt if trap debug is not allowed */
> > + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE))
> > + kvmppc_core_queue_program(vcpu, ESR_PTR);
> > +
> > + return RESUME_GUEST;
> > + }
> > +
> > + /*
> > +* Prepare for userspace exit as debug resources
> > +* are owned by userspace.
> > +*/
> > + vcpu->arch.dbsr = 0;
> > run->debug.arch.status = 0;
> > run->debug.arch.address = vcpu->arch.pc;
>
> Why are you clearing vcpu->arch.dbsr?
It was discussed in some other email, as soon as we decide that the debug
interrupt is handled by QEMU then we clear vcpu->arch->dbsr.
- QEMU cannot inject debug interrupt to guest (as this does not know guest
ability to handle debug interrupt; MSR_DE), so will always clear DBSR.
- If QEMU has to always clear DBSR than in handling KVM_EXIT_DEBUG then this
avoid doing in SET_SREGS
> Userspace might be interested in
> the raw value,
With the current design, If userspace is interested then it will not get the
DBSR. But why userspace will be interested?
> plus it's a change
RE: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 11:20 PM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder
> Stuart-B08248
> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and
> exception
>
> On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
> > On 29.07.14 00:33, Scott Wood wrote:
> > > On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
> > >> On 11.07.14 10:39, Bharat Bhushan wrote:
> > >>> This patch emulates debug registers and debug exception to support
> > >>> guest using debug resource. This enables running gdb/kgdb etc in
> > >>> guest.
> > >>>
> > >>> On BOOKE architecture we cannot share debug resources between QEMU
> > >>> and guest because:
> > >>> When QEMU is using debug resources then debug exception must
> > >>> be always enabled. To achieve this we set MSR_DE and also set
> > >>> MSRP_DEP so guest cannot change MSR_DE.
> > >>>
> > >>> When emulating debug resource for guest we want guest
> > >>> to control MSR_DE (enable/disable debug interrupt on need).
> > >>>
> > >>> So above mentioned two configuration cannot be supported
> > >>> at the same time. So the result is that we cannot share
> > >>> debug resources between QEMU and Guest on BOOKE architecture.
> > >>>
> > >>> In the current design QEMU gets priority over guest, this means
> > >>> that if QEMU is using debug resources then guest cannot use them
> > >>> and if guest is using debug resource then QEMU can overwrite them.
> > >>>
> > >>> Signed-off-by: Bharat Bhushan
> > >>> ---
> > >>> Hi Alex,
> > >>>
> > >>> I thought of having some print in register emulation if QEMU is
> > >>> using debug resource, Also when QEMU overwrites guest written
> > >>> values but that looks excessive. If I uses some variable which get
> > >>> set when guest starts using debug registers and check in debug set
> > >>> ioctl then that look ugly. Looking for suggestions
> > >> Whatever you do, have QEMU do the print, not the kernel.
> > > How would that be accomplished? How would the kernel know to exit
> > > to QEMU, and how would the exit reason be conveyed?
> >
> > QEMU is the one forcefully enabling debug and overwriting guest debug
> > registers, so it also knows when it did overwrite valid ones.
>
> QEMU knows when it overwrites the guest values, but it doesn't know if, after
> enabling host debug, the guest tries to write to the debug registers and it
> gets
> nopped.
Do we want that QEMU first get DBCR0 to know whether it is overwriting whenever
set/clear debug register?
> If we keep the EDM setting, then we can at least say the situation is
> no worse than with a JTAG.
Yes
Thanks
-Bharat
>
> -Scott
>
N�r��yb�X��ǧv�^�){.n�+h����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
RE: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Tuesday, July 29, 2014 7:35 PM > To: Wood Scott-B07421; Bhushan Bharat-R65777 > Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-B08248 > Subject: Re: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE > > > On 29.07.14 00:01, Scott Wood wrote: > > On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote: > >> When userspace is debugging guest then MSR_DE is always set and > >> MSRP_DEP is set so that guest cannot change MSR_DE. > >> Guest debug resources are not yet emulated, So there seems no reason > >> we should stop guest controlling MSR_DE. > >> Also a followup patch will enable debug emulation and that requires > >> guest to control MSR_DE. > > Why does it matter whether we emulate debug resources? We still don't > > want the guest to be able to clear MSR[DE] and thus break host debug. > > The patch description is misleading. This patch changes the default of DEP to > "guest controlled" when it boots up. Once QEMU wants control over the debug > registers, it gets switched to "QEMU controlled" (that code is already there). Yes, now default MSR_DE is controlled by guest and when QEMU wants to use debug resources then MSR_DEP is set, so guest cannot change MSR_DE. Thanks -Bharat > > > Alex
RE: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 3:25 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is
> under
> debug
>
> On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > When userspace (QEMU) is using the debug resource to debug guest then
> > we want MSR_DE to be always set. This patch adds missing MSR_DE
> > setting in "rfci" instruction.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > arch/powerpc/kvm/booke_emulate.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kvm/booke_emulate.c
> > b/arch/powerpc/kvm/booke_emulate.c
> > index 27a4b28..80c51a2 100644
> > --- a/arch/powerpc/kvm/booke_emulate.c
> > +++ b/arch/powerpc/kvm/booke_emulate.c
> > @@ -40,7 +40,11 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
> > static void kvmppc_emul_rfci(struct kvm_vcpu *vcpu) {
> > vcpu->arch.pc = vcpu->arch.csrr0;
> > - kvmppc_set_msr(vcpu, vcpu->arch.csrr1);
> > + /* Force MSR_DE when guest does not own debug facilities */
> > + if (vcpu->guest_debug)
> > + kvmppc_set_msr(vcpu, vcpu->arch.csrr1 | MSR_DE);
> > + else
> > + kvmppc_set_msr(vcpu, vcpu->arch.csrr1);
> > }
> >
> > int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu
> > *vcpu,
>
> It looks like this is already handled by kvmppc_vcpu_sync_debug(), which is
> called by kvmppc_set_msr().
Yes, you are right. This patch is not needed.
Thanks
-Bharat
>
> Plus, it should only be done for HV mode.
>
> -Scott
>
RE: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 29, 2014 3:22 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> B08248
> Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
> visible register
>
> On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
> > This is not used and even I do not remember why this was added in
> > first place.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > arch/powerpc/kvm/booke.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > ab62109..a5ee42c 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
> > kvm_vcpu
> *vcpu,
> > kvm_guest_protect_msr(vcpu, MSR_DE, true);
> > vcpu->guest_debug = dbg->control;
> > vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> > - /* Set DBCR0_EDM in guest visible DBCR0 register. */
> > - vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
> >
> > if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> > vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
>
> This was intended to let the guest know that the host owns the debug
> resources,
> by analogy to what a JTAG debugger would do.
>
> The Power ISA has this "Virtualized Implementation Note":
>
> It is the responsibility of the hypervisor to ensure that
> DBCR0[EDM] is consistent with usage of DEP.
Ok, That means that if MSRP_DEP is set then set DBCR0_EDM and if MSRP_DEP is
clear then clear DBCR0_EDM, right?
We need to implement above mentioned this.
Thanks
-Bharat
>
> -Scott
>
N�r��yb�X��ǧv�^�){.n�+h����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> > > > On 18.07.14 11:57, bharat.bhus...@freescale.com wrote: > > > > > >> -Original Message- > > >> From: Wood Scott-B07421 > > >> Sent: Friday, July 18, 2014 6:19 AM > > >> To: Alexander Graf > > >> Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; > > >> kvm@vger.kernel.org; Yoder > > >> Stuart-B08248 > > >> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering > > >> guest > > >> > > >> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote: > > >>> On 18.07.14 02:36, Scott Wood wrote: > > >>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote: > > >>>>> On 18.07.14 02:28, Scott Wood wrote: > > >>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote: > > >>>>>>> On 17.07.14 18:27, Alexander Graf wrote: > > >>>>>>>> On 17.07.14 18:24, bharat.bhus...@freescale.com wrote: > > >>>>>>>>>> -Original Message- > > >>>>>>>>>> From: Alexander Graf [mailto:ag...@suse.de] > > >>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM > > >>>>>>>>>> To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > > >>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder > > >>>>>>>>>> Stuart-B08248 > > >>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when > > >>>>>>>>>> entering guest > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote: > > >>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by > > >>>>>>>>>>> host or another guest, So this need to be restored when > > >>>>>>>>>>> loading guest > > >> state. > > >>>>>> SPRG3 is not guest writeable. We should be doing this so that > > >>>>>> guest reads of SPRG3 through the alternative read-only SPR > > >>>>>> work, not because > > >>>>>> "SPRG3 can be clobbered by host or another guest". > > >>>>>> > > >>>>>>>>>>> Signed-off-by: Bharat Bhushan > > >>>>>>>>>>> > > >>>>>>>>>>> --- > > >>>>>>>>>>> arch/powerpc/kvm/booke_interrupts.S | 2 ++ > > >>>>>>>>>>> 1 file changed, 2 insertions(+) > > >>>>>>>>>>> > > >>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>>>>> index 2c6deb5ef..0d3403f 100644 > > >>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit: > > >>>>>>>>>>>* written directly to the shared area, so we > > >>>>>>>>>>>* need to reload them here with the guest's values. > > >>>>>>>>>>>*/ > > >>>>>>>>>>> +PPC_LD(r3, VCPU_SHARED_SPRG3, r5) > > >>>>>>>>>>> +mtsprSPRN_SPRG3, r3 > > >>>>>>>>>> We also need to restore it when resuming the host, no? > > >>>>>>>>> I do not think host expect some meaningful value when > > >>>>>>>>> returning from guest, same true for SPRG4-7. > > >>>>>>>>> So there seems no reason to save host values and restore them. > > >>>>>> Linux no longer uses SPRG4-7 for itself. That is not true of > > >>>>>> SPRG3, as Alex points out. > > >>>>>> > > >>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says: > > >>>>>>>> > > >>>>>>>> * All 32-bit:
RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Friday, July 18, 2014 4:25 PM > To: Bhushan Bharat-R65777; Wood Scott-B07421 > Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-B08248 > Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest > > > On 18.07.14 11:57, bharat.bhus...@freescale.com wrote: > > > >> -Original Message- > >> From: Wood Scott-B07421 > >> Sent: Friday, July 18, 2014 6:19 AM > >> To: Alexander Graf > >> Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; > >> kvm@vger.kernel.org; Yoder > >> Stuart-B08248 > >> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering > >> guest > >> > >> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote: > >>> On 18.07.14 02:36, Scott Wood wrote: > >>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote: > >>>>> On 18.07.14 02:28, Scott Wood wrote: > >>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote: > >>>>>>> On 17.07.14 18:27, Alexander Graf wrote: > >>>>>>>> On 17.07.14 18:24, bharat.bhus...@freescale.com wrote: > >>>>>>>>>> -Original Message- > >>>>>>>>>> From: Alexander Graf [mailto:ag...@suse.de] > >>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM > >>>>>>>>>> To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > >>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder > >>>>>>>>>> Stuart-B08248 > >>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when > >>>>>>>>>> entering guest > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote: > >>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host > >>>>>>>>>>> or another guest, So this need to be restored when loading > >>>>>>>>>>> guest > >> state. > >>>>>> SPRG3 is not guest writeable. We should be doing this so that > >>>>>> guest reads of SPRG3 through the alternative read-only SPR work, > >>>>>> not because > >>>>>> "SPRG3 can be clobbered by host or another guest". > >>>>>> > >>>>>>>>>>> Signed-off-by: Bharat Bhushan > >>>>>>>>>>> --- > >>>>>>>>>>> arch/powerpc/kvm/booke_interrupts.S | 2 ++ > >>>>>>>>>>> 1 file changed, 2 insertions(+) > >>>>>>>>>>> > >>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S > >>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S > >>>>>>>>>>> index 2c6deb5ef..0d3403f 100644 > >>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S > >>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S > >>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit: > >>>>>>>>>>>* written directly to the shared area, so we > >>>>>>>>>>>* need to reload them here with the guest's values. > >>>>>>>>>>>*/ > >>>>>>>>>>> +PPC_LD(r3, VCPU_SHARED_SPRG3, r5) > >>>>>>>>>>> +mtsprSPRN_SPRG3, r3 > >>>>>>>>>> We also need to restore it when resuming the host, no? > >>>>>>>>> I do not think host expect some meaningful value when > >>>>>>>>> returning from guest, same true for SPRG4-7. > >>>>>>>>> So there seems no reason to save host values and restore them. > >>>>>> Linux no longer uses SPRG4-7 for itself. That is not true of > >>>>>> SPRG3, as Alex points out. > >>>>>> > >>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says: > >>>>>>>> > >>>>>>>> * All 32-bit: > >>>>>>>>
RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> -Original Message- > From: Wood Scott-B07421 > Sent: Friday, July 18, 2014 6:19 AM > To: Alexander Graf > Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder > Stuart-B08248 > Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest > > On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote: > > On 18.07.14 02:36, Scott Wood wrote: > > > On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote: > > >> On 18.07.14 02:28, Scott Wood wrote: > > >>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote: > > >>>> On 17.07.14 18:27, Alexander Graf wrote: > > >>>>> On 17.07.14 18:24, bharat.bhus...@freescale.com wrote: > > >>>>>>> -Original Message- > > >>>>>>> From: Alexander Graf [mailto:ag...@suse.de] > > >>>>>>> Sent: Thursday, July 17, 2014 9:41 PM > > >>>>>>> To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > > >>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder > > >>>>>>> Stuart-B08248 > > >>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when > > >>>>>>> entering guest > > >>>>>>> > > >>>>>>> > > >>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote: > > >>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host > > >>>>>>>> or another guest, So this need to be restored when loading guest > state. > > >>> SPRG3 is not guest writeable. We should be doing this so that > > >>> guest reads of SPRG3 through the alternative read-only SPR work, > > >>> not because > > >>> "SPRG3 can be clobbered by host or another guest". > > >>> > > >>>>>>>> Signed-off-by: Bharat Bhushan > > >>>>>>>> --- > > >>>>>>>> arch/powerpc/kvm/booke_interrupts.S | 2 ++ > > >>>>>>>> 1 file changed, 2 insertions(+) > > >>>>>>>> > > >>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>> index 2c6deb5ef..0d3403f 100644 > > >>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S > > >>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit: > > >>>>>>>> * written directly to the shared area, so we > > >>>>>>>> * need to reload them here with the guest's values. > > >>>>>>>> */ > > >>>>>>>> +PPC_LD(r3, VCPU_SHARED_SPRG3, r5) > > >>>>>>>> +mtsprSPRN_SPRG3, r3 > > >>>>>>> We also need to restore it when resuming the host, no? > > >>>>>> I do not think host expect some meaningful value when returning > > >>>>>> from guest, same true for SPRG4-7. > > >>>>>> So there seems no reason to save host values and restore them. > > >>> Linux no longer uses SPRG4-7 for itself. That is not true of > > >>> SPRG3, as Alex points out. > > >>> > > >>>>> Hmm - arch/powerpc/include/asm/reg.h says: > > >>>>> > > >>>>>* All 32-bit: > > >>>>>* - SPRG3 current thread_info pointer > > >>>>>*(virtual on BookE, physical on others) > > >>>>> > > >>>>> but I can indeed find no trace of usage anywhere. This at least > > >>>>> needs to go into the patch description. > > >>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so > > >>>> incredibly important that I have no idea how we could possibly > > >>>> run without switching the host value back in very early. And even > > >>>> then our interrupt handlers wouldn't work anymore. > > >>>> > > >>>> This is more complicated :). > > >>> To make this work we need to avoid SPRG3 as well, or at least > > >>> avoid using it for something needed prior to DO_KVM. > > >
RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Thursday, July 17, 2014 9:58 PM > To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248 > Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest > > > On 17.07.14 18:24, bharat.bhus...@freescale.com wrote: > > > >> -Original Message- > >> From: Alexander Graf [mailto:ag...@suse.de] > >> Sent: Thursday, July 17, 2014 9:41 PM > >> To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > >> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248 > >> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering > >> guest > >> > >> > >> On 16.07.14 08:02, Bharat Bhushan wrote: > >>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or > >>> another guest, So this need to be restored when loading guest state. > >>> > >>> Signed-off-by: Bharat Bhushan > >>> --- > >>>arch/powerpc/kvm/booke_interrupts.S | 2 ++ > >>>1 file changed, 2 insertions(+) > >>> > >>> diff --git a/arch/powerpc/kvm/booke_interrupts.S > >>> b/arch/powerpc/kvm/booke_interrupts.S > >>> index 2c6deb5ef..0d3403f 100644 > >>> --- a/arch/powerpc/kvm/booke_interrupts.S > >>> +++ b/arch/powerpc/kvm/booke_interrupts.S > >>> @@ -459,6 +459,8 @@ lightweight_exit: > >>>* written directly to the shared area, so we > >>>* need to reload them here with the guest's values. > >>>*/ > >>> + PPC_LD(r3, VCPU_SHARED_SPRG3, r5) > >>> + mtspr SPRN_SPRG3, r3 > >> We also need to restore it when resuming the host, no? > > I do not think host expect some meaningful value when returning from guest, > same true for SPRG4-7. > > So there seems no reason to save host values and restore them. > > Hmm - arch/powerpc/include/asm/reg.h says: > > * All 32-bit: > * - SPRG3 current thread_info pointer > *(virtual on BookE, physical on others) > > but I can indeed find no trace of usage anywhere. This at least needs to go > into > the patch description. I will add a comment in code as well. Thanks -Bharat > > > Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Thursday, July 17, 2014 9:41 PM > To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org > Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248 > Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest > > > On 16.07.14 08:02, Bharat Bhushan wrote: > > SPRG3 is guest accessible and SPRG3 can be clobbered by host or > > another guest, So this need to be restored when loading guest state. > > > > Signed-off-by: Bharat Bhushan > > --- > > arch/powerpc/kvm/booke_interrupts.S | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/powerpc/kvm/booke_interrupts.S > > b/arch/powerpc/kvm/booke_interrupts.S > > index 2c6deb5ef..0d3403f 100644 > > --- a/arch/powerpc/kvm/booke_interrupts.S > > +++ b/arch/powerpc/kvm/booke_interrupts.S > > @@ -459,6 +459,8 @@ lightweight_exit: > > * written directly to the shared area, so we > > * need to reload them here with the guest's values. > > */ > > + PPC_LD(r3, VCPU_SHARED_SPRG3, r5) > > + mtspr SPRN_SPRG3, r3 > > We also need to restore it when resuming the host, no? I do not think host expect some meaningful value when returning from guest, same true for SPRG4-7. So there seems no reason to save host values and restore them. Thanks -Bharat > > > Alex > > > PPC_LD(r3, VCPU_SHARED_SPRG4, r5) > > mtspr SPRN_SPRG4W, r3 > > PPC_LD(r3, VCPU_SHARED_SPRG5, r5) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] kvm: ppc: bookehv: Save restore SPRN_SPRG9 on guest entry exit
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Thursday, July 17, 2014 9:47 PM
> To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org
> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> Subject: Re: [PATCH] kvm: ppc: bookehv: Save restore SPRN_SPRG9 on guest entry
> exit
>
>
> On 16.07.14 07:49, Bharat Bhushan wrote:
> > SPRN_SPRG is used by debug interrupt handler, so this is required for
> > debug support.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > arch/powerpc/include/asm/kvm_host.h | 1 +
> > arch/powerpc/kernel/asm-offsets.c | 1 +
> > arch/powerpc/kvm/bookehv_interrupts.S | 4
> > 3 files changed, 6 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_host.h
> > b/arch/powerpc/include/asm/kvm_host.h
> > index 372b977..f9e94ed 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -588,6 +588,7 @@ struct kvm_vcpu_arch {
> > u32 mmucfg;
> > u32 eptcfg;
> > u32 epr;
> > + u32 sprg9;
>
> u32? really?
Must be 64, even I did 64bit save/restore below. Will correct in next version.
Thanks
-Bharat
>
>
> Alex
>
> > u32 pwrmgtcr0;
> > u32 crit_save;
> > /* guest debug registers*/
> > diff --git a/arch/powerpc/kernel/asm-offsets.c
> > b/arch/powerpc/kernel/asm-offsets.c
> > index 17ffcb4..ab9ae04 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -668,6 +668,7 @@ int main(void)
> > DEFINE(VCPU_LR, offsetof(struct kvm_vcpu, arch.lr));
> > DEFINE(VCPU_CTR, offsetof(struct kvm_vcpu, arch.ctr));
> > DEFINE(VCPU_PC, offsetof(struct kvm_vcpu, arch.pc));
> > + DEFINE(VCPU_SPRG9, offsetof(struct kvm_vcpu, arch.sprg9));
> > DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
> > DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
> > DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
> > diff --git a/arch/powerpc/kvm/bookehv_interrupts.S
> > b/arch/powerpc/kvm/bookehv_interrupts.S
> > index a1712b8..f45da85 100644
> > --- a/arch/powerpc/kvm/bookehv_interrupts.S
> > +++ b/arch/powerpc/kvm/bookehv_interrupts.S
> > @@ -441,6 +441,7 @@ _GLOBAL(kvmppc_resume_host)
> > #ifdef CONFIG_64BIT
> > PPC_LL r3, PACA_SPRG_VDSO(r13)
> > #endif
> > + mfspr r5, SPRN_SPRG9
> > PPC_STD(r6, VCPU_SHARED_SPRG4, r11)
> > mfspr r8, SPRN_SPRG6
> > PPC_STD(r7, VCPU_SHARED_SPRG5, r11) @@ -448,6 +449,7 @@
> > _GLOBAL(kvmppc_resume_host)
> > #ifdef CONFIG_64BIT
> > mtspr SPRN_SPRG_VDSO_WRITE, r3
> > #endif
> > + PPC_STD(r5, VCPU_SPRG9, r4)
> > PPC_STD(r8, VCPU_SHARED_SPRG6, r11)
> > mfxer r3
> > PPC_STD(r9, VCPU_SHARED_SPRG7, r11) @@ -682,7 +684,9 @@
> > lightweight_exit:
> > mtspr SPRN_SPRG5W, r6
> > PPC_LD(r8, VCPU_SHARED_SPRG7, r11)
> > mtspr SPRN_SPRG6W, r7
> > + PPC_LD(r5, VCPU_SPRG9, r4)
> > mtspr SPRN_SPRG7W, r8
> > + mtspr SPRN_SPRG9, r5
> >
> > /* Load some guest volatiles. */
> > PPC_LL r3, VCPU_LR(r4)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest
> -Original Message- > From: Bhushan Bharat-R65777 > Sent: Wednesday, July 02, 2014 5:07 PM > To: Wood Scott-B07421; Alexander Graf > Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org > Subject: RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection > to > guest > > > > -Original Message- > > From: Wood Scott-B07421 > > Sent: Tuesday, July 01, 2014 10:11 PM > > To: Alexander Graf > > Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; > > kvm@vger.kernel.org > > Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt > > injection to guest > > > > On Tue, 2014-07-01 at 18:22 +0200, Alexander Graf wrote: > > > On 01.07.14 17:35, Scott Wood wrote: > > > > On Tue, 2014-07-01 at 17:04 +0200, Alexander Graf wrote: > > > >> On 01.07.14 16:58, Scott Wood wrote: > > > >>> On Tue, 2014-07-01 at 08:23 +0200, Alexander Graf wrote: > > > I don't think QEMU should be aware of these limitations. > > > >>> OK, but we should at least have some idea of how the whole thing > > > >>> is supposed to work, in order to determine if this is the > > > >>> correct behavior for QEMU. I thought the model was that debug > > > >>> resources are either owned by QEMU or by the guest, and in the > > > >>> latter case, QEMU would never see the debug exception to begin with. > > > >> That's bad for a number of reasons. For starters it's different > > > >> from how > > > >> x86 handles debug registers - and I hate to be different just for > > > >> the sake of being different. > > > > How does it work on x86? > > > > > > It overwrites more-or-less random breakpoints with its own ones, but > > > leaves the others intact ;). > > > > Are you talking about software breakpoints or management of hardware > > debug registers? > > > > > >> So if we do want to declare that debug registers are owned by > > > >> either QEMU or the guest, we should change the semantics for all > > > >> architectures. > > > > If we want to say that ownership of the registers is shared, we > > > > need a plan for how that would actually work. > > > > > > I think you're overengineering here :). When do people actually use > > > gdbstub? Usually when they want to debug a broken guest. We can > > > either > > > > > >* overengineer heavily and reduce the number of registers > > > available to the guest to always have spares > > >* overengineer a bit and turn off guest debugging completely when > > > we use gdbstub > > >* just leave as much alive as we can, hoping that it helps with > > > the debugging > > > > > > Option 3 is what x86 does - and I think it's a reasonable approach. > > > This is not an interface that needs to be 100% consistent and bullet > > > proof, it's a best effort to enable you to debug as much as possible. > > > > I'm not insisting on 100% -- just hoping for some > > explanation/discussion about how it's intended to work for the cases where > > it > can. > > > > How will MSR[DE] and MSRP[DEP] be handled? > > > > How would I go about telling QEMU/KVM that I don't want this shared > > mode, because I don't want guest to interfere with the debugging I'm > > trying to do from QEMU? > > > > Will guest accesses to debug registers cause a userspace exit when > > guest_debug is enabled? > > > > > >> I think we're in a path that is slow enough already to not worry > > > >> about performance. > > > > It's not just about performance, but simplicity of use, and > > > > consistency of API. > > > > > > > > Oh, and it looks like there already exist one reg definitions and > > > > implementations for most of the debug registers. > > > > > > For BookE? Where? > > > > arch/powerpc/kvm/booke.c: KVM_REG_PPC_IACn, KVM_REG_PPC_DACn > > I tried to quickly prototype what I think we want to do (this is not tested) Hi Scott, There is one problem which is stopping us to share debug resource between qemu and guest, looking for suggestions: - As qemu is also using debug resource, We have to set MSR_DE and set MSRP_DEP (guest will not be able to clear MSR_DE). So qemu set debug events will always cause the debug interrupts. - Now guest is also using debug resources and for some reason if guest wants to clear MSR_DE (disable debug interrupt) But it will not be able to disable as MSRP_DEP is set and KVM will not come to know guest willingness to disable MSR_DE. - If the debug interrupts occurs then we will exit to QEMU and this may not a QEMU set event so it will inject interrupt to guest (using one-reg or set-sregs) - Now KVM, when handling one-reg/sregs request to inject debug interrupt, do not know whether guest can handle the debug interrupt or not (as guest might have tried to set/clear MSR_DE). Thanks -Bharat > > --- > diff --git a/arch/powerpc/include/asm/kvm_ppc.h > b/arch/powerpc/include/asm/kvm_ppc.h > index e8b3982..746b5c6 100644 > --- a/arch/powerpc/include/asm/kvm_ppc.h > +++ b/arch/powerpc/include/asm/kvm_ppc.h > @@ -179,6 +
RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 01, 2014 10:11 PM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection
> to
> guest
>
> On Tue, 2014-07-01 at 18:22 +0200, Alexander Graf wrote:
> > On 01.07.14 17:35, Scott Wood wrote:
> > > On Tue, 2014-07-01 at 17:04 +0200, Alexander Graf wrote:
> > >> On 01.07.14 16:58, Scott Wood wrote:
> > >>> On Tue, 2014-07-01 at 08:23 +0200, Alexander Graf wrote:
> > I don't think QEMU should be aware of these limitations.
> > >>> OK, but we should at least have some idea of how the whole thing
> > >>> is supposed to work, in order to determine if this is the correct
> > >>> behavior for QEMU. I thought the model was that debug resources
> > >>> are either owned by QEMU or by the guest, and in the latter case,
> > >>> QEMU would never see the debug exception to begin with.
> > >> That's bad for a number of reasons. For starters it's different
> > >> from how
> > >> x86 handles debug registers - and I hate to be different just for
> > >> the sake of being different.
> > > How does it work on x86?
> >
> > It overwrites more-or-less random breakpoints with its own ones, but
> > leaves the others intact ;).
>
> Are you talking about software breakpoints or management of hardware debug
> registers?
>
> > >> So if we do want to declare that debug registers are owned by
> > >> either QEMU or the guest, we should change the semantics for all
> > >> architectures.
> > > If we want to say that ownership of the registers is shared, we need
> > > a plan for how that would actually work.
> >
> > I think you're overengineering here :). When do people actually use
> > gdbstub? Usually when they want to debug a broken guest. We can either
> >
> >* overengineer heavily and reduce the number of registers available
> > to the guest to always have spares
> >* overengineer a bit and turn off guest debugging completely when
> > we use gdbstub
> >* just leave as much alive as we can, hoping that it helps with the
> > debugging
> >
> > Option 3 is what x86 does - and I think it's a reasonable approach.
> > This is not an interface that needs to be 100% consistent and bullet
> > proof, it's a best effort to enable you to debug as much as possible.
>
> I'm not insisting on 100% -- just hoping for some explanation/discussion about
> how it's intended to work for the cases where it can.
>
> How will MSR[DE] and MSRP[DEP] be handled?
>
> How would I go about telling QEMU/KVM that I don't want this shared mode,
> because I don't want guest to interfere with the debugging I'm trying to do
> from
> QEMU?
>
> Will guest accesses to debug registers cause a userspace exit when guest_debug
> is enabled?
>
> > >> I think we're in a path that is slow enough already to not worry
> > >> about performance.
> > > It's not just about performance, but simplicity of use, and
> > > consistency of API.
> > >
> > > Oh, and it looks like there already exist one reg definitions and
> > > implementations for most of the debug registers.
> >
> > For BookE? Where?
>
> arch/powerpc/kvm/booke.c: KVM_REG_PPC_IACn, KVM_REG_PPC_DACn
I tried to quickly prototype what I think we want to do (this is not tested)
---
diff --git a/arch/powerpc/include/asm/kvm_ppc.h
b/arch/powerpc/include/asm/kvm_ppc.h
index e8b3982..746b5c6 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -179,6 +179,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq,
u32 *server,
extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
+void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu);
+void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
+
/*
* Cuts out inst bits with ordering according to spec.
* That means the leftmost bit is zero. All given bits are included.
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 9f13056..0b7e4e4 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -235,6 +235,16 @@ void kvmppc_core_dequeue_dec(struct kvm_vcpu *vcpu)
clear_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions);
}
+void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
+{
+ kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
+}
+
+void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
+{
+ clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions);
+}
+
void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
struct kvm_interrupt *irq)
{
@@ -841,6 +851,20 @@ static int kvmppc_handle_debug(struct kvm_run *run, struct
kvm_vcpu *vcpu)
struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
u32 dbsr = vcpu->arch.dbsr;
+ /* Userspace (QEMU) is not using debug resou
RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, July 01, 2014 3:42 PM
> To: Bhushan Bharat-R65777; Wood Scott-B07421
> Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection
> to
> guest
>
>
> On 01.07.14 12:06, bharat.bhus...@freescale.com wrote:
> >
> >> -Original Message-
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Tuesday, July 01, 2014 11:53 AM
> >> To: Wood Scott-B07421
> >> Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org;
> >> kvm@vger.kernel.org
> >> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt
> >> injection to guest
> >>
> >>
> >>
> >>> Am 30.06.2014 um 22:25 schrieb Scott Wood :
> >>>
> >>>> On Sun, 2014-06-29 at 23:38 -0500, Bhushan Bharat-R65777 wrote:
> >>>>
> >>>>> -Original Message-
> >>>>> From: Wood Scott-B07421
> >>>>> Sent: Friday, June 27, 2014 11:53 PM
> >>>>> To: Bhushan Bharat-R65777
> >>>>> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> >>>>> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug
> >>>>> interrupt injection to guest
> >>>>>
> >>>>>> On Fri, 2014-06-27 at 11:55 +0530, Bharat Bhushan wrote:
> >>>>>> -/* Force enable debug interrupts when user space wants to debug */
> >>>>>> -if (vcpu->guest_debug) {
> >>>>>> +/*
> >>>>>> + * Force enable debug interrupts when user space wants to debug
> >>>>>> + * and there is no debug interrupt pending for guest to handle.
> >>>>>> + */
> >>>>>> +if (vcpu->guest_debug && !kvmppc_core_pending_debug(vcpu)) {
> >>>>> Are you trying to allow the guest to be simultaneously debugged by
> >>>>> itself and by host userspace? How does this work?
> >>>> Not actually, Currently we are not partitioning debug resources
> >>>> between host userspace and guest. In fact we do not emulate debug
> >>>> registers for guest. But we want host userspace to pass the
> >>>> interrupt to guest if it is not able to handle.
> >>> I don't understand the logic here. A debug interrupt should be
> >>> injected when the programming model in the guest says that a debug
> >>> interrupt should happen. How can that occur currently? If the
> >>> guest didn't set up the debug registers and QEMU still can't handle
> >>> the debug interrupt, that's a bug in QEMU (or KVM, or the hardware...).
> >>> Injecting the interrupt into the guest just adds another bug on top of
> >>> that.
> >> I don't think QEMU should be aware of these limitations.
> >>
> >>>>>> #ifdef CONFIG_KVM_BOOKE_HV
> >>>>>> /*
> >>>>>> * Since there is no shadow MSR, sync MSR_DE into the
> >>>>>> guest @@
> >>>>>> -264,6 +272,16 @@ static void kvmppc_core_dequeue_watchdog(struct
> >>>>>> kvm_vcpu
> >>>>> *vcpu)
> >>>>>> clear_bit(BOOKE_IRQPRIO_WATCHDOG,
> >>>>>> &vcpu->arch.pending_exceptions); }
> >>>>>>
> >>>>>> +static void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) {
> >>>>>> +kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); }
> >>>>>> +
> >>>>>> +static void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) {
> >>>>>> +clear_bit(BOOKE_IRQPRIO_DEBUG,
> >>>>>> +&vcpu->arch.pending_exceptions); }
> >>>>> Is there currently no support for a guest debugging itself (i.e.
> >>>>> guest_debug unset) on e500v2?
> >>>> Yes, It is not yet supported (IACx/DACx/DBCR/DBSR/DSRRx are not yet
> >> emulated).
> >>> How is it useful to inject a debug exception into the guest, until
> >>> these things are emulated?
> >> We don't have to touch QEMU later then ;). But I agree that it would
> >> make a lot of sense to enable guest debugging along the way - it can't be
> that hard, no?
> > Copy pasting my response in another email:
> >
> > "
> > Ok, Till we add support for guest to used debug resource, can we say that
> userspace will still try to inject debug interrupt (as it does not know guest
> capability) to guest but KVM will:
> > - clear guest dbsr
>
> Can we fake the value of DBSR that the guest sees?
Yes; we can hack the dbsr emulation with a comment :)
But why we want that?
Thanks
-Bharat
>
>
> Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, July 01, 2014 11:53 AM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection
> to
> guest
>
>
>
> > Am 30.06.2014 um 22:25 schrieb Scott Wood :
> >
> >> On Sun, 2014-06-29 at 23:38 -0500, Bhushan Bharat-R65777 wrote:
> >>
> >>> -Original Message-
> >>> From: Wood Scott-B07421
> >>> Sent: Friday, June 27, 2014 11:53 PM
> >>> To: Bhushan Bharat-R65777
> >>> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> >>> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt
> >>> injection to guest
> >>>
> On Fri, 2014-06-27 at 11:55 +0530, Bharat Bhushan wrote:
> -/* Force enable debug interrupts when user space wants to debug */
> -if (vcpu->guest_debug) {
> +/*
> + * Force enable debug interrupts when user space wants to debug
> + * and there is no debug interrupt pending for guest to handle.
> + */
> +if (vcpu->guest_debug && !kvmppc_core_pending_debug(vcpu)) {
> >>>
> >>> Are you trying to allow the guest to be simultaneously debugged by
> >>> itself and by host userspace? How does this work?
> >>
> >> Not actually, Currently we are not partitioning debug resources
> >> between host userspace and guest. In fact we do not emulate debug
> >> registers for guest. But we want host userspace to pass the interrupt
> >> to guest if it is not able to handle.
> >
> > I don't understand the logic here. A debug interrupt should be
> > injected when the programming model in the guest says that a debug
> > interrupt should happen. How can that occur currently? If the guest
> > didn't set up the debug registers and QEMU still can't handle the
> > debug interrupt, that's a bug in QEMU (or KVM, or the hardware...).
> > Injecting the interrupt into the guest just adds another bug on top of that.
>
> I don't think QEMU should be aware of these limitations.
>
> >
> #ifdef CONFIG_KVM_BOOKE_HV
> /*
> * Since there is no shadow MSR, sync MSR_DE into the guest
> @@
> -264,6 +272,16 @@ static void kvmppc_core_dequeue_watchdog(struct
> kvm_vcpu
> >>> *vcpu)
> clear_bit(BOOKE_IRQPRIO_WATCHDOG,
> &vcpu->arch.pending_exceptions); }
>
> +static void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) {
> +kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); }
> +
> +static void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) {
> +clear_bit(BOOKE_IRQPRIO_DEBUG,
> +&vcpu->arch.pending_exceptions); }
> >>>
> >>> Is there currently no support for a guest debugging itself (i.e.
> >>> guest_debug unset) on e500v2?
> >>
> >> Yes, It is not yet supported (IACx/DACx/DBCR/DBSR/DSRRx are not yet
> emulated).
> >
> > How is it useful to inject a debug exception into the guest, until
> > these things are emulated?
>
> We don't have to touch QEMU later then ;). But I agree that it would make a
> lot
> of sense to enable guest debugging along the way - it can't be that hard, no?
Copy pasting my response in another email:
"
Ok, Till we add support for guest to used debug resource, can we say that
userspace will still try to inject debug interrupt (as it does not know guest
capability) to guest but KVM will:
- clear guest dbsr
- ratelimited_printk()
"
Thanks
-Bharat
>
> >
> @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
> if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_TSR)
> kvmppc_set_tsr(vcpu, sregs->u.e.tsr);
>
> +if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_DBSR) {
> +vcpu->arch.dbsr = sregs->u.e.dbsr;
> +if (vcpu->arch.dbsr)
> +kvmppc_core_queue_debug(vcpu);
> +else
> +kvmppc_core_dequeue_debug(vcpu);
> +}
> +
> return 0;
> }
> >>>
> >>> one reg?
> >>
> >> We are using SREGS but if required we can use one_reg.
> >
> > I thought we were preferring one reg over sregs for new functionality.
>
> I'm personally torn on this one. The problem here is that the sregs fields and
> values are already reserved. For anything we don't have an API for yet, yes,
> one_reg only. IIUC we have the API here, but were lacking the implementation.
>
>
> Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest
> -Original Message-
> From: Wood Scott-B07421
> Sent: Tuesday, July 01, 2014 1:56 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection
> to
> guest
>
> On Sun, 2014-06-29 at 23:38 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Friday, June 27, 2014 11:53 PM
> > > To: Bhushan Bharat-R65777
> > > Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> > > Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt
> > > injection to guest
> > >
> > > On Fri, 2014-06-27 at 11:55 +0530, Bharat Bhushan wrote:
> > > > - /* Force enable debug interrupts when user space wants to debug
> > > > */
> > > > - if (vcpu->guest_debug) {
> > > > + /*
> > > > +* Force enable debug interrupts when user space wants to debug
> > > > +* and there is no debug interrupt pending for guest to handle.
> > > > +*/
> > > > + if (vcpu->guest_debug && !kvmppc_core_pending_debug(vcpu)) {
> > >
> > > Are you trying to allow the guest to be simultaneously debugged by
> > > itself and by host userspace? How does this work?
> >
> > Not actually, Currently we are not partitioning debug resources
> > between host userspace and guest. In fact we do not emulate debug
> > registers for guest. But we want host userspace to pass the interrupt
> > to guest if it is not able to handle.
>
> I don't understand the logic here. A debug interrupt should be injected when
> the programming model in the guest says that a debug interrupt should happen.
> How can that occur currently? If the guest didn't set up the debug registers
> and QEMU still can't handle the debug interrupt, that's a bug in QEMU (or KVM,
> or the hardware...). Injecting the interrupt into the guest just adds another
> bug on top of that.
Ok, Till we add support for guest to used debug resource, can we say that
userspace will still try to inject debug interrupt (as it does not know guest
capability) to guest but KVM will
- clear guest dbsr
- ratelimited_printk()
Suggestions please ?
Thanks
-Bharat
>
> > > > #ifdef CONFIG_KVM_BOOKE_HV
> > > > /*
> > > > * Since there is no shadow MSR, sync MSR_DE into the
> > > > guest
> @@
> > > > -264,6 +272,16 @@ static void kvmppc_core_dequeue_watchdog(struct
> > > > kvm_vcpu
> > > *vcpu)
> > > > clear_bit(BOOKE_IRQPRIO_WATCHDOG,
> > > > &vcpu->arch.pending_exceptions); }
> > > >
> > > > +static void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) {
> > > > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); }
> > > > +
> > > > +static void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) {
> > > > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions);
> > > > +}
> > >
> > > Is there currently no support for a guest debugging itself (i.e.
> > > guest_debug unset) on e500v2?
> >
> > Yes, It is not yet supported (IACx/DACx/DBCR/DBSR/DSRRx are not yet
> > emulated).
>
> How is it useful to inject a debug exception into the guest, until these
> things
> are emulated?
>
> > > > @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
> > > > if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_TSR)
> > > > kvmppc_set_tsr(vcpu, sregs->u.e.tsr);
> > > >
> > > > + if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_DBSR) {
> > > > + vcpu->arch.dbsr = sregs->u.e.dbsr;
> > > > + if (vcpu->arch.dbsr)
> > > > + kvmppc_core_queue_debug(vcpu);
> > > > + else
> > > > + kvmppc_core_dequeue_debug(vcpu);
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > >
> > > one reg?
> >
> > We are using SREGS but if required we can use one_reg.
>
> I thought we were preferring one reg over sregs for new functionality.
>
> -Scott
>
RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to guest
> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, June 27, 2014 11:53 PM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection
> to
> guest
>
> On Fri, 2014-06-27 at 11:55 +0530, Bharat Bhushan wrote:
> > This patch allow userspace to inject debug interrupt to guest.
> >
> > Signed-off-by: Bharat Bhushan
>
> Could you describe how userspace plans to make use of this, and go into more
> detail about the changes you're making?
When a debug interrupt happens in guest then we switch to host userspace (QEMU)
and if QEMU is not able to handle a debug interrupt then it injects the debug
interrupt to guest. QEMU uses SET_SREGS (not a one_reg interface), with DBSR
have proper values, for injecting the debug interrupt. In SET_SREGS handling
for DBSR register, KVM injects debug interrupt to guest.
>
> > ---
> > arch/powerpc/kvm/booke.c | 31 +--
> > arch/powerpc/kvm/e500mc.c | 10 +-
> > 2 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > bb25937..63ac38c 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -135,6 +135,11 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
> > *vcpu) #endif }
> >
> > +static int kvmppc_core_pending_debug(struct kvm_vcpu *vcpu) {
> > + return test_bit(BOOKE_IRQPRIO_DEBUG,
> > +&vcpu->arch.pending_exceptions); }
> > +
> > static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) {
> > /* Synchronize guest's desire to get debug interrupts into shadow
> > MSR */ @@ -143,8 +148,11 @@ static void kvmppc_vcpu_sync_debug(struct
> > kvm_vcpu
> *vcpu)
> > vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif
> >
> > - /* Force enable debug interrupts when user space wants to debug */
> > - if (vcpu->guest_debug) {
> > + /*
> > +* Force enable debug interrupts when user space wants to debug
> > +* and there is no debug interrupt pending for guest to handle.
> > +*/
> > + if (vcpu->guest_debug && !kvmppc_core_pending_debug(vcpu)) {
>
> Are you trying to allow the guest to be simultaneously debugged by itself and
> by
> host userspace? How does this work?
Not actually, Currently we are not partitioning debug resources between host
userspace and guest. In fact we do not emulate debug registers for guest. But
we want host userspace to pass the interrupt to guest if it is not able to
handle.
>
> > #ifdef CONFIG_KVM_BOOKE_HV
> > /*
> > * Since there is no shadow MSR, sync MSR_DE into the guest @@
> > -264,6 +272,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu
> *vcpu)
> > clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions);
> > }
> >
> > +static void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) {
> > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); }
> > +
> > +static void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) {
> > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); }
>
> Is there currently no support for a guest debugging itself (i.e.
> guest_debug unset) on e500v2?
Yes, It is not yet supported (IACx/DACx/DBCR/DBSR/DSRRx are not yet emulated).
>
> > static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0,
> > u32 srr1) { #ifdef CONFIG_KVM_BOOKE_HV @@ -1332,6 +1350,7 @@ static
> > void get_sregs_base(struct kvm_vcpu *vcpu,
> > sregs->u.e.dec = kvmppc_get_dec(vcpu, tb);
> > sregs->u.e.tb = tb;
> > sregs->u.e.vrsave = vcpu->arch.vrsave;
> > + sregs->u.e.dbsr = vcpu->arch.dbsr;
> > }
> >
> > static int set_sregs_base(struct kvm_vcpu *vcpu, @@ -1356,6 +1375,14
> > @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
> > if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_TSR)
> > kvmppc_set_tsr(vcpu, sregs->u.e.tsr);
> >
> > + if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_DBSR) {
> > + vcpu->arch.dbsr = sregs->u.e.dbsr;
> > + if (vcpu->arch.dbsr)
> > + kvmppc_core_queue_debug(vcpu);
> > + else
> > + kvmppc_core_dequeue_debug(vcpu);
> > + }
> > +
> > return 0;
> > }
>
> one reg?
We are using SREGS but if required we can use one_reg.
>
> > diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> > index 17e4562..ea724f2 100644
> > --- a/arch/powerpc/kvm/e500mc.c
> > +++ b/arch/powerpc/kvm/e500mc.c
> > @@ -212,7 +212,7 @@ static int kvmppc_core_get_sregs_e500mc(struct kvm_vcpu
> *vcpu,
> > struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
> >
> > sregs->u.e.features |= KVM_SREGS_E_ARCH206_MMU | KVM_SREGS_E_PM |
> > - KVM_SREGS_E_PC;
> > + KVM_SREGS_E_PC | KVM_SREGS_E_ED;
> >
> > sregs->u.e.impl_id = KVM_SREGS_E_IMPL_FSL;
> >
> > sregs->u.e.impl.fs
RE: [PATCH] vfio: Fix endianness handling for emulated BARs
> -Original Message- > From: Linuxppc-dev [mailto:linuxppc-dev- > bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Alexey > Kardashevskiy > Sent: Thursday, June 19, 2014 9:18 AM > To: Alex Williamson > Cc: kvm@vger.kernel.org; Nikunj A Dadhania; linux-ker...@vger.kernel.org; > Alexander Graf; linuxppc-...@lists.ozlabs.org > Subject: Re: [PATCH] vfio: Fix endianness handling for emulated BARs > > On 06/19/2014 11:50 AM, Alexey Kardashevskiy wrote: > > On 06/19/2014 10:50 AM, Alexey Kardashevskiy wrote: > >> On 06/19/2014 04:35 AM, Alex Williamson wrote: > >>> On Wed, 2014-06-18 at 21:36 +1000, Alexey Kardashevskiy wrote: > VFIO exposes BARs to user space as a byte stream so userspace can > read it using pread()/pwrite(). Since this is a byte stream, VFIO > should not do byte swapping and simply return values as it gets > them from PCI device. > > Instead, the existing code assumes that byte stream in read/write > is little-endian and it fixes endianness for values which it passes > to ioreadXX/iowriteXX helpers. This works for little-endian as PCI > is little endian and le32_to_cpu/... are stubs. > >>> > >>> vfio read32: > >>> > >>> val = cpu_to_le32(ioread32(io + off)); > >>> > >>> Where the typical x86 case, ioread32 is: > >>> > >>> #define ioread32(addr) readl(addr) > >>> > >>> and readl is: > >>> > >>> __le32_to_cpu(__raw_readl(addr)); > >>> > >>> So we do canceling byte swaps, which are both nops on x86, and end > >>> up returning device endian, which we assume is little endian. > >>> > >>> vfio write32 is similar: > >>> > >>> iowrite32(le32_to_cpu(val), io + off); > >>> > >>> The implicit cpu_to_le32 of iowrite32() and our explicit swap cancel > >>> out, so input data is device endian, which is assumed little. > >>> > This also works for big endian but rather by an accident: it reads > 4 bytes from the stream (@val is big endian), converts to CPU > format (which should be big endian) as it was little endian (@val > becomes actually little > endian) and calls iowrite32() which does not do swapping on big > endian system. > >>> > >>> Really? > >>> > >>> In arch/powerpc/kernel/iomap.c iowrite32() is just a wrapper around > >>> writel(), which seems to use the generic implementation, which does > >>> include a cpu_to_le32. > >> > >> > >> Ouch, wrong comment. iowrite32() does swapping. My bad. > >> > >> > >>> > >>> I also see other big endian archs like parisc doing cpu_to_le32 on > >>> iowrite32, so I don't think this statement is true. I imagine it's > >>> probably working for you because the swap cancel. > >>> > This removes byte swapping and makes use ioread32be/iowrite32be > (and 16bit versions) on big-endian systems. The "be" helpers take > native endian values and do swapping at the moment of writing to a > PCI register using one of "store byte-reversed" instructions. > >>> > >>> So now you want iowrite32() on little endian and iowrite32be() on > >>> big endian, the former does a cpu_to_le32 (which is a nop on little > >>> endian) and the latter does a cpu_to_be32 (which is a nop on big > >>> endian)... > >>> should we just be using __raw_writel() on both? > >> > >> > >> We can do that too. The beauty of iowrite32be on ppc64 is that it > >> does not swap and write separately, it is implemented via the "Store > >> Word Byte-Reverse Indexed X-form" single instruction. > >> > >> And some archs (do not know which ones) may add memory barriers in > >> their implementations of ioread/iowrite. __raw_writel is too raw :) > >> > >>> There doesn't actually > >>> seem to be any change in behavior here, it just eliminates > >>> back-to-back byte swaps, which are a nop on x86, but not power, right? > >> > >> Exactly. No dependency for QEMU. > > > > How about that: > > === > > > > VFIO exposes BARs to user space as a byte stream so userspace can read > > it using pread()/pwrite(). Since this is a byte stream, VFIO should > > not do byte swapping and simply return values as it gets them from PCI > > device. > > > > Instead, the existing code assumes that byte stream in read/write is > > little-endian and it fixes endianness for values which it passes to > > ioreadXX/iowriteXX helpers in native format. The IO helpers do > > swapping again. Since both byte swaps are nops on little-endian host, this > works. > > > > This also works for big endian but rather by an accident: it reads 4 > > bytes from the stream (@val is big endian), converts to CPU format > > (which should be big endian) as it was little endian (and @val becomes > > actually little > > endian) and calls iowrite32() which does swapping on big endian system > > again. So byte swap gets cancelled, __raw_writel() receives a native > > value and then *(volatile unsigned int __force *)PCI_FIX_ADDR(addr) = > > v; just does the right thing. > > I am wrong here, sorry. This is what happens when you watch soccer between 2am >
RE: [PATCH] KVM: PPC: Book3E: Unlock mmu_lock when setting caching atttribute
> -Original Message-
> From: Mihai Caraman [mailto:mihai.cara...@freescale.com]
> Sent: Wednesday, June 18, 2014 9:15 PM
> To: kvm-...@vger.kernel.org
> Cc: kvm@vger.kernel.org; linuxppc-...@lists.ozlabs.org; Caraman Mihai Claudiu-
> B02008; Bhushan Bharat-R65777
> Subject: [PATCH] KVM: PPC: Book3E: Unlock mmu_lock when setting caching
> atttribute
>
> The patch 08c9a188d0d0fc0f0c5e17d89a06bb59c493110f
> kvm: powerpc: use caching attributes as per linux pte
> do not handle properly the error case, letting mmu_lock locked. The lock
> will further generate a RCU stall from kvmppc_e500_emul_tlbwe() caller.
>
> In case of an error go to out label.
>
> Signed-off-by: Mihai Caraman
> Cc: Bharat Bhushan
Thanks mike for fixing this; I am curious to know how you reached to this point
:)
Reviewed-by: Bharat Bhushan
Regards
-Bharat
> ---
> arch/powerpc/kvm/e500_mmu_host.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> b/arch/powerpc/kvm/e500_mmu_host.c
> index 0528fe5..54144c7 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -473,7 +473,8 @@ static inline int kvmppc_e500_shadow_map(struct
> kvmppc_vcpu_e500 *vcpu_e500,
> if (printk_ratelimit())
> pr_err("%s: pte not present: gfn %lx, pfn %lx\n",
> __func__, (long)gfn, pfn);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> kvmppc_e500_ref_setup(ref, gtlbe, pfn, wimg);
>
> --
> 1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] booke/powerpc: define wimge shift mask to fix compilation error
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Tuesday, May 13, 2014 11:18 AM > To: Bhushan Bharat-R65777 > Cc: ; ; Wood Scott-B07421; > Bhushan > Bharat-R65777 > Subject: Re: [PATCH] booke/powerpc: define wimge shift mask to fix compilation > error > > > > > Am 13.05.2014 um 07:05 schrieb Bharat Bhushan : > > > > This fixes below compilation error on SOCs where CONFIG_PHYS_64BIT is > > not defined: > > > > arch/powerpc/kvm/e500_mmu_host.c: In function 'kvmppc_e500_shadow_map': > > | arch/powerpc/kvm/e500_mmu_host.c:631:20: error: 'PTE_WIMGE_SHIFT' > > undeclared > (first use in this function) > > |wimg = (*ptep >> PTE_WIMGE_SHIFT) & MAS2_WIMGE_MASK; > > | ^ > > | arch/powerpc/kvm/e500_mmu_host.c:631:20: note: each undeclared > > | identifier is reported only once for each function it appears in > > | make[1]: *** [arch/powerpc/kvm/e500_mmu_host.o] Error 1 > > > > Signed-off-by: Bharat Bhushan > > This should go to the normal Linux PPC list, no? I was not sure when sending this fix as this fixes issue for KVM build but code changes are in generic files :) No I will send on linuxppc-...@lists.ozlabs.org Thanks -Bharat > > Alex > > > --- > > arch/powerpc/include/asm/pte-fsl-booke.h |2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/pte-fsl-booke.h > > b/arch/powerpc/include/asm/pte-fsl-booke.h > > index 2c12be5..e84dd7e 100644 > > --- a/arch/powerpc/include/asm/pte-fsl-booke.h > > +++ b/arch/powerpc/include/asm/pte-fsl-booke.h > > @@ -37,5 +37,7 @@ > > #define _PMD_PRESENT_MASK (PAGE_MASK) > > #define _PMD_BAD(~PAGE_MASK) > > > > +#define PTE_WIMGE_SHIFT (6) > > + > > #endif /* __KERNEL__ */ > > #endif /* _ASM_POWERPC_PTE_FSL_BOOKE_H */ > > -- > > 1.7.0.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in > > the body of a message to majord...@vger.kernel.org More majordomo info > > at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [1/3] powerpc/vfio: Enable on POWERNV platform
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, December 14, 2013 2:33 AM
> To: Alexey Kardashevskiy
> Cc: linuxppc-...@lists.ozlabs.org; kvm@vger.kernel.org; linux-
> ker...@vger.kernel.org; Alex Williamson; Paul Mackerras; David Gibson; Sethi
> Varun-B16395; Bhushan Bharat-R65777
> Subject: Re: [1/3] powerpc/vfio: Enable on POWERNV platform
>
> On Fri, 2013-12-13 at 14:02 +1100, Alexey Kardashevskiy wrote:
> > On 12/13/2013 10:35 AM, Scott Wood wrote:
> > > On Tue, May 21, 2013 at 01:33:09PM +1000, Alexey Kardashevskiy wrote:
> > >> +static int iommu_add_device(struct device *dev) {
> > >> +struct iommu_table *tbl;
> > >> +int ret = 0;
> > >> +
> > >> +if (WARN_ON(dev->iommu_group)) {
> > >> +pr_warn("iommu_tce: device %s is already in iommu group
> > >> %d,
> skipping\n",
> > >> +dev_name(dev),
> > >> +iommu_group_id(dev->iommu_group));
> > >> +return -EBUSY;
> > >> +}
> > > [snip]
> > >> +static int __init tce_iommu_init(void) {
> > >> +struct pci_dev *pdev = NULL;
> > >> +
> > >> +BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> > >> +
> > >> +for_each_pci_dev(pdev)
> > >> +iommu_add_device(&pdev->dev);
> > >> +
> > >> +bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> > >> +return 0;
> > >> +}
> > >> +
> > >> +subsys_initcall_sync(tce_iommu_init);
> > >
> > > This is missing a check to see whether the appropriate hardware is
> > > present. This file should also be renamed to something less
> > > generic, and depend on a kconfig symbol more specific than CONFIG_PPC64.
> > >
> > > When this is combined with CONFIG_FSL_PAMU on hardware with a PAMU,
> > > I get a bunch of those "WARN_ON(dev->iommu_group)" dumps because
> > > PAMU already got to them. Presumably without PAMU it silently (or
> > > with just pr_debug) bails out at some other point.
> >
> >
> > I posted (yet again) yesterday "[PATCH v11] PPC: POWERNV: move
> > iommu_add_device earlier" which should fix this. And Bharat asked many
> > times for this to get accepted :)
>
> I still get the WARN_ONs even with that patch. You're still registering the
> bus
> notifier unconditionally.
I have not tried v11 but tested V9 version of that patch. And yes, in that
version the bus notifier was not registered unconditionally in kernel/iommu.c .
Thanks
-Bharat
>
> -Scott
>
