>>> On 27.09.16 at 17:57, <roger....@citrix.com> wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -632,6 +632,8 @@ int hvm_domain_initialise(struct domain *d) > goto fail1; > } > memset(d->arch.hvm_domain.io_bitmap, ~0, HVM_IOBITMAP_SIZE); > + INIT_LIST_HEAD(&d->arch.hvm_domain.pt_devices);
This field appears to be redundant with arch.pdev_list. > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -46,6 +46,10 @@ > #include <xen/iocap.h> > #include <public/hvm/ioreq.h> > > +/* Set permissive mode for HVM Dom0 PCI pass-through by default */ > +static bool_t opt_dom0permissive = 1; Plain bool / true / false please. And as mentioned by Andrew, we should stop adding more dom0xyz options, and use a consolidated dom0= one instead. > @@ -258,12 +262,403 @@ static bool_t hw_dpci_portio_accept(const struct > hvm_io_handler *handler, > return 0; > } > > +static struct hvm_pt_device *hw_dpci_get_device(struct domain *d) > +{ > + uint8_t bus, slot, func; > + uint32_t addr; > + struct hvm_pt_device *dev; > + > + /* Decode bus, slot and func. */ > + addr = CF8_BDF(d->arch.pci_cf8); > + bus = PCI_BUS(addr); > + slot = PCI_SLOT(addr); > + func = PCI_FUNC(addr); > + > + list_for_each_entry( dev, &d->arch.hvm_domain.pt_devices, entries ) > + { > + if ( dev->pdev->seg != 0 || dev->pdev->bus != bus || Okay, there's no way segments other than 0 can be handled here. But having glanced over the titles of the rest of the series - where are those going to be handled (read: Where is the MCFG code, which qemu doesn't have)? Also I think the function name is not well chosen: Its prefix suggests some kind of "official" interface, yet it really just is an internal helper which doesn't even "get" a device in the general sense of needing to "put" it later on. And it looks like the parameter could be constified (but this appears to be a wider problem). > +/* Dispatchers */ > + > +/* Find emulate register group entry */ > +struct hvm_pt_reg_group *hvm_pt_find_reg_grp(struct hvm_pt_device *d, > + uint32_t address) Please don't needlessly use fixed width types. > +{ > + struct hvm_pt_reg_group *entry = NULL; > + > + /* Find register group entry */ > + list_for_each_entry( entry, &d->register_groups, entries ) > + { > + /* check address */ > + if ( (entry->base_offset <= address) > + && ((entry->base_offset + entry->size) > address) ) Coding style (&& belongs on the previous line). And actually I guess I'll stop here, realizing that I'm completely unconvinced of the not even spelled out intentions. Alone the lifting of code from qemu is problematic imo: That code has proven to have many issues, only the most severe of which have got fixed over time. I'm therefore of the opinion that a clean re-write from scratch should at least be considered, once it was written down somewhere (docs/misc/hvmlite.markdown?) and agreed upon what the behavior actually ought to be. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel