[Qemu-devel] [PATCH] qemu/pci: optimize pci config handling

[Qemu-devel] [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

There's no need to save all of config space before each config cycle:
just the 64 byte header is enough for our purposes.  This will become
more important as we add pci express support, which has 4K config space.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Isaku Yamahata <yamahata@valinux.co.jp>
---

Isaku Yamahata, I think with this change, you can
increase the size of config space to 4K without
need for helper functions. Makes sense?

 hw/pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 41e99a9..5986937 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -541,11 +541,11 @@ uint32_t pci_default_read_config(PCIDevice *d,
 
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
-    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
+    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
     int i;
 
     /* not efficient, but simple */
-    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
+    memcpy(orig, d->config, sizeof orig);
     for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
         uint8_t wmask = d->wmask[addr];
         d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
-- 
1.6.5.rc2

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Paolo Bonzini]

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

On 10/07/2009 02:33 PM, Michael S. Tsirkin wrote:
> There's no need to save all of config space before each config cycle:
> just the 64 byte header is enough for our purposes.  This will become
> more important as we add pci express support, which has 4K config space.

You can even go a step further and save it only if something is actually 
being changed.  Untested though.

Not-quite-signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

[-- Attachment #2: qemu-write-pci.patch --]
[-- Type: text/plain, Size: 1620 bytes --]

diff --git a/hw/pci.c b/hw/pci.c
index 41e99a9..f9959fc 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -541,19 +541,26 @@ uint32_t pci_default_read_config(PCIDevice *d,
 
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
-    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
+    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
     int i;
 
-    /* not efficient, but simple */
-    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
-    for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
-        uint8_t wmask = d->wmask[addr];
-        d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
+    /* not efficient, but simple.  If modifying the header, save it so we
+       can compare its contents later.  */
+    if (addr < sizeof orig) {
+        memcpy(orig, d->config, sizeof orig);
+    }
+
+    for(i = 0; i < l && addr+i < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i) {
+        uint8_t wmask = d->wmask[addr+i];
+        d->config[addr+i] = (d->config[addr+i] & ~wmask) | (val & wmask);
+    }
+
+    if (addr < sizeof orig) {
+        if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
+            || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
+                & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
+            pci_update_mappings(d);
     }
-    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
-        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
-            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
-        pci_update_mappings(d);
 }
 
 void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

On Wed, Oct 07, 2009 at 03:01:39PM +0200, Paolo Bonzini wrote:
> On 10/07/2009 02:33 PM, Michael S. Tsirkin wrote:
>> There's no need to save all of config space before each config cycle:
>> just the 64 byte header is enough for our purposes.  This will become
>> more important as we add pci express support, which has 4K config space.
>
> You can even go a step further and save it only if something is actually  
> being changed.  Untested though.

I'm actively trying to get out of range-checking address.
What you porpose here is certainly more code than we had.
So why is this a good idea?

> Not-quite-signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Paolo

> diff --git a/hw/pci.c b/hw/pci.c
> index 41e99a9..f9959fc 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -541,19 +541,26 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
> -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
>      int i;
>  
> -    /* not efficient, but simple */
> -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> -    for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
> -        uint8_t wmask = d->wmask[addr];
> -        d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
> +    /* not efficient, but simple.  If modifying the header, save it so we
> +       can compare its contents later.  */
> +    if (addr < sizeof orig) {
> +        memcpy(orig, d->config, sizeof orig);
> +    }
> +
> +    for(i = 0; i < l && addr+i < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i) {
> +        uint8_t wmask = d->wmask[addr+i];
> +        d->config[addr+i] = (d->config[addr+i] & ~wmask) | (val & wmask);
> +    }
> +
> +    if (addr < sizeof orig) {
> +        if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> +            || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> +                & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> +            pci_update_mappings(d);
>      }
> -    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> -        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> -            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> -        pci_update_mappings(d);
>  }
>  
>  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Paolo Bonzini]

> I'm actively trying to get out of range-checking address.

I don't understand what you mean, sorry.

> What you porpose here is certainly more code than we had.
> So why is this a good idea?

Because it avoids the memcpy/memcmp most of the time (when the memcmp 
would surely succeed).  I supposed that would also matter more as the 
config space size increases---correct me and dismiss the patch if I am 
mistaken.

Paolo

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

