[PATCH] slow_map: minor improvements to ROM BAR handling

[PATCH] slow_map: minor improvements to ROM BAR handling

[Michael S. Tsirkin]

ROM BAR can be handled same as regular BAR:
load_option_roms utility will take care of
copying it to RAM as appropriate.

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

This patch applies on top of agraf's one,
it takes care of non-page aligned ROM BARs as well:
they mostly are taken care of, we just do not
need to warn user about them.

 hw/device-assignment.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 000fa61..066fdb6 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -486,25 +486,23 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                 : PCI_BASE_ADDRESS_SPACE_MEMORY;
 
             if (cur_region->size & 0xFFF) {
-                fprintf(stderr, "PCI region %d at address 0x%llx "
-                        "has size 0x%x, which is not a multiple of 4K. "
-                        "You might experience some performance hit due to that.\n",
-                        i, (unsigned long long)cur_region->base_addr,
-                        cur_region->size);
+                if (i != PCI_ROM_SLOT) {
+                    fprintf(stderr, "PCI region %d at address 0x%llx "
+                            "has size 0x%x, which is not a multiple of 4K. "
+                            "You might experience some performance hit "
+                            "due to that.\n",
+                            i, (unsigned long long)cur_region->base_addr,
+                            cur_region->size);
+                }
                 slow_map = 1;
             }
 
