[Qemu-devel] [PATCH 1/2] memory: Fix old portio word accesses

[Qemu-devel] [PATCH 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

As we register old portio regions via ioport_register, we are also
responsible for providing the word access wrapper.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index b3ee232..f712d95 100644
--- a/memory.c
+++ b/memory.c
@@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange *iorange,
         *data = ((uint64_t)1 << (width * 8)) - 1;
         if (mrp) {
             *data = mrp->read(mr->opaque, offset + mr->offset);
+        } else if (width == 2) {
+            mrp = find_portio(mr, offset, 1, false);
+            assert(mrp);
+            *data = mrp->read(mr->opaque, offset + mr->offset) |
+                    mrp->read(mr->opaque, offset + mr->offset + 1);
         }
         return;
     }
@@ -419,6 +424,11 @@ static void memory_region_iorange_write(IORange *iorange,
 
         if (mrp) {
             mrp->write(mr->opaque, offset + mr->offset, data);
+        } else if (width == 2) {
+            mrp = find_portio(mr, offset, 1, false);
+            assert(mrp);
+            mrp->write(mr->opaque, offset + mr->offset, data & 0xff);
+            mrp->write(mr->opaque, offset + mr->offset + 1, data >> 8);
         }
         return;
     }
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

As we register old portio regions via ioport_register, we are also
responsible for providing the word access wrapper.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Oops, was lacking a shift for word reads.

 memory.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index b3ee232..aef4702 100644
--- a/memory.c
+++ b/memory.c
@@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange *iorange,
         *data = ((uint64_t)1 << (width * 8)) - 1;
         if (mrp) {
             *data = mrp->read(mr->opaque, offset + mr->offset);
+        } else if (width == 2) {
+            mrp = find_portio(mr, offset, 1, false);
+            assert(mrp);
+            *data = mrp->read(mr->opaque, offset + mr->offset) |
+                    (mrp->read(mr->opaque, offset + mr->offset + 1) << 8);
         }
         return;
     }
@@ -419,6 +424,11 @@ static void memory_region_iorange_write(IORange *iorange,
 
         if (mrp) {
             mrp->write(mr->opaque, offset + mr->offset, data);
+        } else if (width == 2) {
+            mrp = find_portio(mr, offset, 1, false);
+            assert(mrp);
+            mrp->write(mr->opaque, offset + mr->offset, data & 0xff);
+            mrp->write(mr->opaque, offset + mr->offset + 1, data >> 8);
         }
         return;
     }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/18/2011 03:51 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> As we register old portio regions via ioport_register, we are also
> responsible for providing the word access wrapper.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>
> Oops, was lacking a shift for word reads.
>
>   memory.c |   10 ++++++++++
>   1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index b3ee232..aef4702 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange *iorange,
>           *data = ((uint64_t)1<<  (width * 8)) - 1;
>           if (mrp) {
>               *data = mrp->read(mr->opaque, offset + mr->offset);
> +        } else if (width == 2) {
> +            mrp = find_portio(mr, offset, 1, false);
> +            assert(mrp);
> +            *data = mrp->read(mr->opaque, offset + mr->offset) |
> +                    (mrp->read(mr->opaque, offset + mr->offset + 1)<<  8);
>           }

What about width 4?  Why not use access_with_adjusted_size()?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On 2011-09-18 17:37, Avi Kivity wrote:
> On 09/18/2011 03:51 PM, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> As we register old portio regions via ioport_register, we are also
>> responsible for providing the word access wrapper.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>> Oops, was lacking a shift for word reads.
>>
>>   memory.c |   10 ++++++++++
>>   1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index b3ee232..aef4702 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange
>> *iorange,
>>           *data = ((uint64_t)1<<  (width * 8)) - 1;
>>           if (mrp) {
>>               *data = mrp->read(mr->opaque, offset + mr->offset);
>> +        } else if (width == 2) {
>> +            mrp = find_portio(mr, offset, 1, false);
>> +            assert(mrp);
>> +            *data = mrp->read(mr->opaque, offset + mr->offset) |
>> +                    (mrp->read(mr->opaque, offset + mr->offset +
>> 1)<<  8);
>>           }
> 
> What about width 4?

This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

>  Why not use access_with_adjusted_size()?

