[PATCH qemu-kvm] device-assignment: add config fd qdev property

[PATCH qemu-kvm] device-assignment: add config fd qdev property

[Chris Wright]

When libvirt launches a guest it first chowns the relevenat
/sys/bus/pci/.../config file for an assigned device then drops privileges.

This causes an issue for device assignment because despite being file
owner, the sysfs config space file checks for CAP_SYS_ADMIN before
allowing access to device dependent config space.

This adds a new qdev configfd property which allows libvirt to open the
sysfs config space file and give qemu an already opened file descriptor.
Along with a change pending for the 2.6.35 kernel, this allows the
capability check to compare against privileges from when the file was
opened.

Signed-off-by: Chris Wright <chrisw@redhat.com>
---
 hw/device-assignment.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index eb31c78..172f0c9 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -612,12 +612,15 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
 
     snprintf(name, sizeof(name), "%sconfig", dir);
 
-    fd = open(name, O_RDWR);
+    fd = dev->config_fd;
     if (fd == -1) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
-        return 1;
+        fd = open(name, O_RDWR);
+        if (fd == -1) {
+            fprintf(stderr, "%s: %s: %m\n", __func__, name);
+            return 1;
+        }
+        dev->config_fd = fd;
     }
-    dev->config_fd = fd;
 again:
     r = read(fd, pci_dev->dev.config, pci_config_size(&pci_dev->dev));
     if (r < 0) {
@@ -1433,6 +1436,7 @@ static PCIDeviceInfo assign_info = {
     .qdev.props   = (Property[]) {
         DEFINE_PROP("host", AssignedDevice, host, qdev_prop_hostaddr, PCIHostDevice),
         DEFINE_PROP_UINT32("iommu", AssignedDevice, use_iommu, 1),
+        DEFINE_PROP_INT32("configfd", AssignedDevice, real_device.config_fd, -1),
         DEFINE_PROP_END_OF_LIST(),
     },
 };

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Alex Williamson]

On Wed, 2010-05-19 at 12:00 -0700, Chris Wright wrote:
> When libvirt launches a guest it first chowns the relevenat
> /sys/bus/pci/.../config file for an assigned device then drops privileges.
> 
> This causes an issue for device assignment because despite being file
> owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> allowing access to device dependent config space.
> 
> This adds a new qdev configfd property which allows libvirt to open the
> sysfs config space file and give qemu an already opened file descriptor.
> Along with a change pending for the 2.6.35 kernel, this allows the
> capability check to compare against privileges from when the file was
> opened.
> 
> Signed-off-by: Chris Wright <chrisw@redhat.com>
> ---
>  hw/device-assignment.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)

Acked-by: Alex Williamson <alex.williamson@redhat.com>

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Anthony Liguori]

On 05/19/2010 02:00 PM, Chris Wright wrote:
> When libvirt launches a guest it first chowns the relevenat
> /sys/bus/pci/.../config file for an assigned device then drops privileges.
>
> This causes an issue for device assignment because despite being file
> owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> allowing access to device dependent config space.
>
> This adds a new qdev configfd property which allows libvirt to open the
> sysfs config space file and give qemu an already opened file descriptor.
> Along with a change pending for the 2.6.35 kernel, this allows the
> capability check to compare against privileges from when the file was
> opened.
>
> Signed-off-by: Chris Wright<chrisw@redhat.com>
>    

An fd as a qdev property seems like a bad idea to me.  I'm not sure I 
have a better suggestion though.

Regards,

Anthony Liguori

> ---
>   hw/device-assignment.c |   12 ++++++++----
>   1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index eb31c78..172f0c9 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -612,12 +612,15 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
>
>       snprintf(name, sizeof(name), "%sconfig", dir);
>
> -    fd = open(name, O_RDWR);
> +    fd = dev->config_fd;
>       if (fd == -1) {
> -        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> -        return 1;
> +        fd = open(name, O_RDWR);
> +        if (fd == -1) {
> +            fprintf(stderr, "%s: %s: %m\n", __func__, name);
> +            return 1;
> +        }
> +        dev->config_fd = fd;
>       }
> -    dev->config_fd = fd;
>   again:
>       r = read(fd, pci_dev->dev.config, pci_config_size(&pci_dev->dev));
>       if (r<  0) {
> @@ -1433,6 +1436,7 @@ static PCIDeviceInfo assign_info = {
>       .qdev.props   = (Property[]) {
>           DEFINE_PROP("host", AssignedDevice, host, qdev_prop_hostaddr, PCIHostDevice),
>           DEFINE_PROP_UINT32("iommu", AssignedDevice, use_iommu, 1),
> +        DEFINE_PROP_INT32("configfd", AssignedDevice, real_device.config_fd, -1),
>           DEFINE_PROP_END_OF_LIST(),
>       },
>   };
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>    

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Chris Wright]

* Anthony Liguori (anthony@codemonkey.ws) wrote:
> An fd as a qdev property seems like a bad idea to me.

What is your concern?

> I'm not sure I have a better suggestion though.

Anything else requires inventing some new commandline options and monitor
comnmands (-pcidevice anyone? ;-).  I'm not sure the benefit, esp with
hopes of moving to uio.

thanks,
-chris

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Anthony Liguori]

On 05/19/2010 04:10 PM, Chris Wright wrote:
> * Anthony Liguori (anthony@codemonkey.ws) wrote:
>    
>> An fd as a qdev property seems like a bad idea to me.
>>      
> What is your concern?
>    

qdev properties are supposed to represent device tunables.  A file 
descriptor is not a tunable.

It's like passing the tap device fd via qdev to an e1000.

>> I'm not sure I have a better suggestion though.
>>      
> Anything else requires inventing some new commandline options and monitor
> comnmands (-pcidevice anyone? ;-).  I'm not sure the benefit, esp with
> hopes of moving to uio.
>    

Yeah, I think device passthrough is going to require new command line 
syntax to be more qemu friendly...

Regards,

Anthony Liguori

> thanks,
> -chris
>    

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Chris Wright]

* Anthony Liguori (anthony@codemonkey.ws) wrote:
> On 05/19/2010 04:10 PM, Chris Wright wrote:
> >* Anthony Liguori (anthony@codemonkey.ws) wrote:
> >>An fd as a qdev property seems like a bad idea to me.
> >What is your concern?
> 
> qdev properties are supposed to represent device tunables.  A file
> descriptor is not a tunable.

It tunes which config file to read from ;-)

> It's like passing the tap device fd via qdev to an e1000.
> 
> >>I'm not sure I have a better suggestion though.
> >Anything else requires inventing some new commandline options and monitor
> >comnmands (-pcidevice anyone? ;-).  I'm not sure the benefit, esp with
> >hopes of moving to uio.
> 
> Yeah, I think device passthrough is going to require new command
> line syntax to be more qemu friendly...

Right, and I think we should do that in the context of redoing the
infrastructure.  What do you think?

thanks,
-chris

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Anthony Liguori]

On 05/19/2010 04:59 PM, Chris Wright wrote:
>>>> I'm not sure I have a better suggestion though.
>>>>          
>>> Anything else requires inventing some new commandline options and monitor
>>> comnmands (-pcidevice anyone? ;-).  I'm not sure the benefit, esp with
>>> hopes of moving to uio.
>>>        
>> Yeah, I think device passthrough is going to require new command
>> line syntax to be more qemu friendly...
>>      
> Right, and I think we should do that in the context of redoing the
> infrastructure.  What do you think?
>    

Yeah, as long as it's understood that this in on the horizon 
(particularly on the libvirt side).

Regards,

Anthony Liguori

> thanks,
> -chris
>    

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Daniel P. Berrange]

On Wed, May 19, 2010 at 02:18:35PM -0500, Anthony Liguori wrote:
> On 05/19/2010 02:00 PM, Chris Wright wrote:
> >When libvirt launches a guest it first chowns the relevenat
> >/sys/bus/pci/.../config file for an assigned device then drops privileges.
> >
> >This causes an issue for device assignment because despite being file
> >owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> >allowing access to device dependent config space.
> >
> >This adds a new qdev configfd property which allows libvirt to open the
> >sysfs config space file and give qemu an already opened file descriptor.
> >Along with a change pending for the 2.6.35 kernel, this allows the
> >capability check to compare against privileges from when the file was
> >opened.
> >
> >Signed-off-by: Chris Wright<chrisw@redhat.com>
> >   
> 
> An fd as a qdev property seems like a bad idea to me.  I'm not sure I 
> have a better suggestion though.