-            if (slow_map && (i == PCI_ROM_SLOT)) {
-                fprintf(stderr, "ROM not aligned - can't continue\n");
-                return -1;
-            }
-
             /* map physical memory */
             pci_dev->v_addrs[i].e_physbase = cur_region->base_addr;
             if (i == PCI_ROM_SLOT) {
                 pci_dev->v_addrs[i].u.r_virtbase =
                     mmap(NULL,
-                         (cur_region->size + 0xFFF) & 0xFFFFF000,
+                         cur_region->size,
                          PROT_WRITE | PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE,
                          0, (off_t) 0);
 
-- 
1.6.6.rc1.43.gf55cc

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Michael S. Tsirkin wrote:
> ROM BAR can be handled same as regular BAR:
> load_option_roms utility will take care of
> copying it to RAM as appropriate.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This patch applies on top of agraf's one,
> it takes care of non-page aligned ROM BARs as well:
> they mostly are taken care of, we just do not
> need to warn user about them.
>
>  hw/device-assignment.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 000fa61..066fdb6 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -486,25 +486,23 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
>                  : PCI_BASE_ADDRESS_SPACE_MEMORY;
>  
>              if (cur_region->size & 0xFFF) {
> -                fprintf(stderr, "PCI region %d at address 0x%llx "
> -                        "has size 0x%x, which is not a multiple of 4K. "
> -                        "You might experience some performance hit due to that.\n",
> -                        i, (unsigned long long)cur_region->base_addr,
> -                        cur_region->size);
> +                if (i != PCI_ROM_SLOT) {
> +                    fprintf(stderr, "PCI region %d at address 0x%llx "
> +                            "has size 0x%x, which is not a multiple of 4K. "
> +                            "You might experience some performance hit "
> +                            "due to that.\n",
> +                            i, (unsigned long long)cur_region->base_addr,
> +                            cur_region->size);
> +                }
>                  slow_map = 1;
>   

This is wrong. You're setting slow_map = 1 on code that is very likely
to be executed inside the guest. That doesn't work.

Better pad the ROM size to page boundary and use the shadow mapping we
have in place already.


Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 01:05:23PM +0100, Alexander Graf wrote:
> Michael S. Tsirkin wrote:
> > ROM BAR can be handled same as regular BAR:
> > load_option_roms utility will take care of
> > copying it to RAM as appropriate.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > This patch applies on top of agraf's one,
> > it takes care of non-page aligned ROM BARs as well:
> > they mostly are taken care of, we just do not
> > need to warn user about them.
> >
> >  hw/device-assignment.c |   20 +++++++++-----------
> >  1 files changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index 000fa61..066fdb6 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -486,25 +486,23 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
> >                  : PCI_BASE_ADDRESS_SPACE_MEMORY;
> >  
> >              if (cur_region->size & 0xFFF) {
> > -                fprintf(stderr, "PCI region %d at address 0x%llx "
> > -                        "has size 0x%x, which is not a multiple of 4K. "
> > -                        "You might experience some performance hit due to that.\n",
> > -                        i, (unsigned long long)cur_region->base_addr,
> > -                        cur_region->size);
> > +                if (i != PCI_ROM_SLOT) {
> > +                    fprintf(stderr, "PCI region %d at address 0x%llx "
> > +                            "has size 0x%x, which is not a multiple of 4K. "
> > +                            "You might experience some performance hit "
> > +                            "due to that.\n",
> > +                            i, (unsigned long long)cur_region->base_addr,
> > +                            cur_region->size);
> > +                }
> >                  slow_map = 1;
> >   
> 
> This is wrong. You're setting slow_map = 1 on code that is very likely
> to be executed inside the guest. That doesn't work.

It is? Can you really run code directly from a PCI card?
I looked at BIOS boot specification and it always talks
about shadowing PCI ROMs.


> Better pad the ROM size to page boundary and use the shadow mapping we
> have in place already.

Changing BAR size might break some drivers.
Our BIOS seems to shadow ROM instead of running it directly,
so we should be fine I think?

> 
> Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Michael S. Tsirkin wrote:
> On Tue, Dec 22, 2009 at 01:05:23PM +0100, Alexander Graf wrote:
>   
>> Michael S. Tsirkin wrote:
>>     
>>> ROM BAR can be handled same as regular BAR:
>>> load_option_roms utility will take care of
>>> copying it to RAM as appropriate.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>
>>> This patch applies on top of agraf's one,
>>> it takes care of non-page aligned ROM BARs as well:
>>> they mostly are taken care of, we just do not
>>> need to warn user about them.
>>>
>>>  hw/device-assignment.c |   20 +++++++++-----------
>>>  1 files changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>>> index 000fa61..066fdb6 100644
>>> --- a/hw/device-assignment.c
>>> +++ b/hw/device-assignment.c
>>> @@ -486,25 +486,23 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
>>>                  : PCI_BASE_ADDRESS_SPACE_MEMORY;
>>>  
>>>              if (cur_region->size & 0xFFF) {
>>> -                fprintf(stderr, "PCI region %d at address 0x%llx "
>>> -                        "has size 0x%x, which is not a multiple of 4K. "
>>> -                        "You might experience some performance hit due to that.\n",
>>> -                        i, (unsigned long long)cur_region->base_addr,
>>> -                        cur_region->size);
>>> +                if (i != PCI_ROM_SLOT) {
>>> +                    fprintf(stderr, "PCI region %d at address 0x%llx "
>>> +                            "has size 0x%x, which is not a multiple of 4K. "
>>> +                            "You might experience some performance hit "
>>> +                            "due to that.\n",
>>> +                            i, (unsigned long long)cur_region->base_addr,
>>> +                            cur_region->size);
>>> +                }
>>>                  slow_map = 1;
>>>   
>>>       
>> This is wrong. You're setting slow_map = 1 on code that is very likely
>> to be executed inside the guest. That doesn't work.
>>     
>
> It is? Can you really run code directly from a PCI card?
> I looked at BIOS boot specification and it always talks
> about shadowing PCI ROMs.
>   

I'm not sure the BIOS is the only one executing ROMs. If it is, then I'm
good with the change.
Maybe it'd make sense to also add a read only flag so we don't
accidently try to write to the ROM region with slow_map.

Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 02:34:42PM +0100, Alexander Graf wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Dec 22, 2009 at 01:05:23PM +0100, Alexander Graf wrote:
> >   
> >> Michael S. Tsirkin wrote:
> >>     
> >>> ROM BAR can be handled same as regular BAR:
> >>> load_option_roms utility will take care of
> >>> copying it to RAM as appropriate.
> >>>
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>
> >>> This patch applies on top of agraf's one,
> >>> it takes care of non-page aligned ROM BARs as well:
> >>> they mostly are taken care of, we just do not
> >>> need to warn user about them.
> >>>
> >>>  hw/device-assignment.c |   20 +++++++++-----------
> >>>  1 files changed, 9 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> >>> index 000fa61..066fdb6 100644
> >>> --- a/hw/device-assignment.c
> >>> +++ b/hw/device-assignment.c
> >>> @@ -486,25 +486,23 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
> >>>                  : PCI_BASE_ADDRESS_SPACE_MEMORY;
> >>>  
> >>>              if (cur_region->size & 0xFFF) {
> >>> -                fprintf(stderr, "PCI region %d at address 0x%llx "
> >>> -                        "has size 0x%x, which is not a multiple of 4K. "
> >>> -                        "You might experience some performance hit due to that.\n",
> >>> -                        i, (unsigned long long)cur_region->base_addr,
> >>> -                        cur_region->size);
> >>> +                if (i != PCI_ROM_SLOT) {
> >>> +                    fprintf(stderr, "PCI region %d at address 0x%llx "
> >>> +                            "has size 0x%x, which is not a multiple of 4K. "
> >>> +                            "You might experience some performance hit "
> >>> +                            "due to that.\n",
> >>> +                            i, (unsigned long long)cur_region->base_addr,
> >>> +                            cur_region->size);
> >>> +                }
> >>>                  slow_map = 1;
> >>>   
> >>>       
> >> This is wrong. You're setting slow_map = 1 on code that is very likely
> >> to be executed inside the guest. That doesn't work.
> >>     
> >
> > It is? Can you really run code directly from a PCI card?
> > I looked at BIOS boot specification and it always talks
> > about shadowing PCI ROMs.
> >   
> 
> I'm not sure the BIOS is the only one executing ROMs. If it is, then I'm
> good with the change.
> Maybe it'd make sense to also add a read only flag so we don't
> accidently try to write to the ROM region with slow_map.
> 
> Alex

Correct: I think it's made readonly down the road with mprotect,
so attempt to do so will crash qemu :)

-- 
MST

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Avi Kivity]

On 12/22/2009 05:19 PM, Michael S. Tsirkin wrote:
>
>> I'm not sure the BIOS is the only one executing ROMs. If it is, then I'm
>> good with the change.
>> Maybe it'd make sense to also add a read only flag so we don't
>> accidently try to write to the ROM region with slow_map.
>>
>> Alex
>>      
> Correct: I think it's made readonly down the road with mprotect,
> so attempt to do so will crash qemu :)
>    

Alex, are you happy with this?  I'd like to apply it.

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

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Avi Kivity wrote:
> On 12/22/2009 05:19 PM, Michael S. Tsirkin wrote:
>>
>>> I'm not sure the BIOS is the only one executing ROMs. If it is, then
>>> I'm
>>> good with the change.
>>> Maybe it'd make sense to also add a read only flag so we don't
>>> accidently try to write to the ROM region with slow_map.
>>>
>>> Alex
>>>      
>> Correct: I think it's made readonly down the road with mprotect,
>> so attempt to do so will crash qemu :)
>>    
>
> Alex, are you happy with this?  I'd like to apply it.

I'd like to see the read-only protection in. Apart from that I'm good on
checking it in, though I'm only awaiting the day someone runs code off
such a ROM region ;-).

Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 04:24:43PM +0100, Alexander Graf wrote:
> Avi Kivity wrote:
> > On 12/22/2009 05:19 PM, Michael S. Tsirkin wrote:
> >>
> >>> I'm not sure the BIOS is the only one executing ROMs. If it is, then
> >>> I'm
> >>> good with the change.
> >>> Maybe it'd make sense to also add a read only flag so we don't
> >>> accidently try to write to the ROM region with slow_map.
> >>>
> >>> Alex
> >>>      
> >> Correct: I think it's made readonly down the road with mprotect,
> >> so attempt to do so will crash qemu :)
> >>    
> >
> > Alex, are you happy with this?  I'd like to apply it.
> 
> I'd like to see the read-only protection in.

Yes, this is a must, I am working on it.

> Apart from that I'm good on checking it in, though I'm only awaiting
> the day someone runs code off such a ROM region ;-).
> 
> Alex

Is there a way to trap this and fprintf something?

-- 
MST

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Michael S. Tsirkin wrote:
> On Tue, Dec 22, 2009 at 04:24:43PM +0100, Alexander Graf wrote:
>   
>> Avi Kivity wrote:
>>     
>>> On 12/22/2009 05:19 PM, Michael S. Tsirkin wrote:
>>>       
>>>>> I'm not sure the BIOS is the only one executing ROMs. If it is, then
>>>>> I'm
>>>>> good with the change.
>>>>> Maybe it'd make sense to also add a read only flag so we don't
>>>>> accidently try to write to the ROM region with slow_map.
>>>>>
>>>>> Alex
>>>>>      
>>>>>           
>>>> Correct: I think it's made readonly down the road with mprotect,
>>>> so attempt to do so will crash qemu :)
>>>>    
>>>>         
>>> Alex, are you happy with this?  I'd like to apply it.
>>>       
>> I'd like to see the read-only protection in.
>>     
>
> Yes, this is a must, I am working on it.
>
>   
>> Apart from that I'm good on checking it in, though I'm only awaiting
>> the day someone runs code off such a ROM region ;-).
>>
>> Alex
>>     
>
> Is there a way to trap this and fprintf something?

I don't think so. KVM will just trap on execution outside of RAM and
either fail badly or throw something bad into the guest. MMIO access
works by analyzing the instruction that accesses the MMIO address. That
just doesn't work when we don't have an instruction to analyze.

Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Avi Kivity]

On 12/22/2009 05:36 PM, Alexander Graf wrote:
>
>> Is there a way to trap this and fprintf something?
>>      
> I don't think so. KVM will just trap on execution outside of RAM and
> either fail badly or throw something bad into the guest. MMIO access
> works by analyzing the instruction that accesses the MMIO address. That
> just doesn't work when we don't have an instruction to analyze.
>    

We could certainly extend emulate.c to fetch instruction bytes from 
userspace.  It uses ->read_std() now, so we'd need to switch to 
->read_emulated() and add appropriate buffering.

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

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Avi Kivity wrote:
> On 12/22/2009 05:36 PM, Alexander Graf wrote:
>>
>>> Is there a way to trap this and fprintf something?
>>>      
>> I don't think so. KVM will just trap on execution outside of RAM and
>> either fail badly or throw something bad into the guest. MMIO access
>> works by analyzing the instruction that accesses the MMIO address. That
>> just doesn't work when we don't have an instruction to analyze.
>>    
>
> We could certainly extend emulate.c to fetch instruction bytes from
> userspace.  It uses ->read_std() now, so we'd need to switch to
> ->read_emulated() and add appropriate buffering.

I thought the policy on emulate.c was to not have a full instruction
emulator but only emulate instructions that do PT modifications or MMIO
access?

Btw, we're in the same situation with PowerPC here. The instruction
emulator is _really_ small. It only does a few MMU specific
instructions, a couple of privileged ones and MMIO accessing ones.

Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Avi Kivity]

On 12/22/2009 05:41 PM, Alexander Graf wrote:
>
>> We could certainly extend emulate.c to fetch instruction bytes from
>> userspace.  It uses ->read_std() now, so we'd need to switch to
>> ->read_emulated() and add appropriate buffering.
>>      
> I thought the policy on emulate.c was to not have a full instruction
> emulator but only emulate instructions that do PT modifications or MMIO
> access?
>    

It's not a policy, just laziness.  With emulate_invalid_guest_state=1 we 
need many more instructions.  Of course I don't want to add instructions 
just for the sake of it, since they will be untested.

I'd much prefer not to run from mmio if possible - just pointing out 
it's doable.

> Btw, we're in the same situation with PowerPC here. The instruction
> emulator is _really_ small. It only does a few MMU specific
> instructions, a couple of privileged ones and MMIO accessing ones.
>    

