On Mon, Nov 17, 2014 at 01:58:05PM +0000, Andre Przywara wrote:
> Hej Christoffer,
>
> maybe I just don't see the wood for the trees, so I will explain my view
> on the things below. Please correct me / explain your view!
I think this argument should have taken place as a reply to my e-mail on
the last version of this patch, but ok.
>
> On 14/11/14 11:07, Christoffer Dall wrote:
> > On Fri, Nov 14, 2014 at 10:07:59AM +0000, Andre Przywara wrote:
> >> With everything separated and prepared, we implement a model of a
> >> GICv3 distributor and redistributors by using the existing framework
> >> to provide handler functions for each register group.
> >>
> >> Currently we limit the emulation to a model enforcing a single
> >> security state, with SRE==1 (forcing system register access) and
> >> ARE==1 (allowing more than 8 VCPUs).
> >>
> >> We share some of the functions provided for GICv2 emulation, but take
> >> the different ways of addressing (v)CPUs into account.
> >> Save and restore is currently not implemented.
> >>
> >> Similar to the split-off of the GICv2 specific code, the new emulation
> >> code goes into a new file (vgic-v3-emul.c).
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >
> > å...¨
> >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >> index 335ffe0..b7de0f8 100644
> >> --- a/virt/kvm/arm/vgic.c
> >> +++ b/virt/kvm/arm/vgic.c
> >> @@ -1249,7 +1249,7 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> >> struct kvm_vcpu *vcpu;
> >> int edge_triggered, level_triggered;
> >> int enabled;
> >> - bool ret = true;
> >> + bool ret = true, can_inject = true;
> >>
> >> spin_lock(&dist->lock);
> >>
> >> @@ -1264,6 +1264,11 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> >>
> >> if (irq_num >= VGIC_NR_PRIVATE_IRQS) {
> >> cpuid = dist->irq_spi_cpu[irq_num - VGIC_NR_PRIVATE_IRQS];
> >> + if (cpuid == VCPU_NOT_ALLOCATED) {
> >> + /* Pretend we use CPU0, and prevent injection */
> >> + cpuid = 0;
> >> + can_inject = false;
> >> + }
> >> vcpu = kvm_get_vcpu(kvm, cpuid);
> >> }
> >>
> >> @@ -1285,7 +1290,7 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> >>
> >> enabled = vgic_irq_is_enabled(vcpu, irq_num);
> >>
> >> - if (!enabled) {
> >> + if (!enabled || !can_inject) {
> >> ret = false;
> >> goto out;
> >> }
> >
> > As I wrote, I think this is wrong. What happened here?
>
> can_inject is just a way for stopping "non-targeted" SPIs to be
> _injected_. The spec says in "5.3.4. GICD_IROUTERn":
>
> "If this register is programmed to forward the corresponding interrupt
> to a specific processor (i.e. IRM is zero) and the affinity path does
> not correspond to an implemented processor, then if the corresponding
> interrupt becomes pending it will not be forwarded to any processor and
> will remain pending."
>
> So we can happily make these SPIs pending, but an illegal irq_spi_cpu[]
> entry will just avoid it to be injected, right?
>
> What am I missing?
>
> Do you want a comment here explaining this?
>
No, I missed something. What I missed was that we consider
irq_spi_target in the compute_pending_for_cpu() so that this actually
ends up working.
We really shouldn't be trying to take this "precompute cpu pending
bitmaps instead of calling compute_pending_for_cpu()" shortcut here, at
least I'm beginning to have troubles following the flow. That's for
another time and place though.
I'll review the rest of this version of the series when I get some cycles.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
