| Message ID | 1265752899-26980-8-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 9 Feb 2010, Anthony Liguori wrote: > - fixed bug with size of registered ioport regions > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > hw/ac97.c | 146 +++++++++++++++++++++++++------------------------------------ > 1 files changed, 60 insertions(+), 86 deletions(-) > > diff --git a/hw/ac97.c b/hw/ac97.c > index 4319bc8..9fdf591 100644 > --- a/hw/ac97.c > +++ b/hw/ac97.c > @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r) > { > uint8_t b[8]; > > - cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8); > + pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8); > r->bd_valid = 1; > r->bd.addr = le32_to_cpu (*(uint32_t *) &b[0]) & ~3; > r->bd.ctl_len = le32_to_cpu (*(uint32_t *) &b[4]); > @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s) > > /** > * Native audio mixer > - * I/O Reads > */ > -static uint32_t nam_readb (void *opaque, uint32_t addr) > -{ > - AC97LinkState *s = opaque; > - dolog ("U nam readb %#x\n", addr); > - s->cas = 0; > - return ~0U; > -} > > -static uint32_t nam_readw (void *opaque, uint32_t addr) > +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size) > { > - AC97LinkState *s = opaque; > - uint32_t val = ~0U; > - uint32_t index = addr - s->base[0]; > - s->cas = 0; > - val = mixer_load (s, index); > - return val; > -} > + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); > + uint32_t value; > + > + if (size == 2) { > + value = mixer_load (s, addr); > + } else { > + dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr); > + s->cas = 0; > + value = ~0U; > + } > Yeah right, and then PCI SIG adds qword accessors and all hell breaks loose, this interface sucks.. [..snip..]
On 02/09/2010 04:45 PM, malc wrote: > On Tue, 9 Feb 2010, Anthony Liguori wrote: > > >> - fixed bug with size of registered ioport regions >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> hw/ac97.c | 146 +++++++++++++++++++++++++------------------------------------ >> 1 files changed, 60 insertions(+), 86 deletions(-) >> >> diff --git a/hw/ac97.c b/hw/ac97.c >> index 4319bc8..9fdf591 100644 >> --- a/hw/ac97.c >> +++ b/hw/ac97.c >> @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r) >> { >> uint8_t b[8]; >> >> - cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8); >> + pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8); >> r->bd_valid = 1; >> r->bd.addr = le32_to_cpu (*(uint32_t *)&b[0])& ~3; >> r->bd.ctl_len = le32_to_cpu (*(uint32_t *)&b[4]); >> @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s) >> >> /** >> * Native audio mixer >> - * I/O Reads >> */ >> -static uint32_t nam_readb (void *opaque, uint32_t addr) >> -{ >> - AC97LinkState *s = opaque; >> - dolog ("U nam readb %#x\n", addr); >> - s->cas = 0; >> - return ~0U; >> -} >> >> -static uint32_t nam_readw (void *opaque, uint32_t addr) >> +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size) >> { >> - AC97LinkState *s = opaque; >> - uint32_t val = ~0U; >> - uint32_t index = addr - s->base[0]; >> - s->cas = 0; >> - val = mixer_load (s, index); >> - return val; >> -} >> + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); >> + uint32_t value; >> + >> + if (size == 2) { >> + value = mixer_load (s, addr); >> + } else { >> + dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr); >> + s->cas = 0; >> + value = ~0U; >> + } >> >> > Yeah right, and then PCI SIG adds qword accessors and all hell breaks > loose, this interface sucks.. > We already have this problem with the current interface. The options to address would be to always return/accept a uint64_t or to deal with a void *. The later interface has non-obvious byte order semantics. I avoided the former to avoid complaints about possibly introducing a performance regression with passing larger data types. I don't believe that it would be a problem though. Regards, Anthony Liguori > [..snip..] > >
On Tue, 9 Feb 2010, Anthony Liguori wrote: > On 02/09/2010 04:45 PM, malc wrote: > > On Tue, 9 Feb 2010, Anthony Liguori wrote: > > > > > > > - fixed bug with size of registered ioport regions > > > > > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > > > --- > > > hw/ac97.c | 146 > > > +++++++++++++++++++++++++------------------------------------ > > > 1 files changed, 60 insertions(+), 86 deletions(-) > > > > > > diff --git a/hw/ac97.c b/hw/ac97.c > > > index 4319bc8..9fdf591 100644 > > > --- a/hw/ac97.c > > > +++ b/hw/ac97.c > > > @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s, > > > AC97BusMasterRegs *r) > > > { > > > uint8_t b[8]; > > > > > > - cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8); > > > + pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8); > > > r->bd_valid = 1; > > > r->bd.addr = le32_to_cpu (*(uint32_t *)&b[0])& ~3; > > > r->bd.ctl_len = le32_to_cpu (*(uint32_t *)&b[4]); > > > @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s) > > > > > > /** > > > * Native audio mixer > > > - * I/O Reads > > > */ > > > -static uint32_t nam_readb (void *opaque, uint32_t addr) > > > -{ > > > - AC97LinkState *s = opaque; > > > - dolog ("U nam readb %#x\n", addr); > > > - s->cas = 0; > > > - return ~0U; > > > -} > > > > > > -static uint32_t nam_readw (void *opaque, uint32_t addr) > > > +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size) > > > { > > > - AC97LinkState *s = opaque; > > > - uint32_t val = ~0U; > > > - uint32_t index = addr - s->base[0]; > > > - s->cas = 0; > > > - val = mixer_load (s, index); > > > - return val; > > > -} > > > + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); > > > + uint32_t value; > > > + > > > + if (size == 2) { > > > + value = mixer_load (s, addr); > > > + } else { > > > + dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr); > > > + s->cas = 0; > > > + value = ~0U; > > > + } > > > > > > > > Yeah right, and then PCI SIG adds qword accessors and all hell breaks > > loose, this interface sucks.. > > > > We already have this problem with the current interface. Uh, i've meant the registration of one function to rule them all, instead of how it's done currently - separate accessors for b/w/l/whatever. > > The options to address would be to always return/accept a uint64_t or to deal > with a void *. The later interface has non-obvious byte order semantics. > > I avoided the former to avoid complaints about possibly introducing a > performance regression with passing larger data types. I don't believe that > it would be a problem though. > > Regards, > > Anthony Liguori > > > [..snip..] > > > >
On 02/09/2010 05:06 PM, malc wrote: > >> We already have this problem with the current interface. >> > Uh, i've meant the registration of one function to rule them all, instead > of how it's done currently - separate accessors for b/w/l/whatever. > How does that make any difference? Both the ioport and memory registrations interface take the same function pointer regardless of access size. If you wanted to introduce a quad-word specific accessor, you would need to introduce a different registration mechanism or you would have to change the signature for all of the functions. Regards, Anthony Liguori
On Tue, 9 Feb 2010, Anthony Liguori wrote: > On 02/09/2010 05:06 PM, malc wrote: > > > > > We already have this problem with the current interface. > > > > > Uh, i've meant the registration of one function to rule them all, instead > > of how it's done currently - separate accessors for b/w/l/whatever. > > > > How does that make any difference? Both the ioport and memory registrations > interface take the same function pointer regardless of access size. > > If you wanted to introduce a quad-word specific accessor, you would need to > introduce a different registration mechanism or you would have to change the > signature for all of the functions. Let's see: Currently we have this readb(...): dostuff return stuff readw(...): dostuff return stuff You are replacing it with read(size...): if (size == 1): do1 elif (size == 2): do2 else: # and here your code assumes that everything is handy dandy # and size is 4 do4 The interface being implicit rather than explicit about the sizes makes this possible, so i'm against it. The code was written the way it was written for a purpose.
On 02/09/2010 05:36 PM, malc wrote: > Let's see: > > Currently we have this > > > readb(...): > dostuff > return stuff > > readw(...): > dostuff > return stuff > And this is completely wrong. For the most part, device models don't consistently handle access to registers via their non native sizes. Some devices return the lower part of a dword register when accessed with a word accessor. Some devices only return the register when there's a dword access. What's worse, is that if a device only registers a byte accessor, because of the default accessors, a word access returns two registers but a dword access returns ~0U. Some device models actually rely on this behaviour. > You are replacing it with > > read(size...): > if (size == 1): do1 > elif (size == 2): do2 > else: # and here your code assumes that everything is handy dandy > # and size is 4 > do4 > This is ugly because it's a programmatic conversion. Once we have this API, we can switch to: read(addr, size...): switch(addr): case REG0: return s->reg0; case REG1: return s->reg1; Along with having a table like: pci_regs = { {REG0, DWORD}, {REG1, BYTE}, {REG2, BYTE}, {REG3, WORD} } That allows the PCI layer to invoke the callback and do the right operations to transparently handle the access size without the device model having to deal with it. The reason I've left size in the callback at all is that I suspect there will always be a device that behaves weirdly and needs to know about the accessor size. But for the most part, I think this model will make things much more consistent and eliminate a huge class of emulation bugs. We can even take it further, and do something like: pci_regs = { PCI_REG_DWORD(REG0, DeviceState, reg0), PCI_REG_BYTE(REG1, DeviceState, reg1), ... } In which case, we only need to handle the case in switch() if we need to implement a side effect of the register operation. But none of this is possible if we completely rely on every device to implement non-dword access in it's own broken way. Regards, Anthony Liguori
On Tue, 9 Feb 2010, Anthony Liguori wrote: > On 02/09/2010 05:36 PM, malc wrote: > > Let's see: > > > > Currently we have this > > > > > > readb(...): > > dostuff > > return stuff > > > > readw(...): > > dostuff > > return stuff > > > > And this is completely wrong. It's completely right, until some future C standard implements proper algebraic data types. > > For the most part, device models don't consistently handle access to registers > via their non native sizes. Some devices return the lower part of a dword > register when accessed with a word accessor. Some devices only return the > register when there's a dword access. That's up to device to register an accessor and return what's appropriate. > > What's worse, is that if a device only registers a byte accessor, because of > the default accessors, a word access returns two registers but a dword access > returns ~0U. Some device models actually rely on this behaviour. > > > You are replacing it with > > > > read(size...): > > if (size == 1): do1 > > elif (size == 2): do2 > > else: # and here your code assumes that everything is handy dandy > > # and size is 4 > > do4 > > > > This is ugly because it's a programmatic conversion. Once we have this API, > we can switch to: > > read(addr, size...): > switch(addr): > case REG0: > return s->reg0; > case REG1: > return s->reg1; This is exactly equivalent to your original if, due to the C language being what it is. > Along with having a table like: > > pci_regs = { {REG0, DWORD}, {REG1, BYTE}, {REG2, BYTE}, {REG3, WORD} } > > That allows the PCI layer to invoke the callback and do the right operations > to transparently handle the access size without the device model having to > deal with it. The reason I've left size in the callback at all is that I > suspect there will always be a device that behaves weirdly and needs to know > about the accessor size. But for the most part, I think this model will make > things much more consistent and eliminate a huge class of emulation bugs. Eh? Care to name one emulation bug? > > We can even take it further, and do something like: > > pci_regs = { > PCI_REG_DWORD(REG0, DeviceState, reg0), > PCI_REG_BYTE(REG1, DeviceState, reg1), > ... > } > > In which case, we only need to handle the case in switch() if we need to > implement a side effect of the register operation. > > But none of this is possible if we completely rely on every device to > implement non-dword access in it's own broken way. > Lovely, we are heading full speed into fanatsy land, where PCI BUS itself accesses device specific stuff.
On 02/09/2010 06:24 PM, malc wrote: > On Tue, 9 Feb 2010, Anthony Liguori wrote: > > >> On 02/09/2010 05:36 PM, malc wrote: >> >>> Let's see: >>> >>> Currently we have this >>> >>> >>> readb(...): >>> dostuff >>> return stuff >>> >>> readw(...): >>> dostuff >>> return stuff >>> >>> >> And this is completely wrong. >> > It's completely right, until some future C standard implements proper > algebraic data types. > I've been thinking about what to do here and I think what I've come up with is to change the callbacks to take be a separate type. So instead of just: void (PCIOWriteFunc)(PCIDevice *, pcibus_t addr, int size, uint32_t value); uint32_t (PCIOReadFunc)(PCIDevice *, pcibus_t addr, int size); We would have: typedef struct PCIIOHandler { void (*write)(PCIIOHandler *, pcibus_t addr, int size, uint32_t); uint32_t (*read)(PCIIOHandler *, pcibus_t addr, int size); } PCIIOHandler; And then we would have: pci_register_io_region(PCIDevice *, ... PCIIOHandler *handler); We can then introduce a mechanism to dispatch to individual functions based on size. That way, instead of pushing an if (size == 1) else if (size == 2) check to each device, we let the device decide how it wants to be invoked. >> For the most part, device models don't consistently handle access to registers >> via their non native sizes. Some devices return the lower part of a dword >> register when accessed with a word accessor. Some devices only return the >> register when there's a dword access. >> > That's up to device to register an accessor and return what's appropriate. > I think the point though is to make it possible for common mechanisms to be formalized into common code. For instance, a common set of rules will be, all registers can be accessed in byte, word, or dword size but most be accessed at natural size boundaries. We should make it simple for a device to essentially choose that pattern and not have to worry about explicitly coding it themselves. >> We can even take it further, and do something like: >> >> pci_regs = { >> PCI_REG_DWORD(REG0, DeviceState, reg0), >> PCI_REG_BYTE(REG1, DeviceState, reg1), >> ... >> } >> >> In which case, we only need to handle the case in switch() if we need to >> implement a side effect of the register operation. >> >> But none of this is possible if we completely rely on every device to >> implement non-dword access in it's own broken way. >> >> > Lovely, we are heading full speed into fanatsy land, where PCI BUS itself > accesses device specific stuff. > I never said the PCI BUS is the one that actually got this info. It could just be common code that implements this logic. But there are good reasons to normalize device emulation at this level. For instance, it allows us to implement consistent tracing for device models using symbolic names for registers as opposed to only being able to trace IO operations at addresses. Regards, Anthony Liguori
diff --git a/hw/ac97.c b/hw/ac97.c index 4319bc8..9fdf591 100644 --- a/hw/ac97.c +++ b/hw/ac97.c @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r) { uint8_t b[8]; - cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8); + pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8); r->bd_valid = 1; r->bd.addr = le32_to_cpu (*(uint32_t *) &b[0]) & ~3; r->bd.ctl_len = le32_to_cpu (*(uint32_t *) &b[4]); @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s) /** * Native audio mixer - * I/O Reads */ -static uint32_t nam_readb (void *opaque, uint32_t addr) -{ - AC97LinkState *s = opaque; - dolog ("U nam readb %#x\n", addr); - s->cas = 0; - return ~0U; -} -static uint32_t nam_readw (void *opaque, uint32_t addr) +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size) { - AC97LinkState *s = opaque; - uint32_t val = ~0U; - uint32_t index = addr - s->base[0]; - s->cas = 0; - val = mixer_load (s, index); - return val; -} + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); + uint32_t value; + + if (size == 2) { + value = mixer_load (s, addr); + } else { + dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr); + s->cas = 0; + value = ~0U; + } -static uint32_t nam_readl (void *opaque, uint32_t addr) -{ - AC97LinkState *s = opaque; - dolog ("U nam readl %#x\n", addr); - s->cas = 0; - return ~0U; + return value; } -/** - * Native audio mixer - * I/O Writes - */ -static void nam_writeb (void *opaque, uint32_t addr, uint32_t val) +static void nam_write (PCIDevice *dev, pcibus_t addr, int size, uint32_t val) { - AC97LinkState *s = opaque; - dolog ("U nam writeb %#x <- %#x\n", addr, val); - s->cas = 0; -} + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); + uint32_t index = addr; -static void nam_writew (void *opaque, uint32_t addr, uint32_t val) -{ - AC97LinkState *s = opaque; - uint32_t index = addr - s->base[0]; s->cas = 0; + if (size != 2) { + dolog ("U nam write[%d] %#" FMT_PCIBUS " <- %#x\n", addr, val); + return; + } + switch (index) { case AC97_Reset: mixer_reset (s); @@ -699,22 +684,13 @@ static void nam_writew (void *opaque, uint32_t addr, uint32_t val) } } -static void nam_writel (void *opaque, uint32_t addr, uint32_t val) -{ - AC97LinkState *s = opaque; - dolog ("U nam writel %#x <- %#x\n", addr, val); - s->cas = 0; -} - /** * Native audio bus master * I/O Reads */ -static uint32_t nabm_readb (void *opaque, uint32_t addr) +static uint32_t nabm_readb (AC97LinkState *s, uint32_t index) { - AC97LinkState *s = opaque; AC97BusMasterRegs *r = NULL; - uint32_t index = addr - s->base[1]; uint32_t val = ~0U; switch (index) { @@ -765,11 +741,9 @@ static uint32_t nabm_readb (void *opaque, uint32_t addr) return val; } -static uint32_t nabm_readw (void *opaque, uint32_t addr) +static uint32_t nabm_readw (AC97LinkState *s, uint32_t index) { - AC97LinkState *s = opaque; AC97BusMasterRegs *r = NULL; - uint32_t index = addr - s->base[1]; uint32_t val = ~0U; switch (index) { @@ -794,11 +768,9 @@ static uint32_t nabm_readw (void *opaque, uint32_t addr) return val; } -static uint32_t nabm_readl (void *opaque, uint32_t addr) +static uint32_t nabm_readl (AC97LinkState *s, uint32_t index) { - AC97LinkState *s = opaque; AC97BusMasterRegs *r = NULL; - uint32_t index = addr - s->base[1]; uint32_t val = ~0U; switch (index) { @@ -840,15 +812,29 @@ static uint32_t nabm_readl (void *opaque, uint32_t addr) return val; } +static uint32_t nabm_read (PCIDevice *dev, pcibus_t addr, int size) +{ + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); + uint32_t value; + + if (size == 1) { + value = nabm_readb (s, addr); + } else if (size == 2) { + value = nabm_readw (s, addr); + } else { + value = nabm_readl (s, addr); + } + + return value; +} + /** * Native audio bus master * I/O Writes */ -static void nabm_writeb (void *opaque, uint32_t addr, uint32_t val) +static void nabm_writeb (AC97LinkState *s, uint32_t index, uint32_t val) { - AC97LinkState *s = opaque; AC97BusMasterRegs *r = NULL; - uint32_t index = addr - s->base[1]; switch (index) { case PI_LVI: case PO_LVI: @@ -954,6 +940,19 @@ static void nabm_writel (void *opaque, uint32_t addr, uint32_t val) } } +static void nabm_write (PCIDevice *dev, pcibus_t addr, int size, uint32_t value) +{ + AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev); + + if (size == 1) { + nabm_writeb (s, addr, value); + } else if (size == 2) { + nabm_writew (s, addr, value); + } else { + nabm_writel (s, addr, value); + } +} + static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r, int max, int *stop) { @@ -972,7 +971,7 @@ static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r, while (temp) { int copied; to_copy = audio_MIN (temp, sizeof (tmpbuf)); - cpu_physical_memory_read (addr, tmpbuf, to_copy); + pci_memory_read (&s->dev, addr, tmpbuf, to_copy); copied = AUD_write (s->voice_po, tmpbuf, to_copy); dolog ("write_audio max=%x to_copy=%x copied=%x\n", max, to_copy, copied); @@ -1056,7 +1055,7 @@ static int read_audio (AC97LinkState *s, AC97BusMasterRegs *r, *stop = 1; break; } - cpu_physical_memory_write (addr, tmpbuf, acquired); + pci_memory_write (&s->dev, addr, tmpbuf, acquired); temp -= acquired; addr += acquired; nread += acquired; @@ -1234,32 +1233,6 @@ static const VMStateDescription vmstate_ac97 = { } }; -static void ac97_map (PCIDevice *pci_dev, int region_num, - pcibus_t addr, pcibus_t size, int type) -{ - AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, pci_dev); - PCIDevice *d = &s->dev; - - if (!region_num) { - s->base[0] = addr; - register_ioport_read (addr, 256 * 1, 1, nam_readb, d); - register_ioport_read (addr, 256 * 2, 2, nam_readw, d); - register_ioport_read (addr, 256 * 4, 4, nam_readl, d); - register_ioport_write (addr, 256 * 1, 1, nam_writeb, d); - register_ioport_write (addr, 256 * 2, 2, nam_writew, d); - register_ioport_write (addr, 256 * 4, 4, nam_writel, d); - } - else { - s->base[1] = addr; - register_ioport_read (addr, 64 * 1, 1, nabm_readb, d); - register_ioport_read (addr, 64 * 2, 2, nabm_readw, d); - register_ioport_read (addr, 64 * 4, 4, nabm_readl, d); - register_ioport_write (addr, 64 * 1, 1, nabm_writeb, d); - register_ioport_write (addr, 64 * 2, 2, nabm_writew, d); - register_ioport_write (addr, 64 * 4, 4, nabm_writel, d); - } -} - static void ac97_on_reset (void *opaque) { AC97LinkState *s = opaque; @@ -1321,9 +1294,10 @@ static int ac97_initfn (PCIDevice *dev) /* TODO: RST# value should be 0. */ c[PCI_INTERRUPT_PIN] = 0x01; /* intr_pn interrupt pin ro */ - pci_register_bar (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO, - ac97_map); - pci_register_bar (&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPACE_IO, ac97_map); + pci_register_io_region (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO, + nam_read, nam_write); + pci_register_io_region (&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPACE_IO, + nabm_read, nabm_write); qemu_register_reset (ac97_on_reset, s); AUD_register_card ("ac97", &s->card); ac97_on_reset (s);
- fixed bug with size of registered ioport regions Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/ac97.c | 146 +++++++++++++++++++++++++------------------------------------ 1 files changed, 60 insertions(+), 86 deletions(-)