From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39972) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8wQw-0000Qo-D1 for qemu-devel@nongnu.org; Tue, 04 Sep 2012 12:57:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T8wQu-00056o-Sc for qemu-devel@nongnu.org; Tue, 04 Sep 2012 12:57:02 -0400 Received: from goliath.siemens.de ([192.35.17.28]:31143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8wQu-00056O-Iq for qemu-devel@nongnu.org; Tue, 04 Sep 2012 12:57:00 -0400 Message-ID: <504632D5.2040009@siemens.com> Date: Tue, 04 Sep 2012 18:56:53 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <13a02c99e22d6180bc4a5d58d7b38b249773fa73.1346741532.git.julien.grall@citrix.com> <50461AF8.3090304@siemens.com> <50462D68.1020508@citrix.com> <5046317A.9020200@siemens.com> In-Reply-To: <5046317A.9020200@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V8 4/8] hw/acpi_piix4.c: replace register_ioport* List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Julien Grall Cc: Stefano Stabellini , "kraxel@redhat.com" , "avi@redhat.com" , "afaerber@suse.de" , "qemu-devel@nongnu.org" On 2012-09-04 18:51, Jan Kiszka wrote: > On 2012-09-04 18:33, Julien Grall wrote: >> On 09/04/2012 04:15 PM, Jan Kiszka wrote: >>> On 2012-09-04 09:28, Julien Grall wrote: >>> >>>> This patch replaces all register_ioport* with the new memory API. It permits >>>> to use the new Memory stuff like listener. >>>> >>>> Signed-off-by: Julien Grall >>>> --- >>>> hw/acpi_piix4.c | 145 +++++++++++++++++++++++++++++++++++++++++++------------ >>>> 1 files changed, 113 insertions(+), 32 deletions(-) >>>> >>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c >>>> index 0b4ad24..cd70610 100644 >>>> --- a/hw/acpi_piix4.c >>>> +++ b/hw/acpi_piix4.c >>>> @@ -41,10 +41,10 @@ >>>> >>>> #define GPE_BASE 0xafe0 >>>> #define GPE_LEN 4 >>>> -#define PCI_UP_BASE 0xae00 >>>> -#define PCI_DOWN_BASE 0xae04 >>>> +#define PCI_BASE 0xae00 >>>> #define PCI_EJ_BASE 0xae08 >>>> #define PCI_RMV_BASE 0xae0c >>>> +#define PM_BASE 0x00 >>>> >>>> #define PIIX4_PCI_HOTPLUG_STATUS 2 >>>> >>>> @@ -55,7 +55,7 @@ struct pci_status { >>>> >>>> typedef struct PIIX4PMState { >>>> PCIDevice dev; >>>> - IORange ioport; >>>> + MemoryRegion pm_io; >>>> ACPIREGS ar; >>>> >>>> APMState apm; >>>> @@ -64,6 +64,11 @@ typedef struct PIIX4PMState { >>>> uint32_t smb_io_base; >>>> >>>> MemoryRegion smb_io; >>>> + MemoryRegion acpi_io; >>>> + MemoryRegion acpi_hot_io; >>>> + PortioList pci_hot_port_list; >>>> + MemoryRegion pciej_hot_io; >>>> + MemoryRegion pcirmv_hot_io; >>>> >>>> qemu_irq irq; >>>> qemu_irq smi_irq; >>>> @@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar) >>>> pm_update_sci(s); >>>> } >>>> >>>> -static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width, >>>> - uint64_t val) >>>> +static void pm_ioport_write(void *opaque, target_phys_addr_t addr, >>>> + uint64_t val, unsigned width) >>>> { >>>> - PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport); >>>> + PIIX4PMState *s = opaque; >>>> >>>> if (width != 2) { >>>> PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n", >>>> @@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width, >>>> (unsigned int)val); >>>> } >>>> >>>> -static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width, >>>> - uint64_t *data) >>>> +static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr, >>>> + unsigned width) >>>> { >>>> - PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport); >>>> - uint32_t val; >>>> + PIIX4PMState *s = opaque; >>>> + uint64_t val; >>>> >>>> switch(addr) { >>>> case 0x00: >>>> @@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width, >>>> break; >>>> } >>>> PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned int)addr, val); >>>> - *data = val; >>>> + >>>> + return val; >>>> } >>>> >>>> -static const IORangeOps pm_iorange_ops = { >>>> +static const MemoryRegionOps pm_io_ops = { >>>> .read = pm_ioport_read, >>>> .write = pm_ioport_write, >>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>> + .impl = { >>>> + .min_access_size = 2, >>>> + .max_access_size = 2, >>>> >>> Where do these constraints come from? >>> >> I don't pay enough attention about the size. >> >>> OK, this one breaks my Win7 guest. Following my suspect above and the >>> endless loop over >>> >>> kvm_pio: pio_read at 0xb008 size 4 count 1 >>> >>> I played with max_access_size = 4 for pm_io_ops, and Windows boots >>> again. Looking at the details, the PIO range was apparently not properly >>> specified so far. It implements 2-bytes accesses for the offsets 0x00, >>> 0x02, 0x04 and 4-bytes access for 0x08. But the specification was that >>> accesses of all sizes are provided. >>> >>> Given this experience, we will have to review at least the hacky ACPI >>> stuff very carefully. >>> >> >> Could we change max_access_size to 4 and check on each PIO if >> the size is correct ? ie 2-bytes access for 0x00, 0x02, 0x04 and 4-bytes >> access for 0x08. >> > > TBH, I have no clue what access constraints exist for this PIO region. > Unless someone can point them out, it is probably best to not apply any > additional checks, like in the original code, just extend to 4 as > maximum size. ...and better also allow byte access. Then we should not be able to regress. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux