Patchwork [1/4] s390/kvm: Handle hosts not supporting s390-virtio.

login
register
mail settings
Submitter Cornelia Huck
Date Aug. 7, 2012, 2:52 p.m.
Message ID <1344351168-2568-2-git-send-email-cornelia.huck@de.ibm.com>
Download mbox | patch
Permalink /patch/175665/
State New
Headers show

Comments

Cornelia Huck - Aug. 7, 2012, 2:52 p.m.
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(+)
Avi Kivity - Aug. 9, 2012, 10:03 a.m.
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.
Cornelia Huck - Aug. 9, 2012, 10:41 a.m.
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.
Alexander Graf - Aug. 9, 2012, 11:09 p.m.
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
>
Cornelia Huck - Aug. 10, 2012, 7:45 a.m.
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.
Heiko Carstens - Aug. 10, 2012, 8:42 a.m.
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.
Cornelia Huck - Aug. 10, 2012, 11:03 a.m.
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.

Patch

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);