| Message ID | 1344351168-2568-2-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 08/07/2012 05:52 PM, Cornelia Huck wrote: > Running under a kvm host does not necessarily imply the presence of > a page mapped above the main memory with the virtio information; > however, the code includes a hard coded access to that page. > > Instead, check for the presence of the page and exit gracefully > before we hit an addressing exception if it does not exist. > > /* > * Init function for virtio > * devices are in a single page above top of "normal" mem > @@ -443,6 +458,12 @@ static int __init kvm_devices_init(void) > } > > kvm_devices = (void *) real_memory_size; > + if (test_devices_support() < 0) { > + vmem_remove_mapping(real_memory_size, PAGE_SIZE); > + root_device_unregister(kvm_root); > + /* No error. */ > + return 0; > + } > Cleaner to defer root_device_register() until after the mapping has been verified.
On Thu, 09 Aug 2012 13:03:57 +0300 Avi Kivity <avi@redhat.com> wrote: > On 08/07/2012 05:52 PM, Cornelia Huck wrote: > > Running under a kvm host does not necessarily imply the presence of > > a page mapped above the main memory with the virtio information; > > however, the code includes a hard coded access to that page. > > > > Instead, check for the presence of the page and exit gracefully > > before we hit an addressing exception if it does not exist. > > > > /* > > * Init function for virtio > > * devices are in a single page above top of "normal" mem > > @@ -443,6 +458,12 @@ static int __init kvm_devices_init(void) > > } > > > > kvm_devices = (void *) real_memory_size; > > + if (test_devices_support() < 0) { > > + vmem_remove_mapping(real_memory_size, PAGE_SIZE); > > + root_device_unregister(kvm_root); > > + /* No error. */ > > + return 0; > > + } > > > > Cleaner to defer root_device_register() until after the mapping has been > verified. OK, will reorder.
On 07.08.2012, at 16:52, Cornelia Huck wrote: > Running under a kvm host does not necessarily imply the presence of > a page mapped above the main memory with the virtio information; > however, the code includes a hard coded access to that page. > > Instead, check for the presence of the page and exit gracefully > before we hit an addressing exception if it does not exist. > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > drivers/s390/kvm/kvm_virtio.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c > index 47cccd5..408447c 100644 > --- a/drivers/s390/kvm/kvm_virtio.c > +++ b/drivers/s390/kvm/kvm_virtio.c > @@ -418,6 +418,21 @@ static void kvm_extint_handler(struct ext_code ext_code, > } > } > > +static int __init test_devices_support(void) Today we know what this function does, but the next person running into it has no clue. Maybe add a nice description here that you're trying to access memory above ram? (is this even what you're doing here? my s390 asm is slightly rusty) :) Alex > +{ > + int ccode = -EIO; > + > + asm volatile( > + "0: cli 0(%1),0\n" > + " ipm %0\n" > + " srl %0,28\n" > + "1:\n" > + EX_TABLE(0b,1b) > + : "+d" (ccode) > + : "a" (kvm_devices) > + : "cc"); > + return ccode; > +} > /* > * Init function for virtio > * devices are in a single page above top of "normal" mem > @@ -443,6 +458,12 @@ static int __init kvm_devices_init(void) > } > > kvm_devices = (void *) real_memory_size; > + if (test_devices_support() < 0) { > + vmem_remove_mapping(real_memory_size, PAGE_SIZE); > + root_device_unregister(kvm_root); > + /* No error. */ > + return 0; > + } > > INIT_WORK(&hotplug_work, hotplug_devices); > > -- > 1.7.11.4 >
On Fri, 10 Aug 2012 01:09:15 +0200 Alexander Graf <agraf@suse.de> wrote: > > On 07.08.2012, at 16:52, Cornelia Huck wrote: > > > Running under a kvm host does not necessarily imply the presence of > > a page mapped above the main memory with the virtio information; > > however, the code includes a hard coded access to that page. > > > > Instead, check for the presence of the page and exit gracefully > > before we hit an addressing exception if it does not exist. > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > drivers/s390/kvm/kvm_virtio.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c > > index 47cccd5..408447c 100644 > > --- a/drivers/s390/kvm/kvm_virtio.c > > +++ b/drivers/s390/kvm/kvm_virtio.c > > @@ -418,6 +418,21 @@ static void kvm_extint_handler(struct ext_code ext_code, > > } > > } > > > > +static int __init test_devices_support(void) > > Today we know what this function does, but the next person running into it has no clue. Maybe add a nice description here that you're trying to access memory above ram? (is this even what you're doing here? my s390 asm is slightly rusty) :) Good point, will add a comment.
On Thu, Aug 09, 2012 at 12:41:57PM +0200, Cornelia Huck wrote: > On Thu, 09 Aug 2012 13:03:57 +0300 > Avi Kivity <avi@redhat.com> wrote: > > > On 08/07/2012 05:52 PM, Cornelia Huck wrote: > > > Running under a kvm host does not necessarily imply the presence of > > > a page mapped above the main memory with the virtio information; > > > however, the code includes a hard coded access to that page. > > > > > > Instead, check for the presence of the page and exit gracefully > > > before we hit an addressing exception if it does not exist. > > > > > > /* > > > * Init function for virtio > > > * devices are in a single page above top of "normal" mem > > > @@ -443,6 +458,12 @@ static int __init kvm_devices_init(void) > > > } > > > > > > kvm_devices = (void *) real_memory_size; > > > + if (test_devices_support() < 0) { > > > + vmem_remove_mapping(real_memory_size, PAGE_SIZE); > > > + root_device_unregister(kvm_root); > > > + /* No error. */ > > > + return 0; > > > + } > > > > > > > Cleaner to defer root_device_register() until after the mapping has been > > verified. > > OK, will reorder. Please use the "lura" instruction to figure out if the guest real page even exists _before_ creating the mapping. That way you don't need all the code afterwards.
On Fri, 10 Aug 2012 10:42:48 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote: > On Thu, Aug 09, 2012 at 12:41:57PM +0200, Cornelia Huck wrote: > > On Thu, 09 Aug 2012 13:03:57 +0300 > > Avi Kivity <avi@redhat.com> wrote: > > > > > On 08/07/2012 05:52 PM, Cornelia Huck wrote: > > > > Running under a kvm host does not necessarily imply the presence of > > > > a page mapped above the main memory with the virtio information; > > > > however, the code includes a hard coded access to that page. > > > > > > > > Instead, check for the presence of the page and exit gracefully > > > > before we hit an addressing exception if it does not exist. > > > > > > > > /* > > > > * Init function for virtio > > > > * devices are in a single page above top of "normal" mem > > > > @@ -443,6 +458,12 @@ static int __init kvm_devices_init(void) > > > > } > > > > > > > > kvm_devices = (void *) real_memory_size; > > > > + if (test_devices_support() < 0) { > > > > + vmem_remove_mapping(real_memory_size, PAGE_SIZE); > > > > + root_device_unregister(kvm_root); > > > > + /* No error. */ > > > > + return 0; > > > > + } > > > > > > > > > > Cleaner to defer root_device_register() until after the mapping has been > > > verified. > > > > OK, will reorder. > > Please use the "lura" instruction to figure out if the guest real page > even exists _before_ creating the mapping. > That way you don't need all the code afterwards. New instruction of the day ;) Indeed, this makes the code look nicer.
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c index 47cccd5..408447c 100644 --- a/drivers/s390/kvm/kvm_virtio.c +++ b/drivers/s390/kvm/kvm_virtio.c @@ -418,6 +418,21 @@ static void kvm_extint_handler(struct ext_code ext_code, } } +static int __init test_devices_support(void) +{ + int ccode = -EIO; + + asm volatile( + "0: cli 0(%1),0\n" + " ipm %0\n" + " srl %0,28\n" + "1:\n" + EX_TABLE(0b,1b) + : "+d" (ccode) + : "a" (kvm_devices) + : "cc"); + return ccode; +} /* * Init function for virtio * devices are in a single page above top of "normal" mem @@ -443,6 +458,12 @@ static int __init kvm_devices_init(void) } kvm_devices = (void *) real_memory_size; + if (test_devices_support() < 0) { + vmem_remove_mapping(real_memory_size, PAGE_SIZE); + root_device_unregister(kvm_root); + /* No error. */ + return 0; + } INIT_WORK(&hotplug_work, hotplug_devices);
Running under a kvm host does not necessarily imply the presence of a page mapped above the main memory with the virtio information; however, the code includes a hard coded access to that page. Instead, check for the presence of the page and exit gracefully before we hit an addressing exception if it does not exist. Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- drivers/s390/kvm/kvm_virtio.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)