Message ID | 1330401082-10073-1-git-send-email-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
On Tue, Feb 28, 2012 at 02:51:22PM +1100, David Gibson wrote: > This patch fixes two bugs in the OHCI device where the device writes > back data to system memory that should be exclusively under the > control of the guest side driver. > > In OHCI specification Section 5.2.7, it mentioned "In all cases, Host > Controller Driver is responsible for the insertion and removal of all > Endpoint Descriptors in the various Host Controller Endpoint > Descriptor lists". In the ohci_frame_boundary(), ohci_put_hcca() > writes the entire hcca back including the interrupt ED lists which > should be under driver control. This violates the specification and > can race with a host driver updating that list at the same time. > > In the OHCI Spec Section 4.6, Transfer Descriptor Queue Processing, it > mentioned "Since the TD pointed to by TailP is not accessed by the HC, > the Host Controller Driver can initialize that TD and link at least > one other to it without creating a coherency or synchronization > problem". While the function ohci_put_ed() writes the entire endpoint > descriptor back including the TailP which should under driver > control. This violate the specification and can race with a host > driver updating the TD list at the same time. > > In each case the solution is to make sure we don't write data which is > under driver control. Arrrgh, sorry, screwed up yet again. This version has some redundant #defines left in.
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c index 7aa19fe..c0ffc3b 100644 --- a/hw/usb-ohci.c +++ b/hw/usb-ohci.c @@ -122,6 +122,11 @@ struct ohci_hcca { uint16_t frame, pad; uint32_t done; }; +#define HCCA_WRITEBACK_OFFSET offsetof(struct ohci_hcca, frame) +#define HCCA_WRITEBACK_SIZE 8 /* frame, pad, done */ + +#define ED_WBACK_OFFSET offsetof(struct ohci_ed, head) +#define ED_WBACK_SIZE 4 static void ohci_bus_stop(OHCIState *ohci); static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev); @@ -189,6 +194,10 @@ struct ohci_ed { uint32_t head; uint32_t next; }; +#define ED_TAILP_OFFSET offsetof(struct ohci_ed, tail) +#define ED_PART1_SIZE ED_TAILP_OFFSET +#define ED_HEADP_OFFSET offsetof(struct ohci_ed, head) +#define ED_PART2_SIZE (sizeof(struct ohci_ed) - ED_HEADP_OFFSET) /* General transfer descriptor */ struct ohci_td { @@ -569,7 +578,13 @@ static inline int ohci_read_hcca(OHCIState *ohci, static inline int ohci_put_ed(OHCIState *ohci, uint32_t addr, struct ohci_ed *ed) { - return put_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed) >> 2); + /* ed->tail is under control of the HCD. + * Since just ed->head is changed by HC, just write back this + */ + + return put_dwords(ohci, addr + ED_WBACK_OFFSET, + (uint32_t *)((char *)ed + ED_WBACK_OFFSET), + ED_WBACK_SIZE >> 2); } static inline int ohci_put_td(OHCIState *ohci, @@ -588,7 +603,9 @@ static inline int ohci_put_iso_td(OHCIState *ohci, static inline int ohci_put_hcca(OHCIState *ohci, uint32_t addr, struct ohci_hcca *hcca) { - cpu_physical_memory_write(addr + ohci->localmem_base, hcca, sizeof(*hcca)); + cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, + (char *)hcca + HCCA_WRITEBACK_OFFSET, + HCCA_WRITEBACK_SIZE); return 1; }