Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
Boris Ostrovsky writes ("Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses"): > On 05/25/2016 12:09 PM, Ian Jackson wrote: > > I think this question can only be resolved de jure by looking at what > > previous ACPI specifications (before this AccessSize field) said. > > It's been around since 3.0 (which is 2004). Prior to that --- my cursory > read of 2.0 suggests that accesses were 8-bit. That would mean that in the absence of indication that the new standard is supported, accesses should be 8-bit. > > But, I think: de facto, what is going on here is that ACPICA and hence > > Linux have changed their behaviour in a way that is not compatible > > with at least some existing "hardware". Is this not arguably a > > compatibility defect Linux ? > > > > It would surely be better to make Linux do whatever it did before, > > when AccessSize is not supplied. That will avoid breaking any other > > things (whether or not those other things are de jure broken according > > to previous specs). It will also avoid us having to make changes our > > ACPI tables which themselves come with a risk of compatibility > > problems. > > ACPICA will use 32-bit accesses for access_size=0: > https://github.com/acpica/acpica/commit/c49a751b That commit message does not justify the decision other than as an optimisation. Backward-incompatible `optimisations' are a bad idea both de jure and de facto. > However, Linux appears to have some sort of workaround for FreeBSD, > which *appears* as it should be applicable to hvmloader's tables as > well. But it clearly does not as we are failing on Linux. > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/acpi/acpica/hwregs.c?id=b314a172ee968d45f72dffea68ab8af38aa80ded > > Let me see whether which path we take. I confess I don't understand that patch, but it does seem to be an attempt to provide backward-compatible behaviour. Overall, all this new information tends to reinforce my initial supposition that the bug is in ACPICA and that qemu-xen-traditional ought not to be changed. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On 05/25/2016 12:51 PM, Boris Ostrovsky wrote: > Let me see whether which path we take. That is "Let me see which code path we are taking in Linux" -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On 05/25/2016 12:09 PM, Ian Jackson wrote: > Jan Beulich writes ("Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support > 32-bit default_ioport_* accesses"): >> On 25.05.16 at 17:36, <boris.ostrov...@oracle.com> wrote: >>> AccesSize parameter is optional when invoking the Register macro. If the >>> AccessSize parameter is >>> not supplied then the AccessSize field will be set to zero. In this >>> case, OSPM will assume the access >>> size. >>> >>> I don't think I understand what the last sentence means. Does it imply >>> that SW can do whatever it thinks is appropriate? >> I think so, yes. > I think this question can only be resolved de jure by looking at what > previous ACPI specifications (before this AccessSize field) said. It's been around since 3.0 (which is 2004). Prior to that --- my cursory read of 2.0 suggests that accesses were 8-bit. > > But, I think: de facto, what is going on here is that ACPICA and hence > Linux have changed their behaviour in a way that is not compatible > with at least some existing "hardware". Is this not arguably a > compatibility defect Linux ? > > It would surely be better to make Linux do whatever it did before, > when AccessSize is not supplied. That will avoid breaking any other > things (whether or not those other things are de jure broken according > to previous specs). It will also avoid us having to make changes our > ACPI tables which themselves come with a risk of compatibility > problems. ACPICA will use 32-bit accesses for access_size=0: https://github.com/acpica/acpica/commit/c49a751b However, Linux appears to have some sort of workaround for FreeBSD, which *appears* as it should be applicable to hvmloader's tables as well. But it clearly does not as we are failing on Linux. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/acpi/acpica/hwregs.c?id=b314a172ee968d45f72dffea68ab8af38aa80ded Let me see whether which path we take. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
Jan Beulich writes ("Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses"): > On 25.05.16 at 17:36, <boris.ostrov...@oracle.com> wrote: > > AccesSize parameter is optional when invoking the Register macro. If the > > AccessSize parameter is > > not supplied then the AccessSize field will be set to zero. In this > > case, OSPM will assume the access > > size. > > > > I don't think I understand what the last sentence means. Does it imply > > that SW can do whatever it thinks is appropriate? > > I think so, yes. I think this question can only be resolved de jure by looking at what previous ACPI specifications (before this AccessSize field) said. But, I think: de facto, what is going on here is that ACPICA and hence Linux have changed their behaviour in a way that is not compatible with at least some existing "hardware". Is this not arguably a compatibility defect Linux ? It would surely be better to make Linux do whatever it did before, when AccessSize is not supplied. That will avoid breaking any other things (whether or not those other things are de jure broken according to previous specs). It will also avoid us having to make changes our ACPI tables which themselves come with a risk of compatibility problems. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
>>> On 25.05.16 at 17:36,wrote: > This is what the spec says: > > AccessSize evaluates to an 8-bit integer that specifies the size of data > values used when accessing the > address space as follows: > 0 - Undefined (legacy) > 1 - Byte access > 2 - Word access > 3 - DWord access > 4 - QWord access > The 8-bit field DescriptorName . _ASZ is automatically created in order > to refer to this portion of the > resource descriptor. See _ASZ (page 368) for more information. For > backwards compatibility, the > AccesSize parameter is optional when invoking the Register macro. If the > AccessSize parameter is > not supplied then the AccessSize field will be set to zero. In this > case, OSPM will assume the access > size. > > I don't think I understand what the last sentence means. Does it imply > that SW can do whatever it thinks is appropriate? I think so, yes. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
>>> On 25.05.16 at 17:08,wrote: > On 05/25/2016 10:35 AM, Ian Jackson wrote: >> Ian Jackson writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit > default_ioport_* accesses"): >>> Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit > default_ioport_* accesses"): Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: ACPI 2.0, Hardware: Add access_width/bit_offset support for acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. QEMU needs to be able to handle them. >>> I'm kind of missing something here. If the specification has recently >>> been updated to permit this, why should old hardware support it ? >>> >>> (I tried to find the Linux upstream git commit you're referring to but >>> my linux.git is up to date and it seems not to be fetching within a >>> reasonable time, so I thought I would reply now.) >> I have looked at this commit now and I am none the wiser. >> >> It says just "This patch adds access_width/bit_offset support in >> acpi_hw_write()". I also looked at the two linked messages: >> https://github.com/acpica/acpica/commit/48eea5e7 >> https://bugs.acpica.org/show_bug.cgi?id=1240 >> and none of this explains why this supported is needed in a >> our deep-frozen ancient branch. > > IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's > Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the > patch it used register's bit_width and now it will use access_size. > According to the spec access_size 0 means undefined/legacy access. > > I just looked at what hvmloader provides and at least for FADT > address_size is 0. And I wonder whether ACPICA uses 4-byte-access for > these cases. > > So maybe instead of trying to patch qemu-trad I should see if I can make > hvmloader provide proper access size. Let me poke at that. But don't forget about the risk of breaking other OSes with any kind of change like this one. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On 05/25/2016 11:22 AM, Ian Jackson wrote: > Boris Ostrovsky writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit > default_ioport_* accesses"): >> IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's >> Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the >> patch it used register's bit_width and now it will use access_size. >> According to the spec access_size 0 means undefined/legacy access. > I see. (Well, sort of.) > >> I just looked at what hvmloader provides and at least for FADT >> address_size is 0. And I wonder whether ACPICA uses 4-byte-access for >> these cases. > If 0 is "undefined/legacy access", shouldn't it be using the > register's bit width ? Ie, isn't this then a bug in ACPICA ? > >> So maybe instead of trying to patch qemu-trad I should see if I can make >> hvmloader provide proper access size. Let me poke at that. > If this "access size" attribute is new, things should work without it, > surely ? This is what the spec says: AccessSize evaluates to an 8-bit integer that specifies the size of data values used when accessing the address space as follows: 0 - Undefined (legacy) 1 - Byte access 2 - Word access 3 - DWord access 4 - QWord access The 8-bit field DescriptorName . _ASZ is automatically created in order to refer to this portion of the resource descriptor. See _ASZ (page 368) for more information. For backwards compatibility, the AccesSize parameter is optional when invoking the Register macro. If the AccessSize parameter is not supplied then the AccessSize field will be set to zero. In this case, OSPM will assume the access size. I don't think I understand what the last sentence means. Does it imply that SW can do whatever it thinks is appropriate? -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
Boris Ostrovsky writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses"): > IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's > Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the > patch it used register's bit_width and now it will use access_size. > According to the spec access_size 0 means undefined/legacy access. I see. (Well, sort of.) > I just looked at what hvmloader provides and at least for FADT > address_size is 0. And I wonder whether ACPICA uses 4-byte-access for > these cases. If 0 is "undefined/legacy access", shouldn't it be using the register's bit width ? Ie, isn't this then a bug in ACPICA ? > So maybe instead of trying to patch qemu-trad I should see if I can make > hvmloader provide proper access size. Let me poke at that. If this "access size" attribute is new, things should work without it, surely ? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On 05/25/2016 10:35 AM, Ian Jackson wrote: > Ian Jackson writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit > default_ioport_* accesses"): >> Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit >> default_ioport_* accesses"): >>> Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: >>> ACPI 2.0, Hardware: Add access_width/bit_offset support for >>> acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. >>> >>> QEMU needs to be able to handle them. >> I'm kind of missing something here. If the specification has recently >> been updated to permit this, why should old hardware support it ? >> >> (I tried to find the Linux upstream git commit you're referring to but >> my linux.git is up to date and it seems not to be fetching within a >> reasonable time, so I thought I would reply now.) > I have looked at this commit now and I am none the wiser. > > It says just "This patch adds access_width/bit_offset support in > acpi_hw_write()". I also looked at the two linked messages: > https://github.com/acpica/acpica/commit/48eea5e7 > https://bugs.acpica.org/show_bug.cgi?id=1240 > and none of this explains why this supported is needed in a > our deep-frozen ancient branch. IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the patch it used register's bit_width and now it will use access_size. According to the spec access_size 0 means undefined/legacy access. I just looked at what hvmloader provides and at least for FADT address_size is 0. And I wonder whether ACPICA uses 4-byte-access for these cases. So maybe instead of trying to patch qemu-trad I should see if I can make hvmloader provide proper access size. Let me poke at that. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
Ian Jackson writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses"): > Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit > default_ioport_* accesses"): > > Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: > > ACPI 2.0, Hardware: Add access_width/bit_offset support for > > acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. > > > > QEMU needs to be able to handle them. > > I'm kind of missing something here. If the specification has recently > been updated to permit this, why should old hardware support it ? > > (I tried to find the Linux upstream git commit you're referring to but > my linux.git is up to date and it seems not to be fetching within a > reasonable time, so I thought I would reply now.) I have looked at this commit now and I am none the wiser. It says just "This patch adds access_width/bit_offset support in acpi_hw_write()". I also looked at the two linked messages: https://github.com/acpica/acpica/commit/48eea5e7 https://bugs.acpica.org/show_bug.cgi?id=1240 and none of this explains why this supported is needed in a our deep-frozen ancient branch. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses"): > Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: > ACPI 2.0, Hardware: Add access_width/bit_offset support for > acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. > > QEMU needs to be able to handle them. I'm kind of missing something here. If the specification has recently been updated to permit this, why should old hardware support it ? (I tried to find the Linux upstream git commit you're referring to but my linux.git is up to date and it seems not to be fetching within a reasonable time, so I thought I would reply now.) Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On Fri, May 20, 2016 at 09:52:40AM -0400, Boris Ostrovsky wrote: > Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: > ACPI 2.0, Hardware: Add access_width/bit_offset support for > acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. > > QEMU needs to be able to handle them. > > Signed-off-by: Boris OstrovskyReviewed-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On Mon, May 23, 2016 at 09:02:58AM -0400, Boris Ostrovsky wrote: > On 05/23/2016 08:02 AM, Wei Liu wrote: > > On Fri, May 20, 2016 at 09:52:40AM -0400, Boris Ostrovsky wrote: > >> Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: > >> ACPI 2.0, Hardware: Add access_width/bit_offset support for > >> acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. > >> > >> QEMU needs to be able to handle them. > >> > >> Signed-off-by: Boris Ostrovsky> >> --- > >> vl.c | 15 --- > >> 1 file changed, 8 insertions(+), 7 deletions(-) > >> > >> diff --git a/vl.c b/vl.c > >> index c864e7d..79d3ab5 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -350,17 +350,18 @@ static void default_ioport_writew(void *opaque, > >> uint32_t address, uint32_t data) > >> > >> static uint32_t default_ioport_readl(void *opaque, uint32_t address) > >> { > >> -#ifdef DEBUG_UNUSED_IOPORT > >> -fprintf(stderr, "unused inl: port=0x%04x\n", address); > >> -#endif > >> -return 0x; > >> +uint32_t data; > >> +data = default_ioport_readw(opaque, address) & 0x; > >> +address = (address + 2) & (MAX_IOPORTS - 1); > > I'm not very familiar with how hardware behaves, but shouldn't we return > > some sort of invalid result (~0) and log when the port wraps? > > Intel SDM says that trying to access ports beyond 0x is > implementation-specific. So wrapping them I guess is valid (it's how > qemu implements it for default_ioport_readw/writew, which is what I am > following here). > Fair enough. Wei. > -boris > > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On 05/23/2016 08:02 AM, Wei Liu wrote: > On Fri, May 20, 2016 at 09:52:40AM -0400, Boris Ostrovsky wrote: >> Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: >> ACPI 2.0, Hardware: Add access_width/bit_offset support for >> acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. >> >> QEMU needs to be able to handle them. >> >> Signed-off-by: Boris Ostrovsky>> --- >> vl.c | 15 --- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index c864e7d..79d3ab5 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -350,17 +350,18 @@ static void default_ioport_writew(void *opaque, >> uint32_t address, uint32_t data) >> >> static uint32_t default_ioport_readl(void *opaque, uint32_t address) >> { >> -#ifdef DEBUG_UNUSED_IOPORT >> -fprintf(stderr, "unused inl: port=0x%04x\n", address); >> -#endif >> -return 0x; >> +uint32_t data; >> +data = default_ioport_readw(opaque, address) & 0x; >> +address = (address + 2) & (MAX_IOPORTS - 1); > I'm not very familiar with how hardware behaves, but shouldn't we return > some sort of invalid result (~0) and log when the port wraps? Intel SDM says that trying to access ports beyond 0x is implementation-specific. So wrapping them I guess is valid (it's how qemu implements it for default_ioport_readw/writew, which is what I am following here). -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
On Fri, May 20, 2016 at 09:52:40AM -0400, Boris Ostrovsky wrote: > Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: > ACPI 2.0, Hardware: Add access_width/bit_offset support for > acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. > > QEMU needs to be able to handle them. > > Signed-off-by: Boris Ostrovsky> --- > vl.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/vl.c b/vl.c > index c864e7d..79d3ab5 100644 > --- a/vl.c > +++ b/vl.c > @@ -350,17 +350,18 @@ static void default_ioport_writew(void *opaque, > uint32_t address, uint32_t data) > > static uint32_t default_ioport_readl(void *opaque, uint32_t address) > { > -#ifdef DEBUG_UNUSED_IOPORT > -fprintf(stderr, "unused inl: port=0x%04x\n", address); > -#endif > -return 0x; > +uint32_t data; > +data = default_ioport_readw(opaque, address) & 0x; > +address = (address + 2) & (MAX_IOPORTS - 1); I'm not very familiar with how hardware behaves, but shouldn't we return some sort of invalid result (~0) and log when the port wraps? Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses
Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA: ACPI 2.0, Hardware: Add access_width/bit_offset support for acpi_hw_write()") result in guests issuing 32-bit accesses to IO space. QEMU needs to be able to handle them. Signed-off-by: Boris Ostrovsky--- vl.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/vl.c b/vl.c index c864e7d..79d3ab5 100644 --- a/vl.c +++ b/vl.c @@ -350,17 +350,18 @@ static void default_ioport_writew(void *opaque, uint32_t address, uint32_t data) static uint32_t default_ioport_readl(void *opaque, uint32_t address) { -#ifdef DEBUG_UNUSED_IOPORT -fprintf(stderr, "unused inl: port=0x%04x\n", address); -#endif -return 0x; +uint32_t data; +data = default_ioport_readw(opaque, address) & 0x; +address = (address + 2) & (MAX_IOPORTS - 1); +data |= default_ioport_readw(opaque, address) << 16; +return data; } static void default_ioport_writel(void *opaque, uint32_t address, uint32_t data) { -#ifdef DEBUG_UNUSED_IOPORT -fprintf(stderr, "unused outl: port=0x%04x data=0x%02x\n", address, data); -#endif +default_ioport_writew(opaque, address, data & 0x); +address = (address + 2) & (MAX_IOPORTS - 1); +default_ioport_writew(opaque, address, data >> 16); } /* size is the word size in byte */ -- 1.8.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel