Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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.
Signed-off-by: Michael S. Tsirkin m...@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) 0xF000,
+ cur_region-size,
PROT_WRITE | PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE,
0, (off_t) 0);
--
1.6.6.rc1.43.gf55cc
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 m...@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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 m...@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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 */ -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] slow_map: minor improvements to ROM BAR handling
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
