| Message ID | 1422555950-31821-9-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ian, On 02/02/15 15:59, Ian Campbell wrote: > On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote: >> There is a one-to-one mapping between each re-distributors and processors. >> Each re-distributors can be accessed by any processor at any time. For >> instance during the initialization of the GIC, the drivers will browse >> the re-distributor to find the one associated to the current processor >> (via GICR_TYPER). So each re-distributor has its own MMIO region. >> >> The current implementation of the vGICv3 emulation assumes that the >> re-distributor region is banked. Therefore, the processor won't be able >> to access other re-distributor. While this is working fine for Linux, a >> processor will only access GICR_TYPER to find the associated re-distributor, >> we should have a correct implementation for the other operating system. > > You could state something stronger here, which is that it is wrong and > should be fixed as a matter of principal. The fact that we happen to get > away with it for some versions of Linux is an aside (although worth > mentioning) I will rework the paragraph. >> >> All emulated registers of the re-distributors take a vCPU in parameter >> and necessary lock. Therefore concurrent access is already properly handled. >> >> The missing bit is retrieving the right vCPU following the region accessed. >> Retrieving the right vCPU could be slow, so it has been divided in 2 paths: >> - fast path: The current vCPU is accessing its own re-distributor >> - slow path: The current vCPU is accessing an other re-distributor > > "another". > >> >> As the processor needs to initialize itself, the former case is very >> common. To handle the access quickly, the base address of the >> re-distributor is computed and stored per-vCPU during the vCPU initialization. >> >> The latter is less common and more complicate to handle. The re-distributors >> can be spread accross multiple regions in the memory. > > "across" > >> + /* >> + * Find the region where the re-distributor lives. For this purpose, >> + * we look one region ahead as only MMIO range for redistributors >> + * traps here. >> + * Note: We assume that the region are ordered. > > You could also check base+size vs gpa to avoid this limitation. IHMO, this limitation is harmless. This would happen for DOM0 if the device tree doesn't sort the region. AFAICT, we have a similar limitation for the memory region. Although I could sort them when we build DOM0. >> + >> + /* >> + * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If >> + * it's the case, the guest will receive a data abort and won't be >> + * able to boot. > > Is cpu hotplug a factor here? Do we support guests booting with offline > cpus yet? The "problem" is not because of CPU hotpluging. XEN_DOMCTL_max_vcpus doesn't allow the change of the number of vCPUs. AFAICT, this won't be supported (see comments within the code). But, the domctl may not set a vCPU if it's fail to initialize it. So in theory it would be possible to have d->vcpu[vcpu_id] == NULL. But in practice, the toolstack won't continue if one of the VCPU has not been allocated. I felt it was good to mention it for documentation purpose. > >> + /* >> + * Safety check mostly for DOM0. It's possible to have more vCPU >> + * than the number of physical CPU. Maybe we should deny this case? > > In general it's allowed, if a bit silly. Mainly for e.g. people working > on PV spinlock code who want to force contention... Unlikely for dom0 I > suppose. We don't control the DOM0 memory layout, so in practice we can't have more vCPUs than allowed by the hardware. This is because every re-distributors have it's own MMIO region. It's different for guests as we control the memory layout. Regards,
On 02/02/15 16:47, Ian Campbell wrote: > On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 02/02/15 15:59, Ian Campbell wrote: >>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote: >>>> There is a one-to-one mapping between each re-distributors and processors. >>>> Each re-distributors can be accessed by any processor at any time. For >>>> instance during the initialization of the GIC, the drivers will browse >>>> the re-distributor to find the one associated to the current processor >>>> (via GICR_TYPER). So each re-distributor has its own MMIO region. >>>> >>>> The current implementation of the vGICv3 emulation assumes that the >>>> re-distributor region is banked. Therefore, the processor won't be able >>>> to access other re-distributor. While this is working fine for Linux, a >>>> processor will only access GICR_TYPER to find the associated re-distributor, >>>> we should have a correct implementation for the other operating system. >>> >>> You could state something stronger here, which is that it is wrong and >>> should be fixed as a matter of principal. The fact that we happen to get >>> away with it for some versions of Linux is an aside (although worth >>> mentioning) >> >> I will rework the paragraph. >> >>>> >>>> All emulated registers of the re-distributors take a vCPU in parameter >>>> and necessary lock. Therefore concurrent access is already properly handled. >>>> >>>> The missing bit is retrieving the right vCPU following the region accessed. >>>> Retrieving the right vCPU could be slow, so it has been divided in 2 paths: >>>> - fast path: The current vCPU is accessing its own re-distributor >>>> - slow path: The current vCPU is accessing an other re-distributor >>> >>> "another". >>> >>>> >>>> As the processor needs to initialize itself, the former case is very >>>> common. To handle the access quickly, the base address of the >>>> re-distributor is computed and stored per-vCPU during the vCPU initialization. >>>> >>>> The latter is less common and more complicate to handle. The re-distributors >>>> can be spread accross multiple regions in the memory. >>> >>> "across" >>> >>>> + /* >>>> + * Find the region where the re-distributor lives. For this purpose, >>>> + * we look one region ahead as only MMIO range for redistributors >>>> + * traps here. >>>> + * Note: We assume that the region are ordered. >>> >>> You could also check base+size vs gpa to avoid this limitation. >> >> IHMO, this limitation is harmless. This would happen for DOM0 if the >> device tree doesn't sort the region. > > If it can happen then it isn't harmless, and it's easy to avoid so why > not do so. The code is looking one region a-head to avoid check in the case there is only one big re-distributors region. >> AFAICT, we have a similar limitation for the memory region. Although I >> could sort them when we build DOM0. > > I'm not sure we do these days, but in any case two wrongs don't make a > right. See consider_modules. We implicitly assume memory ordering. Anyway, if we do that, we should also check the overlapping between regions, the size of the region is valid (i.e aligned to 64K and correctly sized...),... The main drawbacks here would be DOM0 access the wrong vCPU or receive a data abort. It's not too bad compare to "slowing down" the lookup. If you really want to support non-ordered access, I could do at Domain initialization. >> >>>> + >>>> + /* >>>> + * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If >>>> + * it's the case, the guest will receive a data abort and won't be >>>> + * able to boot. >>> >>> Is cpu hotplug a factor here? Do we support guests booting with offline >>> cpus yet? >> >> The "problem" is not because of CPU hotpluging. XEN_DOMCTL_max_vcpus >> doesn't allow the change of the number of vCPUs. AFAICT, this won't be >> supported (see comments within the code). > >> I felt it was good to mention it for documentation purpose. > > The important thing is that the vcpu array is fully populated by that > hypercall, rather than e.g. deferred until vcpuop_initialise, that's > what matters here really. > > I think the comment is redundant then, or at least misleading in that > implies there is some possibility that d->vcpu[valid_vcpu] is NULL when > there isn't. > >>>> + /* >>>> + * Safety check mostly for DOM0. It's possible to have more vCPU >>>> + * than the number of physical CPU. Maybe we should deny this case? >>> >>> In general it's allowed, if a bit silly. Mainly for e.g. people working >>> on PV spinlock code who want to force contention... Unlikely for dom0 I >>> suppose. >> >> We don't control the DOM0 memory layout, so in practice we can't have >> more vCPUs than allowed by the hardware. This is because every >> re-distributors have it's own MMIO region. > > I'd not be all that surprised to see systems with more rdist region > space than they have physical processors e.g. sku's with different > numbers of cores, but otherwise the same platform. So the current check would be enough? Even though, having more vCPUs than physical CPUs for DOM0 sounds silly. Regards,
Hello Vijay, On 03/02/15 06:47, Vijay Kilari wrote: > On Thu, Jan 29, 2015 at 11:55 PM, Julien Grall <julien.grall@linaro.org> wrote: >> @@ -909,8 +911,15 @@ static int gicv_v3_init(struct domain *d) >> >> for ( i = 0; i < gicv3.rdist_count; i++ ) >> { >> + paddr_t size = gicv3.rdist_regions[i].size; >> + >> d->arch.vgic.rdist_regions[i].base = gicv3.rdist_regions[i].base; >> - d->arch.vgic.rdist_regions[i].size = gicv3.rdist_regions[i].size; >> + d->arch.vgic.rdist_regions[i].size = size; >> + >> + /* Set the first CPU handled by this region */ >> + d->arch.vgic.rdist_regions[i].first_cpu = first_cpu; >> + >> + first_cpu += size / d->arch.vgic.rdist_stride; > > Here you rely on size, The size might not be always map to > number of cpus in that region. We should rely on GICR_TYPER_LAST > to know the last cpu in the region. This is the emulated GIC for DOM0... we don't have to expose the same re-distributors layout as the hardware. By same layout I mean, the base address of the redistributor for virtual CPUn is equal to base address of the re-distributor for physical CPUn. If your DOM0 rely on having exactly the same layout, that means that your kernel is clearly buggy... Regards,
Hi Ian, On 02/02/15 17:38, Ian Campbell wrote: > On Mon, 2015-02-02 at 17:05 +0000, Julien Grall wrote: >> On 02/02/15 16:47, Ian Campbell wrote: >>> On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote: >>>> Hi Ian, >>>> >>>> On 02/02/15 15:59, Ian Campbell wrote: >>>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote: >>>>>> There is a one-to-one mapping between each re-distributors and processors. >>>>>> Each re-distributors can be accessed by any processor at any time. For >>>>>> instance during the initialization of the GIC, the drivers will browse >>>>>> the re-distributor to find the one associated to the current processor >>>>>> (via GICR_TYPER). So each re-distributor has its own MMIO region. >>>>>> >>>>>> The current implementation of the vGICv3 emulation assumes that the >>>>>> re-distributor region is banked. Therefore, the processor won't be able >>>>>> to access other re-distributor. While this is working fine for Linux, a >>>>>> processor will only access GICR_TYPER to find the associated re-distributor, >>>>>> we should have a correct implementation for the other operating system. >>>>> >>>>> You could state something stronger here, which is that it is wrong and >>>>> should be fixed as a matter of principal. The fact that we happen to get >>>>> away with it for some versions of Linux is an aside (although worth >>>>> mentioning) >>>> >>>> I will rework the paragraph. >>>> >>>>>> >>>>>> All emulated registers of the re-distributors take a vCPU in parameter >>>>>> and necessary lock. Therefore concurrent access is already properly handled. >>>>>> >>>>>> The missing bit is retrieving the right vCPU following the region accessed. >>>>>> Retrieving the right vCPU could be slow, so it has been divided in 2 paths: >>>>>> - fast path: The current vCPU is accessing its own re-distributor >>>>>> - slow path: The current vCPU is accessing an other re-distributor >>>>> >>>>> "another". >>>>> >>>>>> >>>>>> As the processor needs to initialize itself, the former case is very >>>>>> common. To handle the access quickly, the base address of the >>>>>> re-distributor is computed and stored per-vCPU during the vCPU initialization. >>>>>> >>>>>> The latter is less common and more complicate to handle. The re-distributors >>>>>> can be spread accross multiple regions in the memory. >>>>> >>>>> "across" >>>>> >>>>>> + /* >>>>>> + * Find the region where the re-distributor lives. For this purpose, >>>>>> + * we look one region ahead as only MMIO range for redistributors >>>>>> + * traps here. >>>>>> + * Note: We assume that the region are ordered. >>>>> >>>>> You could also check base+size vs gpa to avoid this limitation. >>>> >>>> IHMO, this limitation is harmless. This would happen for DOM0 if the >>>> device tree doesn't sort the region. >>> >>> If it can happen then it isn't harmless, and it's easy to avoid so why >>> not do so. >> >> The code is looking one region a-head to avoid check in the case there >> is only one big re-distributors region. >> >>>> AFAICT, we have a similar limitation for the memory region. Although I >>>> could sort them when we build DOM0. >>> >>> I'm not sure we do these days, but in any case two wrongs don't make a >>> right. >> >> See consider_modules. We implicitly assume memory ordering. >> >> Anyway, if we do that, we should also check the overlapping between >> regions, the size of the region is valid (i.e aligned to 64K and >> correctly sized...),... > > I don't see how all that follows, certainly not at run time. Those are > the sorts of things which should be checked at initialisation time and > cause us to complain loudly. My point was, we have to trust a bit the device tree given by the platform. If the device tree is buggy, then bare-metal linux will unlikely boot. But I agree, that we have to sort the region of re-distributors at init. >> The main drawbacks here would be DOM0 access the wrong vCPU or receive a >> data abort. It's not too bad compare to "slowing down" the lookup. > >> If you really want to support non-ordered access, I could do at Domain >> initialization. > > If we are at liberty to sort the list at init time then that would be > fine. I don't think it's matter if we don't expose exactly the same layout for the re-distributors as the physical hardware. So, we can do whatever we want :). > We probably ought to do the same with the memory regions > >>> I'd not be all that surprised to see systems with more rdist region >>> space than they have physical processors e.g. sku's with different >>> numbers of cores, but otherwise the same platform. >> >> So the current check would be enough? Even though, having more vCPUs >> than physical CPUs for DOM0 sounds silly. >> > > I expect so, yes. Great, thanks. I will update the comment in the next version. Regards,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index fdfda0b..1b7ddb3 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -895,6 +895,8 @@ static int gicv_v3_init(struct domain *d) */ if ( is_hardware_domain(d) ) { + unsigned int first_cpu = 0; + d->arch.vgic.dbase = gicv3.dbase; d->arch.vgic.dbase_size = gicv3.dbase_size; @@ -909,8 +911,15 @@ static int gicv_v3_init(struct domain *d) for ( i = 0; i < gicv3.rdist_count; i++ ) { + paddr_t size = gicv3.rdist_regions[i].size; + d->arch.vgic.rdist_regions[i].base = gicv3.rdist_regions[i].base; - d->arch.vgic.rdist_regions[i].size = gicv3.rdist_regions[i].size; + d->arch.vgic.rdist_regions[i].size = size; + + /* Set the first CPU handled by this region */ + d->arch.vgic.rdist_regions[i].first_cpu = first_cpu; + + first_cpu += size / d->arch.vgic.rdist_stride; } d->arch.vgic.nr_regions = gicv3.rdist_count; } @@ -929,6 +938,7 @@ static int gicv_v3_init(struct domain *d) BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS); d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE; d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE; + d->arch.vgic.rdist_regions[0].first_cpu = 0; } return 0; diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 13481ac..378ac82 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -114,6 +114,10 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); *r = aff; + + if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST ) + *r |= GICR_TYPER_LAST; + return 1; case GICR_STATUSR: /* Not implemented */ @@ -619,13 +623,61 @@ write_ignore: return 1; } +static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa, + struct vcpu *v, + uint32_t *offset) +{ + struct domain *d = v->domain; + uint32_t stride = d->arch.vgic.rdist_stride; + paddr_t base; + int i, vcpu_id; + struct vgic_rdist_region *region; + + *offset = gpa & (stride - 1); + base = gpa & ~((paddr_t)stride - 1); + + /* Fast path: the VCPU is trying to access its re-distributor */ + if ( likely(v->arch.vgic.rdist_base == base) ) + return v; + + /* Slow path: the VCPU is trying to access another re-distributor */ + + /* + * Find the region where the re-distributor lives. For this purpose, + * we look one region ahead as only MMIO range for redistributors + * traps here. + * Note: We assume that the region are ordered. + */ + for ( i = 1; i < d->arch.vgic.nr_regions; i++ ) + { + if ( base < d->arch.vgic.rdist_regions[i].base ) + break; + } + + region = &d->arch.vgic.rdist_regions[i - 1]; + + vcpu_id = region->first_cpu + ((base - region->base) / stride); + + if ( unlikely(vcpu_id >= d->max_vcpus) ) + return NULL; + + /* + * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If + * it's the case, the guest will receive a data abort and won't be + * able to boot. + */ + return d->vcpu[vcpu_id]; +} + static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info) { uint32_t offset; perfc_incr(vgicr_reads); - offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); + v = get_vcpu_from_rdist(info->gpa, v, &offset); + if ( unlikely(!v) ) + return 0; if ( offset < SZ_64K ) return __vgic_v3_rdistr_rd_mmio_read(v, info, offset); @@ -645,11 +697,9 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info) perfc_incr(vgicr_writes); - if ( v->domain->arch.vgic.rdist_stride != 0 ) - offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); - else - /* If stride is not set. Default 128K */ - offset = info->gpa & (SZ_128K - 1); + v = get_vcpu_from_rdist(info->gpa, v, &offset); + if ( unlikely(!v) ) + return 0; if ( offset < SZ_64K ) return __vgic_v3_rdistr_rd_mmio_write(v, info, offset); @@ -1084,6 +1134,13 @@ static int vgic_v3_vcpu_init(struct vcpu *v) { int i; uint64_t affinity; + paddr_t rdist_base; + struct vgic_rdist_region *region; + unsigned int last_cpu; + + /* Convenient alias */ + struct domain *d = v->domain; + uint32_t rdist_stride = d->arch.vgic.rdist_stride; /* For SGI and PPI the target is always this CPU */ affinity = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 32 | @@ -1094,6 +1151,48 @@ static int vgic_v3_vcpu_init(struct vcpu *v) for ( i = 0 ; i < 32 ; i++ ) v->arch.vgic.private_irqs->v3.irouter[i] = affinity; + /* + * Find the region where the re-distributor lives. For this purpose, + * we look one region ahead as we have only the first CPU in hand. + */ + for ( i = 1; i < d->arch.vgic.nr_regions; i++ ) + { + if ( v->vcpu_id < d->arch.vgic.rdist_regions[i].first_cpu ) + break; + } + + region = &d->arch.vgic.rdist_regions[i - 1]; + + /* Get the base address of the redistributor */ + rdist_base = region->base; + rdist_base += (v->vcpu_id - region->first_cpu) * rdist_stride; + + /* + * Safety check mostly for DOM0. It's possible to have more vCPU + * than the number of physical CPU. Maybe we should deny this case? + */ + if ( (rdist_base < region->base) || + ((rdist_base + rdist_stride) > (region->base + region->size)) ) + { + dprintk(XENLOG_ERR, + "d%u: Unable to find a re-distributor for VCPU %u\n", + d->domain_id, v->vcpu_id); + return -EINVAL; + } + + v->arch.vgic.rdist_base = rdist_base; + + /* + * If the redistributor is the last one of the + * contiguous region of the vCPU is the last of the domain, set + * VGIC_V3_RDIST_LAST flags. + * Note that we are assuming max_vcpus will never change. + */ + last_cpu = (region->size / rdist_stride) + region->first_cpu - 1; + + if ( v->vcpu_id == last_cpu || (v->vcpu_id == (d->max_vcpus - 1)) ) + v->arch.vgic.flags |= VGIC_V3_RDIST_LAST; + return 0; } diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 3eaa7f0..81e3185 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -106,6 +106,7 @@ struct arch_domain struct vgic_rdist_region { paddr_t base; /* Base address */ paddr_t size; /* Size */ + unsigned int first_cpu; /* First CPU handled */ } rdist_regions[MAX_RDIST_COUNT]; int nr_regions; /* Number of rdist regions */ uint32_t rdist_stride; /* Re-Distributor stride */ @@ -239,6 +240,11 @@ struct arch_vcpu * lr_pending is a subset of vgic.inflight_irqs. */ struct list_head lr_pending; spinlock_t lock; + + /* GICv3: redistributor base and flags for this vCPU */ + paddr_t rdist_base; +#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ + uint8_t flags; } vgic; /* Timer registers */
There is a one-to-one mapping between each re-distributors and processors. Each re-distributors can be accessed by any processor at any time. For instance during the initialization of the GIC, the drivers will browse the re-distributor to find the one associated to the current processor (via GICR_TYPER). So each re-distributor has its own MMIO region. The current implementation of the vGICv3 emulation assumes that the re-distributor region is banked. Therefore, the processor won't be able to access other re-distributor. While this is working fine for Linux, a processor will only access GICR_TYPER to find the associated re-distributor, we should have a correct implementation for the other operating system. All emulated registers of the re-distributors take a vCPU in parameter and necessary lock. Therefore concurrent access is already properly handled. The missing bit is retrieving the right vCPU following the region accessed. Retrieving the right vCPU could be slow, so it has been divided in 2 paths: - fast path: The current vCPU is accessing its own re-distributor - slow path: The current vCPU is accessing an other re-distributor As the processor needs to initialize itself, the former case is very common. To handle the access quickly, the base address of the re-distributor is computed and stored per-vCPU during the vCPU initialization. The latter is less common and more complicate to handle. The re-distributors can be spread accross multiple regions in the memory. During the domain creation, Xen will browse those regions to find the first vCPU handled by this region. When an access hits the slow path, Xen will: 1) Retrieve the region using the base address of the re-distributor accessed 2) Find the vCPU ID attached to the redistributor 3) Check the validity of the vCPU. If it's not valid, a data abort will be injected to the guest Finally, this patch also correctly support the bit GICR_TYPER.LAST which indicates if the redistributor is the last one of the contiguous region. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Linux doesn't access the redistributor from another processor, except for GICR_TYPER during processor initialization. As it banks it will quickly get the "correct" redistributor. But ideally this should be backported to Xen 4.5. Changes in v2: - Patch added --- xen/arch/arm/gic-v3.c | 12 ++++- xen/arch/arm/vgic-v3.c | 111 ++++++++++++++++++++++++++++++++++++++++--- xen/include/asm-arm/domain.h | 6 +++ 3 files changed, 122 insertions(+), 7 deletions(-)