The entire requirement to pass the open FD to qemu is a bad idea, but
the kernel makes it impossible to do otherwise :-( Personally I wish the
kernel just used the file ownership, so we could chown() the sysfs file 
to 'qemu' user in a normal manner :-(

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/19/2010 02:00 PM, Chris Wright wrote:
>> When libvirt launches a guest it first chowns the relevenat
>> /sys/bus/pci/.../config file for an assigned device then drops privileges.
>>
>> This causes an issue for device assignment because despite being file
>> owner, the sysfs config space file checks for CAP_SYS_ADMIN before
>> allowing access to device dependent config space.
>>
>> This adds a new qdev configfd property which allows libvirt to open the
>> sysfs config space file and give qemu an already opened file descriptor.
>> Along with a change pending for the 2.6.35 kernel, this allows the
>> capability check to compare against privileges from when the file was
>> opened.
>>
>> Signed-off-by: Chris Wright<chrisw@redhat.com>
>>    
>
> An fd as a qdev property seems like a bad idea to me.  I'm not sure I
> have a better suggestion though.

Shot from the hip without much thought: could we use monitor command
getfd?  That associates the fd with a name.

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Alex Williamson]

On Wed, 2010-05-19 at 12:00 -0700, Chris Wright wrote:
> When libvirt launches a guest it first chowns the relevenat
> /sys/bus/pci/.../config file for an assigned device then drops privileges.
> 
> This causes an issue for device assignment because despite being file
> owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> allowing access to device dependent config space.
> 
> This adds a new qdev configfd property which allows libvirt to open the
> sysfs config space file and give qemu an already opened file descriptor.
> Along with a change pending for the 2.6.35 kernel, this allows the
> capability check to compare against privileges from when the file was
> opened.

We need to make configfd be a string option so that we can pass a
descriptor from libvirt for the hotplug case.  Here's a rework.

Signed-off-by: Chris Wright <chrisw@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index eb31c78..3c5184f 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -37,6 +37,7 @@
 #include "console.h"
 #include "device-assignment.h"
 #include "loader.h"
+#include "monitor.h"
 #include <pci/pci.h>
 
 /* From linux/ioport.h */
@@ -612,14 +613,28 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
 
     snprintf(name, sizeof(name), "%sconfig", dir);
 
-    fd = open(name, O_RDWR);
-    if (fd == -1) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
-        return 1;
+    if (pci_dev->configfd_name && *pci_dev->configfd_name) {
+        if (qemu_isdigit(pci_dev->configfd_name[0])) {
+            dev->config_fd = strtol(pci_dev->configfd_name, NULL, 0);
+        } else {
+            dev->config_fd = monitor_get_fd(cur_mon, pci_dev->configfd_name);
+            if (dev->config_fd < 0) {
+                fprintf(stderr, "%s: (%s) unkown\n", __func__,
+                        pci_dev->configfd_name);
+                return 1;
+            }
+        }
+    } else {
+        dev->config_fd = open(name, O_RDWR);
+
+        if (dev->config_fd == -1) {
+            fprintf(stderr, "%s: %s: %m\n", __func__, name);
+            return 1;
+        }
     }
-    dev->config_fd = fd;
 again:
-    r = read(fd, pci_dev->dev.config, pci_config_size(&pci_dev->dev));
+    r = read(dev->config_fd, pci_dev->dev.config,
+             pci_config_size(&pci_dev->dev));
     if (r < 0) {
         if (errno == EINTR || errno == EAGAIN)
             goto again;
@@ -1433,6 +1448,7 @@ static PCIDeviceInfo assign_info = {
     .qdev.props   = (Property[]) {
         DEFINE_PROP("host", AssignedDevice, host, qdev_prop_hostaddr, PCIHostDevice),
         DEFINE_PROP_UINT32("iommu", AssignedDevice, use_iommu, 1),
+        DEFINE_PROP_STRING("configfd", AssignedDevice, configfd_name),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/device-assignment.h b/hw/device-assignment.h
index d561112..f70ace9 100644
--- a/hw/device-assignment.h
+++ b/hw/device-assignment.h
@@ -104,6 +104,7 @@ typedef struct AssignedDevice {
     target_phys_addr_t msix_table_addr;
     int mmio_index;
     int need_emulate_cmd;
+    char *configfd_name;
     QLIST_ENTRY(AssignedDevice) next;
 } AssignedDevice;
 

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Chris Wright]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 2010-05-19 at 12:00 -0700, Chris Wright wrote:
> > When libvirt launches a guest it first chowns the relevenat
> > /sys/bus/pci/.../config file for an assigned device then drops privileges.
> > 
> > This causes an issue for device assignment because despite being file
> > owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> > allowing access to device dependent config space.
> > 
> > This adds a new qdev configfd property which allows libvirt to open the
> > sysfs config space file and give qemu an already opened file descriptor.
> > Along with a change pending for the 2.6.35 kernel, this allows the
> > capability check to compare against privileges from when the file was
> > opened.
> 
> We need to make configfd be a string option so that we can pass a
> descriptor from libvirt for the hotplug case.  Here's a rework.


ACK, thanks.

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 2010-05-19 at 12:00 -0700, Chris Wright wrote:
>> When libvirt launches a guest it first chowns the relevenat
>> /sys/bus/pci/.../config file for an assigned device then drops privileges.
>> 
>> This causes an issue for device assignment because despite being file
>> owner, the sysfs config space file checks for CAP_SYS_ADMIN before
>> allowing access to device dependent config space.
>> 
>> This adds a new qdev configfd property which allows libvirt to open the
>> sysfs config space file and give qemu an already opened file descriptor.
>> Along with a change pending for the 2.6.35 kernel, this allows the
>> capability check to compare against privileges from when the file was
>> opened.
>
> We need to make configfd be a string option so that we can pass a
> descriptor from libvirt for the hotplug case.  Here's a rework.
>
> Signed-off-by: Chris Wright <chrisw@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index eb31c78..3c5184f 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -37,6 +37,7 @@
>  #include "console.h"
>  #include "device-assignment.h"
>  #include "loader.h"
> +#include "monitor.h"
>  #include <pci/pci.h>
>  
>  /* From linux/ioport.h */
> @@ -612,14 +613,28 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
>  
>      snprintf(name, sizeof(name), "%sconfig", dir);
>  
> -    fd = open(name, O_RDWR);
> -    if (fd == -1) {
> -        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> -        return 1;
> +    if (pci_dev->configfd_name && *pci_dev->configfd_name) {
> +        if (qemu_isdigit(pci_dev->configfd_name[0])) {
> +            dev->config_fd = strtol(pci_dev->configfd_name, NULL, 0);
> +        } else {
> +            dev->config_fd = monitor_get_fd(cur_mon, pci_dev->configfd_name);
> +            if (dev->config_fd < 0) {
> +                fprintf(stderr, "%s: (%s) unkown\n", __func__,
> +                        pci_dev->configfd_name);
> +                return 1;
> +            }
> +        }

Similar to net_handle_fd_param().  What about factoring into a common
helper?

Rest looks good to me.

[...]

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Alex Williamson]

On Tue, 2010-05-25 at 08:43 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Wed, 2010-05-19 at 12:00 -0700, Chris Wright wrote:
> >> When libvirt launches a guest it first chowns the relevenat
> >> /sys/bus/pci/.../config file for an assigned device then drops privileges.
> >> 
> >> This causes an issue for device assignment because despite being file
> >> owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> >> allowing access to device dependent config space.
> >> 
> >> This adds a new qdev configfd property which allows libvirt to open the
> >> sysfs config space file and give qemu an already opened file descriptor.
> >> Along with a change pending for the 2.6.35 kernel, this allows the
> >> capability check to compare against privileges from when the file was
> >> opened.
> >
> > We need to make configfd be a string option so that we can pass a
> > descriptor from libvirt for the hotplug case.  Here's a rework.
> >
> > Signed-off-by: Chris Wright <chrisw@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index eb31c78..3c5184f 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -37,6 +37,7 @@
> >  #include "console.h"
> >  #include "device-assignment.h"
> >  #include "loader.h"
> > +#include "monitor.h"
> >  #include <pci/pci.h>
> >  
> >  /* From linux/ioport.h */
> > @@ -612,14 +613,28 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
> >  
> >      snprintf(name, sizeof(name), "%sconfig", dir);
> >  
> > -    fd = open(name, O_RDWR);
> > -    if (fd == -1) {
> > -        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> > -        return 1;
> > +    if (pci_dev->configfd_name && *pci_dev->configfd_name) {
> > +        if (qemu_isdigit(pci_dev->configfd_name[0])) {
> > +            dev->config_fd = strtol(pci_dev->configfd_name, NULL, 0);
> > +        } else {
> > +            dev->config_fd = monitor_get_fd(cur_mon, pci_dev->configfd_name);
> > +            if (dev->config_fd < 0) {
> > +                fprintf(stderr, "%s: (%s) unkown\n", __func__,
> > +                        pci_dev->configfd_name);
> > +                return 1;
> > +            }
> > +        }
> 
> Similar to net_handle_fd_param().  What about factoring into a common
> helper?

Yep, maybe we should move that into monitor.c.  Thanks,

Alex

Re: [PATCH qemu-kvm] device-assignment: add config fd qdev property

[Marcelo Tosatti]

On Mon, May 24, 2010 at 10:53:49AM -0600, Alex Williamson wrote:
> On Wed, 2010-05-19 at 12:00 -0700, Chris Wright wrote:
> > When libvirt launches a guest it first chowns the relevenat
> > /sys/bus/pci/.../config file for an assigned device then drops privileges.
> > 
> > This causes an issue for device assignment because despite being file
> > owner, the sysfs config space file checks for CAP_SYS_ADMIN before
> > allowing access to device dependent config space.
> > 
> > This adds a new qdev configfd property which allows libvirt to open the
> > sysfs config space file and give qemu an already opened file descriptor.
> > Along with a change pending for the 2.6.35 kernel, this allows the
> > capability check to compare against privileges from when the file was
> > opened.
> 
> We need to make configfd be a string option so that we can pass a
> descriptor from libvirt for the hotplug case.  Here's a rework.
> 
> Signed-off-by: Chris Wright <chrisw@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Applied, thanks.