| Message ID | 20241022094110.1574011-2-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix write incorrect data into flash in user mode | expand |
On 10/22/24 11:40, Jamin Lin wrote: > According to the design of ASPEED SPI controllers user mode, users write the > data to flash, the SPI drivers set the Control Register(0x10) bit 0 and 1 > enter user mode. Then, SPI drivers send flash commands for writing data. > Finally, SPI drivers set the Control Register (0x10) bit 2 to stop > active control and restore bit 0 and 1. > > According to the design of ASPEED SMC model, firmware writes the > Control Register and the "aspeed_smc_flash_update_ctrl" function is called. > Then, this function verify Control Register(0x10) bit 0 and 1. If it set user > mode, the value of s->snoop_index is SNOOP_START else SNOOP_OFF. > If s->snoop_index is SNOOP_START, the "aspeed_smc_do_snoop" function verify > the first incomming data is a new flash command and writes the corresponding > dummy bytes if need. > > However, it did not check the current unselect status. If current unselect > status is "false" and firmware set the IO MODE by Control Register bit 31:28, > the value of s->snoop_index will be changed to SNOOP_START again and > "aspeed_smc_do_snoop" misunderstand that the incomming data is the new flash > command and it causes writing unexpected data into flash. > > Example: > 1. Firmware set user mode by Control Register bit 0 and 1(0x03) > 2. SMC model set s->snoop SNOOP_START > 3. Firmware set Quad Page Program with 4-Byte Address command (0x34) > 4. SMC model verify this flash command and it needs 4 dummy bytes. > 5. Firmware send 4 bytes address. > 6. SMC model receives 4 bytes address > 7. Firmware set QPI IO MODE by Control Register bit 31. (0x80000003) > 8. SMC model verify new user mode by Control Register bit 0 and 1. > Then, set s->snoop SNOOP_START again. (It is the wrong behavior.) > 9. Firmware send 0xebd8c134 data and it should be written into flash. > However, SMC model misunderstand that the first incoming data, 0x34, > is the new command because the value of s->snoop is changed to SNOOP_START. > Finally, SMC sned the incorrect data to flash model. > > Introduce a new unselect attribute in AspeedSMCState to save the current > unselect status for user mode and set it "true" by default. > Update "aspeed_smc_flash_update_ctrl" function to check the previous unselect > status. If both new unselect status and previous unselect status is different, > update s->snoop_index value and call "aspeed_smc_flash_do_select". > > Increase VMStateDescription version. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > hw/ssi/aspeed_smc.c | 40 ++++++++++++++++++++++++++----------- > include/hw/ssi/aspeed_smc.h | 1 + > 2 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c > index e3fdc66cb2..7b0f6599f9 100644 > --- a/hw/ssi/aspeed_smc.c > +++ b/hw/ssi/aspeed_smc.c > @@ -417,7 +417,7 @@ static void aspeed_smc_flash_do_select(AspeedSMCFlash *fl, bool unselect) > AspeedSMCState *s = fl->controller; > > trace_aspeed_smc_flash_select(fl->cs, unselect ? "un" : ""); > - > + s->unselect = unselect; > qemu_set_irq(s->cs_lines[fl->cs], unselect); > } > > @@ -677,22 +677,35 @@ static const MemoryRegionOps aspeed_smc_flash_ops = { > static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value) > { > AspeedSMCState *s = fl->controller; > - bool unselect; > + bool unselect = false; > + uint32_t old_mode; > + uint32_t new_mode; > + > + old_mode = s->regs[s->r_ctrl0 + fl->cs] & CTRL_CMD_MODE_MASK; > + new_mode = value & CTRL_CMD_MODE_MASK; > > - /* User mode selects the CS, other modes unselect */ > - unselect = (value & CTRL_CMD_MODE_MASK) != CTRL_USERMODE; > + if (old_mode == CTRL_USERMODE) { > + if (new_mode != CTRL_USERMODE) { > + unselect = true; > + } > > - /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */ > - if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) && > - value & CTRL_CE_STOP_ACTIVE) { > - unselect = true; > + /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */ > + if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) && > + value & CTRL_CE_STOP_ACTIVE) { > + unselect = true; > + } > + } else { > + if (new_mode != CTRL_USERMODE) { > + unselect = true; > + } > } > > s->regs[s->r_ctrl0 + fl->cs] = value; > > - s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START; > - > - aspeed_smc_flash_do_select(fl, unselect); > + if (unselect != s->unselect) { > + s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START; > + aspeed_smc_flash_do_select(fl, unselect); > + } > } > > static void aspeed_smc_reset(DeviceState *d) > @@ -729,6 +742,8 @@ static void aspeed_smc_reset(DeviceState *d) > qemu_set_irq(s->cs_lines[i], true); > } > > + s->unselect = true; > + > /* setup the default segment register values and regions for all */ > for (i = 0; i < asc->cs_num_max; ++i) { > aspeed_smc_flash_set_segment_region(s, i, > @@ -1261,12 +1276,13 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp) > > static const VMStateDescription vmstate_aspeed_smc = { > .name = "aspeed.smc", > - .version_id = 2, > + .version_id = 3, > .minimum_version_id = 2, > .fields = (const VMStateField[]) { > VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), > VMSTATE_UINT8(snoop_index, AspeedSMCState), > VMSTATE_UINT8(snoop_dummies, AspeedSMCState), > + VMSTATE_BOOL(unselect, AspeedSMCState), > VMSTATE_END_OF_LIST() > } > }; > diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h > index 234dca32b0..25b95e7406 100644 > --- a/include/hw/ssi/aspeed_smc.h > +++ b/include/hw/ssi/aspeed_smc.h > @@ -82,6 +82,7 @@ struct AspeedSMCState { > > uint8_t snoop_index; > uint8_t snoop_dummies; > + bool unselect; > }; > > typedef struct AspeedSegments {
Am 22.10.2024 um 11:40 hat Jamin Lin geschrieben: > According to the design of ASPEED SPI controllers user mode, users write the > data to flash, the SPI drivers set the Control Register(0x10) bit 0 and 1 > enter user mode. Then, SPI drivers send flash commands for writing data. > Finally, SPI drivers set the Control Register (0x10) bit 2 to stop > active control and restore bit 0 and 1. > > According to the design of ASPEED SMC model, firmware writes the > Control Register and the "aspeed_smc_flash_update_ctrl" function is called. > Then, this function verify Control Register(0x10) bit 0 and 1. If it set user > mode, the value of s->snoop_index is SNOOP_START else SNOOP_OFF. > If s->snoop_index is SNOOP_START, the "aspeed_smc_do_snoop" function verify > the first incomming data is a new flash command and writes the corresponding > dummy bytes if need. > > However, it did not check the current unselect status. If current unselect > status is "false" and firmware set the IO MODE by Control Register bit 31:28, > the value of s->snoop_index will be changed to SNOOP_START again and > "aspeed_smc_do_snoop" misunderstand that the incomming data is the new flash > command and it causes writing unexpected data into flash. > > Example: > 1. Firmware set user mode by Control Register bit 0 and 1(0x03) > 2. SMC model set s->snoop SNOOP_START > 3. Firmware set Quad Page Program with 4-Byte Address command (0x34) > 4. SMC model verify this flash command and it needs 4 dummy bytes. > 5. Firmware send 4 bytes address. > 6. SMC model receives 4 bytes address > 7. Firmware set QPI IO MODE by Control Register bit 31. (0x80000003) > 8. SMC model verify new user mode by Control Register bit 0 and 1. > Then, set s->snoop SNOOP_START again. (It is the wrong behavior.) > 9. Firmware send 0xebd8c134 data and it should be written into flash. > However, SMC model misunderstand that the first incoming data, 0x34, > is the new command because the value of s->snoop is changed to SNOOP_START. > Finally, SMC sned the incorrect data to flash model. > > Introduce a new unselect attribute in AspeedSMCState to save the current > unselect status for user mode and set it "true" by default. > Update "aspeed_smc_flash_update_ctrl" function to check the previous unselect > status. If both new unselect status and previous unselect status is different, > update s->snoop_index value and call "aspeed_smc_flash_do_select". > > Increase VMStateDescription version. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > @@ -1261,12 +1276,13 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp) > > static const VMStateDescription vmstate_aspeed_smc = { > .name = "aspeed.smc", > - .version_id = 2, > + .version_id = 3, > .minimum_version_id = 2, > .fields = (const VMStateField[]) { > VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), > VMSTATE_UINT8(snoop_index, AspeedSMCState), > VMSTATE_UINT8(snoop_dummies, AspeedSMCState), > + VMSTATE_BOOL(unselect, AspeedSMCState), > VMSTATE_END_OF_LIST() > } > }; I think this will break migration compatibility. In order to enable at least forward migration, it should be: VMSTATE_BOOL_V(unselect, AspeedSMCState, 3), For allowing backwards migration, too, we should consider making it a subsection instead that allows migration in the default case of an idle device. Kevin
>> >> static const VMStateDescription vmstate_aspeed_smc = { >> .name = "aspeed.smc", >> - .version_id = 2, >> + .version_id = 3, >> .minimum_version_id = 2, >> .fields = (const VMStateField[]) { >> VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), >> VMSTATE_UINT8(snoop_index, AspeedSMCState), >> VMSTATE_UINT8(snoop_dummies, AspeedSMCState), >> + VMSTATE_BOOL(unselect, AspeedSMCState), >> VMSTATE_END_OF_LIST() >> } >> }; > > I think this will break migration compatibility. In order to enable > at least forward migration, it should be: > > VMSTATE_BOOL_V(unselect, AspeedSMCState, 3), This is correct. I will fix the patch. Some background, The aspeed machines are fully emulated and the Aspeed SoC models are not part of any virt* machines (yet). So migration support is a bit of a theory. We have done our best to maintain some support, compatibility not being a priority. IOW, it's not perfectly tuned as on virt machines. Also, on ARM, migration of the CPU secure mode (I think this is the reason, Peter please correct me !) is not supported and if migration is initiated after Linux has started, the machine will hang. However, if one day, an aspeed model becomes part of a virt machine, we should be more careful. I would start by resetting all vmstate versions to 1! Thanks, C. > > For allowing backwards migration, too, we should consider making it a > subsection instead that allows migration in the default case of an idle > device. > > Kevin >
Am 22.10.2024 um 15:40 hat Cédric Le Goater geschrieben: > > > static const VMStateDescription vmstate_aspeed_smc = { > > > .name = "aspeed.smc", > > > - .version_id = 2, > > > + .version_id = 3, > > > .minimum_version_id = 2, > > > .fields = (const VMStateField[]) { > > > VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), > > > VMSTATE_UINT8(snoop_index, AspeedSMCState), > > > VMSTATE_UINT8(snoop_dummies, AspeedSMCState), > > > + VMSTATE_BOOL(unselect, AspeedSMCState), > > > VMSTATE_END_OF_LIST() > > > } > > > }; > > > > I think this will break migration compatibility. In order to enable > > at least forward migration, it should be: > > > > VMSTATE_BOOL_V(unselect, AspeedSMCState, 3), > > This is correct. I will fix the patch. > > Some background, > > The aspeed machines are fully emulated and the Aspeed SoC models are not > part of any virt* machines (yet). So migration support is a bit of a > theory. We have done our best to maintain some support, compatibility > not being a priority. IOW, it's not perfectly tuned as on virt machines. > > Also, on ARM, migration of the CPU secure mode (I think this is the reason, > Peter please correct me !) is not supported and if migration is initiated > after Linux has started, the machine will hang. That's a good reason not to implement backwards migration for now, it would only complicate things. But as long as we claim to be migratable by having VMStateDescriptions and even increasing version_id, we should at least try to keep that part correct. > However, if one day, an aspeed model becomes part of a virt machine, we > should be more careful. I would start by resetting all vmstate versions > to 1! Why would you reset it? Keeping 3 (or whatever it will be by then) as the first serious supported version shouldn't hurt and probably avoids some confusion. Kevin
Hi Cedric, > Subject: Re: [PATCH v2 01/18] aspeed/smc: Fix write incorrect data into flash in > user mode > > >> > >> static const VMStateDescription vmstate_aspeed_smc = { > >> .name = "aspeed.smc", > >> - .version_id = 2, > >> + .version_id = 3, > >> .minimum_version_id = 2, > >> .fields = (const VMStateField[]) { > >> VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, > ASPEED_SMC_R_MAX), > >> VMSTATE_UINT8(snoop_index, AspeedSMCState), > >> VMSTATE_UINT8(snoop_dummies, AspeedSMCState), > >> + VMSTATE_BOOL(unselect, AspeedSMCState), > >> VMSTATE_END_OF_LIST() > >> } > >> }; > > > > I think this will break migration compatibility. In order to enable at > > least forward migration, it should be: > > > > VMSTATE_BOOL_V(unselect, AspeedSMCState, 3), > > This is correct. I will fix the patch. > If I need to re-send v3 patch, I will fix it. Thanks for help. Jamin > Some background, > > The aspeed machines are fully emulated and the Aspeed SoC models are not > part of any virt* machines (yet). So migration support is a bit of a theory. We > have done our best to maintain some support, compatibility not being a > priority. IOW, it's not perfectly tuned as on virt machines. > > Also, on ARM, migration of the CPU secure mode (I think this is the reason, > Peter please correct me !) is not supported and if migration is initiated after > Linux has started, the machine will hang. > > However, if one day, an aspeed model becomes part of a virt machine, we > should be more careful. I would start by resetting all vmstate versions to 1! > > Thanks, > > C. > > > > > > > For allowing backwards migration, too, we should consider making it a > > subsection instead that allows migration in the default case of an > > idle device. > > > > Kevin > >
On 10/22/24 17:23, Kevin Wolf wrote: > Am 22.10.2024 um 15:40 hat Cédric Le Goater geschrieben: >>>> static const VMStateDescription vmstate_aspeed_smc = { >>>> .name = "aspeed.smc", >>>> - .version_id = 2, >>>> + .version_id = 3, >>>> .minimum_version_id = 2, >>>> .fields = (const VMStateField[]) { >>>> VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), >>>> VMSTATE_UINT8(snoop_index, AspeedSMCState), >>>> VMSTATE_UINT8(snoop_dummies, AspeedSMCState), >>>> + VMSTATE_BOOL(unselect, AspeedSMCState), >>>> VMSTATE_END_OF_LIST() >>>> } >>>> }; >>> >>> I think this will break migration compatibility. In order to enable >>> at least forward migration, it should be: >>> >>> VMSTATE_BOOL_V(unselect, AspeedSMCState, 3), >> >> This is correct. I will fix the patch. >> >> Some background, >> >> The aspeed machines are fully emulated and the Aspeed SoC models are not >> part of any virt* machines (yet). So migration support is a bit of a >> theory. We have done our best to maintain some support, compatibility >> not being a priority. IOW, it's not perfectly tuned as on virt machines. >> >> Also, on ARM, migration of the CPU secure mode (I think this is the reason, >> Peter please correct me !) is not supported and if migration is initiated >> after Linux has started, the machine will hang. > > That's a good reason not to implement backwards migration for now, it > would only complicate things. But as long as we claim to be migratable > by having VMStateDescriptions and even increasing version_id, we should > at least try to keep that part correct. Sure. Thanks for pointing that out. >> However, if one day, an aspeed model becomes part of a virt machine, we >> should be more careful. I would start by resetting all vmstate versions >> to 1! > > Why would you reset it? Keeping 3 (or whatever it will be by then) as > the first serious supported version shouldn't hurt and probably avoids > some confusion. OK . We'll see when we get to that point. Who knows. The Aspeed machines have gotten much bigger than we expected ! C.
On 10/23/24 03:41, Jamin Lin wrote: > Hi Cedric, > >> Subject: Re: [PATCH v2 01/18] aspeed/smc: Fix write incorrect data into flash in >> user mode >> >>>> >>>> static const VMStateDescription vmstate_aspeed_smc = { >>>> .name = "aspeed.smc", >>>> - .version_id = 2, >>>> + .version_id = 3, >>>> .minimum_version_id = 2, >>>> .fields = (const VMStateField[]) { >>>> VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, >> ASPEED_SMC_R_MAX), >>>> VMSTATE_UINT8(snoop_index, AspeedSMCState), >>>> VMSTATE_UINT8(snoop_dummies, AspeedSMCState), >>>> + VMSTATE_BOOL(unselect, AspeedSMCState), >>>> VMSTATE_END_OF_LIST() >>>> } >>>> }; >>> >>> I think this will break migration compatibility. In order to enable at >>> least forward migration, it should be: >>> >>> VMSTATE_BOOL_V(unselect, AspeedSMCState, 3), >> >> This is correct. I will fix the patch. >> > If I need to re-send v3 patch, I will fix it. I took care of it. No need to resend. Thanks C.
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index e3fdc66cb2..7b0f6599f9 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -417,7 +417,7 @@ static void aspeed_smc_flash_do_select(AspeedSMCFlash *fl, bool unselect) AspeedSMCState *s = fl->controller; trace_aspeed_smc_flash_select(fl->cs, unselect ? "un" : ""); - + s->unselect = unselect; qemu_set_irq(s->cs_lines[fl->cs], unselect); } @@ -677,22 +677,35 @@ static const MemoryRegionOps aspeed_smc_flash_ops = { static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value) { AspeedSMCState *s = fl->controller; - bool unselect; + bool unselect = false; + uint32_t old_mode; + uint32_t new_mode; + + old_mode = s->regs[s->r_ctrl0 + fl->cs] & CTRL_CMD_MODE_MASK; + new_mode = value & CTRL_CMD_MODE_MASK; - /* User mode selects the CS, other modes unselect */ - unselect = (value & CTRL_CMD_MODE_MASK) != CTRL_USERMODE; + if (old_mode == CTRL_USERMODE) { + if (new_mode != CTRL_USERMODE) { + unselect = true; + } - /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */ - if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) && - value & CTRL_CE_STOP_ACTIVE) { - unselect = true; + /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */ + if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) && + value & CTRL_CE_STOP_ACTIVE) { + unselect = true; + } + } else { + if (new_mode != CTRL_USERMODE) { + unselect = true; + } } s->regs[s->r_ctrl0 + fl->cs] = value; - s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START; - - aspeed_smc_flash_do_select(fl, unselect); + if (unselect != s->unselect) { + s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START; + aspeed_smc_flash_do_select(fl, unselect); + } } static void aspeed_smc_reset(DeviceState *d) @@ -729,6 +742,8 @@ static void aspeed_smc_reset(DeviceState *d) qemu_set_irq(s->cs_lines[i], true); } + s->unselect = true; + /* setup the default segment register values and regions for all */ for (i = 0; i < asc->cs_num_max; ++i) { aspeed_smc_flash_set_segment_region(s, i, @@ -1261,12 +1276,13 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp) static const VMStateDescription vmstate_aspeed_smc = { .name = "aspeed.smc", - .version_id = 2, + .version_id = 3, .minimum_version_id = 2, .fields = (const VMStateField[]) { VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX), VMSTATE_UINT8(snoop_index, AspeedSMCState), VMSTATE_UINT8(snoop_dummies, AspeedSMCState), + VMSTATE_BOOL(unselect, AspeedSMCState), VMSTATE_END_OF_LIST() } }; diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index 234dca32b0..25b95e7406 100644 --- a/include/hw/ssi/aspeed_smc.h +++ b/include/hw/ssi/aspeed_smc.h @@ -82,6 +82,7 @@ struct AspeedSMCState { uint8_t snoop_index; uint8_t snoop_dummies; + bool unselect; }; typedef struct AspeedSegments {
According to the design of ASPEED SPI controllers user mode, users write the data to flash, the SPI drivers set the Control Register(0x10) bit 0 and 1 enter user mode. Then, SPI drivers send flash commands for writing data. Finally, SPI drivers set the Control Register (0x10) bit 2 to stop active control and restore bit 0 and 1. According to the design of ASPEED SMC model, firmware writes the Control Register and the "aspeed_smc_flash_update_ctrl" function is called. Then, this function verify Control Register(0x10) bit 0 and 1. If it set user mode, the value of s->snoop_index is SNOOP_START else SNOOP_OFF. If s->snoop_index is SNOOP_START, the "aspeed_smc_do_snoop" function verify the first incomming data is a new flash command and writes the corresponding dummy bytes if need. However, it did not check the current unselect status. If current unselect status is "false" and firmware set the IO MODE by Control Register bit 31:28, the value of s->snoop_index will be changed to SNOOP_START again and "aspeed_smc_do_snoop" misunderstand that the incomming data is the new flash command and it causes writing unexpected data into flash. Example: 1. Firmware set user mode by Control Register bit 0 and 1(0x03) 2. SMC model set s->snoop SNOOP_START 3. Firmware set Quad Page Program with 4-Byte Address command (0x34) 4. SMC model verify this flash command and it needs 4 dummy bytes. 5. Firmware send 4 bytes address. 6. SMC model receives 4 bytes address 7. Firmware set QPI IO MODE by Control Register bit 31. (0x80000003) 8. SMC model verify new user mode by Control Register bit 0 and 1. Then, set s->snoop SNOOP_START again. (It is the wrong behavior.) 9. Firmware send 0xebd8c134 data and it should be written into flash. However, SMC model misunderstand that the first incoming data, 0x34, is the new command because the value of s->snoop is changed to SNOOP_START. Finally, SMC sned the incorrect data to flash model. Introduce a new unselect attribute in AspeedSMCState to save the current unselect status for user mode and set it "true" by default. Update "aspeed_smc_flash_update_ctrl" function to check the previous unselect status. If both new unselect status and previous unselect status is different, update s->snoop_index value and call "aspeed_smc_flash_do_select". Increase VMStateDescription version. Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> --- hw/ssi/aspeed_smc.c | 40 ++++++++++++++++++++++++++----------- include/hw/ssi/aspeed_smc.h | 1 + 2 files changed, 29 insertions(+), 12 deletions(-)