Because of different accessor prototypes.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/18/2011 06:43 PM, Jan Kiszka wrote:
> On 2011-09-18 17:37, Avi Kivity wrote:
> >  On 09/18/2011 03:51 PM, Jan Kiszka wrote:
> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >>
> >>  As we register old portio regions via ioport_register, we are also
> >>  responsible for providing the word access wrapper.
> >>
> >>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>  ---
> >>
> >>  Oops, was lacking a shift for word reads.
> >>
> >>    memory.c |   10 ++++++++++
> >>    1 files changed, 10 insertions(+), 0 deletions(-)
> >>
> >>  diff --git a/memory.c b/memory.c
> >>  index b3ee232..aef4702 100644
> >>  --- a/memory.c
> >>  +++ b/memory.c
> >>  @@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange
> >>  *iorange,
> >>            *data = ((uint64_t)1<<   (width * 8)) - 1;
> >>            if (mrp) {
> >>                *data = mrp->read(mr->opaque, offset + mr->offset);
> >>  +        } else if (width == 2) {
> >>  +            mrp = find_portio(mr, offset, 1, false);
> >>  +            assert(mrp);
> >>  +            *data = mrp->read(mr->opaque, offset + mr->offset) |
> >>  +                    (mrp->read(mr->opaque, offset + mr->offset +
> >>  1)<<   8);
> >>            }
> >
> >  What about width 4?
>
> This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

x86 PIO is not limited to 16 bits, just ISA, which memory.c knows 
nothing about.

> >   Why not use access_with_adjusted_size()?
>
> Because of different accessor prototypes.
>

Can be thunked.  There is a different issue, a_w_a_s() can use small 
accesses to emulate large ones, but not vice versa.  It needs fixing anyway.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On 2011-09-18 17:45, Avi Kivity wrote:
> On 09/18/2011 06:43 PM, Jan Kiszka wrote:
>> On 2011-09-18 17:37, Avi Kivity wrote:
>> >  On 09/18/2011 03:51 PM, Jan Kiszka wrote:
>> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >>  As we register old portio regions via ioport_register, we are also
>> >>  responsible for providing the word access wrapper.
>> >>
>> >>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> >>  ---
>> >>
>> >>  Oops, was lacking a shift for word reads.
>> >>
>> >>    memory.c |   10 ++++++++++
>> >>    1 files changed, 10 insertions(+), 0 deletions(-)
>> >>
>> >>  diff --git a/memory.c b/memory.c
>> >>  index b3ee232..aef4702 100644
>> >>  --- a/memory.c
>> >>  +++ b/memory.c
>> >>  @@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange
>> >>  *iorange,
>> >>            *data = ((uint64_t)1<<   (width * 8)) - 1;
>> >>            if (mrp) {
>> >>                *data = mrp->read(mr->opaque, offset + mr->offset);
>> >>  +        } else if (width == 2) {
>> >>  +            mrp = find_portio(mr, offset, 1, false);
>> >>  +            assert(mrp);
>> >>  +            *data = mrp->read(mr->opaque, offset + mr->offset) |
>> >>  +                    (mrp->read(mr->opaque, offset + mr->offset +
>> >>  1)<<   8);
>> >>            }
>> >
>> >  What about width 4?
>>
>> This is PIO, limited by the x86 address space to 16 bit. Will add a
>> comment.
> 
> x86 PIO is not limited to 16 bits, just ISA, which memory.c knows
> nothing about.

Confused address and data, the former is limited 16, the latter can be
32 as well. But I guess only ISA models made use of the core's split up
service, and that's why QEMU limited itself accordingly.

> 
>> >   Why not use access_with_adjusted_size()?
>>
>> Because of different accessor prototypes.
>>
> 
> Can be thunked.  There is a different issue, a_w_a_s() can use small
> accesses to emulate large ones, but not vice versa.  It needs fixing
> anyway.
> 

IIRC, that's a feature: Devices not implementing small accesses tend to
refuse them in reality.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/18/2011 07:28 PM, Jan Kiszka wrote:
> >>
> >>  This is PIO, limited by the x86 address space to 16 bit. Will add a
> >>  comment.
> >
> >  x86 PIO is not limited to 16 bits, just ISA, which memory.c knows
> >  nothing about.
>
> Confused address and data, the former is limited 16, the latter can be
> 32 as well. But I guess only ISA models made use of the core's split up
> service, and that's why QEMU limited itself accordingly.

