From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L9d8q-0001GG-F0 for qemu-devel@nongnu.org; Mon, 08 Dec 2008 05:15:04 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L9d8p-0001Fy-TH for qemu-devel@nongnu.org; Mon, 08 Dec 2008 05:15:04 -0500 Received: from [199.232.76.173] (port=33331 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L9d8p-0001Fv-Fy for qemu-devel@nongnu.org; Mon, 08 Dec 2008 05:15:03 -0500 Received: from yx-out-1718.google.com ([74.125.44.158]:41289) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L9d8p-0008Hj-6E for qemu-devel@nongnu.org; Mon, 08 Dec 2008 05:15:03 -0500 Received: by yx-out-1718.google.com with SMTP id 3so625118yxi.82 for ; Mon, 08 Dec 2008 02:15:01 -0800 (PST) Message-ID: <43d6ff410812080215j77fbf4a4y2729dd330b6a8ce7@mail.gmail.com> Date: Mon, 8 Dec 2008 11:15:00 +0100 From: "Thomas Bandelier" Subject: Re: [Qemu-devel] [PATCH] usb-ohci: Add address masking. In-Reply-To: <200812051352.mB5DqhQU026115@smtp11.dti.ne.jp> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_37969_16416479.1228731300597" References: <200812011859.35859.paul@codesourcery.com> <200812051352.mB5DqhQU026115@smtp11.dti.ne.jp> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Paul Brook ------=_Part_37969_16416479.1228731300597 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline Hi, Why are you writing "usb-ohci is *really* broken now." ?? Is it supposed to be working or not ? On which status are you basing your affirmation? Is it a good thing to completely break USB ohci in the development trunk? Regards Thomas On Fri, Dec 5, 2008 at 2:52 PM, wrote: > Hi, > About this > > [1] It's actually the offset from the start of the first page of that > region. > > In practice this difference doesn't matter, and makes the implementation > a > > lot simpler. > again. > > The patch below adds address mask(0xff) to mmio read/write funcs in > usb-ohci. > I know now we are deleting this kind of address mask/subtract things. > But because of the restriction [1] above, we have to add address mask here. > > The mmio change potentially breaks PCI device emulations which has mmio > region > smaller than page size, like usb-ohci and rtl8139 (both have 256byte), > because > PCI memory regions are configured to be aligned to it size, but to page. > > usb-ohci is really broken now. > rtl8139 is OK because it already has address mask. > Others are not checked. > > I think the most important thing is eliminating dependency to base address, > and small numbers of masks should be acceptable. > > Regards, > /yoshii > > This patch adds address mask to mmio read/write function. > Because its memory region is not always aligned to page. > > Signed-off-by: Takashi YOSHII > --- > hw/usb-ohci.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c > index 585f3a4..bfa7d77 100644 > --- a/hw/usb-ohci.c > +++ b/hw/usb-ohci.c > @@ -1366,6 +1366,7 @@ static uint32_t ohci_mem_read(void *ptr, > target_phys_addr_t addr) > fprintf(stderr, "usb-ohci: Mis-aligned read\n"); > return 0xffffffff; > } > + addr &= 0xff; > > if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) { > /* HcRhPortStatus */ > @@ -1462,6 +1463,7 @@ static void ohci_mem_write(void *ptr, > target_phys_addr_t addr, uint32_t val) > fprintf(stderr, "usb-ohci: Mis-aligned write\n"); > return; > } > + addr &= 0xff; > > if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) { > /* HcRhPortStatus */ > -- > 1.5.6.3 > > > > ------=_Part_37969_16416479.1228731300597 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline Hi,

Why are you writing "usb-ohci is really broken now." ??
Is it supposed to be working or not ?
On which status are you basing your affirmation?

Is it a good thing to completely break USB ohci in the development trunk?

Regards

Thomas




On Fri, Dec 5, 2008 at 2:52 PM, <takasi-y@ops.dti.ne.jp> wrote:
Hi,
About this
> [1] It's actually the offset from the start of the first page of that region.
> In practice this difference doesn't matter, and makes the implementation a
> lot simpler.
again.

The patch below adds address mask(0xff) to mmio read/write funcs in usb-ohci.
I know now we are deleting this kind of address mask/subtract things.
But because of the restriction [1] above, we have to add address mask here.

The mmio change potentially breaks PCI device emulations which has mmio region
smaller than page size, like usb-ohci and rtl8139 (both have 256byte), because
PCI memory regions are configured to be aligned to it size, but to page.

usb-ohci is really broken now.
rtl8139 is OK because it already has address mask.
Others are not checked.

I think the most important thing is eliminating dependency to base address,
and small numbers of masks should be acceptable.

Regards,
/yoshii

This patch adds address mask to mmio read/write function.
Because its memory region is not always aligned to page.

Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
---
 hw/usb-ohci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 585f3a4..bfa7d77 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -1366,6 +1366,7 @@ static uint32_t ohci_mem_read(void *ptr, target_phys_addr_t addr)
        fprintf(stderr, "usb-ohci: Mis-aligned read\n");
        return 0xffffffff;
    }
+    addr &= 0xff;

    if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) {
        /* HcRhPortStatus */
@@ -1462,6 +1463,7 @@ static void ohci_mem_write(void *ptr, target_phys_addr_t addr, uint32_t val)
        fprintf(stderr, "usb-ohci: Mis-aligned write\n");
        return;
    }
+    addr &= 0xff;

    if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) {
        /* HcRhPortStatus */
--
1.5.6.3




------=_Part_37969_16416479.1228731300597--