qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks bas


From: address@hidden
Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks based on memory region offset
Date: Thu, 11 Sep 2014 10:14:44 +0000

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Wednesday, September 10, 2014 4:56 PM
> To: Purcareata Bogdan-B43198; Wood Scott-B07421
> Cc: Caraman Mihai Claudiu-B02008; address@hidden; address@hidden
> Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks
> based on memory region offset
> 
> 
> 
> On 10.09.14 13:40, address@hidden wrote:
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Friday, September 05, 2014 6:47 PM
> >> To: Purcareata Bogdan-B43198
> >> Cc: address@hidden; Caraman Mihai Claudiu-B02008; qemu-
> address@hidden
> >> Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region
> callbacks
> >> based on memory region offset
> >>
> >> On Wed, 2014-09-03 at 14:36 -0400, Bogdan Purcareata wrote:
> >>> This is done due to the fact that the kvm-openpic region_{add,del}
> callbacks
> >>> can be invoked for sections generated from other memory regions as well.
> >> These
> >>> callbacks should handle only requests for the kvm-openpic memory region.
> >>>
> >>> The patch fixes a bug on target-ppc occuring when the "e500-pci-bar0"
> memory
> >>> region is added. This memory region registers an alias to the "e500-ccsr"
> >> memory
> >>> region, which further contains the "kvm-openpic" subregion. Due to this
> >> alias,
> >>> the kvm_openpic_region_add is called once more, with an offset within the
> >>> "e500-pci-bar" memory region. This generates the remapping of the
> >>> in-kernel MPIC at a wrong offset.
> >>
> >> OK, so the problem is that we really do have the MPIC mapped in two
> >> locations (and the kernel API only lets us map one of them).  It would
> >> be nice if the MemoryRegionSection struct indicated the alias being
> >> represented rather than needing to recalculate the non-aliased address.
> >
> > Not sure I fully understand the terminology of the alias being represented.
> In fact, out of the two mentioned in the previous mail, "e500-pci-bar0" and
> "e500-ccsr", I don't know which one is the "alias".
> >
> > If it's the former, thus "e500-pci-bar0" is an alias for "e500-ccsr", this
> information could be propagated down to the MemoryRegionSection. However,
> according to [1], "aliases may point to any type of region, including other
> aliases". So if the MemoryRegionSection has been built by going through a
> chain of aliases, all this information must be included in the structure.
> >
> > If it's the latter, thus "e500-ccsr" is an alias for "e500-pci-bar0", we can
> get to it in the current model as well. For our specific case, the
> MemoryRegionSection points to the "kvm-openpic" MemoryRegion, which in turn
> points to "e500-ccsr" as its parent (container).
> >
> > What do you think?
> 
> I don't think it matters whether a mapping is an alias or not. What your
> patch really does is it only allows for a single mapping to happen. The
> first one wins.
> 
> I think that's reasonable.
> 
> However, there's no need to extend the memory API with anything here.
> The only reason you need the additional call is because you need to
> figure out where you think you were mapped. But since we set up the map
> ourselves in an ioctl, we already know where we are mapped.
> 
> How about this patch?

Yes, this patch fixes the issue and follows a simpler approach.

Would you like to submit it or should I send a v2 with your changes?

Thanks,
Bogdan P.

> 
> Alex
> 
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index e3bce04..3e2cd18 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -45,6 +45,7 @@ typedef struct KVMOpenPICState {
>      MemoryListener mem_listener;
>      uint32_t fd;
>      uint32_t model;
> +    hwaddr mapped;
>  } KVMOpenPICState;
> 
>  static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level)
> @@ -128,7 +129,16 @@ static void kvm_openpic_region_add(MemoryListener
> *listener,
>          return;
>      }
> 
> +    if (opp->mapped) {
> +        /*
> +         * We can only map the MPIC once. Since we are already mapped,
> +         * the best we can do is ignore new maps.
> +         */
> +        return;
> +    }
> +
>      reg_base = section->offset_within_address_space;
> +    opp->mapped = reg_base;
> 
>      attr.group = KVM_DEV_MPIC_GRP_MISC;
>      attr.attr = KVM_DEV_MPIC_BASE_ADDR;
> @@ -155,6 +165,15 @@ static void kvm_openpic_region_del(MemoryListener
> *listener,
>          return;
>      }
> 
> +    if (section->offset_within_address_space != opp->mapped) {
> +        /*
> +         * We can only map the MPIC once. This mapping was a secondary
> +         * one that we couldn't fulfill. Ignore it.
> +         */
> +        return;
> +    }
> +    opp->mapped = 0;
> +
>      attr.group = KVM_DEV_MPIC_GRP_MISC;
>      attr.attr = KVM_DEV_MPIC_BASE_ADDR;
>      attr.addr = (uint64_t)(unsigned long)&reg_base;

reply via email to

[Prev in Thread] Current Thread [Next in Thread]