[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt PATCH v4 6/6] Include vdpa devices in node device list



On Wed, 30 Sep 2020 10:02:04 -0500
Jonathon Jongsma <jjongsma redhat com> wrote:

> Adding Cindy and Jason to cc for input on the path issue below
> 
> On Tue, 29 Sep 2020 15:56:13 -0400
> Laine Stump <laine redhat com> wrote:
> 
> > On 9/24/20 5:45 PM, Jonathon Jongsma wrote:  
> > > The current udev node device driver ignores all events related to
> > > vdpa devices. Since libvirt now supports vDPA network devices,
> > > include these devices in the device list.    
> > 
> > 
> > Can you provide an example in the commit log of what the output xml 
> > looks like for nodedev-list and nodedev-dumpxml?  
> 
> So, this is what the xml looks like for the vdpa node device:
> 
> <device>
>   <name>vdpa_vdpa0</name>
>   <path>/sys/devices/vdpa0</path>
>   <parent>computer</parent>
>   <driver>
>     <name>vhost_vdpa</name>
>   </driver>
>   <capability type='vdpa'>
>   </capability>
> </device>
>  
> 
> The /sys/devices/ path is provided by udev, but unfortunately that
> path is not particularly useful for using the device with libvirt.
> Ideally, the node device xml should include the path to the chardev
> (e.g. /dev/vhost-vdpa-0) which is used to actually connect to the
> device. I don't see an obvious way (aside from string manipulation,
> which doesn't seem very reliable) to get from this sysfs path to the
> /dev/ path. Jason or Cindy, do you have any input here?

So after after a little more thinking and poking it looks like it may
not be too hard to connect the sysfs path to the chardev after all. I
don't have any vdpa hardware at the moment, but for vdpa_sim, the sysfs
path /sys/devices/vdpa0 has a subdirectory named vhost-vdpa-0 that
matches the name under /dev/ (/dev/vhost-vdpa-0). So perhaps I can use
that to generate the device path. 

Something like this pseudo-code: 

for file in syspath {
  if file.name begins with "vhost-vdpa" {
    return "/dev/" + file.name;
  }
}

Are these assumptions safe? Will the file always begin with
"vhost-vdpa"? Will it always match the /dev file? Jason?

In that case, I can adjust the node device XML to be something like:

<device>
  <name>vdpa_vdpa0</name>
  <path>/sys/devices/vdpa0</path>
  <parent>computer</parent>
  <driver>
    <name>vhost_vdpa</name>
  </driver>
  <capability type='vdpa'>
    <chardev>/dev/vhost-vdpa-0</chardev>
  </capability>
</device>

(not totally sure about the naming of the xml element).