Plus, you have a fixed length instruction length, likely more regular 
too.  I imagine powerpc is load/store, so you don't have to emulate a 
zillion ALU instructions?

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

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Avi Kivity wrote:
> On 12/22/2009 05:41 PM, Alexander Graf wrote:
>>
>>> We could certainly extend emulate.c to fetch instruction bytes from
>>> userspace.  It uses ->read_std() now, so we'd need to switch to
>>> ->read_emulated() and add appropriate buffering.
>>>      
>> I thought the policy on emulate.c was to not have a full instruction
>> emulator but only emulate instructions that do PT modifications or MMIO
>> access?
>>    
>
> It's not a policy, just laziness.  With emulate_invalid_guest_state=1
> we need many more instructions.  Of course I don't want to add
> instructions just for the sake of it, since they will be untested.
>
> I'd much prefer not to run from mmio if possible - just pointing out
> it's doable.

Right...

>> emulator is _really_ small. It only does a few MMU specific
>> instructions, a couple of privileged ones and MMIO accessing ones.
>>    
> Btw, we're in the same situation with PowerPC here. The instruction
>
> Plus, you have a fixed length instruction length, likely more regular
> too.  I imagine powerpc is load/store, so you don't have to emulate a
> zillion ALU instructions?

Well, it's certainly doable (and easier than on x86). But I'm on the
same position as you on the x86 side. Why increase the emulator size at
least 10 times if we don't have to?

Either way, people will report bugs when / if they actually start
executing code off MMIO. So let's not care too much about it for now.
Just make sure the read-only check is in.

Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 05:00:52PM +0100, Alexander Graf wrote:
> Avi Kivity wrote:
> > On 12/22/2009 05:41 PM, Alexander Graf wrote:
> >>
> >>> We could certainly extend emulate.c to fetch instruction bytes from
> >>> userspace.  It uses ->read_std() now, so we'd need to switch to
> >>> ->read_emulated() and add appropriate buffering.
> >>>      
> >> I thought the policy on emulate.c was to not have a full instruction
> >> emulator but only emulate instructions that do PT modifications or MMIO
> >> access?
> >>    
> >
> > It's not a policy, just laziness.  With emulate_invalid_guest_state=1
> > we need many more instructions.  Of course I don't want to add
> > instructions just for the sake of it, since they will be untested.
> >
> > I'd much prefer not to run from mmio if possible - just pointing out
> > it's doable.
> 
> Right...
> 
> >> emulator is _really_ small. It only does a few MMU specific
> >> instructions, a couple of privileged ones and MMIO accessing ones.
> >>    
> > Btw, we're in the same situation with PowerPC here. The instruction
> >
> > Plus, you have a fixed length instruction length, likely more regular
> > too.  I imagine powerpc is load/store, so you don't have to emulate a
> > zillion ALU instructions?
> 
> Well, it's certainly doable (and easier than on x86). But I'm on the
> same position as you on the x86 side. Why increase the emulator size at
> least 10 times if we don't have to?
> 
> Either way, people will report bugs when / if they actually start
> executing code off MMIO. So let's not care too much about it for now.
> Just make sure the read-only check is in.
> 
> Alex

So I think all we need is this on top?

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 066fdb6..0c3c8f4 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -233,7 +233,8 @@ static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
     int m;
 
     DEBUG("slow map\n");
-    m = cpu_register_io_memory(slow_bar_read, slow_bar_write, region);
+    m = cpu_register_io_memory(slow_bar_read, region_num == PCI_ROM_SLOT ?
+                               NULL : slow_bar_write, region);
     cpu_register_physical_memory(e_phys, e_size, m);
 
     /* MSI-X MMIO page */

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Alexander Graf]

Michael S. Tsirkin wrote:
> On Tue, Dec 22, 2009 at 05:00:52PM +0100, Alexander Graf wrote:
>   
>> Avi Kivity wrote:
>>     
>>> On 12/22/2009 05:41 PM, Alexander Graf wrote:
>>>       
>>>>> We could certainly extend emulate.c to fetch instruction bytes from
>>>>> userspace.  It uses ->read_std() now, so we'd need to switch to
>>>>> ->read_emulated() and add appropriate buffering.
>>>>>      
>>>>>           
>>>> I thought the policy on emulate.c was to not have a full instruction
>>>> emulator but only emulate instructions that do PT modifications or MMIO
>>>> access?
>>>>    
>>>>         
>>> It's not a policy, just laziness.  With emulate_invalid_guest_state=1
>>> we need many more instructions.  Of course I don't want to add
>>> instructions just for the sake of it, since they will be untested.
>>>
>>> I'd much prefer not to run from mmio if possible - just pointing out
>>> it's doable.
>>>       
>> Right...
>>
>>     
>>>> emulator is _really_ small. It only does a few MMU specific
>>>> instructions, a couple of privileged ones and MMIO accessing ones.
>>>>    
>>>>         
>>> Btw, we're in the same situation with PowerPC here. The instruction
>>>
>>> Plus, you have a fixed length instruction length, likely more regular
>>> too.  I imagine powerpc is load/store, so you don't have to emulate a
>>> zillion ALU instructions?
>>>       
>> Well, it's certainly doable (and easier than on x86). But I'm on the
>> same position as you on the x86 side. Why increase the emulator size at
>> least 10 times if we don't have to?
>>
>> Either way, people will report bugs when / if they actually start
>> executing code off MMIO. So let's not care too much about it for now.
>> Just make sure the read-only check is in.
>>
>> Alex
>>     
>
> So I think all we need is this on top?
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 066fdb6..0c3c8f4 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -233,7 +233,8 @@ static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
>      int m;
>  
>      DEBUG("slow map\n");
> -    m = cpu_register_io_memory(slow_bar_read, slow_bar_write, region);
> +    m = cpu_register_io_memory(slow_bar_read, region_num == PCI_ROM_SLOT ?
> +                               NULL : slow_bar_write, region);
>      cpu_register_physical_memory(e_phys, e_size, m);
>  
>      /* MSI-X MMIO page */
>   

I guess so, yes. I'd prefer a written out if statement though, but
that's probably personal preference.

Alex

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Avi Kivity]

On 12/22/2009 06:00 PM, Alexander Graf wrote:
>> Plus, you have a fixed length instruction length, likely more regular
>> too.  I imagine powerpc is load/store, so you don't have to emulate a
>> zillion ALU instructions?
>>      
> Well, it's certainly doable (and easier than on x86). But I'm on the
> same position as you on the x86 side. Why increase the emulator size at
> least 10 times if we don't have to?
>    

I'm not suggesting you do, just expressing envy.

> Either way, people will report bugs when / if they actually start
> executing code off MMIO. So let's not care too much about it for now.
> Just make sure the read-only check is in.
>    

Yah.

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

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 05:39:22PM +0200, Avi Kivity wrote:
> On 12/22/2009 05:36 PM, Alexander Graf wrote:
>>
>>> Is there a way to trap this and fprintf something?
>>>      
>> I don't think so. KVM will just trap on execution outside of RAM and
>> either fail badly or throw something bad into the guest. MMIO access
>> works by analyzing the instruction that accesses the MMIO address. That
>> just doesn't work when we don't have an instruction to analyze.
>>    
>
> We could certainly extend emulate.c to fetch instruction bytes from  
> userspace.  It uses ->read_std() now, so we'd need to switch to  
> ->read_emulated() and add appropriate buffering.

You mean run with KVM, and TCG will kick in when there's
an instruction we can't support natively?

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

Re: [PATCH] slow_map: minor improvements to ROM BAR handling

[Avi Kivity]

On 12/23/2009 11:15 AM, Michael S. Tsirkin wrote:
>
>> We could certainly extend emulate.c to fetch instruction bytes from
>> userspace.  It uses ->read_std() now, so we'd need to switch to
>> ->read_emulated() and add appropriate buffering.
>>      
> You mean run with KVM, and TCG will kick in when there's
> an instruction we can't support natively?
>    

No.  Read the instruction bytes from qemu and emulate them using emulate.c.

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