Let's not bury such details in the core.

>
> >
> >>  >    Why not use access_with_adjusted_size()?
> >>
> >>  Because of different accessor prototypes.
> >>
> >
> >  Can be thunked.  There is a different issue, a_w_a_s() can use small
> >  accesses to emulate large ones, but not vice versa.  It needs fixing
> >  anyway.
> >
>
> IIRC, that's a feature: Devices not implementing small accesses tend to
> refuse them in reality.

I don't think this holds for pci; there the bus always generates 32-bit 
writes with separate byte enables for each lane.  The device need not 
even be aware of a sub-word access, for reads.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On 2011-09-18 18:49, Avi Kivity wrote:
> On 09/18/2011 07:28 PM, Jan Kiszka wrote:
>> >>
>> >>  This is PIO, limited by the x86 address space to 16 bit. Will add a
>> >>  comment.
>> >
>> >  x86 PIO is not limited to 16 bits, just ISA, which memory.c knows
>> >  nothing about.
>>
>> Confused address and data, the former is limited 16, the latter can be
>> 32 as well. But I guess only ISA models made use of the core's split up
>> service, and that's why QEMU limited itself accordingly.
> 
> Let's not bury such details in the core.

It's already in the core (ioport), and would refrain from changing it in
this fix.

> 
>>
>> >
>> >>  >    Why not use access_with_adjusted_size()?
>> >>
>> >>  Because of different accessor prototypes.
>> >>
>> >
>> >  Can be thunked.  There is a different issue, a_w_a_s() can use small
>> >  accesses to emulate large ones, but not vice versa.  It needs fixing
>> >  anyway.
>> >
>>
>> IIRC, that's a feature: Devices not implementing small accesses tend to
>> refuse them in reality.
> 
> I don't think this holds for pci; there the bus always generates 32-bit
> writes with separate byte enables for each lane.  The device need not
> even be aware of a sub-word access, for reads.

The problem is that once we "enhance" the core with such a support to
potentially help one use case, we need to validate all users again if
they depend on the old behavior. That's tricky as breakage may only show
up with odd guests that issue invalid but so far harmless requests.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/18/2011 10:07 PM, Jan Kiszka wrote:
> On 2011-09-18 18:49, Avi Kivity wrote:
> >  On 09/18/2011 07:28 PM, Jan Kiszka wrote:
> >>  >>
> >>  >>   This is PIO, limited by the x86 address space to 16 bit. Will add a
> >>  >>   comment.
> >>  >
> >>  >   x86 PIO is not limited to 16 bits, just ISA, which memory.c knows
> >>  >   nothing about.
> >>
> >>  Confused address and data, the former is limited 16, the latter can be
> >>  32 as well. But I guess only ISA models made use of the core's split up
> >>  service, and that's why QEMU limited itself accordingly.
> >
> >  Let's not bury such details in the core.
>
> It's already in the core (ioport), and would refrain from changing it in
> this fix.

That's the bad old core we're trying to get away from.

> >
> >  I don't think this holds for pci; there the bus always generates 32-bit
> >  writes with separate byte enables for each lane.  The device need not
> >  even be aware of a sub-word access, for reads.
>
> The problem is that once we "enhance" the core with such a support to
> potentially help one use case, we need to validate all users again if
> they depend on the old behavior. That's tricky as breakage may only show
> up with odd guests that issue invalid but so far harmless requests.

It's opt-in.  If a device sets 
MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed 
byte accesses (the core will take care of breaking apart larger 
writes).  If it sets MemoryRegionOps::impl.{min,max}_access_size = 4, it 
will only get long accesses (and the core will/should shift/mask or 
RMW).  Refusing illegal access sizes is done using 
MemoryRegionOps::valid.  Most of this is unimplemented unfortunately.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