> 
> 
> > 
> >   
> > >
> > > Signed-off-by: Jonathon Jongsma <jjongsma redhat com>
> > > ---
> > >   include/libvirt/libvirt-nodedev.h  |  1 +
> > >   src/conf/node_device_conf.c        |  5 +++++
> > >   src/conf/node_device_conf.h        |  4 +++-
> > >   src/conf/virnodedeviceobj.c        |  4 +++-
> > >   src/node_device/node_device_udev.c | 16 ++++++++++++++++
> > >   tools/virsh-nodedev.c              |  3 +++
> > >   6 files changed, 31 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libvirt/libvirt-nodedev.h
> > > b/include/libvirt/libvirt-nodedev.h index dd2ffd5782..b73b076f14
> > > 100644 --- a/include/libvirt/libvirt-nodedev.h
> > > +++ b/include/libvirt/libvirt-nodedev.h
> > > @@ -82,6 +82,7 @@ typedef enum {
> > >       VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV          = 1 << 14,
> > > /* Mediated device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV
> > > = 1 << 15, /* CCW device */
> > > VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV       = 1 << 16, /* CSS
> > > device */
> > > +    VIR_CONNECT_LIST_NODE_DEVICES_CAP_VDPA          = 1 << 17, /*
> > > vDPA device */ } virConnectListAllNodeDeviceFlags;
> > >   
> > >   int                     virConnectListAllNodeDevices
> > > (virConnectPtr conn, diff --git a/src/conf/node_device_conf.c
> > > b/src/conf/node_device_conf.c index a9a03ad6c2..3eab1cda75 100644
> > > --- a/src/conf/node_device_conf.c
> > > +++ b/src/conf/node_device_conf.c
> > > @@ -66,6 +66,7 @@ VIR_ENUM_IMPL(virNodeDevCap,
> > >                 "mdev",
> > >                 "ccw",
> > >                 "css",
> > > +              "vdpa",
> > >   );
> > >   
> > >   VIR_ENUM_IMPL(virNodeDevNetCap,
> > > @@ -614,6 +615,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef
> > > *def) case VIR_NODE_DEV_CAP_MDEV_TYPES:
> > >           case VIR_NODE_DEV_CAP_FC_HOST:
> > >           case VIR_NODE_DEV_CAP_VPORTS:
> > > +        case VIR_NODE_DEV_CAP_VDPA:
> > >           case VIR_NODE_DEV_CAP_LAST:
> > >               break;
> > >           }
> > > @@ -1913,6 +1915,7 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr
> > > ctxt, case VIR_NODE_DEV_CAP_FC_HOST:
> > >       case VIR_NODE_DEV_CAP_VPORTS:
> > >       case VIR_NODE_DEV_CAP_SCSI_GENERIC:
> > > +    case VIR_NODE_DEV_CAP_VDPA:
> > >       case VIR_NODE_DEV_CAP_LAST:
> > >           virReportError(VIR_ERR_INTERNAL_ERROR,
> > >                          _("unknown capability type '%d' for
> > > '%s'"), @@ -2232,6 +2235,7 @@
> > > virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) case
> > > VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_CCW_DEV:
> > >       case VIR_NODE_DEV_CAP_CSS_DEV:
> > > +    case VIR_NODE_DEV_CAP_VDPA:
> > >       case VIR_NODE_DEV_CAP_LAST:
> > >           /* This case is here to shutup the compiler */
> > >           break;
> > > @@ -2286,6 +2290,7 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr
> > > def) case VIR_NODE_DEV_CAP_MDEV:
> > >           case VIR_NODE_DEV_CAP_CCW_DEV:
> > >           case VIR_NODE_DEV_CAP_CSS_DEV:
> > > +        case VIR_NODE_DEV_CAP_VDPA:
> > >           case VIR_NODE_DEV_CAP_LAST:
> > >               break;
> > >           }
> > > diff --git a/src/conf/node_device_conf.h
> > > b/src/conf/node_device_conf.h index 5484bc340f..4f8e47a068 100644
> > > --- a/src/conf/node_device_conf.h
> > > +++ b/src/conf/node_device_conf.h
> > > @@ -65,6 +65,7 @@ typedef enum {
> > >       VIR_NODE_DEV_CAP_MDEV,              /* Mediated device */
> > >       VIR_NODE_DEV_CAP_CCW_DEV,           /* s390 CCW device */
> > >       VIR_NODE_DEV_CAP_CSS_DEV,           /* s390 channel
> > > subsystem device */
> > > +    VIR_NODE_DEV_CAP_VDPA,              /* vDPA device */
> > >   
> > >       VIR_NODE_DEV_CAP_LAST
> > >   } virNodeDevCapType;
> > > @@ -369,7 +370,8 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr
> > > caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES    | \
> > >                    VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV
> > > | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV       | \
> > > -                 VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV)
> > > +                 VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV
> > > | \
> > > +                 VIR_CONNECT_LIST_NODE_DEVICES_CAP_VDPA)
> > >   
> > >   int
> > >   virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr
> > > scsi_host); diff --git a/src/conf/virnodedeviceobj.c
> > > b/src/conf/virnodedeviceobj.c index 8aefd15e94..83c58ebe91 100644
> > > --- a/src/conf/virnodedeviceobj.c
> > > +++ b/src/conf/virnodedeviceobj.c
> > > @@ -711,6 +711,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj
> > > *obj, case VIR_NODE_DEV_CAP_MDEV:
> > >           case VIR_NODE_DEV_CAP_CCW_DEV:
> > >           case VIR_NODE_DEV_CAP_CSS_DEV:
> > > +        case VIR_NODE_DEV_CAP_VDPA:
> > >           case VIR_NODE_DEV_CAP_LAST:
> > >               break;
> > >           }
> > > @@ -862,7 +863,8 @@ virNodeDeviceObjMatch(virNodeDeviceObjPtr obj,
> > >                 MATCH(MDEV_TYPES)    ||
> > >                 MATCH(MDEV)          ||
> > >                 MATCH(CCW_DEV)       ||
> > > -              MATCH(CSS_DEV)))
> > > +              MATCH(CSS_DEV)       ||
> > > +              MATCH(VDPA)))
> > >               return false;
> > >       }
> > >   
> > > diff --git a/src/node_device/node_device_udev.c
> > > b/src/node_device/node_device_udev.c index 12e3f30bad..fda72f9071
> > > 100644 --- a/src/node_device/node_device_udev.c
> > > +++ b/src/node_device/node_device_udev.c
> > > @@ -1144,6 +1144,18 @@ udevProcessCSS(struct udev_device *device,
> > >       return 0;
> > >   }
> > >   
> > > +
> > > +static int
> > > +udevProcessVDPA(struct udev_device *device,
> > > +                virNodeDeviceDefPtr def)
> > > +{
> > > +    if (udevGenerateDeviceName(device, def, NULL) != 0)
> > > +        return -1;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +
> > >   static int
> > >   udevGetDeviceNodes(struct udev_device *device,
> > >                      virNodeDeviceDefPtr def)
> > > @@ -1224,6 +1236,8 @@ udevGetDeviceType(struct udev_device
> > > *device, *type = VIR_NODE_DEV_CAP_CCW_DEV;
> > >           else if (STREQ_NULLABLE(subsystem, "css"))
> > >               *type = VIR_NODE_DEV_CAP_CSS_DEV;
> > > +        else if (STREQ_NULLABLE(subsystem, "vdpa"))
> > > +            *type = VIR_NODE_DEV_CAP_VDPA;
> > >   
> > >           VIR_FREE(subsystem);
> > >       }
> > > @@ -1270,6 +1284,8 @@ udevGetDeviceDetails(struct udev_device
> > > *device, return udevProcessCCW(device, def);
> > >       case VIR_NODE_DEV_CAP_CSS_DEV:
> > >           return udevProcessCSS(device, def);
> > > +    case VIR_NODE_DEV_CAP_VDPA:
> > > +        return udevProcessVDPA(device, def);
> > >       case VIR_NODE_DEV_CAP_MDEV_TYPES:
> > >       case VIR_NODE_DEV_CAP_SYSTEM:
> > >       case VIR_NODE_DEV_CAP_FC_HOST:
> > > diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
> > > index 2edd403a64..19f0c17b4f 100644
> > > --- a/tools/virsh-nodedev.c
> > > +++ b/tools/virsh-nodedev.c
> > > @@ -464,6 +464,9 @@ cmdNodeListDevices(vshControl *ctl, const
> > > vshCmd *cmd G_GNUC_UNUSED) case VIR_NODE_DEV_CAP_CSS_DEV:
> > >               flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV;
> > >               break;
> > > +        case VIR_NODE_DEV_CAP_VDPA:
> > > +            flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_VDPA;
> > > +            break;
> > >           case VIR_NODE_DEV_CAP_LAST:
> > >               break;
> > >           }    
> > 
> >   
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]