Message ID | 20170418114207.81088-1-roger.pau@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Apr 18, 2017 at 12:42:07PM +0100, Roger Pau Monne wrote: >The current vIO APIC for PVH Dom0 doesn't allow non-contiguous GSIs, which >means that all GSIs must belong to an IO APIC. This doesn't match reality, >where there are systems with non-contiguous GSIs. > >In order to fix this add a base_gsi field to each hvm_vioapic struct, in order >to store the base GSI for each emulated IO APIC. For PVH Dom0 those values are >populated based on the hardware ones. > >Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >Reported-by: Chao Gao <chao.gao@intel.com> Test this version again. Tested-by: Chao Gao <chao.gao@intel.com> Thanks Chao
>>> On 18.04.17 at 13:42, <roger.pau@citrix.com> wrote: > @@ -538,7 +523,8 @@ void vioapic_reset(struct domain *d) > for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) > { > struct hvm_vioapic *vioapic = domain_vioapic(d, i); > - unsigned int pin, nr_pins = vioapic->nr_pins; > + unsigned int nr_pins = vioapic->nr_pins, base_gsi = vioapic->base_gsi; > + unsigned int pin; > > memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > for ( pin = 0; pin < nr_pins; pin++ ) > @@ -546,7 +532,7 @@ void vioapic_reset(struct domain *d) > > if ( !is_hardware_domain(d) ) > { > - ASSERT(!i); > + ASSERT(!i && base_gsi == 0); If you really want to check this here, then please don't mix styles: Either ! in both cases or, less preferred, == 0. > @@ -554,6 +540,7 @@ void vioapic_reset(struct domain *d) > { > vioapic->base_address = mp_ioapics[i].mpc_apicaddr; > vioapic->id = mp_ioapics[i].mpc_apicid; > + vioapic->base_gsi = base_gsi; Why did you leave this in place? There's no need to write back what you've just read. I can offer to drop all three hunks left in place above while committing; with them dropped Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Tue, Apr 18, 2017 at 05:52:47AM -0600, Jan Beulich wrote: > >>> On 18.04.17 at 13:42, <roger.pau@citrix.com> wrote: > > @@ -538,7 +523,8 @@ void vioapic_reset(struct domain *d) > > for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) > > { > > struct hvm_vioapic *vioapic = domain_vioapic(d, i); > > - unsigned int pin, nr_pins = vioapic->nr_pins; > > + unsigned int nr_pins = vioapic->nr_pins, base_gsi = vioapic->base_gsi; > > + unsigned int pin; > > > > memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > > for ( pin = 0; pin < nr_pins; pin++ ) > > @@ -546,7 +532,7 @@ void vioapic_reset(struct domain *d) > > > > if ( !is_hardware_domain(d) ) > > { > > - ASSERT(!i); > > + ASSERT(!i && base_gsi == 0); > > If you really want to check this here, then please don't mix styles: > Either ! in both cases or, less preferred, == 0. Please fix that while committing. > > @@ -554,6 +540,7 @@ void vioapic_reset(struct domain *d) > > { > > vioapic->base_address = mp_ioapics[i].mpc_apicaddr; > > vioapic->id = mp_ioapics[i].mpc_apicid; > > + vioapic->base_gsi = base_gsi; > > Why did you leave this in place? There's no need to write back what > you've just read. vioapic_reset zeroes the whole hvm_vioapic structure, so this is needed in order to set the base_gsi to it's value. > I can offer to drop all three hunks left in place above while > committing; with them dropped > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks.
>>> On 18.04.17 at 13:55, <roger.pau@citrix.com> wrote: > On Tue, Apr 18, 2017 at 05:52:47AM -0600, Jan Beulich wrote: >> >>> On 18.04.17 at 13:42, <roger.pau@citrix.com> wrote: >> > @@ -538,7 +523,8 @@ void vioapic_reset(struct domain *d) >> > for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) >> > { >> > struct hvm_vioapic *vioapic = domain_vioapic(d, i); >> > - unsigned int pin, nr_pins = vioapic->nr_pins; >> > + unsigned int nr_pins = vioapic->nr_pins, base_gsi = vioapic->base_gsi; >> > + unsigned int pin; >> > >> > memset(vioapic, 0, hvm_vioapic_size(nr_pins)); >> > for ( pin = 0; pin < nr_pins; pin++ ) >> > @@ -546,7 +532,7 @@ void vioapic_reset(struct domain *d) >> > >> > if ( !is_hardware_domain(d) ) >> > { >> > - ASSERT(!i); >> > + ASSERT(!i && base_gsi == 0); >> >> If you really want to check this here, then please don't mix styles: >> Either ! in both cases or, less preferred, == 0. > > Please fix that while committing. > >> > @@ -554,6 +540,7 @@ void vioapic_reset(struct domain *d) >> > { >> > vioapic->base_address = mp_ioapics[i].mpc_apicaddr; >> > vioapic->id = mp_ioapics[i].mpc_apicid; >> > + vioapic->base_gsi = base_gsi; >> >> Why did you leave this in place? There's no need to write back what >> you've just read. > > vioapic_reset zeroes the whole hvm_vioapic structure, so this is needed in > order to set the base_gsi to it's value. Oh, indeed. But that would have been far better noticeable if you had placed the assignment next to the nr_pins one (which it also clearly pairs with). I guess I'll do that if I end up committing this. Jan
Hi, On 04/18/2017 12:42 PM, Roger Pau Monne wrote: > The current vIO APIC for PVH Dom0 doesn't allow non-contiguous GSIs, which > means that all GSIs must belong to an IO APIC. This doesn't match reality, > where there are systems with non-contiguous GSIs. > > In order to fix this add a base_gsi field to each hvm_vioapic struct, in order > to store the base GSI for each emulated IO APIC. For PVH Dom0 those values are > populated based on the hardware ones. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reported-by: Chao Gao <chao.gao@intel.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Chao Gao <chao.gao@intel.com> > --- > Changes since v2: > - The base GSI cannot be changed by the guest, so there's no need to call > io_apic_gsi_base in vioapic_reset. > - Use an if ... else ... to set base_gsi and nr_pins for the Dom0 and DomU > cases. This avoids having two different checks for is_hardware_domain. > > Changes since v1: > - Calculate the maximum GSI in vioapic_init taking into account the base GSI > of each IO APIC. > - Use the vioapic base_gsi field in the PVH Dom0 build in order to populate > the MADT IO APIC entries. > > I think this is a good canditate to be included in 4.9, it's a bugfix, and it's > only used by PVH Dom0, so the risk is low IMHO. Release-acked-by: Julien Grall <julien.grall@arm.com> Cheers, > --- > xen/arch/x86/hvm/dom0_build.c | 2 +- > xen/arch/x86/hvm/vioapic.c | 63 ++++++++++++++++++++------------------- > xen/include/asm-x86/hvm/vioapic.h | 1 + > 3 files changed, 35 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index db9be87612..020c355faf 100644 > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -719,7 +719,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr) > io_apic->header.length = sizeof(*io_apic); > io_apic->id = domain_vioapic(d, i)->id; > io_apic->address = domain_vioapic(d, i)->base_address; > - io_apic->global_irq_base = io_apic_gsi_base(i); > + io_apic->global_irq_base = domain_vioapic(d, i)->base_gsi; > io_apic++; > } > > diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c > index 5157db7a4e..209d420ae2 100644 > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -64,37 +64,23 @@ static struct hvm_vioapic *addr_vioapic(const struct domain *d, > struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi, > unsigned int *pin) > { > - unsigned int i, base_gsi = 0; > + unsigned int i; > > for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) > { > struct hvm_vioapic *vioapic = domain_vioapic(d, i); > > - if ( gsi >= base_gsi && gsi < base_gsi + vioapic->nr_pins ) > + if ( gsi >= vioapic->base_gsi && > + gsi < vioapic->base_gsi + vioapic->nr_pins ) > { > - *pin = gsi - base_gsi; > + *pin = gsi - vioapic->base_gsi; > return vioapic; > } > - > - base_gsi += vioapic->nr_pins; > } > > return NULL; > } > > -static unsigned int base_gsi(const struct domain *d, > - const struct hvm_vioapic *vioapic) > -{ > - unsigned int nr_vioapics = d->arch.hvm_domain.nr_vioapics; > - unsigned int base_gsi = 0, i = 0; > - const struct hvm_vioapic *tmp; > - > - while ( i < nr_vioapics && (tmp = domain_vioapic(d, i++)) != vioapic ) > - base_gsi += tmp->nr_pins; > - > - return base_gsi; > -} > - > static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic) > { > uint32_t result = 0; > @@ -180,7 +166,7 @@ static void vioapic_write_redirent( > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > union vioapic_redir_entry *pent, ent; > int unmasked = 0; > - unsigned int gsi = base_gsi(d, vioapic) + idx; > + unsigned int gsi = vioapic->base_gsi + idx; > > spin_lock(&d->arch.hvm_domain.irq_lock); > > @@ -340,7 +326,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin) > struct domain *d = vioapic_domain(vioapic); > struct vlapic *target; > struct vcpu *v; > - unsigned int irq = base_gsi(d, vioapic) + pin; > + unsigned int irq = vioapic->base_gsi + pin; > > ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock)); > > @@ -451,7 +437,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector) > { > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > union vioapic_redir_entry *ent; > - unsigned int i, base_gsi = 0; > + unsigned int i; > > ASSERT(has_vioapic(d)); > > @@ -473,19 +459,18 @@ void vioapic_update_EOI(struct domain *d, u8 vector) > if ( iommu_enabled ) > { > spin_unlock(&d->arch.hvm_domain.irq_lock); > - hvm_dpci_eoi(d, base_gsi + pin, ent); > + hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent); > spin_lock(&d->arch.hvm_domain.irq_lock); > } > > if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) && > !ent->fields.mask && > - hvm_irq->gsi_assert_count[base_gsi + pin] ) > + hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] ) > { > ent->fields.remote_irr = 1; > vioapic_deliver(vioapic, pin); > } > } > - base_gsi += vioapic->nr_pins; > } > > spin_unlock(&d->arch.hvm_domain.irq_lock); > @@ -538,7 +523,8 @@ void vioapic_reset(struct domain *d) > for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) > { > struct hvm_vioapic *vioapic = domain_vioapic(d, i); > - unsigned int pin, nr_pins = vioapic->nr_pins; > + unsigned int nr_pins = vioapic->nr_pins, base_gsi = vioapic->base_gsi; > + unsigned int pin; > > memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > for ( pin = 0; pin < nr_pins; pin++ ) > @@ -546,7 +532,7 @@ void vioapic_reset(struct domain *d) > > if ( !is_hardware_domain(d) ) > { > - ASSERT(!i); > + ASSERT(!i && base_gsi == 0); > vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; > vioapic->id = 0; > } > @@ -554,6 +540,7 @@ void vioapic_reset(struct domain *d) > { > vioapic->base_address = mp_ioapics[i].mpc_apicaddr; > vioapic->id = mp_ioapics[i].mpc_apicid; > + vioapic->base_gsi = base_gsi; > } > vioapic->nr_pins = nr_pins; > vioapic->domain = d; > @@ -588,8 +575,18 @@ int vioapic_init(struct domain *d) > > for ( i = 0; i < nr_vioapics; i++ ) > { > - unsigned int nr_pins = is_hardware_domain(d) ? nr_ioapic_entries[i] : > - ARRAY_SIZE(domain_vioapic(d, 0)->domU.redirtbl); > + unsigned int nr_pins, base_gsi; > + > + if ( is_hardware_domain(d) ) > + { > + nr_pins = nr_ioapic_entries[i]; > + base_gsi = io_apic_gsi_base(i); > + } > + else > + { > + nr_pins = ARRAY_SIZE(domain_vioapic(d, 0)->domU.redirtbl); > + base_gsi = 0; > + } > > if ( (domain_vioapic(d, i) = > xmalloc_bytes(hvm_vioapic_size(nr_pins))) == NULL ) > @@ -598,10 +595,16 @@ int vioapic_init(struct domain *d) > return -ENOMEM; > } > domain_vioapic(d, i)->nr_pins = nr_pins; > - nr_gsis += nr_pins; > + domain_vioapic(d, i)->base_gsi = base_gsi; > + nr_gsis = max(nr_gsis, base_gsi + nr_pins); > } > > - ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis); > + /* > + * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but > + * there might be holes in this range (ie: GSIs that don't belong to any > + * vIO APIC). > + */ > + ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis); > > d->arch.hvm_domain.nr_vioapics = nr_vioapics; > vioapic_reset(d); > diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h > index 8ec91d2bd1..2ceb60eaef 100644 > --- a/xen/include/asm-x86/hvm/vioapic.h > +++ b/xen/include/asm-x86/hvm/vioapic.h > @@ -50,6 +50,7 @@ > struct hvm_vioapic { > struct domain *domain; > uint32_t nr_pins; > + unsigned int base_gsi; > union { > XEN_HVM_VIOAPIC(,); > struct hvm_hw_vioapic domU; >
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index db9be87612..020c355faf 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -719,7 +719,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr) io_apic->header.length = sizeof(*io_apic); io_apic->id = domain_vioapic(d, i)->id; io_apic->address = domain_vioapic(d, i)->base_address; - io_apic->global_irq_base = io_apic_gsi_base(i); + io_apic->global_irq_base = domain_vioapic(d, i)->base_gsi; io_apic++; } diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 5157db7a4e..209d420ae2 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -64,37 +64,23 @@ static struct hvm_vioapic *addr_vioapic(const struct domain *d, struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi, unsigned int *pin) { - unsigned int i, base_gsi = 0; + unsigned int i; for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) { struct hvm_vioapic *vioapic = domain_vioapic(d, i); - if ( gsi >= base_gsi && gsi < base_gsi + vioapic->nr_pins ) + if ( gsi >= vioapic->base_gsi && + gsi < vioapic->base_gsi + vioapic->nr_pins ) { - *pin = gsi - base_gsi; + *pin = gsi - vioapic->base_gsi; return vioapic; } - - base_gsi += vioapic->nr_pins; } return NULL; } -static unsigned int base_gsi(const struct domain *d, - const struct hvm_vioapic *vioapic) -{ - unsigned int nr_vioapics = d->arch.hvm_domain.nr_vioapics; - unsigned int base_gsi = 0, i = 0; - const struct hvm_vioapic *tmp; - - while ( i < nr_vioapics && (tmp = domain_vioapic(d, i++)) != vioapic ) - base_gsi += tmp->nr_pins; - - return base_gsi; -} - static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic) { uint32_t result = 0; @@ -180,7 +166,7 @@ static void vioapic_write_redirent( struct hvm_irq *hvm_irq = hvm_domain_irq(d); union vioapic_redir_entry *pent, ent; int unmasked = 0; - unsigned int gsi = base_gsi(d, vioapic) + idx; + unsigned int gsi = vioapic->base_gsi + idx; spin_lock(&d->arch.hvm_domain.irq_lock); @@ -340,7 +326,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin) struct domain *d = vioapic_domain(vioapic); struct vlapic *target; struct vcpu *v; - unsigned int irq = base_gsi(d, vioapic) + pin; + unsigned int irq = vioapic->base_gsi + pin; ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock)); @@ -451,7 +437,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector) { struct hvm_irq *hvm_irq = hvm_domain_irq(d); union vioapic_redir_entry *ent; - unsigned int i, base_gsi = 0; + unsigned int i; ASSERT(has_vioapic(d)); @@ -473,19 +459,18 @@ void vioapic_update_EOI(struct domain *d, u8 vector) if ( iommu_enabled ) { spin_unlock(&d->arch.hvm_domain.irq_lock); - hvm_dpci_eoi(d, base_gsi + pin, ent); + hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent); spin_lock(&d->arch.hvm_domain.irq_lock); } if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) && !ent->fields.mask && - hvm_irq->gsi_assert_count[base_gsi + pin] ) + hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] ) { ent->fields.remote_irr = 1; vioapic_deliver(vioapic, pin); } } - base_gsi += vioapic->nr_pins; } spin_unlock(&d->arch.hvm_domain.irq_lock); @@ -538,7 +523,8 @@ void vioapic_reset(struct domain *d) for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) { struct hvm_vioapic *vioapic = domain_vioapic(d, i); - unsigned int pin, nr_pins = vioapic->nr_pins; + unsigned int nr_pins = vioapic->nr_pins, base_gsi = vioapic->base_gsi; + unsigned int pin; memset(vioapic, 0, hvm_vioapic_size(nr_pins)); for ( pin = 0; pin < nr_pins; pin++ ) @@ -546,7 +532,7 @@ void vioapic_reset(struct domain *d) if ( !is_hardware_domain(d) ) { - ASSERT(!i); + ASSERT(!i && base_gsi == 0); vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; vioapic->id = 0; } @@ -554,6 +540,7 @@ void vioapic_reset(struct domain *d) { vioapic->base_address = mp_ioapics[i].mpc_apicaddr; vioapic->id = mp_ioapics[i].mpc_apicid; + vioapic->base_gsi = base_gsi; } vioapic->nr_pins = nr_pins; vioapic->domain = d; @@ -588,8 +575,18 @@ int vioapic_init(struct domain *d) for ( i = 0; i < nr_vioapics; i++ ) { - unsigned int nr_pins = is_hardware_domain(d) ? nr_ioapic_entries[i] : - ARRAY_SIZE(domain_vioapic(d, 0)->domU.redirtbl); + unsigned int nr_pins, base_gsi; + + if ( is_hardware_domain(d) ) + { + nr_pins = nr_ioapic_entries[i]; + base_gsi = io_apic_gsi_base(i); + } + else + { + nr_pins = ARRAY_SIZE(domain_vioapic(d, 0)->domU.redirtbl); + base_gsi = 0; + } if ( (domain_vioapic(d, i) = xmalloc_bytes(hvm_vioapic_size(nr_pins))) == NULL ) @@ -598,10 +595,16 @@ int vioapic_init(struct domain *d) return -ENOMEM; } domain_vioapic(d, i)->nr_pins = nr_pins; - nr_gsis += nr_pins; + domain_vioapic(d, i)->base_gsi = base_gsi; + nr_gsis = max(nr_gsis, base_gsi + nr_pins); } - ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis); + /* + * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but + * there might be holes in this range (ie: GSIs that don't belong to any + * vIO APIC). + */ + ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis); d->arch.hvm_domain.nr_vioapics = nr_vioapics; vioapic_reset(d); diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h index 8ec91d2bd1..2ceb60eaef 100644 --- a/xen/include/asm-x86/hvm/vioapic.h +++ b/xen/include/asm-x86/hvm/vioapic.h @@ -50,6 +50,7 @@ struct hvm_vioapic { struct domain *domain; uint32_t nr_pins; + unsigned int base_gsi; union { XEN_HVM_VIOAPIC(,); struct hvm_hw_vioapic domU;
The current vIO APIC for PVH Dom0 doesn't allow non-contiguous GSIs, which means that all GSIs must belong to an IO APIC. This doesn't match reality, where there are systems with non-contiguous GSIs. In order to fix this add a base_gsi field to each hvm_vioapic struct, in order to store the base GSI for each emulated IO APIC. For PVH Dom0 those values are populated based on the hardware ones. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reported-by: Chao Gao <chao.gao@intel.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Chao Gao <chao.gao@intel.com> --- Changes since v2: - The base GSI cannot be changed by the guest, so there's no need to call io_apic_gsi_base in vioapic_reset. - Use an if ... else ... to set base_gsi and nr_pins for the Dom0 and DomU cases. This avoids having two different checks for is_hardware_domain. Changes since v1: - Calculate the maximum GSI in vioapic_init taking into account the base GSI of each IO APIC. - Use the vioapic base_gsi field in the PVH Dom0 build in order to populate the MADT IO APIC entries. I think this is a good canditate to be included in 4.9, it's a bugfix, and it's only used by PVH Dom0, so the risk is low IMHO. --- xen/arch/x86/hvm/dom0_build.c | 2 +- xen/arch/x86/hvm/vioapic.c | 63 ++++++++++++++++++++------------------- xen/include/asm-x86/hvm/vioapic.h | 1 + 3 files changed, 35 insertions(+), 31 deletions(-)