On 2011-09-19 14:19, Avi Kivity wrote:
> On 09/18/2011 10:07 PM, Jan Kiszka wrote:
>> On 2011-09-18 18:49, Avi Kivity wrote:
>> >  On 09/18/2011 07:28 PM, Jan Kiszka wrote:
>> >>  >>
>> >>  >>   This is PIO, limited by the x86 address space to 16 bit. Will
>> add a
>> >>  >>   comment.
>> >>  >
>> >>  >   x86 PIO is not limited to 16 bits, just ISA, which memory.c knows
>> >>  >   nothing about.
>> >>
>> >>  Confused address and data, the former is limited 16, the latter
>> can be
>> >>  32 as well. But I guess only ISA models made use of the core's
>> split up
>> >>  service, and that's why QEMU limited itself accordingly.
>> >
>> >  Let's not bury such details in the core.
>>
>> It's already in the core (ioport), and would refrain from changing it in
>> this fix.
> 
> That's the bad old core we're trying to get away from.

We overcome it at the point the last portio user was converted and that
legacy is removed.

> 
>> >
>> >  I don't think this holds for pci; there the bus always generates
>> 32-bit
>> >  writes with separate byte enables for each lane.  The device need not
>> >  even be aware of a sub-word access, for reads.
>>
>> The problem is that once we "enhance" the core with such a support to
>> potentially help one use case, we need to validate all users again if
>> they depend on the old behavior. That's tricky as breakage may only show
>> up with odd guests that issue invalid but so far harmless requests.
> 
> It's opt-in.  If a device sets
> MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed
> byte accesses (the core will take care of breaking apart larger
> writes).  If it sets MemoryRegionOps::impl.{min,max}_access_size = 4, it
> will only get long accesses (and the core will/should shift/mask or
> RMW).  Refusing illegal access sizes is done using
> MemoryRegionOps::valid.  Most of this is unimplemented unfortunately.

That makes sense (for non-old_portio users).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/19/2011 03:32 PM, Jan Kiszka wrote:
> >  It's opt-in.  If a device sets
> >  MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed
> >  byte accesses (the core will take care of breaking apart larger
> >  writes).  If it sets MemoryRegionOps::impl.{min,max}_access_size = 4, it
> >  will only get long accesses (and the core will/should shift/mask or
> >  RMW).  Refusing illegal access sizes is done using
> >  MemoryRegionOps::valid.  Most of this is unimplemented unfortunately.
>
> That makes sense (for non-old_portio users).
>
>

