Xen devel - Jan 2010 - [PATCH] xend: passthrough: also do_FLR when a device is assigned

To workaround a race condition about guest hotplug, c/s 18338:7c10be016e4
disabled do_FLR when we create guest or ''xm pci-attach'' device
into guest, so
now we actually only do_FLR when a guest is destroyed or ''xm
pci-detach''.

By moving the FLR-related checking/do_FLR logic a little earlier, this patch
re-enables do_FLR in these 2 cases disabled by 18338.

Thanks,
-- Dexuan
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> xend: passthrough: also do_FLR when a device is assigned.
> 
> To workaround a race condition about guest hotplug, c/s 18338:7c10be016e4
> disabled do_FLR when we create guest or ''xm pci-attach''
device into guest, so
> now we actually only do_FLR when a guest is destroyed or ''xm
pci-detach''.
> 
> By moving the FLR-related checking/do_FLR logic a little earlier, this
patch
> re-enables do_FLR in these 2 cases disabled by 18338.
> 
> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
Hi Dexuan,

I''m not sure that I''ve entirely got my head around this
change.
But it does seem like a reasonable approach. Comments below.
> 
> diff -r 4feec90815a0 tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py	Tue Jan 05 08:40:18 2010
+0000
> +++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jan 05 22:17:13 2010
+0800
> @@ -689,11 +689,24 @@ class XendDomainInfo:
>                      raise VmError("device is already inserted")
>  
>          # Test whether the devices can be assigned.
> -        self.pci_device_check_attachability(new_dev)
> +        self.pci_dev_check_attachability_and_do_FLR(new_dev)
>  
>          return self.hvm_pci_device_insert_dev(new_dev)
>  
> -    def pci_device_check_attachability(self, new_dev):
> +    def pci_dev_check_assignability_and_do_FLR(self, config):
> +        """ In the case of static device assignment(i.e.,
the ''pci'' string in
> +        guest config file), we check if the device(s) specified in the
''pci''
> +        can be  assigned to guest or not; if yes, we do_FLR the device(s).
> +        """
> +        pci_dev_ctrl = self.getDeviceController(''pci'')
> +        return pci_dev_ctrl.dev_check_assignability_and_do_FLR(config)
> +
> +    def pci_dev_check_attachability_and_do_FLR(self, new_dev):
> +        """ In the case of dynamic device assignment(i.e.,
xm pci-attach), we
> +        check if the device can be attached to guest or not; if yes, we
do_FLR
> +        the device.
> +        """
> +
>          # Test whether the devices can be assigned
>  
>          pci_name = pci_dict_to_bdf_str(new_dev)
> @@ -720,6 +733,8 @@ class XendDomainInfo:
>  
>          # PV guest has less checkings.
>          if not self.info.is_hvm():
> +            # try to do FLR for PV guest
> +            pci_device.do_FLR(self.info.is_hvm(), strict_check)
>              return
>  
>          if not strict_check:
> @@ -748,6 +763,9 @@ class XendDomainInfo:
>                      " because one of its co-assignment device %s has
been" + \
>                      " assigned to other domain." \
>                      )% (pci_device.name,
self.info[''name_label''], pci_str))
> +
> +        # try to do FLR for HVM guest
> +        pci_device.do_FLR(self.info.is_hvm(), strict_check)
>  
>      def hvm_pci_device_insert(self, dev_config):
>          log.debug("XendDomainInfo.hvm_pci_device_insert: %s"
> @@ -919,7 +937,7 @@ class XendDomainInfo:
>          # Do PV specific checking
>              if pci_state == ''Initialising'':
>                  # PV PCI device attachment
> -                self.pci_device_check_attachability(dev)
> +                self.pci_dev_check_attachability_and_do_FLR(dev)
>  
>          # If pci platform does not exist, create and exit.
>          if existing_dev_info is None :
> @@ -1218,7 +1236,8 @@ class XendDomainInfo:
>          coassignment_list.remove(pci_device.name)
>          assigned_pci_device_str_list = self._get_assigned_pci_devices()
>          for pci_str in coassignment_list:
> -            if pci_str in assigned_pci_device_str_list:
> +            if xoptions.get_pci_dev_assign_strict_check() and \
> +                pci_str in assigned_pci_device_str_list:
>                  raise VmError(("pci: failed to pci-detach %s from
domain %s" + \
>                      " because one of its co-assignment device %s is
still " + \
>                      " assigned to the domain." \
> @@ -2312,6 +2331,10 @@ class XendDomainInfo:
>              if devclass in XendDevices.valid_devices() and devclass !=
''vscsi'':
>                  log.info("createDevice: %s : %s" % (devclass,
scrub_password(config)))
>                  dev_uuid = config.get(''uuid'')
> +
> +                if devclass == ''pci'':
> +                    self.pci_dev_check_assignability_and_do_FLR(config)
> +
>                  if devclass != ''pci'' or not
self.info.is_hvm() :
>                      devid = self._createDevice(devclass, config)
>                  
> diff -r 4feec90815a0 tools/python/xen/xend/server/pciif.py
> --- a/tools/python/xen/xend/server/pciif.py	Tue Jan 05 08:40:18 2010 +0000
> +++ b/tools/python/xen/xend/server/pciif.py	Tue Jan 05 22:06:37 2010 +0800
> @@ -274,8 +274,8 @@ class PciController(DevController):
>                  continue
>  
>              if sdev.driver!=''pciback'' and
sdev.driver!=''pci-stub'':
> -                raise VmError(("pci: PCI Backend and pci-stub
don''t\n "+ \
> -                    "own sibling device %s of device %s\n"\
> +                raise VmError(("pci: PCI Backend and pci-stub
don''t "+ \
> +                    "own sibling device %s of device %s"\
>                      )%(sdev.name, dev.name))
Minor nit, and yes I realise many of the lines affected are my own.
''\'' is not needed to continue a line if the element being
continued is
enclosed in (). So if you are updating a multi-line error as above, you
might as well get rid of the trailing ''\'' at the same time.
There are a
few other candidates below.

Also, "don''t" should be "doesn''t".
>          return
>  
> @@ -292,16 +292,9 @@ class PciController(DevController):
>  
>          if dev.driver!=''pciback'' and
dev.driver!=''pci-stub'':
>              raise VmError(("pci: PCI Backend and pci-stub
don''t own "+ \
> -                    "device %s\n") %(dev.name))
> +                    "device %s") %(dev.name))
>  
>          self.CheckSiblingDevices(fe_domid, dev)
> -
> -        # We don''t do FLR when we create domain and hotplug
device into guest,
> -        # namely, we only do FLR when we destroy domain or hotplug device
from
> -        # guest. This is mainly to work around the race condition in
hotplug code
> -        # paths. See the changeset''s description for details.
> -        # if arch.type != "ia64":
> -        #    dev.do_FLR()
>  
>          if dev.driver == ''pciback'':
>              PCIQuirk(dev)
> @@ -353,9 +346,7 @@ class PciController(DevController):
>                  raise VmError((''pci: failed to configure irq on
device ''+
>                              ''%s -
errno=%d'')%(dev.name,rc))
>  
> -    def setupDevice(self, config):
> -        """Setup devices from config
> -        """
> +    def dev_check_assignability_and_do_FLR(self, config):
>          pci_dev_list = config.get(''devs'', [])
>          pci_str_list = map(pci_dict_to_bdf_str, pci_dev_list)
>  
> @@ -363,12 +354,18 @@ class PciController(DevController):
>              raise VmError(''pci: duplicate devices specified in
guest config?'')
>  
>          strict_check = xoptions.get_pci_dev_assign_strict_check()
> +        devs = []
>          for pci_dev in pci_dev_list:
>              try:
>                  dev = PciDevice(pci_dev)
>              except Exception, e:
>                  raise VmError("pci: failed to locate device and
"+
>                          "parse its resources - "+str(e))
> +            if dev.driver!=''pciback'' and
dev.driver!=''pci-stub'':
> +                raise VmError(("pci: PCI Backend and pci-stub
don''t own device"\
> +                    " %s") %(dev.name))
> +
> +            devs.append(dev)
I''m not sure that I understand why devs is needed. There seem to be two
cases:

	1) An exception is thrown before devs is fully seeded
	2) devs is fully seeded as a copy of pci_dev_list

Can devs be removed and pci_dev_list be used directly below?
>  
>              if dev.has_non_page_aligned_bar and strict_check:
>                  raise VmError("pci: %s: non-page-aligned MMIO BAR
found." % dev.name)
> @@ -435,18 +432,16 @@ class PciController(DevController):
>                                  err_msg = ''pci: %s must be
co-assigned to the''+\
>                                      '' same guest with
%s''
>                                  raise VmError(err_msg % (s, dev.name))
> -
> -        # Assigning device staticaly (namely, the pci string in guest
config
> -        # file) to PV guest needs this setupOneDevice().
> -        # Assigning device dynamically (namely, ''xm
pci-attach'') to PV guest
> -        #  would go through reconfigureDevice().
> -        #
> -        # For hvm guest, (from c/s 19679 on) assigning device statically
and
> -        # dynamically both go through reconfigureDevice(), so HERE the
> -        # setupOneDevice() is not necessary.
> -        if not self.vm.info.is_hvm():
> -            for d in pci_dev_list:
> -                self.setupOneDevice(d)
> +        # try to do FLR
> +        for dev in devs:
> +            dev.do_FLR(self.vm.info.is_hvm(), strict_check)
Not really part of this change, but can self.vm.info.is_hvm() just be moved
to inside do_FLR() ?
> +
> +    def setupDevice(self, config):
> +        """Setup devices from config
> +        """
Some logic seems to be lost from above. Is that intentional?
> +        pci_dev_list = config.get(''devs'', [])
	   if not self.vm.info.is_hvm(): <--- Should this be here?
> +        for d in pci_dev_list:
> +            self.setupOneDevice(d)
>          wPath =
''/local/domain/0/backend/pci/%u/0/aerState'' %
(self.getDomid())
>          self.aerStateWatch = xswatch(wPath, self._handleAerStateWatch)
>          log.debug(''pci: register aer watch %s'', wPath)
> @@ -477,7 +472,7 @@ class PciController(DevController):
>  
>          if dev.driver!=''pciback'' and
dev.driver!=''pci-stub'':
>              raise VmError(("pci: PCI Backend and pci-stub
don''t own device "+ \
> -                    "%s\n") %(dev.name))
> +                    "%s") %(dev.name))
>  
>          # Need to do FLR here before deassign device in order to terminate
>          # DMA transaction, etc
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman wrote:
>> xend: passthrough: also do_FLR when a device is assigned.
>> 
>> To workaround a race condition about guest hotplug, c/s
>> 18338:7c10be016e4 
>> disabled do_FLR when we create guest or ''xm
pci-attach'' device into
>> guest, so 
>> now we actually only do_FLR when a guest is destroyed or ''xm
>> pci-detach''. 
>> 
>> By moving the FLR-related checking/do_FLR logic a little earlier,
>> this patch re-enables do_FLR in these 2 cases disabled by 18338.
> I''m not sure that I''ve entirely got my head around this
change.
> But it does seem like a reasonable approach. Comments below.
Hi Simon,
Thanks for the review! Please see my replies below.
>> diff -r 4feec90815a0 tools/python/xen/xend/server/pciif.py
>> --- a/tools/python/xen/xend/server/pciif.py	Tue Jan 05 08:40:18 2010
>> +0000 +++ b/tools/python/xen/xend/server/pciif.py	Tue Jan 05
>> 22:06:37 2010 +0800 @@ -274,8 +274,8 @@ class
>> PciController(DevController):                  continue 
>> 
>>              if sdev.driver!=''pciback'' and
sdev.driver!=''pci-stub'':
>> -                raise VmError(("pci: PCI Backend and pci-stub
>> don''t\n "+ \ 
>> -                    "own sibling device %s of device %s\n"\
>> +                raise VmError(("pci: PCI Backend and pci-stub
don''t
>> "+ \ +                    "own sibling device %s of device
%s"\
>>                      )%(sdev.name, dev.name))
> 
> Minor nit, and yes I realise many of the lines affected are my own.
> ''\'' is not needed to continue a line if the element being
continued is
> enclosed in (). So if you are updating a multi-line error as above,
> you 
> might as well get rid of the trailing ''\'' at the same
time. There are
> a 
> few other candidates below.
> 
> Also, "don''t" should be "doesn''t".
My patch doesn''t fix all the minor literal/format issues. 
I just saw the obvious ones and fixed them in the patch.
I''m very glad if you can help to double check all the issues and send a
patch. :-)
At present I''m busy with some other more important bugs.
>> @@ -363,12 +354,18 @@ class PciController(DevController):
>>              raise VmError(''pci: duplicate devices specified
in
>> guest config?'') 
>> 
>>          strict_check = xoptions.get_pci_dev_assign_strict_check() +
>>          devs = [] for pci_dev in pci_dev_list:
>>              try:
>>                  dev = PciDevice(pci_dev)
>>              except Exception, e:
>>                  raise VmError("pci: failed to locate device and
"+
>>                          "parse its resources - "+str(e))
>> +            if dev.driver!=''pciback'' and
dev.driver!=''pci-stub'':
>> +                raise VmError(("pci: PCI Backend and pci-stub
don''t
>> own device"\ +                    " %s") %(dev.name))
>> +
>> +            devs.append(dev)
> 
> I''m not sure that I understand why devs is needed. There seem to
be
> two cases: 
> 
> 	1) An exception is thrown before devs is fully seeded
> 	2) devs is fully seeded as a copy of pci_dev_list
> 
> Can devs be removed and pci_dev_list be used directly below?
No, we can''t.
An element in pci_dev_list is a python Dict object
An element in the ''devs'' is a python PciDevice object.

The intention of "devs" is to record the PciDevice objects generated
at the beginning of dev_check_assignability_and_do_FLR(), so at the end of the
function, we can simply use
"for dev in devs: dev.do_FLR(self.vm.info.is_hvm(), strict_check)"
without generating the PciDevice objects again.
This is only for programming convenience and slightly faster execution of code.
:-)
>> 
>>              if dev.has_non_page_aligned_bar and strict_check:
>>                  raise VmError("pci: %s: non-page-aligned MMIO BAR
>> found." % dev.name) @@ -435,18 +432,16 @@ class
>>                                  PciController(DevController):
>>                                      err_msg = ''pci: %s must
be
>>                                  co-assigned to the''+\
'' same guest
>> with %s'' raise VmError(err_msg % (s, dev.name)) - 
>> -        # Assigning device staticaly (namely, the pci string in
>> guest config 
>> -        # file) to PV guest needs this setupOneDevice().
>> -        # Assigning device dynamically (namely, ''xm
pci-attach'') to
>> PV guest 
>> -        #  would go through reconfigureDevice().
>> -        #
>> -        # For hvm guest, (from c/s 19679 on) assigning device
>> statically and 
>> -        # dynamically both go through reconfigureDevice(), so HERE
>> the 
>> -        # setupOneDevice() is not necessary.
>> -        if not self.vm.info.is_hvm():
>> -            for d in pci_dev_list:
>> -                self.setupOneDevice(d)
>> +        # try to do FLR
>> +        for dev in devs:
>> +            dev.do_FLR(self.vm.info.is_hvm(), strict_check)
> 
> Not really part of this change, but can self.vm.info.is_hvm() just be
> moved to inside do_FLR() ?
I don''t think we can. do_FLR() is a member function of the utility
class PciDevice in util/pci.py -- this file should not involve vm info.
>> +
>> +    def setupDevice(self, config):
>> +        """Setup devices from config
>> +        """
> 
> Some logic seems to be lost from above. Is that intentional?
No actual logic is lost. I just move the FLR-related checking logic ahead into
xend/XendDomainInfo.py: def _createDevices().
What setupDevice() does is simply invoking "for each dev:
dev.setupOneDevice()".
You could "hg checkout 18044" to see the very old version of
setupDevice() that is before my original FLR patches(c/s 18045~18047)
> 
>> +        pci_dev_list = config.get(''devs'', [])
> 
>  -	   if not self.vm.info.is_hvm(): <--- Should this be here?
Here I removed the "if not" because I think we''d better also
invoke setupOneDevice for hvm guest (however, I''m not sure this is a
must as I haven''t checked that very carefully; what I ses is in the
bootup passthrough case, we do invoke it) in the case of dynamic
assignment(namely, xm pci-attach).
You can verify: in the current code (e.g, 20756), after we create an hvm gust
without any "pci" specified in guest config file and when we "xm
pci-attach" the *first* device into guest, setupOneDevice() is not invoked
at all!
Yes, I know after my patch is checked in, there would be redundant invokings of
setupOneDevice. Anyway, this is a minor issue and dosesn''t impact the
correctness and we can further improve it in future.

BTW: actually I found the pci passthrough code in xend had become very complex
and hacky now... Things are easily broken when new changes are made, e.g.
Stefano added the pci passthrough support in the stubdomain case and the code
broke non-stubdomain case for weeks and the msi-translate is still broken even
till now.
It would be really great somebody can help to re-organize/cleanup the code some
time. :-)

So I think for Xen 4.0, the patch should be fine, and after 4.0, hope somebody
could do some cleanup.

Thanks
-- Dexuan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, Jan 06, 2010 at 01:48:52PM +0800, Cui, Dexuan
wrote:
> Simon Horman wrote:
> >> xend: passthrough: also do_FLR when a device is assigned.
> >> 
> >> To workaround a race condition about guest hotplug, c/s
> >> 18338:7c10be016e4 
> >> disabled do_FLR when we create guest or ''xm
pci-attach'' device into
> >> guest, so 
> >> now we actually only do_FLR when a guest is destroyed or
''xm
> >> pci-detach''. 
> >> 
> >> By moving the FLR-related checking/do_FLR logic a little earlier,
> >> this patch re-enables do_FLR in these 2 cases disabled by 18338.
> 
> > I''m not sure that I''ve entirely got my head around
this change.
> > But it does seem like a reasonable approach. Comments below.
> Hi Simon,
> Thanks for the review! Please see my replies below.
> 
> >> diff -r 4feec90815a0 tools/python/xen/xend/server/pciif.py
> >> --- a/tools/python/xen/xend/server/pciif.py	Tue Jan 05 08:40:18
2010
> >> +0000 +++ b/tools/python/xen/xend/server/pciif.py	Tue Jan 05
> >> 22:06:37 2010 +0800 @@ -274,8 +274,8 @@ class
> >> PciController(DevController):                  continue 
> >> 
> >>              if sdev.driver!=''pciback'' and
sdev.driver!=''pci-stub'':
> >> -                raise VmError(("pci: PCI Backend and
pci-stub
> >> don''t\n "+ \ 
> >> -                    "own sibling device %s of device
%s\n"\
> >> +                raise VmError(("pci: PCI Backend and
pci-stub don''t
> >> "+ \ +                    "own sibling device %s of
device %s"\
> >>                      )%(sdev.name, dev.name))
> > 
> > Minor nit, and yes I realise many of the lines affected are my own.
> > ''\'' is not needed to continue a line if the element
being continued is
> > enclosed in (). So if you are updating a multi-line error as above,
> > you 
> > might as well get rid of the trailing ''\'' at the
same time. There are
> > a 
> > few other candidates below.
> > 
> > Also, "don''t" should be
"doesn''t".
> My patch doesn''t fix all the minor literal/format issues.  I just
saw the
> obvious ones and fixed them in the patch.  I''m very glad if you
can help
> to double check all the issues and send a patch. :-) At present
I''m busy
> with some other more important bugs.
Understood.
> 
> >> @@ -363,12 +354,18 @@ class PciController(DevController):
> >>              raise VmError(''pci: duplicate devices
specified in
> >> guest config?'') 
> >> 
> >>          strict_check = xoptions.get_pci_dev_assign_strict_check()
+
> >>          devs = [] for pci_dev in pci_dev_list:
> >>              try:
> >>                  dev = PciDevice(pci_dev)
> >>              except Exception, e:
> >>                  raise VmError("pci: failed to locate device
and "+
> >>                          "parse its resources -
"+str(e))
> >> +            if dev.driver!=''pciback'' and
dev.driver!=''pci-stub'':
> >> +                raise VmError(("pci: PCI Backend and
pci-stub don''t
> >> own device"\ +                    " %s")
%(dev.name))
> >> +
> >> +            devs.append(dev)
> > 
> > I''m not sure that I understand why devs is needed. There seem
to be
> > two cases: 
> > 
> > 	1) An exception is thrown before devs is fully seeded
> > 	2) devs is fully seeded as a copy of pci_dev_list
> > 
> > Can devs be removed and pci_dev_list be used directly below?
> No, we can''t.
> An element in pci_dev_list is a python Dict object
> An element in the ''devs'' is a python PciDevice object.
> 
> The intention of "devs" is to record the PciDevice objects
generated at
> the beginning of dev_check_assignability_and_do_FLR(), so at the end of
> the function, we can simply use "for dev in devs:
> dev.do_FLR(self.vm.info.is_hvm(), strict_check)" without generating
the
> PciDevice objects again.  This is only for programming convenience and
> slightly faster execution of code. :-)
Thanks for clarifying that, I missed the difference between
devs and pci_dev_list.
> 
> >> 
> >>              if dev.has_non_page_aligned_bar and strict_check:
> >>                  raise VmError("pci: %s: non-page-aligned
MMIO BAR
> >> found." % dev.name) @@ -435,18 +432,16 @@ class
> >>                                  PciController(DevController):
> >>                                      err_msg = ''pci: %s
must be
> >>                                  co-assigned to the''+\
'' same guest
> >> with %s'' raise VmError(err_msg % (s, dev.name)) - 
> >> -        # Assigning device staticaly (namely, the pci string in
> >> guest config 
> >> -        # file) to PV guest needs this setupOneDevice().
> >> -        # Assigning device dynamically (namely, ''xm
pci-attach'') to
> >> PV guest 
> >> -        #  would go through reconfigureDevice().
> >> -        #
> >> -        # For hvm guest, (from c/s 19679 on) assigning device
> >> statically and 
> >> -        # dynamically both go through reconfigureDevice(), so
HERE
> >> the 
> >> -        # setupOneDevice() is not necessary.
> >> -        if not self.vm.info.is_hvm():
> >> -            for d in pci_dev_list:
> >> -                self.setupOneDevice(d)
> >> +        # try to do FLR
> >> +        for dev in devs:
> >> +            dev.do_FLR(self.vm.info.is_hvm(), strict_check)
> > 
> > Not really part of this change, but can self.vm.info.is_hvm() just be
> > moved to inside do_FLR() ?
> I don''t think we can. do_FLR() is a member function of the utility
class PciDevice in util/pci.py -- this file should not involve vm info.
> 
> >> +
> >> +    def setupDevice(self, config):
> >> +        """Setup devices from config
> >> +        """
> > 
> > Some logic seems to be lost from above. Is that intentional?
> No actual logic is lost. I just move the FLR-related checking logic ahead
into xend/XendDomainInfo.py: def _createDevices().
> What setupDevice() does is simply invoking "for each dev:
dev.setupOneDevice()".
> You could "hg checkout 18044" to see the very old version of
setupDevice() that is before my original FLR patches(c/s 18045~18047)
> 
> > 
> >> +        pci_dev_list = config.get(''devs'', [])
> > 
> >  -	   if not self.vm.info.is_hvm(): <--- Should this be here?
>
> Here I removed the "if not" because I think we''d better
also invoke
> setupOneDevice for hvm guest (however, I''m not sure this is a must
as I
> haven''t checked that very carefully; what I ses is in the bootup
> passthrough case, we do invoke it) in the case of dynamic
> assignment(namely, xm pci-attach).  You can verify: in the current code
> (e.g, 20756), after we create an hvm gust without any "pci"
specified in
> guest config file and when we "xm pci-attach" the *first* device
into
> guest, setupOneDevice() is not invoked at all!  Yes, I know after my
> patch is checked in, there would be redundant invokings of
> setupOneDevice. Anyway, this is a minor issue and dosesn''t impact
the
> correctness and we can further improve it in future.
I have no problem with duplicate invocations of setupOneDevice()
as long as the result is correct. Removing the logic above
seems to slightly simplify things. And thats good :-)
> BTW: actually I found the pci passthrough code in xend had become very
> complex and hacky now... Things are easily broken when new changes are
> made, e.g. Stefano added the pci passthrough support in the stubdomain
> case and the code broke non-stubdomain case for weeks and the
> msi-translate is still broken even till now.
Yes, I found that that too. It was very difficult for me to make changes too.
>
> It would be really great somebody can help to re-organize/cleanup the
> code some time. :-)
Is the ocaml replacement for xend still being worked on?
Refactoring the current (python) code is incredibly painful
in my experience.
> So I think for Xen 4.0, the patch should be fine, and after 4.0, hope
> somebody could do some cleanup. 
> 
> Thanks -- Dexuan _______________________________________________
> Xen-devel mailing list Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman wrote:
>> BTW: actually I found the pci passthrough code in xend had become
>> very 
>> complex and hacky now... Things are easily broken when new changes
>> are 
>> made, e.g. Stefano added the pci passthrough support in the
>> stubdomain 
>> case and the code broke non-stubdomain case for weeks and the
>> msi-translate is still broken even till now.
> 
> Yes, I found that that too. It was very difficult for me to make
> changes too. 
> 
>> 
>> It would be really great somebody can help to re-organize/cleanup the
>> code some time. :-)
> 
> Is the ocaml replacement for xend still being worked on?
I know few about it.
> Refactoring the current (python) code is incredibly painful
> in my experience.
Agree.

Thanks
-- Dexuan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 6 Jan 2010, Simon Horman wrote:
> > BTW: actually I found the pci passthrough code in xend had become very
> > complex and hacky now... Things are easily broken when new changes are
> > made, e.g. Stefano added the pci passthrough support in the stubdomain
> > case and the code broke non-stubdomain case for weeks and the
> > msi-translate is still broken even till now.
> 
> Yes, I found that that too. It was very difficult for me to make changes
too.
> 
Xend is definitely very difficult to work with; hacky code paths
started to appear long ago unfortunately.
Few changes some months ago tried to simplify the code but broke stubdoms
even more.
Then it was very difficult for me to fix stubdoms and at the same time
keep the normal case working.
The whole thing needs to be re-architectured from scratch and that is
what libxenlight is for.
> >
> > It would be really great somebody can help to re-organize/cleanup the
> > code some time. :-)
> 
> Is the ocaml replacement for xend still being worked on?
> Refactoring the current (python) code is incredibly painful
> in my experience.
> 
Take a look a libxenlight: the idea is to have a common low level
library to take care of these things in the future.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel