slow_map: minor improvements to ROM BAR handling
PATCH 1/1 slow_map: minor improvements to ROM BAR handling
ROM BAR can be handled same as regular BAR:load_option_roms utility will take care of
copying it to RAM as appropriate.
---
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(-)
hw/device-assignment.c +9 -11
@@ -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 );
Signature · 1 line
1.6.6.rc1.43.gf55cc
Quoted 28 lines · Michael S. Tsirkin
ROM BAR can be handled same as regular BAR:
load_option_romsutility will take care of
copying it to RAM as appropriate.Signed-off-by Michael S. Tsirkin <[email protected]>---
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(-)hw/device-assignment.c +0
-5@@ -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);
hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 498
Quoted 10 lines
hw/device-assignment.c +8
-0+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
Quoted 34 lines · Alexander Graf
Quoted 33 lines · Michael S. Tsirkin
ROM BAR can be handled same as regular BAR:
load_option_romsutility will take care of
copying it to RAM as appropriate.Signed-off-by Michael S. Tsirkin <[email protected]>---
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(-)hw/device-assignment.c +5
-5@@ -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",
Quoted 8 lines
hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 499
Quoted 5 lines
hw/device-assignment.c +3
-0+ 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.
Quoted 2 lines
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?
Quoted 2 lines
Alex
Quoted 47 lines · Michael S. Tsirkin
Quoted 35 lines · Alexander Graf
Quoted 33 lines · Michael S. Tsirkin
ROM BAR can be handled same as regular BAR:
load_option_romsutility will take care of
copying it to RAM as appropriate.Signed-off-by Michael S. Tsirkin <[email protected]>---
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(-)hw/device-assignment.c +5
-5@@ -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",Quoted 9 lines
hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 508
Quoted 6 lines
hw/device-assignment.c +3
-0+ 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.
Quoted 4 lines
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
Quoted 53 lines · Alexander Graf
Quoted 47 lines · Michael S. Tsirkin
Quoted 35 lines · Alexander Graf
Quoted 33 lines · Michael S. Tsirkin
ROM BAR can be handled same as regular BAR:
load_option_romsutility will take care of
copying it to RAM as appropriate.Signed-off-by Michael S. Tsirkin <[email protected]>---
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(-)hw/device-assignment.c +5
-5@@ -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",Quoted 9 lines
hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 508
Quoted 6 lines
hw/device-assignment.c +3
-0+ 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.Quoted 4 lines
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.
Quoted 6 lines
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 withslow_map.Alex
Correct: I think it's made readonly down the road with mprotect, so attempt to do so will crash qemu :)
Signature · 1 line
MST
Quoted 11 lines · Michael S. Tsirkin
Quoted 8 lines
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 withslow_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.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Michael S. Tsirkin
Quoted 7 lines
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 withslow_map.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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
Quoted 11 lines · Alexander Graf
Quoted 10 lines · Avi Kivity
Quoted 9 lines · Michael S. Tsirkin
Quoted 9 lines
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 withslow_map.Alex
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 4 lines
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?
Signature · 1 line
MST
Quoted 27 lines · Michael S. Tsirkin
Quoted 13 lines · Alexander Graf
Quoted 11 lines · Avi Kivity
Quoted 9 lines · Michael S. Tsirkin
Quoted 9 lines
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 withslow_map.Alex
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 5 lines
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
Quoted 8 lines · Alexander Graf
Quoted 3 lines
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.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Alexander Graf
Quoted 3 lines
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.
Quoted 3 lines
We could certainly extend
emulate.cto 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
Quoted 9 lines · Alexander Graf
Quoted 5 lines
We could certainly extend
emulate.cto 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.cwas 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.
Quoted 4 lines
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?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 9 lines · Alexander Graf
Quoted 5 lines
We could certainly extend
emulate.cto 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.cwas to not have a full instruction emulator but only emulate instructions that do PT modifications or MMIO access?
Quoted 6 lines
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...
Quoted 8 lines
Quoted 3 lines
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
Quoted 34 lines · Alexander Graf
Quoted 11 lines · Avi Kivity
Quoted 9 lines · Alexander Graf
Quoted 5 lines
We could certainly extend
emulate.cto 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.cwas to not have a full instruction emulator but only emulate instructions that do PT modifications or MMIO access?Quoted 6 lines
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...
Quoted 8 lines
Quoted 3 lines
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?
Quoted 5 lines
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?
hw/device-assignment.c +2 -1
@@ -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 */
Quoted 55 lines · Michael S. Tsirkin
Quoted 38 lines · Alexander Graf
Quoted 12 lines · Avi Kivity
Quoted 10 lines · Alexander Graf
Quoted 5 lines
We could certainly extend
emulate.cto 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.cwas to not have a full instruction emulator but only emulate instructions that do PT modifications or MMIO access?Quoted 7 lines
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...
Quoted 10 lines
Quoted 4 lines
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?
Quoted 6 lines
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?
hw/device-assignment.c +0
-0
hw/device-assignment.c static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num, · line 240
Quoted 9 lines
hw/device-assignment.c +2
-1@@ -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
Quoted 8 lines · Alexander Graf
Quoted 4 lines
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.
Quoted 4 lines
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.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Alexander Graf
Quoted 3 lines
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.
Quoted 3 lines
We could certainly extend
emulate.cto 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?
Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 8 lines · Michael S. Tsirkin
Quoted 5 lines
We could certainly extend
emulate.cto 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.
Signature · 1 line
error compiling committee.c: too many arguments to function