The trick of having a way to register N callbacks with one shot is worth 
growing.  Ideally each register in a BAR would have a callback and we'd 
do something like

     MemoryRegionOps mydev_ops = {
         .registers = {
              { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
              ...
          },
     }

with hints to the core like "this register sits at this offset, use it 
for reads instead of a callback", or, "this is a read-only register".

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 2011-09-19 14:42, Avi Kivity wrote:
> On 09/19/2011 03:32 PM, Jan Kiszka wrote:
>> >  It's opt-in.  If a device sets
>> >  MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed
>> >  byte accesses (the core will take care of breaking apart larger
>> >  writes).  If it sets MemoryRegionOps::impl.{min,max}_access_size =
>> 4, it
>> >  will only get long accesses (and the core will/should shift/mask or
>> >  RMW).  Refusing illegal access sizes is done using
>> >  MemoryRegionOps::valid.  Most of this is unimplemented unfortunately.
>>
>> That makes sense (for non-old_portio users).
>>
>>
> 
> The trick of having a way to register N callbacks with one shot is worth
> growing.  Ideally each register in a BAR would have a callback and we'd
> do something like
> 
>     MemoryRegionOps mydev_ops = {
>         .registers = {
>              { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
>              ...
>          },
>     }
> 
> with hints to the core like "this register sits at this offset, use it
> for reads instead of a callback", or, "this is a read-only register".

This has pros and cons. If you have n registers to dispatch, you then
have to write n function prologues and maybe epilogues instead of just
one. Specifically if the register access is trivial, that could case
quite some LoC blowup on the device side.

What may have a better ratio are generic register get/set handlers.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/19/2011 03:55 PM, Jan Kiszka wrote:
> >
> >  The trick of having a way to register N callbacks with one shot is worth
> >  growing.  Ideally each register in a BAR would have a callback and we'd
> >  do something like
> >
> >      MemoryRegionOps mydev_ops = {
> >          .registers = {
> >               { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
> >               ...
> >           },
> >      }
> >
> >  with hints to the core like "this register sits at this offset, use it
> >  for reads instead of a callback", or, "this is a read-only register".
>
> This has pros and cons. If you have n registers to dispatch, you then
> have to write n function prologues and maybe epilogues instead of just
> one. Specifically if the register access is trivial, that could case
> quite some LoC blowup on the device side.
>
> What may have a better ratio are generic register get/set handlers.
>

With C++ pointers-to-members and pointers-to-member-functions, you 
actually get some nice representation:

class MyDev {

     void reg_1_read(...) { return some_computation(); }
     void reg_1_write(...) { do_something(); }

     uint32_t reg_2;
     void reg_2_write(...) { reg_2 = value; do_something(); }

     uint64_t reg_3;

     static const Register registers[] = {
           Register(REG_1, &MyDev::reg_1_read, &MyDev::reg_1_write),
           Register(REG_2, &MyDev::reg_2, &MyDev::reg_2_write),
           Register(REG_1, &MyDev::reg_3),
     };
};

... and the Register class generates the appropriate accessors.  We can 
emulate some of this with macros, but the conversion from opaque to the 
actual type will always be ugly.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Gerd Hoffmann]

   Hi,

>> The trick of having a way to register N callbacks with one shot is worth
>> growing.  Ideally each register in a BAR would have a callback and we'd
>> do something like
>>
>>      MemoryRegionOps mydev_ops = {
>>          .registers = {
>>               { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
>>               ...
>>           },
>>      }
>>
>> with hints to the core like "this register sits at this offset, use it
>> for reads instead of a callback", or, "this is a read-only register".
>
> This has pros and cons. If you have n registers to dispatch, you then
> have to write n function prologues and maybe epilogues instead of just
> one. Specifically if the register access is trivial, that could case
> quite some LoC blowup on the device side.

If the register access is trivial then you don't need to call into the 
driver at all ...

You can have a look at hw/intel-hda.c which actually implements 
something like this, with some commonly needed features:

   * The "offset" field already mentioned by avi is there, so trivial
     reads/writes can be handled by the core.
   * A "wmask" field to specify which bits are guest writable.
   * A "wclear" field to specify which bits have write-one-to-clear
     semantics.
   * A "reset" field which specified the value this field has after
     device reset.  Also serves as value for read-only registers.
   * read/write handlers of course.  The write handler is called after
     the core applied sanity checks and calculated the new register
     value (using wmask+wclear).
   * A "name" field (for debug logging).

It's pretty nice, alot more readable that a big switch, forces you to 
think which bits the guest can set (not specifying a wmask gives you a 
read-only register ;).

Also no bloat.  With this moving to memory core the all the handlers 
will gain a line with a container_of(), but that isn't too bad too IMHO.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/19/2011 04:55 PM, Gerd Hoffmann wrote:
>
> If the register access is trivial then you don't need to call into the 
> driver at all ...
>
> You can have a look at hw/intel-hda.c which actually implements 
> something like this, with some commonly needed features:
>
>   * The "offset" field already mentioned by avi is there, so trivial
>     reads/writes can be handled by the core.
>   * A "wmask" field to specify which bits are guest writable.
>   * A "wclear" field to specify which bits have write-one-to-clear
>     semantics.
>   * A "reset" field which specified the value this field has after
>     device reset.  Also serves as value for read-only registers.
>   * read/write handlers of course.  The write handler is called after
>     the core applied sanity checks and calculated the new register
>     value (using wmask+wclear).
>   * A "name" field (for debug logging).
>
> It's pretty nice, alot more readable that a big switch, forces you to 
> think which bits the guest can set (not specifying a wmask gives you a 
> read-only register ;).
>
> Also no bloat.  With this moving to memory core the all the handlers 
> will gain a line with a container_of(), but that isn't too bad too IMHO.

It's also more secure.  Move as much as possible into the core, and 
review (and fuzz) that like hell.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Avi Kivity]

On 09/18/2011 03:51 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> As we register old portio regions via ioport_register, we are also
> responsible for providing the word access wrapper.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>
> Oops, was lacking a shift for word reads.
>
>

Thanks, applied; while I don't like it, vga is such a mess now that we 
have to reduce the number of fronts and get something working.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] memory: Fix old portio word accesses

[Richard Henderson]

On 09/18/2011 05:44 AM, Jan Kiszka wrote:
> +            *data = mrp->read(mr->opaque, offset + mr->offset) |
> +                    mrp->read(mr->opaque, offset + mr->offset + 1);

Missing shift.

Also, a comment about the fact that the legacy interface doesn't
handle size 4 would also be welcome.


r~