On Wed, Oct 07, 2009 at 04:24:45PM +0200, Paolo Bonzini wrote:
>> I'm actively trying to get out of range-checking address.
>
> I don't understand what you mean, sorry.
>
>> What you porpose here is certainly more code than we had.
>> So why is this a good idea?
>
> Because it avoids the memcpy/memcmp most of the time (when the memcmp  
> would surely succeed).

Yes :) But at the cost of more code.  I don't think speed
matters there, so less code is good.  But even if it did, extra
read of a single cache line might be cheaper than an extra branch,
and generated code will be more compact.

>  I supposed that would also matter more as the  
> config space size increases---correct me and dismiss the patch if I am  
> mistaken.

No, we'll always only look need to look at the header, whatever the size
of the config space.  That's the point of the patch I posted - future
proof against config space size increases, not optimization.

> Paolo

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Paolo Bonzini]

>>> What you porpose here is certainly more code than we had.
>>> So why is this a good idea?
>>
>> Because it avoids the memcpy/memcmp most of the time (when the memcmp
>> would surely succeed).
>
> Yes :) But at the cost of more code.  I don't think speed
> matters there, so less code is good.

Fine.

>>   I supposed that would also matter more as the
>> config space size increases---correct me and dismiss the patch if I am
>> mistaken.
>
> No, we'll always only look need to look at the header, whatever the size
> of the config space.  That's the point of the patch I posted - future
> proof against config space size increases, not optimization.

But fewer reads on average will not modify the header, so there will be 
even fewer memcpy with my patch when the config space will be 4k.

Paolo

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 10:23:41AM +0200, Paolo Bonzini wrote:
>
>>>> What you porpose here is certainly more code than we had.
>>>> So why is this a good idea?
>>>
>>> Because it avoids the memcpy/memcmp most of the time (when the memcmp
>>> would surely succeed).
>>
>> Yes :) But at the cost of more code.  I don't think speed
>> matters there, so less code is good.
>
> Fine.
>
>>>   I supposed that would also matter more as the
>>> config space size increases---correct me and dismiss the patch if I am
>>> mistaken.
>>
>> No, we'll always only look need to look at the header, whatever the size
>> of the config space.  That's the point of the patch I posted - future
>> proof against config space size increases, not optimization.
>
> But fewer reads on average will not modify the header, so there will be  
> even fewer memcpy with my patch when the config space will be 4k.

Oh, I see. I was worrying about us adding some side effect where you
write into PCI extended register and header changes.  Maybe that won't
ever happen.

> Paolo

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Isaku Yamahata]

On Wed, Oct 07, 2009 at 02:33:49PM +0200, Michael S. Tsirkin wrote:
> There's no need to save all of config space before each config cycle:
> just the 64 byte header is enough for our purposes.  This will become
> more important as we add pci express support, which has 4K config space.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> 
> Isaku Yamahata, I think with this change, you can
> increase the size of config space to 4K without
> need for helper functions. Makes sense?

It is good that the patch makes the function header size
independent(256 or 4K).
However, your patch just makes the code special.
I think helper functions should be introduced eventually.
With helper function, the implementation detail/logic will be encapsulated
cleanly within them.
When I tried to introduce callback claiming ad-hoc range check is fragile,
you did suggest such helper functions would help.
My helper function is so generic that each pci device can use
and it will surely simplify them.
With your patch, each pci device doesn't get any benefit.

If your are opposing to range check claiming that we should stick
to memcpy/memcmp and aren't opposing to helper functions,
I'd like to rewrite the helper functions with memcpy/memcmp
adding new member, orig, in PCIDevice.
At least those helper functions should be generic enough such that
they can be used by not only pci_default_write_config(), but also other
pci devices. So whole configuration space would be copied.
i.e. while 256 bytes or 4Kbytes.

Although I don't see why range check is so bad as you claim,
since memcpy or range change isn't essential to me, I compromise.

thanks,

> 
>  hw/pci.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 41e99a9..5986937 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -541,11 +541,11 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
> -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
>      int i;
>  
>      /* not efficient, but simple */
> -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> +    memcpy(orig, d->config, sizeof orig);
>      for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
>          uint8_t wmask = d->wmask[addr];
>          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);

-- 
yamahata

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 09:08:07AM +0900, Isaku Yamahata wrote:
> On Wed, Oct 07, 2009 at 02:33:49PM +0200, Michael S. Tsirkin wrote:
> > There's no need to save all of config space before each config cycle:
> > just the 64 byte header is enough for our purposes.  This will become
> > more important as we add pci express support, which has 4K config space.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> > 
> > Isaku Yamahata, I think with this change, you can
> > increase the size of config space to 4K without
> > need for helper functions. Makes sense?
> 
> It is good that the patch makes the function header size
> independent(256 or 4K).
> However, your patch just makes the code special.
> I think helper functions should be introduced eventually.
> With helper function, the implementation detail/logic will be encapsulated
> cleanly within them.

I think really most devices should be fine with range checking
address and length as they do now. Unfortunately they open-code
them, I think helpers that encapsulate range checks and
byte covered by range checks would be very good.
Note they aren't pci specific at all.

How about:
	ranges_overlap(ofset1, len1, ofset2, len2)
and
	range_covers_byte(ofset1, len1, ofset2))
?

OTOH the code *inside* pci is trying to implement most of the spec within
pci_default_config_write, so that devices do not have to worry.
So it does not really need to be encapsulated from itself.
So it is special, and the reason saving state is a good idea there
is that there is a lot of state.

Maybe we are being too smart.  Maybe any write into PCI header should
just trigger bar update routine. Then we would have a single range
check, and no memcpy, and it would work for both bridge and simple
device.  To try and scope the problem, need to count the number of
config writes that a typical OS boot does. Care to do this?

> When I tried to introduce callback claiming ad-hoc range check is fragile,
> you did suggest such helper functions would help.
> My helper function is so generic that each pci device can use
> and it will surely simplify them.

Only if they happen to only care about what you programmed in.  For
example there are already two kinds with size and without one, that is a
sign that more is to come, when we care about bits being changed.
Another problem will happen when writes have side effects on config (e.g.
VPD).  Since init helper saves state, you have to be very careful to
call the "changed" callback before you do any other changes there.
There are also too many parameters, all of which are integers, so no type
checking and easy to misuse, hard to figure out what each parameter
does.

> With your patch, each pci device doesn't get any benefit.

So generally I think what everyone does is either
(pci itself):

memcpy(orig, dev->config + REGISTER);
.....
if (memcmp(orig, dev->config + REGISTER))
	value changed do something

Here we need two operations to see that something
has changed, but we do not need to worry about
address/length, and we know when value changed,
not just was written into.

or 
	if (ranges_overlap(addr, len, REGISTER, 2))
		value might have changed do something
	if (range_covers_byte(addr, len, REGISTER))
		value might have changed do something

Here there is one operation, on the other hand it only tells us register
was written into, and we have to worry about address and length.

It's actually pretty hard to simplify each of these further :)

> If your are opposing to range check claiming that we should stick
> to memcpy/memcmp and aren't opposing to helper functions,
> I'd like to rewrite the helper functions with memcpy/memcmp
> adding new member, orig, in PCIDevice.
> At least those helper functions should be generic enough such that
> they can be used by not only pci_default_write_config(), but also other
> pci devices. So whole configuration space would be copied.
> i.e. while 256 bytes or 4Kbytes.

I had difficulty defining what does "orig" mean.
Does it just mean "state before default config was called"?
If so it's better as local variable inside default config ...

> Although I don't see why range check is so bad as you claim,

It's not bad for a single register a typical device cares about.  pci
internally is different I think.


> since memcpy or range change isn't essential to me, I compromise.

I would suggest that we split the cleanup and the functional parts.
When there is code duplication in two places, it will be much easier to
see how we remove it by using helpers.



> thanks,
> 
> > 
> >  hw/pci.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 41e99a9..5986937 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -541,11 +541,11 @@ uint32_t pci_default_read_config(PCIDevice *d,
> >  
> >  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >  {
> > -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> > +    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
> >      int i;
> >  
> >      /* not efficient, but simple */
> > -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> > +    memcpy(orig, d->config, sizeof orig);
> >      for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
> >          uint8_t wmask = d->wmask[addr];
> >          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 09:08:07AM +0900, Isaku Yamahata wrote:
> On Wed, Oct 07, 2009 at 02:33:49PM +0200, Michael S. Tsirkin wrote:
> > There's no need to save all of config space before each config cycle:
> > just the 64 byte header is enough for our purposes.  This will become
> > more important as we add pci express support, which has 4K config space.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> > 
> > Isaku Yamahata, I think with this change, you can
> > increase the size of config space to 4K without
> > need for helper functions. Makes sense?
> 
> It is good that the patch makes the function header size
> independent(256 or 4K).
> However, your patch just makes the code special.
> I think helper functions should be introduced eventually.

So maybe we should get away from both memcpy and range checks.  How
about the following?  Note: it's untested, and we should see what this
does to boot times. Quite likely nothing. I'd like to see some number on
how many times does PCI header get written to during boot. I might check
it myself but not anytime soon.

Not-Quite-Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/pci.c b/hw/pci.c
index 1debdab..19b8cc6 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -583,18 +583,14 @@ uint32_t pci_default_read_config(PCIDevice *d,
 
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
-    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
     int i;
 
     /* not efficient, but simple */
-    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
     for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
         uint8_t wmask = d->wmask[addr];
         d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
     }
-    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
-        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
-            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
+    if (ranges_overlap(addr, l, PCI_COMMAND, PCI_ROM_ADDRESS + 3))
         pci_update_mappings(d);
 }
 
diff --git a/hw/pci.h b/hw/pci.h
index 93f93fb..b9374d1 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -366,4 +366,27 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
 PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                             void *pic, int devfn_min, int nirq);
 
+/* These are not pci specific. Should move into a separate header. */
+
+/* Check whether a given range covers a given byte. */
+static inline int range_covers_byte(uint64_t first, uint64_t last,
+				    uint64_t byte)
+{
+	return first <= byte && last >= byte;
+}
+
+/* Check whether 2 given ranges overlap. Undefined if last < first. */
+static inline int ranges_overlap(uint64_t first1, uint64_t last1,
+				 uint64_t first2, uint64_t last2)
+{
+	return first1 >= last2 && first2 >= last1;
+}
+
+/* Get last byte of a range from offset + length.
+ * Undefined for ranges that wrap around 0. */
+static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
+{
+	return offset + len - 1;
+}
+
 #endif

[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Isaku Yamahata]

On Thu, Oct 08, 2009 at 11:29:11AM +0200, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2009 at 09:08:07AM +0900, Isaku Yamahata wrote:
> > On Wed, Oct 07, 2009 at 02:33:49PM +0200, Michael S. Tsirkin wrote:
> > > There's no need to save all of config space before each config cycle:
> > > just the 64 byte header is enough for our purposes.  This will become
> > > more important as we add pci express support, which has 4K config space.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > > 
> > > Isaku Yamahata, I think with this change, you can
> > > increase the size of config space to 4K without
> > > need for helper functions. Makes sense?
> > 
> > It is good that the patch makes the function header size
> > independent(256 or 4K).
> > However, your patch just makes the code special.
> > I think helper functions should be introduced eventually.
> 
> So maybe we should get away from both memcpy and range checks.  How
> about the following?  Note: it's untested, and we should see what this
> does to boot times. Quite likely nothing. I'd like to see some number on
> how many times does PCI header get written to during boot. I might check
> it myself but not anytime soon.

Looks good. I'll go for this approach.

thanks,

> Not-Quite-Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 1debdab..19b8cc6 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -583,18 +583,14 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
> -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
>      int i;
>  
>      /* not efficient, but simple */
> -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
>      for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
>          uint8_t wmask = d->wmask[addr];
>          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
>      }
> -    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> -        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> -            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> +    if (ranges_overlap(addr, l, PCI_COMMAND, PCI_ROM_ADDRESS + 3))
>          pci_update_mappings(d);
>  }
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index 93f93fb..b9374d1 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -366,4 +366,27 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>  PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                              void *pic, int devfn_min, int nirq);
>  
> +/* These are not pci specific. Should move into a separate header. */
> +
> +/* Check whether a given range covers a given byte. */
> +static inline int range_covers_byte(uint64_t first, uint64_t last,
> +				    uint64_t byte)
> +{
> +	return first <= byte && last >= byte;
> +}
> +
> +/* Check whether 2 given ranges overlap. Undefined if last < first. */
> +static inline int ranges_overlap(uint64_t first1, uint64_t last1,
> +				 uint64_t first2, uint64_t last2)
> +{
> +	return first1 >= last2 && first2 >= last1;
> +}
> +
> +/* Get last byte of a range from offset + length.
> + * Undefined for ranges that wrap around 0. */
> +static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
> +{
> +	return offset + len - 1;
> +}
> +
>  #endif
> 

-- 
yamahata